Page MenuHomePhabricator

DM/DOM sync can break if you delete annotated text then type
Closed, ResolvedPublic8 Story Points

Description

  • In VE standalone, edit <h1><u>x</u>y</h1> (So the CBN innerHTML is '<u class="ve-ce-textStyleAnnotation ve-ce-underlineAnnotation">x</u>y')
  • Inspect the 'x' in the debugger, then click to its left to restore the cursor
  • Press Delete. The <u> and its contents should disappear from the DOM, and the 'x' should disappear from the DM.
  • Press 'z'. The DM infers an unannotated 'z' but the DOM CBN innerHTML is '<u>z</u>y' (i.e. a bare <u> node with no ve-ce-* class).

In Firefox, the bare <u> node is only inserted if the just-deleted text contained an entire <u ...> node. In Chromium, it's worse - a <u> node is inserted if the just-deleted text was the entirety of a node whose computed style was underlined. This issue isn't particular to underlining: the same quirk can create other tags e.g. <b> and <i> (and on Chromium, <font> and <span style=...> - see https://code.google.com/p/chromium/issues/detail?id=546461 ).

We don't currently detect the DOM inline tag, because ve.ce.modelChangeFromContentChange works on the plaintext, adding annotations heuristically based on the existing annotations that neighbour the diff boundaries. If we actually apply the browser richtext ( https://gerrit.wikimedia.org/r/247800 ) that will technically resolve this issue, but as the Chromium examples linked above show, it will leave us wide open to replacing semantically meaningful spans with tag-styled mush that looks confusingly similar. We obviously don't want that to happen: even losing the spans completely is less bad, because it's visually obvious and the editor can fix it.

Event Timeline

dchan created this task.Oct 22 2015, 9:25 AM
dchan claimed this task.
dchan raised the priority of this task from to Normal.
dchan updated the task description. (Show Details)
dchan added a subscriber: Esanders.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 22 2015, 9:25 AM

Change 248012 had a related patch set uploaded (by Divec):
Reapply the cursor after delete to kill browser preannotations

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

Jdforrester-WMF set Security to None.
Jdforrester-WMF edited a custom field.

Change 248012 merged by jenkins-bot:
Reapply the cursor after delete to kill browser preannotations

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