Page MenuHomePhabricator

Annotations don't merge properly, because their index hashes include original DOM element contents
Closed, ResolvedPublic8 Estimated Story Points

Description

Steps to reproduce: In VE standalone, edit <h1><i>abc</i>X<i>def</i><h1> , delete the 'X', then switch back to source mode .

Expected value: <h1><i>abcdef</i></h1>
Observed value: <h1><i>abc</i><i>def</i></h1>

This happens because an annotation data element generated by ve.dm.Converter contains an originalDomElementsIndex property, which is the index for the hashStore item that holds the DOM element from which it was generated. Currently, that index is generated using store.index( domElements, domElements.map( ve.getNodeHtml ).join( '' ) , which means the innerHTML content of the DOM element gets included in the hash string -- so <b>foo</b> indexes differently to <b>bar</b>.

Including the innerHTML content doesn't give any reliable guarantees of DOM element uniqueness (because a document can contain <b>foo</b>...<b>foo</b>). So unless I'm missing something, ve.dm.Converter should omit the innerHTML of a ve.dm.Annotation's DOM element from the hash string.

Event Timeline

dchan created this task.Mar 18 2017, 8:21 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 18 2017, 8:21 PM

Change 343458 had a related patch set uploaded (by Divec):
[VisualEditor/VisualEditor] When indexing originalDomElements for ve.dm.Annotations, disregard child nodes

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

Jdforrester-WMF triaged this task as High priority.Mar 18 2017, 10:25 PM
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.
Esanders updated the task description. (Show Details)Mar 20 2017, 10:27 AM

Change 343458 merged by jenkins-bot:
[VisualEditor/VisualEditor] When indexing originalDomElements for ve.dm.Annotations, disregard child nodes

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

Jdforrester-WMF closed this task as Resolved.Mar 21 2017, 4:52 PM
Jdforrester-WMF changed the point value for this task from 1 to 8.
Jdforrester-WMF removed a project: Patch-For-Review.
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptMar 21 2017, 4:52 PM

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

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

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

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