Page MenuHomePhabricator

VisualEditor: MWSyntaxHighlight* is badly broken
Open, MediumPublic

Description

ve.dm.MWSyntaxHighlightNode.static.toDataElements:

  • Assumes it always receives two DOM elements, one div and one span. This is dependent on an interaction issue between Parsoid and MediaWiki, and isn't always the case.
  • Stores the span and the div as two separate attributes, rather than a single domElements attribute containing an array
  • Stores the span and the div by reference rather than cloning them

ve.dm.MWSyntaxHighlightNode.static.toDomElements:

  • Assumes attributes.span is always set, although per the above it may be undefined. In case it is undefined, this function returns [<div>, undefined] which ends up crashing VE with a DOM error ("DOM Exception 8") because undefined is fed to appendChild() by the converter
  • Uses attributes.div and attributes.span by reference. This is bad because:
    • calling .appendChild() on a node that's already attached elsewhere removes it from its old location
    • the original nodes received by toDataElements() are in a different document than the output of this function is supposed to be, so we end up attaching nodes to the wrong document
    • the correct way to do this is ve.copyDomElements( originalDomElements, doc ) where doc is the second parameter
  • Unconditionally sets the data-mw attribute, without checking if anything actually changed. This should be avoided, because reordering of keys in the JSON blob may dirty the DOM and trigger a corruption warning. See the last bit of ve.dm.MWReferenceNode.static.toDomElements (dealing with originalMw) for an example of how to do this correctly
    • Note that this .setAttribute() call is once again modifying the original <div> by reference
  • Assumes attributes.div is always set, which won't be true for newly created MWSyntaxHighlightNodes that didn't come from Parsoid but were created by the user in VE. For such nodes, the .setAttribute() call will crash the editor. But we never get there because ...

ve.ui.MWSyntaxHighlightDialog.prototype.onOpen:

  • Assumes that there's always a FocusedNode when the dialog opens. However, this isn't true in creation mode (i.e. when opening the dialog from the toolbar without selecting an existing node), and so opening the dialog in creation mode crashes the editor when trying to do this.sourceNode.getModel(). This means it's impossible to create new MWSyntaxHighlightNodes with VE
  • Note that there's also no if ( this.sourceNode instanceof ve.dm.MWSyntaxHighlightNode ) check, which means this dialog will attempt to edit other types of nodes if it's opened while such a node is focused

Version: unspecified
Severity: normal

Details

Reference
bz55533

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:21 AM
bzimport set Reference to bz55533.

Change 88901 had a related patch set uploaded by Catrope:
Remove SyntaxHighlight from the experimental set

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

Change 88901 merged by jenkins-bot:
Remove SyntaxHighlight from the experimental set

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

Patch to disable SyntaxHighlight is now merged, so this problem will be suppressed with the production push tomorrow morning; restoring to "ASSIGNED".

Another bug discovered:

On every click in the edit area in the dialog, an error appears in the console:

TypeError: e.innerText is undefined
parseInt(e.getAttribute('idx'),10) + e.innerText.length > model);

in VisualEditor/modules/syntaxhighlight/ve.ui.MWSyntaxHighlightSimpleSurface.js
line #1252