Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#31669 closed defect (bug) (fixed)

views improvements continued

Reported by: iseulde's profile iseulde Owned by: azaozz's profile azaozz
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: TinyMCE Keywords: has-patch commit
Focuses: javascript Cc:

Description

See #31412.

  • Remove all views on hide.
  • Bind and unbind method.

Attachments (16)

31669.patch (1.8 KB) - added by iseulde 8 years ago.
31669.2.patch (2.5 KB) - added by iseulde 8 years ago.
31669.3.patch (3.0 KB) - added by iseulde 8 years ago.
31669.4.patch (5.2 KB) - added by iseulde 8 years ago.
31669.5.patch (7.4 KB) - added by iseulde 8 years ago.
31669.6.patch (6.9 KB) - added by azaozz 8 years ago.
31669.7.patch (7.0 KB) - added by iseulde 8 years ago.
31669.8.patch (7.3 KB) - added by iseulde 8 years ago.
31669.9.patch (7.0 KB) - added by iseulde 8 years ago.
31669.10.patch (7.0 KB) - added by iseulde 8 years ago.
31669.11.patch (8.6 KB) - added by azaozz 8 years ago.
31669.diff (9.1 KB) - added by mattheu 8 years ago.
31669.12.patch (9.0 KB) - added by mattheu 8 years ago.
Try again - Without debug code!
31669.13.patch (8.4 KB) - added by iseulde 8 years ago.
31669.14.patch (8.7 KB) - added by azaozz 8 years ago.
31669.15.patch (2.2 KB) - added by iseulde 8 years ago.

Download all attachments as: .zip

Change History (47)

@iseulde
8 years ago

#1 follow-up: @DrewAPicture
8 years ago

Are these changes not also enhancements as the changes in #31412 were?

#2 in reply to: ↑ 1 @azaozz
8 years ago

Replying to DrewAPicture:

They do both, enhance the code and solve some problems/bugs. For example: possible memory leak from not cleaning up unbound nodes/events. Can happen when switching Visual => Text and back many times.

@iseulde
8 years ago

#3 @iseulde
8 years ago

  • Keywords has-patch added

@iseulde
8 years ago

@iseulde
8 years ago

#4 @iseulde
8 years ago

This should take care of a couple of memory leaks and pause everything (also YouTube etc.) when switching to the text editor.

Last edited 8 years ago by iseulde (previous) (diff)

@iseulde
8 years ago

#5 @iseulde
8 years ago

  • Better hide callback.
  • Makes the BeforeAddUndo callback a bit lighter and undo level content consistent.
  • Let the render method accept content and cache the getContent calls.
  • Unbind methods: see earlier.

@azaozz
8 years ago

#6 @azaozz
8 years ago

In 31669.6.patch: remove views directly in the DOM, not after it is serialized into HTML string.

#7 @iseulde
8 years ago

What's the reason behind the change from iframe.contentWindow to iframe?
I see it's left out in the last patch, but we can improve the BeforeAddUndo callback by only calling emptyViews once.
The rest looks good. :)

#8 follow-up: @iseulde
8 years ago

What's the reason behind the change from iframe.contentWindow to iframe?

Getting jQuery errors, so reverting that.

@iseulde
8 years ago

#9 @iseulde
8 years ago

.7: Also remove markers when removing views. Especially important for undo levels.

@iseulde
8 years ago

#10 @iseulde
8 years ago

.8: Only empty views once in the BeforeAddUndo callback. It's faster and the data is consistent over time.

@iseulde
8 years ago

@iseulde
8 years ago

#11 @iseulde
8 years ago

.10: Unbind views before removing them on hide. Rename the unbind methods.

#12 follow-up: @iseulde
8 years ago

Looking good now I think.

#13 @DrewAPicture
8 years ago

  • Owner set to azaozz
  • Status changed from new to reviewing

#14 in reply to: ↑ 8 @azaozz
8 years ago

Replying to iseulde:

What's the reason behind the change from iframe.contentWindow to iframe?

Getting jQuery errors, so reverting that.

Hmm, didn't see any jQuery errors. iframe.contentWindow is throwing errors in (native) IE8. Seems the iframe exists but there is no contentWindow in it yet. Maybe we are trying to use it too fast? Wasn't there some delay before?

Last edited 8 years ago by azaozz (previous) (diff)

#15 in reply to: ↑ 12 @azaozz
8 years ago

Replying to iseulde:

Yep, looks good. Missed couple of small things (need to pass the root node to resetViews(), etc.). Will fix and test asap :)

@azaozz
8 years ago

#16 @azaozz
8 years ago

In 31669.11.patch:

  • When we are removing markers, we don't need to get the data attr, decode it and insert it replacing the current node. The "marked" <p> already contain the string that the user entered. We only need to remove the data-wpview-marker attribute.
  • Seems the timing errors in IE and sometimes in FF are not happening when we look for stylesheets in the editor head before setting the timeout in setIframes() (don't ask why... magic) :)

#17 @iseulde
8 years ago

need to pass the root node to resetViews()

Is that necessary? If it's not provided mce will fall back to the root, i.e. the body?

#18 @iseulde
8 years ago

But looks good to go. :)

#19 follow-up: @mattheu
8 years ago

This breaks the the core AV view.

https://dl.dropboxusercontent.com/s/747b7mhefdiaoyd/2015-03-30%20at%2023.11%202x.png

Looks like this happend in 31669.11.patch.

<code>head = editor.getDoc().getElementsByTagName( 'head' )[0]</code>

Firstly - I think this should be set to the value of innerHTML.
Secondly - The value of head is always overridden. I think that if this is neccessary it should check whether this has been set already or not.

Adding a patch.

@mattheu
8 years ago

@mattheu
8 years ago

Try again - Without debug code!

#20 in reply to: ↑ 19 @azaozz
8 years ago

Replying to mattheu:

This breaks the the core AV view.

Uh, my bad, that shouldn't override the parent function's head arg. It is the editor iframe head we are looking at there, not the html for the head for the sandbox iframe.

Last edited 8 years ago by azaozz (previous) (diff)

#21 @azaozz
8 years ago

Hmm, there are other problems in these patches too... Multiple sandbox iframes are rendered.

#22 follow-up: @iseulde
8 years ago

Seems the browsers need a bit of time to insert/set the view nodes,

How do I reproduce that?

#23 in reply to: ↑ 22 @azaozz
8 years ago

Replying to iseulde:

Run 31669.10.patch in native IE8 :)

Other problem I see with the patch:

  • Upload an mp3 file.
  • Insert it (audio) in 3 places in the same post.
  • Save draft.

There are 3 iframes in each view, 9 in total.

@iseulde
8 years ago

#24 @iseulde
8 years ago

.13:

  • Don't override head argument
  • Empty the content in the timeout, so it doesn't render iframes twice.

TODO: encoding issue in undo levels, but that was an issue before. Imo it's better to replace the text in the marker node too, this is just temporary anyway.

@azaozz
8 years ago

#25 @azaozz
8 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 32022:

TinyMCE wpView:

  • Fix editor undo levels.
  • Remove views and markers in the DOM before serialization.
  • Unbind views before removing them on hide.
  • Empty the content in the timeout, so it doesn't render iframes twice.

Props iseulde. Fixes #31669.

@iseulde
8 years ago

#26 @iseulde
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[32022] replaced apply with call, but does not pass all arguments.

#27 @azaozz
8 years ago

  • Keywords commit added

31669.15.patch looks good. Ensures the contentNode is passed to all callbacks and normalizes the name. It is a child of the node argument, which should probably be named wrapNode for better readability. The changes mostly affect the callbacks for use in plugins.

This ticket was mentioned in Slack in #core by drew. View the logs.


8 years ago

#29 @helen
8 years ago

@azaozz Going to leave this one for you to commit, given that you're suggesting a rename.

#30 @azaozz
8 years ago

Actually we can rename in 4.3. We will be continuing to work on wpView, structure and names be handled better then.

#31 @azaozz
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 32255:

TinyMCE wpView: ensure contentNode is passed to all callbacks.
Props iseulde. Fixes #31669.

Note: See TracTickets for help on using tickets.