WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 17 months ago

Last modified 17 months ago

#36434 closed task (blessed) (fixed)

Consider TinyMCE's implementation of `contenteditable="false"` for views

Reported by: iseulde Owned by: iseulde
Milestone: 4.6 Priority: high
Severity: normal Version:
Component: TinyMCE Keywords: has-patch
Focuses: javascript Cc:

Description

This was added to TinyMCE 4.3. See https://www.tinymce.com/docs/plugins/noneditable/.
Inspired by how we're doing it in WordPress, it works very similarly.

Attachments (5)

36434.patch (33.3 KB) - added by iseulde 20 months ago.
36434.2.patch (34.3 KB) - added by iseulde 20 months ago.
36434.3.patch (35.0 KB) - added by iseulde 19 months ago.
36434.4.patch (888 bytes) - added by iseulde 17 months ago.
36434.5.patch (4.7 KB) - added by azaozz 17 months ago.

Download all attachments as: .zip

Change History (34)

#1 @johnbillion
21 months ago

  • Summary changed from Consider the new non-enditable plugin for views to Consider the new non-editable plugin for views
  • Type changed from defect (bug) to enhancement

This ticket was mentioned in Slack in #core-editor by azaozz. View the logs.


20 months ago

#3 @iseulde
20 months ago

  • Milestone changed from Future Release to 4.6

#4 @iseulde
20 months ago

  • Summary changed from Consider the new non-editable plugin for views to Consider TinyMCE's implementation of `contenteditable="false"` for views

@iseulde
20 months ago

#5 @iseulde
20 months ago

  • Keywords has-patch added; needs-patch removed

To do:

  • Solve unbinding.
  • Set cursor on paste.
  • Set cursor/selection on first focus (if the view is first in the DOM).
  • Toolbar position does not update.

Bugs:

  • Cursor before view + delete key creates new paragraph.

Differences:

  • Typing, pasting, commands etc. don't work when a view is selected. We deleted the view, as an image would be deleted when it's selected.

@iseulde
20 months ago

#6 @iseulde
20 months ago

Not sure how to solve unbinding. Now we provide the node and unbinding happens before removal. If we switch to some kind of cleanup instead, running after the node's been removed, some information necessary to unbind is missing.

E.g.

In the case were we don't use iframes, and have mejs.

We need the ID associated with the node to remove the instance from the global object.

window.mejs.players[ $( node ).find( '.mejs-container' ).get( 0 ).id ].remove()

Maybe in this case the view instance needs to save IDs associated with its nodes, and check which nodes are missing on cleanup.

This ticket was mentioned in Slack in #core-editor by iseulde. View the logs.


19 months ago

@iseulde
19 months ago

#8 @iseulde
19 months ago

In 37446:

TinyMCE: use new non-editable implementation

Use TinyMCE's implementation of contenteditable="false" for views.

First pass.

See #36434.

This ticket was mentioned in Slack in #core-editor by iseulde. View the logs.


19 months ago

#10 @iseulde
19 months ago

  • Owner set to iseulde
  • Status changed from new to accepted

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


18 months ago

This ticket was mentioned in Slack in #core-tinymce by iseulde. View the logs.


18 months ago

#13 @azaozz
18 months ago

  • Type changed from enhancement to task (blessed)

Waiting for addition of an improvement to handle unloading from upstream. Should be ready soon.

#14 @bduclos
18 months ago

There's an issue with the new code when I try to copy paste a shortcode view containing HTML code.
e.g: I have the following shortcode:

[toggle title="Toggle Title"]<h1>Lorem ipsum</h1>[/toggle]

in Visual Mode, copy the view and paste it, it generates the following view:

http://i.imgur.com/dYqUr8P.gif

Switching back and forth between the Text and Visual mode restores the correct view.

The issue seems to come from the code inside wp-includes/js/tinymce/plugins/wpview/plugin.js line 90 (removing it fixes this issue):

// Make sure views are copied as their text.
editor.on( 'drop objectselected', function( event ) {
    if ( isView( event.targetClone ) ) {
	 event.targetClone = editor.getDoc().createTextNode(
		window.decodeURIComponent( editor.dom.getAttrib( event.targetClone, 'data-wpview-text' ) )
	);
    }
} );

#15 @ocean90
17 months ago

  • Priority changed from normal to high

Related: #37320, #37258


Replying to azaozz:

Waiting for addition of an improvement to handle unloading from upstream. Should be ready soon.

Is there an issue which we can track? Or was it part of the TinyMCE update to 4.3.13 in [37910]?

This ticket was mentioned in Slack in #core-editor by iseulde. View the logs.


17 months ago

@iseulde
17 months ago

#17 @iseulde
17 months ago

#37258 was marked as a duplicate.

#18 @iseulde
17 months ago

#37320 was marked as a duplicate.

#19 @iseulde
17 months ago

@programmin would a patch like 36434.4.patch help? So restoring the old -wrap class and passing the node as a third parameter as well, to prevent errors?

But you are right, there should be a note for developers in either case making them aware of the changes.

#20 @iseulde
17 months ago

@ocean90 It is fixed in 4.4.0... See https://github.com/tinymce/tinymce/blob/master/changelog.txt line 5 and 6.
(which is not in trunk)

Last edited 17 months ago by iseulde (previous) (diff)

#21 @rachelbaker
17 months ago

@iseulde TinyMCE was updated to 4.4.0 in #37327/[38034].

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


17 months ago

#23 @ocean90
17 months ago

@programmin Did you get a chance to test 36434.4.patch?

This ticket was mentioned in Slack in #core-editor by ocean90. View the logs.


17 months ago

#25 @iseulde
17 months ago

As discussed with @azaozz, we think it is better to noop the unbind method and remove the event. Before this change it was very unreliable, and it's not clear how it should be done now. In any case it is best that views are iframed. All of core's views are iframed and the api encourages iframing.

@azaozz
17 months ago

#26 @azaozz
17 months ago

In 36434.5.patch:

  • Add the back-compat changes from 36434.4.patch.
  • Restore changing of the views iframes body classes when the editor body class name changes.
  • Bind the mutation observer used to resize the view iframes to the iframe window. That prevents eventual memory leak when the view is deleted.
  • Stop triggering/remove the wp-mce-view-unbind event as previously discussed.

Thinking we can keep the unbind method for now. In theory it can still be used to unbind before the view node is removed by the user (if a plugin ever needs that). This will need another look in 4.7.

Last edited 17 months ago by azaozz (previous) (diff)

#27 follow-up: @bduclos
17 months ago

Still having issue when copy paste a shortcode view containing HTML tags. It was working in WP 4.5.3
The issue is line 119 of wp-includes/js/tinymce/plugins/wpview/plugin.js which creates a text node instead of an html node.

event.targetClone = editor.getDoc().createTextNode(
     window.decodeURIComponent( editor.dom.getAttrib( event.targetClone, 'data-wpview-text' ) )
);

#28 @azaozz
17 months ago

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

In 38158:

TinyMCE, wpView:

  • Add the wpview-wrap class and pass third param to the getNodes() callback for back-compat.
  • Attach the mutation observer that resizes a view iframe inside the iframe to minimize memory use/leaks.
  • Remove the wp-mce-view-unbind event. It has never been particularly reliable and now it doesn't fire when the user deletes a view by typing or pasting over it.
  • Restore changing of a view iframe body classes when the editor body classes change.

Props iseulde, azaozz.
Fixes #36434.

#29 in reply to: ↑ 27 @azaozz
17 months ago

Replying to bduclos:

Still having issue when copy paste a shortcode view containing HTML tags.

Could you open a new ticket for this. I can see where it gets converted to text, just don't want to introduce potential security problem as the browsers will parse the inserted HTML.

Note: See TracTickets for help on using tickets.