Page MenuHomePhabricator

jQuery 3 breaks template rendering in VE
Closed, ResolvedPublic1 Estimated Story Points

Description

If you edit a simple template, for example {{1x|foo '''bar'''}} in VE and re-render, the top-level text nodes of the template disappear if you are using jQuery 3. In this example, the template renders as "bar" and the "foo" disappears.

Tracing through the problem, it seems to be this line in ve.ce.GeneratedContentNode.js method getRenderedDomElements:

	$rendering = $rendering.not( 'link, style' );

In our example. $rendering would contain [ <text node>, <b node> ] prior to this line.

In jQuery 1.x (ie, in production), this preserves the text nodes, and the result is an array of length 2.

In jQuery 3.2.1, the text node is removed (presumably because it is not an Element) and the result is an array of length 1. This causes the rendering to lose those top-level text nodes.

Event Timeline

cscott created this task.Jun 7 2017, 6:00 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 7 2017, 6:00 PM
cscott added a comment.Jun 7 2017, 6:10 PM

The documentation for jQuery's .not() method says: https://api.jquery.com/not/

Note: When a CSS selector string is passed to .not(), text and comment nodes will always be removed from the resulting jQuery object during the filtering process. When a specific node or array of nodes are provided, text or comment nodes will only be removed from the jQuery object if they match one of the nodes in the filtering array.

which makes it seem like the fault in on VE's side, even though jQuery 1.x behaved differently. May be worth auditing VE's use of .not()...

Change 357656 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[VisualEditor/VisualEditor@master] Don't let jQuery 3 remove top-level text nodes from generated content

https://gerrit.wikimedia.org/r/357656

I was pretty sure I already fixed this...

Looks like I did but forgot to push it for review.

Change 357656 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Don't let jQuery 3 remove top-level text nodes from generated content

https://gerrit.wikimedia.org/r/357656

Jdforrester-WMF closed this task as Resolved.Jun 7 2017, 8:36 PM
Jdforrester-WMF assigned this task to cscott.
Jdforrester-WMF triaged this task as Medium priority.
Jdforrester-WMF set the point value for this task to 1.
Jdforrester-WMF moved this task from To Triage to TR1: Releases on the VisualEditor board.
Jdforrester-WMF removed a project: Patch-For-Review.
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptJun 7 2017, 8:36 PM

Change 357405 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (aec7813fb)

https://gerrit.wikimedia.org/r/357405

Change 357405 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (aec7813fb)

https://gerrit.wikimedia.org/r/357405