Page MenuHomePhabricator

VisualEditor: Copying references is completely broken
Closed, ResolvedPublic

Description

Comment to follow from Roan.


Version: unspecified
Severity: critical

Details

Reference
bz60117

Event Timeline

bzimport raised the priority of this task from to Unbreak Now!.Nov 22 2014, 2:58 AM
bzimport set Reference to bz60117.

Well, not *completely*, just mostly :)

Go to https://www.mediawiki.org/w/index.php?title=VisualEditor:TestReferenceContents&veaction=edit and select reference [3] by clicking on it, then press Ctrl+C. You'll get a JS error.

There are several things wrong here:

In findEndOfNode() inside of ve.dm.Converter.prototype.getDomSubtreeFromData:

  • Line 1172 tries to throw an error using dataElement.type, but dataElement isn't set anywhere, so the generation of the error itself throws an error

In ve.ce.Surface.prototype.onCopy:

  • On line 693, cloneSliceFromRange() returns a slice with unbalanced data: [ {type:'paragraph'}, {type:'mwReference'}, {type:'/mwReference'}, {type:'internalList'}, ..., {type:'/internalList'} ]

In ve.dm.Document.prototype.cloneSliceFromRange:

  • On both lines 378 and 431, selectNodes() for a range exactly covering a reference returns the text node as well. In my test case, the return value is [ { node: TextNode, range: [69,69] }, { node: ReferenceNode } ]. This is clearly a bug in selectNodes()
  • Even if selection was correctly set to something of length 1, line 388 does not allow for selection[0].range to be undefined, but that's actually expected when a node is completely selected. This case should be handled, and selection[0].nodeRange should be checked against range instead in that case. So something like else if ( selection.length === 1 && range.equalsSelection( selection[0].range || selection[0].nodeRange ) ) . Otherwise, even if we fixed the selectNodes bug, the selection.length===1 code path still wouldn't be taken.
  • Because selection.length>1, the else path is taken, and we populate balanceOpenings and balanceClosings. These two are populated separately in a way that's broken: because the first node is a text node (not wrapped) we add an opening for that node's parent, but because the second node is a reference node (wrapped), we don't add a closing after it. The result is unbalanced output. This code seems to wrongly assume that leaf nodes are always unwrapped.

I forgot to include how these things lead to the actual failure:

  • cloneSliceFromRange returns a slice with an unclosed paragraph opening at the very beginning, as described above
  • The tree build algorithm in ve.dm.Document doesn't actually care about the unclosed opening and builds the tree as if the mwReference was the first node. This means we get a mostly valid tree except that all the offsets are off by one because the paragraph opening element was ignored
  • When we try to get the contents of the internalItem associated with the reference from this unbalanced slice, the range in the tree is one off from the range in the data array. And so instead of [ {type:'paragraph'}, 'F', 'o', 'o', {type:'/paragraph'} ] we get [ {type:'internalItem'}, {type:'paragraph'}, 'F', 'o', 'o' ] when calling .getFullData( itemNodeRange, true ) in ve.dm.MWReferenceNode.static.toDomElements on line 135 (it's actually written as .getFullData( new ve.Range( itemNodeRange.start, itemNodeRange.end ), true ) which is just WTF and should be simplified)
  • This data with two openings and zero closings then gets fed to getDomSubtreeFromData(), which calls removeInternalNodes(), which spots an internalItem as the very first element and tries to find its end with findEndOfNode()
  • findEndOfNode() can't find the internalItem closing because there are no closings, so it tries to throw an error, but that fails because dataElement is undefined and so accessing dataElement.type is itself an error

Based on these findings, my expectation of what will and won't work is:

Probably broken:

  • Copying a reference by itself, where that reference is immediately preceded by text in the same paragraph (foo|[1]|bar)
  • Copying a reference plus some text before it (f|oo[1]|bar)
  • Copying a reference plus some text after it, where there is no text immediately preceding that reference in the same paragraph (|[1]ba|r)

Probably works:

  • Copying a reference plus some text after it, where that reference is immediately preceded by text in the same paragraph (foo|[1]ba|r)
  • Copying a reference plus text before and after it (f|oo[1]ba|r)
  • Copying a reference by itself, where there is no text immediately preceding that reference in the same paragraph (|[1]|bar)

Change 108692 had a related patch set uploaded by Esanders:
Fix exception thrown by findEndOfNode

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

Change 108692 merged by jenkins-bot:
Fix exception thrown by findEndOfNode

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

Change 108720 had a related patch set uploaded by Esanders:
Fix up result of selectNodes to remove empty items

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

Change 108720 merged by jenkins-bot:
Fix up result of selectNodes

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

Change 109481 had a related patch set uploaded by Esanders:
Fix balancing of data in cloneSliceFromRange

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

Change 109481 merged by jenkins-bot:
Fix balancing of data in cloneSliceFromRange

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

The following JS error appears if I copy a reference note with some text following it.
Uncaught Error: Unbalanced input passed to document

This patch may not have been merged into ve-mw yet. Please check commit history.

Checked the cases mentioned above.All of them are working now on Betalabs.No JS error is appearing now after copy -paste of reference.