Page MenuHomePhabricator

Backspacing multiple VE paragraphs in Gboard causes model corruption
Open, LowPublic

Description

Steps to reproduce

  1. In Android Chrome using Gboard in English, open the VE standalone h1 demo (e.g. go to https://rawcdn.githack.com/wikimedia/VisualEditor/4669eff2bd4441da2b0c3e19cecafdbc2bcc59b8/demos/ve/mobile.html#!h1 ).
  1. The document should read "abcdefg". Put the cursor after the "d" and press Enter to break the paragraph.
  1. Turn on "Show model" and "Update on change".
  1. Make a selection that crosses the paragraph break (e.g. select "cd" and "ef"). Press backspace.

Expected behaviour: the selected content is removed and the paragraphs are unified.

Actual behaviour: an extra copy of the second paragraph is left behind.

I would guess this is due to the details of the HTML tree changes and DOM events caused by pressing backspace in Gboard in some languages. It happens on mobile Firefox too. It occurs with Gboard English and Welsh, but not with Gboard Cantonese Jyutping. Other input methods seem unaffected.

Event Timeline

Ok, with Gboard English, this is what happens when you select "de</h1><h1>fg" and press backspace:

  1. The selection collapses to the end
  2. A keydown event with an unknown keycode is fired
  3. The content is deleted (unifying the paragraphs)

This means our code misunderstands the change as regards the first paragraph, because we make the assumption that keystrokes can only trigger native DOM changes in paragraphs touched by the selection.

The behaviour with Gboard Cantonese Jyutping is different. The backspace key triggers an actual keycode=8 keydown event, and so our code suppresses the event and performs the deletion itself, just as it would on desktop with no IME.

Change 498528 had a related patch set uploaded (by Divec; owner: Divec):
[VisualEditor/VisualEditor@master] WIP: Call handleInsertion on beforeinput

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

I am reasonably satisfied with Change 498528 now. I cannot find a case where this breaks input, having tested on multiple types of IME, on Chromium, Firefox and Safari, and on Android, iOS, Windows, Linux and MacOS. Of course, there are almost limitless combinations, so it's always possible there is some breakage I've not managed to find.

It remains to be discussed whether we're willing to merge code that depends on beforeinput, which is experimental (currently implemented in webkit browsers, i.e. Chrome and Safari). See https://developer.mozilla.org/en-US/docs/Web/Events/beforeinput .

Note that the change adds beforeinput to the set of events watched by the ve.EventSequencer. In theory this can cause after-event handlers to run slightly earlier. I don't *believe* this has a visible effect in practice, but technically the change is not quite as simple as just changing whether we listen to beforeinput or not.

I think we'll just have to try it and keep an eye out for new IME bugs.

Change 498528 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Call handleInsertion on beforeinput

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

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

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

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

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

matmarex added a project: Skipped QA.

I assume this should be fixed by this patch.

Since David already did extensive testing, we should probably skip formal QA.

Change 523770 had a related patch set uploaded (by Divec; owner: Esanders):
[VisualEditor/VisualEditor@master] Revert "Call handleInsertion on beforeinput"

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

Change 523770 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Revert "Call handleInsertion on beforeinput"

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

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

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

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

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

Removing task assignee due to inactivity, as this open task has been assigned for more than two years (see emails sent to assignee on May26 and Jun17, and T270544). Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be very welcome!

(See https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator.)