Page MenuHomePhabricator

VisualEditor: Pawning prevention code in ve.ce.Surface sometimes prevents some legit keypresses from being handled (e.g. typing “ą” in Polish on a new line)
Closed, ResolvedPublic

Description

Writing https://gerrit.wikimedia.org/r/143322 , I realized that the ve.ce.isShortcutKey( e ) check in ve.ce.Surface (which rejects every keypress event where Ctrl or Cmd is pressed) sometimes prevent legit keypresses from being handled.

To reproduce (in any browser):

  • Place cursor on an empty line.
  • Input 'ą' ([[ą]]). This is accomplished by pressing AltGr+a (right Alt+a, Alt+Ctrl+a) when using the "Polish (Programmers)" keyboard layout on Windows 7 (this is the default layout for Polish).
  • Type in something else. The 'ą' disappears, because it was never handled by ve.ce, only by native contenteditable element.

I only tested this on Windows 7, but I believe other operating systems all use the same method. Other letters with diacritics from the [[Polish alphabet]] are input in the same way (AltGr + base key), we have nine of them.

On Windows 7 you can enable this keyboard layout by typing "keyboard layout" into the Start menu, choosing "Change keyboards…" and adding the input method.


Version: unspecified
Severity: normal

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:41 AM
bzimport set Reference to bz67367.

Is this now fixed, given that we've killed pawns?

(In reply to James Forrester from comment #1)

Is this now fixed, given that we've killed pawns?

We haven't quite killed them yet, the chimera commit does that.

(In reply to Roan Kattouw from comment #2)

(In reply to James Forrester from comment #1)

Is this now fixed, given that we've killed pawns?

We haven't quite killed them yet, the chimera commit does that.

But we should make sure that that commit also kills this logic; which right now I'm not sure that it does.

I can still reproduce on betalabs.

Hmph, we can't kill this code yet. It's not just to prevent pawning, it's also to prevent the selection from being deleted when you press a shortcut key. We'll need David's next project (prepare-observe-fixup) to be done before we can get rid of this, I think.

This no longer happens. Just for the kicks I bisected and found that the commit that fixed the bug is 40101e42a4544768ac624ef2b4da0e7d44b69b65 (https://gerrit.wikimedia.org/r/#/c/151046/).

I wonder if we could have a browser test to prevent this from regressing? (It was fixed accidentally, could be broken accidentally again.)

I also wonder if we need the checks in ve.ce.Surface.prototype.onDocumentKeyPress at all anymore? Removing them doesn't seem to cause the issue they're supposed to prevent ("Unexpected content deletion when selection is not collapsed and the user presses, for example, the Home key (Firefox fires 'keypress' for it)").

Yes! See fakeIMETests. Please use the script to generate a previously-failing test case so we lock this in. :-)

Change 181173 had a related patch set uploaded (by Bartosz Dziewoński):
imetests: Add a regression test for Polish keyboard input

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

Patch-For-Review

Change 181174 had a related patch set uploaded (by Bartosz Dziewoński):
ve.ce: Remove hacks from Surface#onDocumentKeyPress, add related IME test

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

Patch-For-Review

Change 181173 merged by jenkins-bot:
imetests: Add a regression test for Polish keyboard input

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

Change 181174 merged by jenkins-bot:
ve.ce: Remove hacks from Surface#onDocumentKeyPress; add related IME test

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