Page MenuHomePhabricator

Switching a new Bullet point to Numbered list corrupts user input in VisualEditor
Closed, ResolvedPublic

Description

Reproduce problem

Given a page like the following:

== Section 1 ==

=== Subheading 1 ===

* Bullet 1
* Bullet 2
* Bullet 3

== Section 2 ==
Some text here.
  1. Open the editor (example on test2wiki).
  2. Place cursor after "Bullet 3". Press Enter to create the fourth bullet. Press Tab to make it a sub item of Bullet 3.
  3. In the toolbar, switch from Bullet list to Numbered.
  4. Start typing some text.
step2a.png (821×1 px, 38 KB)
step2b.png (792×1 px, 37 KB)
step4a.png (474×1 px, 9 KB)
Actual result
step4b.png (860×1 px, 44 KB)
reviewchanges.png (1×1 px, 74 KB)

The input visually appears in a different place, below "Bullet 3" but above the created numbered sub item.

More over, this "differenet" place is an impossible place because moving the cursor above or below it in the document, one can not move "in" to this place using the cursor keys (suggesting it isn't really there).

And when attempting to save it, the diff reveals there is indeed something funky going on. This corrupted input ends up serialised in an even stranger way, placing the added word Oops within the previous Bullet 3 line, offset by one character to the left, producing Bulllet Oops3.

Other notes

Logged-in. Firefox on macOS (latest).

Event Timeline

Krinkle renamed this task from Switching to a new Bullet point to Numbered list corrupts user input in VisualEditor to Switching a new Bullet point to Numbered list corrupts user input in VisualEditor.Jan 28 2020, 6:17 PM
matmarex added subscribers: Ryasmeen, dchan, Deskana.
matmarex added a subscriber: matmarex.

I could swear I also filed a bug about this a couple years ago, but I can't find it for the life of me.

This happens because the native selection (document.getSelection().getRangeAt(0)) ends up before the <ol> node, but it should be inside of it (nested in <li> and <p> also). VE doesn't normally allow typing there, and that's why it blows up.

I think this weird native selection is a result of normal native selection handling – when we remove the <ul>, it just gets moved to the closest parent node that is still attached.

ve.ce.Surface should re-apply the selection, but it doesn't. I think this is because our model selection actually doesn't change when changing the list type, so we don't realize we have to re-apply it.

This is similar to T122291, where replacing a DM node like {type: 'heading'} (corresponding to e.g. <h1> in CE) with a {type: 'paragraph'} (<p>) etc. would cause the native selection to end up in silly places. That was fixed by always re-applying the selection after a ContentBranchNode is replaced.

However, in the current problem, the structure doesn't change, so that code doesn't trigger. The only change is in the DOM nodes, not in our CE or DM.

Change 569534 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[VisualEditor/VisualEditor@master] ve.ce.BranchNode: Force re-applying of selection if tag name changes

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

Change 569534 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] ve.ce.BranchNode: Force re-applying of selection if tag name changes

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

Change 570978 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (4ff5f2555)

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

Change 570978 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (4ff5f2555)

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

ppelberg edited projects, added Skipped QA; removed Editing QA.