views improvements continued

[iseulde]

See #31412.

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

[DrewAPicture]

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

[azaozz]

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]

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

[iseulde]

• 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]

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

[iseulde]

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. :)

[iseulde]

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

Getting jQuery errors, so reverting that.

[iseulde]

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

[iseulde]

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

[iseulde]

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

[iseulde]

Looking good now I think.

[azaozz]

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?

[azaozz]

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]

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) :)

[iseulde]

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?

[iseulde]

But looks good to go. :)

[mattheu]

This breaks the the core AV view.

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]

Try again - Without debug code!

[azaozz]

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.

[azaozz]

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

[iseulde]

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

How do I reproduce that?

[azaozz]

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]

.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]

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]

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

[azaozz]

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.

[helen]

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

[azaozz]

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

[azaozz]

In 32255:

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