Last modified: 2014-04-16 01:48:16 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T57533, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 55533 - VisualEditor: MWSyntaxHighlight* is badly broken
VisualEditor: MWSyntaxHighlight* is badly broken
Status: ASSIGNED
Product: VisualEditor
Classification: Unclassified
Editing Tools (Other open bugs)
unspecified
All All
: Normal normal
: ---
Assigned To: Editing team bugs – take if you're interested!
:
Depends on: 55934 55935 56780
Blocks: 55453
  Show dependency treegraph
 
Reported: 2013-10-09 22:45 UTC by Roan Kattouw
Modified: 2014-04-16 01:48 UTC (History)
8 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Roan Kattouw 2013-10-09 22:45:06 UTC
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
Comment 1 Gerrit Notification Bot 2013-10-09 23:30:03 UTC
Change 88901 had a related patch set uploaded by Catrope:
Remove SyntaxHighlight from the experimental set

https://gerrit.wikimedia.org/r/88901
Comment 2 Gerrit Notification Bot 2013-10-09 23:36:38 UTC
Change 88901 merged by jenkins-bot:
Remove SyntaxHighlight from the experimental set

https://gerrit.wikimedia.org/r/88901
Comment 3 James Forrester 2013-10-10 02:08:38 UTC
Patch to disable SyntaxHighlight is now merged, so this problem will be suppressed with the production push tomorrow morning; restoring to "ASSIGNED".
Comment 4 Moriel Schottlender 2013-11-08 21:50:47 UTC
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

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links