Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove UA sniffing around event.keyCode/charCode for keypress events in Firefox #932

Closed
miketaylr opened this Issue Oct 11, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@miketaylr
Copy link

miketaylr commented Oct 11, 2018

In order to ship window.event in Gecko, we've discovered that supporting event.keyCode for keypress events is required.

Our plan is to essentially do what Chrome does, explained by the first part of the commit message:

Chrome sets both KeyboardEvent.keyCode and KeyboardEvent.charCode of "keypress" event to same value. On the other hand, our traditional behavior is, sets one of them to 0.

Therefore, we need to set keyCode value to charCode value if the keypress event is caused by a non-function key, i.e., it may be a printable key with specific modifier state and/or different keyboard layout for compatibility with Chrome. Similarly, we need to set charCode value to keyCode value if the keypress event is caused by a function key which is not mapped to producing a character.

See https://hg.mozilla.org/mozilla-central/rev/a9db3033d9cb

However, once we landed this we discovered that keyboards shortcuts break in sites using Closure break (GMail, GSuite, Remember The Milk, etc).

Places that need to be updated (possibly more?) to not expect a Firefox UA string will have 0 for event.keyCode or event.charCode in keypress events:

// Mozilla reports the character code in the charCode field.
} else {
keyCode = be.keyCode || this.keyCode_;
charCode = be.charCode || 0;
if (goog.events.KeyHandler.SAVE_ALT_FOR_KEYPRESS_ &&
e.type == goog.events.EventType.KEYPRESS) {
altKey = this.altKey_;
}
// On the Mac, shift-/ triggers a question mark char code and no key code
// (WIN_KEY_FF_LINUX), so we synthesize the latter.
if (goog.userAgent.MAC && charCode == goog.events.KeyCodes.QUESTION_MARK &&
keyCode == goog.events.KeyCodes.WIN_KEY) {
keyCode = goog.events.KeyCodes.SLASH;
}

// If this was a redundant keypress event, we ignore it to avoid double-firing
// an event as the event would've been handled by KEYDOWN. Gecko is currently
// in the process of removing keypress events for non-printable characters
// (https://bugzilla.mozilla.org/show_bug.cgi?id=968056) so we simulate this
// logic here for older Gecko versions which still fire the events.
if (goog.userAgent.GECKO && goog.events.KeyHandler.USES_KEYDOWN_ &&
e.type == goog.events.EventType.KEYPRESS &&
!goog.events.KeyCodes.firesKeyPressEvent(
key, this.lastKey_, e.shiftKey, e.ctrlKey, altKey, e.metaKey)) {
return;
}

goog.events.KeyCodes.normalizeKeyCode = function(keyCode) {
if (goog.userAgent.GECKO) {
return goog.events.KeyCodes.normalizeGeckoKeyCode(keyCode);

@miketaylr

This comment has been minimized.

Copy link
Author

miketaylr commented Oct 11, 2018

@jplaisted

This comment has been minimized.

Copy link
Contributor

jplaisted commented Oct 15, 2018

Hey @miketaylr,

Thanks for reporting this. We've tried testing with Firefox Nightly on both OS X and Linux, enabling the dom.keyboardevent.keypress.set_keycode_and_charcode_to_same_value flag, and are not seeing any obvious issues with keyboard shortcuts on Docs, Slides, and Gmail.

Can you give a more specific repro case? Thanks!

@miketaylr

This comment has been minimized.

Copy link
Author

miketaylr commented Oct 15, 2018

Hi @jplaisted, thanks!

Here's more specific pointers to bugs, with better STR, all assuming dom.keyboardevent.keypress.set_keycode_and_charcode_to_same_value is set to true (which shouldn't require a browser restart):

Gmail

  1. https://bugzilla.mozilla.org/show_bug.cgi?id=1497519

  2. go to gmail

  3. in settings, enable keyboard shortcuts

  4. from your inbox, click on a conversation to give it focus

  5. press x

Expected:
The checkbox for that conversation is filled and it's "selected"

screen shot 2018-10-15 at 1 56 42 pm

Actual:
Nothing happens

Google Docs
In https://bugzilla.mozilla.org/show_bug.cgi?id=1497518#c6

It says the user was not able to type certain keys.

  1. go to google docs, start a new document
  2. try to type the top letters of a QWERTY keyboard: qwertyuiop

expected output:
qwertyuiop

actual output:
eio

Note: I'm testing on OSX 10.13.6, using Firefox Nightly 64.0a1 (2018-10-15) (64-bit)

@miketaylr

This comment has been minimized.

Copy link
Author

miketaylr commented Oct 15, 2018

(ok, i've updated the gmail STR -- i can reproduce it now)

@masayuki-nakano

This comment has been minimized.

Copy link

masayuki-nakano commented Oct 16, 2018

The new behavior can be detected with:

function onKeyPress(event) {
  let keyCode, charCode;
  if (isGecko) {
    if (event.keyCode === event.charCode) {
      if (event.keyCode < 0x20) {
        keyCode = event.keyCode;
        charCode = 0;
      } else {
        keyCode = 0;
        charCode = event.charCode;
      }
    } else {
      keyCode = event.keyCode;
      charCode = event.charCode;
    }
  }
  ...
}

Note that Gecko won't take the new behavior without stopping dispatching non-printable keypress events. So, you need to treat only Enter key press specially if keyCode and charCode are same.

@jplaisted jplaisted closed this in 8da4a93 Oct 18, 2018

@jplaisted

This comment has been minimized.

Copy link
Contributor

jplaisted commented Oct 19, 2018

Thanks again for reporting this, and for the additional info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.