501496 - preventDefault on keydown does not cancel following keypress

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Emil A Eklund]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.10) Gecko/2009042513 Ubuntu/8.04 (hardy) Firefox/3.0.10
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.10) Gecko/2009042513 Ubuntu/8.04 (hardy) Firefox/3.0.10

Calling preventDefault on a keydown event does not cancel the following keypress event. This is different from the behavior in IE (6/7/8) and WebKit (Safari/Chrome).

Tested on ff2/x11, ff3/x11 and ff3/win, all exhibit the same behavior. See linked test case for an example.

Reproducible: Always

Steps to Reproduce:
1. Open linked test case.
2. Focus textarea.
3. Hit enter
Actual Results:  
keypress event handler for document is triggered even though preventDefault was called on the keydown event.

Expected Results:  
keypress event handler for document should not be triggered.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This is probably a dup of some other bug, and note, there
isn't any specification for key events!

Comment 2

[Emil A Eklund]

I found a bunch of existing bugs mentioning keydown and preventdefault, 500392 beeing the closest match. That bug is about inconsistent behavior across platforms with regards to preventing key input though rather than the relationship between keydown and keypress.

I realize that there's no specification for key events but the policy so far has been to either try to do the right thing or to match the behavior of other browsers. 

Thanks for listening.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that this is invalid.

http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-keyboard-event-order

Looks like the keypress event is existing in DOM3 Events only for compatibility with current web contents because there is no difference between keydown and keypress event in new properties. If so, it doesn't make sense that we change the behavior.

But I'm not sure whether it is correct that we're setting prevent default flag to keypress event if the relevant keydown event was prevented the default because this could make compatibility problem with other browsers.

Comment 4

[gdennis]

Under the keydown event, the specification reads: "if the key is associated with a character, the default action must be to dispatch a keypress event".
http://www.w3.org/TR/DOM-Level-3-Events/#event-type-keydown

From my reading, the implication is that is the keydown has defaultPrevented, no keypress should be dispatched. It is extra strange that this keypress always has defaultPrevented regardless of what the applications keypress handler does.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, right.

http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#event-type-keydown
> If this event is canceled, the associated event types must not be dispatched, and the associated actions must not be performed.

However, anyway, we cannot fix this bug soon. I have a plan for reimplementing keypress event but it needs a lot of work. E.g., I need to research all keypress event handlers in layout, content and XUL.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Windows

I'll request review to each widget review after smaug agrees with this change.

This patch is for both Desktop and Metro. Now, keyboard event handling is completely shared between them.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on GTK

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Cocoa

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Qt

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on OS/2

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Android

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.7 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Gonk

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.8 Native key event tests should prevent default only when the event is keypress

This fixes the new orange of test_keycodes.xul.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.9 Remove nsIDOMWindowUtils::KEY_FLAG_PREVENT_DEFAULT and don't synthesize keypress event if precede keydown event is consumed

Like widget dispatched event, nsIDOMWindowUtils::SendKeyEvent() callers should not call keypress event if keydown event is consumed.

Then, nsIDOMWindowUtils::KEY_FLAG_PREVENT_DEFAULT isn't necessary. We can remove it.

The test_bug348236.html is strange. I'm not sure what's being tested with this code. I requested a needs info in bug 348236.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.10 nsXULPopupManager should handle keydown event as far as possible and don't prevent its default if it needs to handle keypress event

nsXULPopupManager listens both keydown and keypress. The keydown event handler is only used for handling modifier key events since modifier keys don't cause keypress event.

However, the keydown event handler call preventDefault() for all key events. And keypress event handler doesn't check defaultPrevented value (the reason is documented by the comment in KeyPress(), though). This is very bad manner.

Basically, non-printable keys which don't generate charCode for keypress event should be handled by keydown event handler.

This patch moves the keyCode checking code in KeyPress() to HandleKeyboardEventWithKeyCode() and this is called by both KeyDown() and KeyPress(). If this isn't handle the key event, KeyDown() now doesn't call preventDefault().

This fixes a lot of oranges.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.11 <browser> should handle keydown event instead of keypress event during auto scroll

This is similar to part.10 and also related to it.

Esc key should be handled with keydown event instead of keypress event while auto scroll.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.12 tilt-visualizer should use keydown event handler instead of keypress event handler for Escape key

Similar to part.10 and part.11, escape key should be handled in keydown event handler. Then, we can remove the keypress event handler.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

You can get test builds from here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=354d8d102bbc

Comment 20

[Neil Deakin]

Comment on attachment 771211 [details] [diff] [review]
part.10 nsXULPopupManager should handle keydown event as far as possible and don't prevent its default if it needs to handle keypress event

>-  // Since a menu was open, eat the event to keep other event
>-  // listeners from becoming confused.
>-  aKeyEvent->StopPropagation();
>-  aKeyEvent->PreventDefault();
>-  return NS_OK; // I am consuming event
>+  HandleKeyboardEventWithKeyCode(aKeyEvent, item);
>+  return NS_OK;

Why do we need to call HandleKeyboardEventWithKeyCode in both KeyDown and KeyPress?

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Neil Deakin from comment #20)
> Comment on attachment 771211 [details] [diff] [review]
> part.10 nsXULPopupManager should handle keydown event as far as possible and
> don't prevent its default if it needs to handle keypress event
> 
> >-  // Since a menu was open, eat the event to keep other event
> >-  // listeners from becoming confused.
> >-  aKeyEvent->StopPropagation();
> >-  aKeyEvent->PreventDefault();
> >-  return NS_OK; // I am consuming event
> >+  HandleKeyboardEventWithKeyCode(aKeyEvent, item);
> >+  return NS_OK;
> 
> Why do we need to call HandleKeyboardEventWithKeyCode in both KeyDown and
> KeyPress?

Just for the risk management. If you don't like it, I think that it's OK KeyPress() not to call it. This patch keeps the compatibility in some edge cases, e.g., some Add-ons *might* synthesize only keypress event for controlling nsXULPopupManager. However, yes, it must be very rare case.

Comment 22

[Neil Deakin]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> Just for the risk management. If you don't like it, I think that it's OK
> KeyPress() not to call it. This patch keeps the compatibility in some edge
> cases, e.g., some Add-ons *might* synthesize only keypress event for
> controlling nsXULPopupManager. However, yes, it must be very rare case.

I don't see any reason we need to support that. I'd rather the code as simpler.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Neil Deakin from comment #22)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> > Just for the risk management. If you don't like it, I think that it's OK
> > KeyPress() not to call it. This patch keeps the compatibility in some edge
> > cases, e.g., some Add-ons *might* synthesize only keypress event for
> > controlling nsXULPopupManager. However, yes, it must be very rare case.
> 
> I don't see any reason we need to support that. I'd rather the code as
> simpler.

Okay, I'll update the patch tomorrow.

Comment 24

[Victor Porof [:vporof][:vp]]

Comment on attachment 771215 [details] [diff] [review]
part.12 tilt-visualizer should use keydown event handler instead of keypress event handler for Escape key

Review of attachment 771215 [details] [diff] [review]:
-----------------------------------------------------------------

Should be fine. r+ if green on try.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #23)
> (In reply to Neil Deakin from comment #22)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> > > Just for the risk management. If you don't like it, I think that it's OK
> > > KeyPress() not to call it. This patch keeps the compatibility in some edge
> > > cases, e.g., some Add-ons *might* synthesize only keypress event for
> > > controlling nsXULPopupManager. However, yes, it must be very rare case.
> > 
> > I don't see any reason we need to support that. I'd rather the code as
> > simpler.
> 
> Okay, I'll update the patch tomorrow.

Hmm, I was wrong. It's necessary. HandleKeyboardEventWithKeyCode() does NOT consume the key event whose key code is not in the switch statement even when (mPopups || mActiveMenuBar) is true. In this case, KeyPress() is called and then, KeyPress() doesn't check correctly whether the keypress event should be consumed with HandleKeyboardEventWithKeyCode().

So, some necessary key event for other UI may be consumed if we don't call HandleKeyboardEventWithKeyCode().

So, the current patch is correct approach. Would you review it again?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Windows

merged with the patch for bug 891292.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Reviews coming today or tomorrow.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 772603 [details] [diff] [review]
part.1 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Windows


>@@ -2272,23 +2268,26 @@ KeyboardLayout::SynthesizeNativeKeyEvent
>       if (chars.IsEmpty()) {
>         NativeKey nativeKey(aWidget, keyDownMsg, modKeyState);
>         nativeKey.HandleKeyDownMessage();
>       } else {
>         NativeKey::FakeCharMsg fakeMsgForKeyDown = { chars.CharAt(0), scanCode,
>                                                      makeDeadCharMsg };
>         NativeKey nativeKey(aWidget, keyDownMsg, modKeyState,
>                             &fakeMsgForKeyDown);
>-        nativeKey.HandleKeyDownMessage();
>-        for (uint32_t j = 1; j < chars.Length(); j++) {
>-          NativeKey::FakeCharMsg fakeMsgForChar = { chars.CharAt(j), scanCode,
>-                                                    false };
>-          MSG charMsg = fakeMsgForChar.GetCharMsg(aWidget->GetWindowHandle());
>-          NativeKey nativeKey(aWidget, charMsg, modKeyState);
>-          nativeKey.HandleCharMessage(charMsg);
>+        bool dispatched, keyDownDefaultPrevented;
I'd prefer having these variables initialized to false


This needs another review from a Windows widget peer

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 771201 [details] [diff] [review]
part.2 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on GTK

I wish also karlt could look at this one. At least to let him know about the change.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 771202 [details] [diff] [review]
part.3 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Cocoa

This needs OSX widget review.

_Might_ be nice to have some tiny helper for
  currentKeyEvent->mKeyPressHandled = DispatchEvent(keypressEvent);
  currentKeyEvent->mKeyPressDispatched = true;
but perhaps it doesn't matter.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 771203 [details] [diff] [review]
part.4 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Qt

romaxa might want to look at this.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 771204 [details] [diff] [review]
part.5 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on OS/2

huh, we still have OS/2

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 771205 [details] [diff] [review]
part.6 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Android

>--- a/widget/android/nsWindow.cpp
>+++ b/widget/android/nsWindow.cpp
>@@ -1662,16 +1662,17 @@ nsWindow::HandleSpecialKey(AndroidGeckoE
>             case AKEYCODE_MENU:
>                 gMenu = true;
>                 gMenuConsumed = isLongPress;
>                 break;
>         }
>     } else {
>         switch (keyCode) {
>             case AKEYCODE_BACK: {
>+                // XXX Where is the keydown event for this??
>                 nsKeyEvent pressEvent(true, NS_KEY_PRESS, this);

Uh, please file a bug about this. CC whoever ones Android widget level code these days.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 771209 [details] [diff] [review]
part.9 Remove nsIDOMWindowUtils::KEY_FLAG_PREVENT_DEFAULT and don't synthesize keypress event if precede keydown event is consumed

>+++ b/dom/base/nsDOMWindowUtils.cpp
>@@ -993,20 +993,16 @@ nsDOMWindowUtils::SendKeyEvent(const nsA
>           break;
>       }
>       break;
>   }
> 
>   event.refPoint.x = event.refPoint.y = 0;
>   event.time = PR_IntervalNow();
> 
>-  if (aAdditionalFlags & KEY_FLAG_PREVENT_DEFAULT) {
>-    event.mFlags.mDefaultPrevented = true;
>-  }
Could we just keep the flag for testing?



>-  // If this is set, preventDefault() the event before dispatch.
>-  const unsigned long KEY_FLAG_PREVENT_DEFAULT   = 0x0001;
ditto.

It might be useful in some cases, and there isn't really need to remove that stuff.


r+ for the test changes, but I'd keep the nsIDOMWindowUtils.idl/cpp as they are now, so sr-

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 772603 [details] [diff] [review]
part.1 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Windows

If the WM_KEYDOWN message causes WM_CHAR, we need to remove all following WM_CHAR messages if the keydown event's default is prevented default.

Additionally, we don't need to dispatch default prevented keypress event anymore, therefore, we can remove the extra flag argument from a lot of methods.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 771202 [details] [diff] [review]
part.3 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Cocoa

I think that if a native keydown event causes other keydown events, we shouldn't dispatch keypress event for the original keydown event because it's already done its default action (by using other key events), so, handled keypress event for it must cause making the user confused.
# we don't confirm such case except Japanese IME's special function.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 771203 [details] [diff] [review]
part.4 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Qt

I don't test this patch actually because I cannot build it due to OOM.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #34)
> Comment on attachment 771209 [details] [diff] [review]
> part.9 Remove nsIDOMWindowUtils::KEY_FLAG_PREVENT_DEFAULT and don't
> synthesize keypress event if precede keydown event is consumed

> It might be useful in some cases, and there isn't really need to remove that
> stuff.

Hmm, I have no idea of such cases, however, I think that it's not matter. I remove the change for nsDOMWindowUtils.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Enn:

ping.

See comment 25. We need to call HandleKeyboardEventWithKeyCode() from KeyPress() too. So, I'd like you to review the current patches.

Comment 40

[Michael Wu [:mwu]]

Comment on attachment 771206 [details] [diff] [review]
part.7 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Gonk

Excellent. This makes a lot more sense.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Cocoa (r=smaug)

Updated for the latest m-c.

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.9 EventUtils and some tests shouldn't synthesize keypress event if precede keydown event is consumed (r=smaug)

Dropped the change of nsDOMWindowUtils.cpp and nsIDOMWindowUtils.idl.

Comment 43

[Chris Peterson [:cpeterson]]

Comment on attachment 771205 [details] [diff] [review]
part.6 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Android

I think jchen would be a better Android reviewer because I haven't touched the Android keyboard code in a while.

Comment 44

[Neil Deakin]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #39)
> See comment 25. We need to call HandleKeyboardEventWithKeyCode() from
> KeyPress() too. So, I'd like you to review the current patches.

I'm still not clear why the exact same code needs to be run twice. Either it will duplicate something or part of that code doesn't need to run.

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Neil Deakin from comment #44)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #39)
> > See comment 25. We need to call HandleKeyboardEventWithKeyCode() from
> > KeyPress() too. So, I'd like you to review the current patches.
> 
> I'm still not clear why the exact same code needs to be run twice. Either it
> will duplicate something or part of that code doesn't need to run.

If we call HandleKeyboardEventWithKeyCode() only from KeyPress() like current m-c, keydown event might be consumed by other event listeners. Then, KeyPress() won't be called after this bug fixed. So, I believe that we need to call it from KeyDown().

Additionally, some key events only handled at KeyPress(). I guess that the reason is, keydown event is already consumed by other modules such as autocomplete.

Actually, HandleKeyboardEventWithKeyCode() isn't called from KeyPress(), some tests become permanent orange.

So, for preventing to break the behavior, we should handle the keys both with keydown event and keypress event.

If we don't need to consume the keydown event for handling keypress event, we don't need to call HandleKeyboardEventWithKeyCode() from KeyDown(). But I think that it would cause something broken especially with some add-ons.

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Additionally, some key events only handled at KeyPress(). I guess that the reason
> is, keydown event is already consumed by other modules such as autocomplete.

Er, if it's consumed, keypress event is never fired. I guess something just calls StopPropagation() before nsXULPopupManager::KeyDown().

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If we call only stopImmediatePropagation() at KeyDown(), all tests might pass. However, it cannot stop the propagation in system event group. So, anyway, this is not good solution.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I mean:

1. HandleKeyboardEventWithKeyCode() _must_ be called by KeyPress() for keeping current behavior.
2. HandleKeyboardEventWithKeyCode() _should_ be called by KeyDown() for preventing to run other keydown event listeners as far as possible.

Perhaps, we don't need to duplicate all of them, but I believe that my patch's approach is simple and safe.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Enn:

Anyway, if you still think that my approach isn't good, please let me know what I should research more.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.10 nsXULPopupManager should stop propagation of keydown event while menu is open

This is alternative patch of attachment 771211 [details] [diff] [review].

This calls HandleKeyboardEventWithKeyCode() only from KeyPress() and makes KeyDown() call StopImmidiatePropagation() in both normal event group and system event group.

By the latter change, this tries to prevent to run other keydown evnet listeners as far as possible. Although, like part.11, others can handle the following keypress event handles before KeyPress().

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.11 <browser> should not handle keydown and keypress event when escape key is pressed during auto scroll

This is alternative patch of attachment 771213 [details] [diff] [review]. This must go with attachment 777754 [details] [diff] [review].

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Enn:

These patches don't duplicate the handler. However, some keypress event might be handled before KeyPress() is called. So, I believe that the first patches are safer than the newer patches.

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.10 nsXULPopupManager should stop propagation of keydown event while menu is open

The previous patch forgot to remove the new system event listener...

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I posted a patch which doesn't call HandleKeyboardEventWithKeyCode() from KeyPress(). Let's see what will happen.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=92c84c1d094b

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Enn:

I think that the key handling with keyCode value should be completely moved to KeyDown(). However, it causes a lot of bugs.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=92c84c1d094b

I guess that the reason is, a lot of code uses keypress event instead of keydown event for similar situation. Then, the handling order is changed.

Its impact is too large for this bug. Even if we should do it, we should file a new bug.

So, you said you don't like the approach of attachment 771211 [details] [diff] [review]. However, I believe that it's the best way for now. I think that it's safer than the approach of attachment 777761 [details] [diff] [review].

Comment 56

[Karl Tomlinson (:karlt)]

Comment on attachment 771201 [details] [diff] [review]
part.2 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on GTK

>     bool isKeyDownCancelled = false;
>     if (DispatchKeyDownEvent(aEvent, &isKeyDownCancelled) &&
>-        MOZ_UNLIKELY(mIsDestroyed)) {
>+        (MOZ_UNLIKELY(mIsDestroyed) || isKeyDownCancelled)) {
>         return TRUE;
>     }
> 
>     // If a keydown event handler causes to enable IME, i.e., it moves
>     // focus from IME unusable content to IME usable editor, we should
>     // send the native key event to IME for the first input on the editor.
>     if (!IMEWasEnabled && mIMModule && mIMModule->IsEnabled()) {
>         // Notice our keydown event was already dispatched.  This prevents

I don't know much about this IME code, but I assume focus could still have changed even when preventDefault() has been called.
Should this code still run when isKeyDownCancelled is true?

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Karl Tomlinson (:karlt, back 22 July) from comment #56)
> I don't know much about this IME code, but I assume focus could still have
> changed even when preventDefault() has been called.
> Should this code still run when isKeyDownCancelled is true?

No, if keydown event is canceled, following default action(s) shouldn't performed.

On GTK2, this is not done perfectly even with this patch yet. I'll file follow up bugs.
http://mxr.mozilla.org/mozilla-central/source/widget/gtk2/nsGtkIMModule.cpp#1072

This is similar to bug 887695.

Comment 58

[Neil Deakin]

In the patch for that try build (comment 55), you need to also add to the KeyPress method the code below, as key events, except for escape which is now handled by KeyDown, for non-menus should just be ignored:

  if (item && item->PopupType() != ePopupTypeMenu)
    return NS_OK;

This fixes a number of the chrome and browser-chrome tests, although I didn't test all of them.

The autocomplete test test_basic_form_autocomplete.html is using it's own key simulation code rather than synthesizeKey, which might affect this test. It should be converted to just the synthesizeKey. I don't know if this will fix the problem.

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Neil Deakin from comment #58)
> In the patch for that try build (comment 55), you need to also add to the
> KeyPress method the code below, as key events, except for escape which is
> now handled by KeyDown, for non-menus should just be ignored:
> 
>   if (item && item->PopupType() != ePopupTypeMenu)
>     return NS_OK;
> 
> This fixes a number of the chrome and browser-chrome tests, although I
> didn't test all of them.

Good catch! You're right!!

> The autocomplete test test_basic_form_autocomplete.html is using it's own
> key simulation code rather than synthesizeKey, which might affect this test.
> It should be converted to just the synthesizeKey. I don't know if this will
> fix the problem.

Yeah, but we cannot use it there because it causes JS error. I don't know the reason. Anyway, the code is also fixed by part.9.

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

jimm: ping

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.10 nsXULPopupManager should handle keydown events for non-printable keys

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.11 <browser> should not handle keydown event when escape key is pressed during auto scroll

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sigh, I met new orange:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a051bc4973ff

It's caused by bug 291082.

Comment 64

[Jim Mathies [:jimm]]

Comment on attachment 772603 [details] [diff] [review]
part.1 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Windows

Review of attachment 772603 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looks ok with one nit. Sorry for holding up the patch train, this one slipped through.

::: widget/windows/KeyboardLayout.h
@@ +450,5 @@
>    /**
> +   * Remove all following WM_CHAR, WM_SYSCHAR and WM_DEADCHAR messages for the
> +   * WM_KEYDOWN or WM_SYSKEYDOWN message.  Additionally, dispatches plugin
> +   * events if it's necessary.
> +   * Retruns true if the widget is destroyed.  Otherwise, false.

nit - 'Retruns'

@@ +455,2 @@
>     */
> +  bool DispatchPluginEventsAndDiscardsCharMessages() const;

Not specific to this work here, this is a really odd method. :)

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Enn and Jim!

Comment 66

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.13 nsListControlFrame should handle keydown event for non-printable keys

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.13 (-w)

Comment 68

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 779757 [details] [diff] [review]
part.13 nsListControlFrame should handle keydown event for non-printable keys

We need to handle default actions of nsListControlFrame which are not prevented by a call of Event.preventDefault() in keydown event handler because when keydown event's default is prevented, keypress event is never fired after this bug fix.

For that, this patch separates KeyPress() for incremental search and the others. Fortunately, only incremental search needs to check defaultPrevented. So, the others should be handled in KeyDown().

With this change, test_bug672810.html will fail since it does EventUtils.synthesizeKey(" ", ...); but it doesn't set DOM_VK_SPACE to the keyCode. So, the change of EventUtils.js is for this.

Comment 69

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Neil:

And see attachment 779758 [details] [diff] [review] for the differences without only whitespace changes.

Comment 70

[[e-mail]]

Comment on attachment 779757 [details] [diff] [review]
part.13 nsListControlFrame should handle keydown event for non-printable keys

>-  uint32_t numOptions = 0;
>-  options->GetLength(&numOptions);
>+  int32_t numOptions = 0;
>+  options->GetLength(reinterpret_cast<uint32_t*>(&numOptions));
I don't really like this change.

>+    case NS_VK_BACK:
>+      // Don't handle incremental search if the event has already consumed.
>+      if (keyEvent->mFlags.mDefaultPrevented ||
>+          GetIncrementalString().IsEmpty()) {
>+        return NS_OK;
>+      }
>+      // Backspace key will delete the last char in the string
>+      GetIncrementalString().Truncate(GetIncrementalString().Length() - 1);
>+      break;
break; looks wrong because you truncate the incremental string below. Do you need to handle NS_VK_BACK in the keypress handler?
>+    default: // printable key will be handled by keypress event.
>       return NS_OK;
>+  }
>+
>+  aKeyEvent->PreventDefault();
>+
>+  // Cancel incremental search if it's being performed.
>+  GetIncrementalString().Truncate();


>+  if (keyEvent->keyCode || keyEvent->IsAlt()) {
>+    return NS_OK;
>+  }
...
>+  if (!keyEvent->charCode) {
>+    return NS_OK;
>+  }
Is it possible for an event to have neither a keyCode nor a charCode?

Comment 71

[[e-mail]]

Comment on attachment 779757 [details] [diff] [review]
part.13 nsListControlFrame should handle keydown event for non-printable keys

>+  // Don't check defaultPrevented value because other browsers don't prevent
>+  // the key navigation of list control even if preventDefault() is called.
This comment...
>+  // We skip processing all key events in the keys that are not in the
>+  // cases keydown, if they have been prevented. The keys listed in the cases
>+  // above are required to navigate inside the list. These keys are also
>+  // not prevented in most UAs, so this is also a compatibility issue.
... makes this one obsolete.

>+  // Select option with this as the first character
>+  // XXX Not I18N compliant
>+  for (;;) {
Oh, and what's the for (;;) for?

Comment 72

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.13 nsListControlFrame should handle key navigation at keydown event handler

(In reply to [e-mail] from comment #70)
> Comment on attachment 779757 [details] [diff] [review]
> part.13 nsListControlFrame should handle keydown event for non-printable keys
> 
> >-  uint32_t numOptions = 0;
> >-  options->GetLength(&numOptions);
> >+  int32_t numOptions = 0;
> >+  options->GetLength(reinterpret_cast<uint32_t*>(&numOptions));
> I don't really like this change.

OK. I it as using static_cast every time.

> >+    case NS_VK_BACK:
> >+      // Don't handle incremental search if the event has already consumed.
> >+      if (keyEvent->mFlags.mDefaultPrevented ||
> >+          GetIncrementalString().IsEmpty()) {
> >+        return NS_OK;
> >+      }
> >+      // Backspace key will delete the last char in the string
> >+      GetIncrementalString().Truncate(GetIncrementalString().Length() - 1);
> >+      break;
> break; looks wrong because you truncate the incremental string below. Do you
> need to handle NS_VK_BACK in the keypress handler?

Indeed, I moved the handling into KeyPress().

But I found a problem during test, see the XXX comment. I'll file a bug for it.

> >+  if (keyEvent->keyCode || keyEvent->IsAlt()) {
> >+    return NS_OK;
> >+  }
> ...
> >+  if (!keyEvent->charCode) {
> >+    return NS_OK;
> >+  }
> Is it possible for an event to have neither a keyCode nor a charCode?

Yes. For example, Japanese keyboard has a lot of special keys for IME. They become "unknown key" if active keyboard layout is not Japanese. Then, the keyCode becomes 0. And they're not printable, therefore, the charCode becomes 0 too.

(In reply to [e-mail] from comment #71)
> Comment on attachment 779757 [details] [diff] [review]
> part.13 nsListControlFrame should handle keydown event for non-printable keys
> 
> >+  // Don't check defaultPrevented value because other browsers don't prevent
> >+  // the key navigation of list control even if preventDefault() is called.
> This comment...
> >+  // We skip processing all key events in the keys that are not in the
> >+  // cases keydown, if they have been prevented. The keys listed in the cases
> >+  // above are required to navigate inside the list. These keys are also
> >+  // not prevented in most UAs, so this is also a compatibility issue.
> ... makes this one obsolete.

I rewrote the comment, please check it.

> >+  // Select option with this as the first character
> >+  // XXX Not I18N compliant
> >+  for (;;) {
> Oh, and what's the for (;;) for?

It was used for this break:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#2415

However, I realized that newIndex is never modified except this case. And PostHandleKeyEvent() does nothing if it's kNothingSelected.

Therefore, I call it directly from there. Additionally, I removed the variable. Then, I succeeded to remove the hacky |for (;;)|.

Comment 73

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.13 (-w)

Comment 74

[[e-mail]]

(In reply to Masayuki Nakano from comment #72)
> OK. I it as using static_cast every time.
Thanks.

> Indeed, I moved the handling into KeyPress().
> 
> But I found a problem during test, see the XXX comment. I'll file a bug for
> it.
I just tried IE and it doesn't support backspace in incremental string anyway.

> Yes. For example, Japanese keyboard has a lot of special keys for IME. They
> become "unknown key" if active keyboard layout is not Japanese. Then, the
> keyCode becomes 0. And they're not printable, therefore, the charCode
> becomes 0 too.
I think I asked the wrong question here, sorry.

> I rewrote the comment, please check it.
All the comments look fine now.

> However, I realized that newIndex is never modified except this case. And
> PostHandleKeyEvent() does nothing if it's kNothingSelected.
> 
> Therefore, I call it directly from there. Additionally, I removed the
> variable. Then, I succeeded to remove the hacky |for (;;)|.
Very nice!

Comment 75

[[e-mail]]

Comment on attachment 780233 [details] [diff] [review]
part.13 nsListControlFrame should handle key navigation at keydown event handler

>+  // NOTE: If keyCode of keypress event is not 0, charCode is always 0.
>+  //       Therefore, typically, we don't need to handle the key event for
>+  //       incremental search except backspace key.
>+  if (keyEvent->keyCode) {
...
>+    return NS_OK;
>+  }
...
>+  if (!keyEvent->charCode) {
>+    return NS_OK;
>+  }
The old code didn't seem to check for a keyCode of 0, because if the charCode is not 0 then the keyCode is always 0 (as alluded to in your excellent comment!); was there a particular reason you added the extra keyCode check?

Comment 76

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to [e-mail] from comment #75)
> Comment on attachment 780233 [details] [diff] [review]
> part.13 nsListControlFrame should handle key navigation at keydown event
> handler
> 
> >+  // NOTE: If keyCode of keypress event is not 0, charCode is always 0.
> >+  //       Therefore, typically, we don't need to handle the key event for
> >+  //       incremental search except backspace key.
> >+  if (keyEvent->keyCode) {
> ...
> >+    return NS_OK;
> >+  }
> ...
> >+  if (!keyEvent->charCode) {
> >+    return NS_OK;
> >+  }
> The old code didn't seem to check for a keyCode of 0, because if the
> charCode is not 0 then the keyCode is always 0 (as alluded to in your
> excellent comment!); was there a particular reason you added the extra
> keyCode check?

Ah, there is no reason. I just did it for easier to read. If you don't like it, I'll move the backspace key handling code into |if (!keyEvent->charCode) {}|.

Comment 77

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that most developers don't know when keyCode is not 0, then, charCode must be 0. So, my code is easier to read, I think.

Comment 78

[[e-mail]]

(In reply to Masayuki Nakano from comment #77)
> I think that most developers don't know when keyCode is not 0, then,
> charCode must be 0. So, my code is easier to read, I think.

I think I'd prefer it in the if (!keyEvent->charCode) {} block although don't bother creating a separate patch just for that change.

Comment 79

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 780233 [details] [diff] [review]
part.13 nsListControlFrame should handle key navigation at keydown event handler

>+  // NOTE: If keyCode of keypress event is not 0, charCode is always 0.
>+  //       Therefore, typically, we don't need to handle the key event for
>+  //       incremental search except backspace key.
>+  if (keyEvent->keyCode) {
>+    // Backspace key will delete the last char in the string
>+    // XXX Backspace key causes "go back the history" on Windows.  Shouldn't we
>+    //     prevent its default action if incremental search is used since
>+    //     getting focus?  When I tested this, it worked accidentally.
>+    if (keyEvent->keyCode == NS_VK_BACK && !GetIncrementalString().IsEmpty()) {
>+      GetIncrementalString().Truncate(GetIncrementalString().Length() - 1);
>+      aKeyEvent->PreventDefault();
>     }
>+    return NS_OK;
>+  }

Oops, this block and...

>+  // With some keyboard layout, space key causes non-ASCII space.
>+  // So, the check in keydown event handler isn't enough, we need to check it
>+  // again with keypress event.
>+  if (keyEvent->charCode != ' ') {
>+    mControlSelectMode = false;
>+  }

This block should be swapped anyway. The handling order is changed by this patch.

Comment 80

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to [e-mail] from comment #78)
> (In reply to Masayuki Nakano from comment #77)
> > I think that most developers don't know when keyCode is not 0, then,
> > charCode must be 0. So, my code is easier to read, I think.
> 
> I think I'd prefer it in the if (!keyEvent->charCode) {} block although
> don't bother creating a separate patch just for that change.

Okay, I'll update the patch, anyway.

Comment 81

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.13 nsListControlFrame should handle key navigation at keydown event handler

Comment 82

[[e-mail]]

Comment on attachment 780342 [details] [diff] [review]
part.13 nsListControlFrame should handle key navigation at keydown event handler

(Hmm, I think I may have been supposed to test this along while having another patch applied, but anyway it seems to work fine on its own.)

Comment 83

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32862e87f64c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d41fb45a0523
https://hg.mozilla.org/integration/mozilla-inbound/rev/48788ac2e02f
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd7f9080d390
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ab0b048dbad
https://hg.mozilla.org/integration/mozilla-inbound/rev/abadf199f87a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d29dd6534e43
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab40003ed2dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/441c1fc13ebd
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5dc2d51f002
https://hg.mozilla.org/integration/mozilla-inbound/rev/998d70e156bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bab9863c657
https://hg.mozilla.org/integration/mozilla-inbound/rev/949a5e125a8d

Comment 84

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This bug fix may cause some regressions of *our* behavior (not for web contents) which cannot be detected by automated tests. Please block this bug when you find regression. I'll fix each regression ASAP.

Note that the new behavior is strict for D3E spec. And its compatibility with other browsers is better. So, if you find a trouble in web contents, it may be invalid bug.

Comment 85

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Requirements of regression by this bug:

1. keypress event handler handles keypress events even if their default is prevented.
2. keydown event handler calls preventDefault() even though following keypress event handler will handle it.

So, regression only occurs with hacky keydown/keypress event handlers.

Comment 86

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/32862e87f64c
https://hg.mozilla.org/mozilla-central/rev/d41fb45a0523
https://hg.mozilla.org/mozilla-central/rev/48788ac2e02f
https://hg.mozilla.org/mozilla-central/rev/bd7f9080d390
https://hg.mozilla.org/mozilla-central/rev/0ab0b048dbad
https://hg.mozilla.org/mozilla-central/rev/abadf199f87a
https://hg.mozilla.org/mozilla-central/rev/d29dd6534e43
https://hg.mozilla.org/mozilla-central/rev/ab40003ed2dc
https://hg.mozilla.org/mozilla-central/rev/441c1fc13ebd
https://hg.mozilla.org/mozilla-central/rev/d5dc2d51f002
https://hg.mozilla.org/mozilla-central/rev/998d70e156bd
https://hg.mozilla.org/mozilla-central/rev/3bab9863c657
https://hg.mozilla.org/mozilla-central/rev/949a5e125a8d

Comment 87

[Kohei Yoshino]

Added to the site-compat doc.
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25/Site_Compatibility

Comment 88

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Added a section for this change to keydown event document:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/keydown#preventDefault%28%29_of_keydown_event

Comment 89

[[e-mail] (:bhearsum)]

It might be a problem with the website rather than a problem with this bug, but I believe that this caused bug 899105.

Comment 90

[Mingyi Liu]

Ouch, this "bug fix" (with quote because I think it's a useful behavior, and if NOT fixed, the people who complain about it would've been able to work around it; but when it's fixed, people who need the behavior don't have a workaround) really caused issues for my addon (and I wouldn't be surprised if it causes issue for similar scenarios).

So the issue is: My addon Fastest Search provides Find-as-you-type - users can type in any non-input area of a webpage to search for the text on that page. So basically the "input" event wouldn't work. keypress is the only proper event to use, in case user pressed and hold a key (only one keydown will fire but a proper # of keypress will fire) AND I think also because of some keycode issues that I discovered much before & don't remember now (I think keydown sometimes would be 0 keycode when keypress would handle proper input).

However, on some webpages, the webpage itself captures keyboard events, frequently on keydown (like news.google.com which allows one-char shortcuts on that page), AND then the webpage's script calls preventDefault. To prevent the webpage script to interfere, I had to catch keydown and call preventDefault first (stopPropagation alone doesn't work).

Now here's the dilemma - if I call preventDefault in my addon, in FF25+, this causes the keypress event to be gone. Sure I can arbitrarily call my keypress handler after calling preventDefault on keydown, but what if user pressed and held the key? My handler wouldn't know how many times that key was repeated (not to mention occasional keycode issue)!  But if I don't call preventDefault, some webpages will capture keys on keydown and cancel my addon's keypress handling, causing inconsistent and "buggy" (in users' eyes) behavior of the Find-as-you-type feature.

In FF24, everything was working well for me - my addon preventDefault on keydown to prevent webpage script's trapping the key, and keypress handler processes the key(s) afterward.  In FF25, this is not possible any more.

Is there any suggestion anyone can offer? I really hope this "bug" is not "fixed", because in those cases, one could easily set keypress handler to cancel it as well (if needed, set a signal in keydown to indicate cancel is needed). No real harm is done.  Now with this behavior change, is there any workaround that'd make things work properly for my FAYT feature? BTW I need the keyboard focus remain on the page, not some input element for my FAYT, so creating a separate input element doesn't work.

Thanks!

Comment 91

[Victor Porof [:vporof][:vp]]

I don't know much about the implications of this bug, but maybe you can use the nsIEventListenerService [0] [1] to actually remove those events from the page instead of trying to cancel them.

Something like this (untested):

var els = Cc["@mozilla.org/eventlistenerservice;1"]
    .getService(Ci.nsIEventListenerService);

let nodes = document.querySelectorAll("*"); // or query only the nodes you need

for (let node of nodes) {
    let targets = els.getEventTargetChainFor(node);
    for (let target of targets) {
        let listeners = els.getListenerInfoFor(target);
        for (let listener of listeners) {
            let { listenerObject, type, capturing } = listener;
            if (type == "keypress") { // or test only the event you need
                target.removeEventListener(type, listenerObject, capturing);
            }
        }
    }
}

[0] https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIEventListenerService
[1] https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIEventListenerInfo

Comment 92

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Mingyi Liu:

In strict rules of DOM Events, nobody should not handle key events which are already handled by something others because if you break this rule, two or more actions may be performed for an input operation which looks like buggy in almost of all cases for the users. Preventing keypress event for a call of keydown event's preventDefault() is good behavior for this basic rule.

On the other hand, if addon wants to override the web page's behavior, addon should handle the event *before* web contents handle it. I.e., addon should add its event listener to the window or its ancestor (chrome's) element. And then, you should call perventDefault() and stopImmediatePropagation().

Additionally, there is another scenario. That is, key event should be handled by web content first, but addon want to know it *after* that. In this situation, you can use an event listener which is registered in system event group. This doesn't blocked by a call of stop(Immediate)Propagation(). However, you shouldn't do any visible reaction in the handler if Event.defaultPrevented is true.
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIEventListenerService#addSystemEventListener%28%29

If your claim is that you cannot know the input character by the key event, a part of it is a bug of us. KeyboardEvent.key of keydown event should indicate input character. It'd be fixed later. However, actually, until keypress event is fired, the actual input character isn't decided in platform level because the key event may be passed to text composition system like IME after we dispatch keydown event. In other words, web applications shouldn't handle their own shortcut keys with printable keys in keydown event handler. They should use keypress event handler instead.

This means that web applications are a part of your addon because they are in sub DOM tree if you see the contents from chrome scope. So, such "a part of your application" don't handle events ideally, there may be something impossible cases which you want to do, unfortunately.

Comment 93

[Mingyi Liu]

Thanks for the replies, but:
Victor - your suggestion would prevent the webpage to handle ctrl-q shortcut which my FAYT doesn't handle.
Masayuki - 1. I agree it makes sense (philosophically) to cancel keypress if keydown's preventDefault is called, but then why keyup is still fired even after this bug's fixed? It's a ghose keyup (philosophically speaking). One might say W3C didn't say clearly about keyup, but then again W3C RFC or docs were made by humans and it could have mistakes that didn't foresee some scenarios.
2. As I said I can't use keydown to handle keys because it doesn't do repeats.
3. Does systemeventlistener gets notified even if page listeners preventDefault? I see that it gets notified even if other listeners tried to stopPropagation. I'd test later.
4. I'm well aware of IME issue - I had to deal with that with my FAYT feature. I kinda remember that contributed to my having to use keypress than keydown to handle FAYT. I don't remember detail of .key issues any more that might be in keydown that forced me to use keypress.
5. Web apps might be considered same working scope as my addon, but the big difference is that I have no control of them (I can't blanketly cancel them), and thus as you said, with this issue, it might not be possible to ever have the proper FAYT behavior that Fastest Search provides in FF24 right now.  Not possible with FF25+, and that's sad.

Comment 94

[Mingyi Liu]

I meant ghost keyup.

Comment 95

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Mingyi Liu from comment #93)
> Masayuki - 1. I agree it makes sense (philosophically) to cancel keypress if
> keydown's preventDefault is called, but then why keyup is still fired even
> after this bug's fixed? It's a ghose keyup (philosophically speaking). One
> might say W3C didn't say clearly about keyup, but then again W3C RFC or docs
> were made by humans and it could have mistakes that didn't foresee some
> scenarios.

The type of keydown/keyup and keypress are really different.

keydown/keyup events are physical key events even though keydown is fired by auto repeat function of the platform. 

keypress event is logical key event which supports preceding keydown event. keypress event represents text input event caused by the receding keydown event even tough it's fired with Ctrl key or some other modifiers which cause not inputting text actually.

Therefore, keypress event is one of default actions of keydown event. However, keyup event is not a default action of keydown event. This is why keydown event's preventDefault() cannot prevent keyup event.


> 2. As I said I can't use keydown to handle keys because it doesn't do
> repeats.

keydown event should be fired even it's caused by auto repeat. We know older GTK platform also dispatches keyup event during auto repeat. But it shouldn't be matter for you. Could you explain more detail, what is your problem about repeated key events?


> 3. Does systemeventlistener gets notified even if page listeners
> preventDefault? I see that it gets notified even if other listeners tried to
> stopPropagation. I'd test later.

Calls of preventDefault() do never cause blocking the event propagation. So, you can receive keydown event whose defaultPrevented is true.

System event group is an independent event propagation group. In the group, events are fired after all default event group's event handlers are performed even if stop(Immediate)Propagation() is called. However, of course, if stop(Immediate)Propagation() is called in system event group, it blocks the propagation of the event in system event group.

> 4. I'm well aware of IME issue - I had to deal with that with my FAYT
> feature. I kinda remember that contributed to my having to use keypress than
> keydown to handle FAYT. I don't remember detail of .key issues any more that
> might be in keydown that forced me to use keypress.

Using keypress event is right approach. .key has been implemented Gecko 23:
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent#Key_names_and_Char_values
The value of .key for printable key should be actual input text without Ctrl modifier or similar modifiers. But this hasn't be defined at implementing it. Therefore, current Gecko sets "MozPrintableKey" for all printable keys, temporarily.

> 5. Web apps might be considered same working scope as my addon, but the big
> difference is that I have no control of them (I can't blanketly cancel
> them), and thus as you said, with this issue, it might not be possible to
> ever have the proper FAYT behavior that Fastest Search provides in FF24
> right now.  Not possible with FF25+, and that's sad.

Yes, it's really unfortunate fact. As I said, keydown event shouldn't be used by web applications in most cases...

Comment 96

[Mingyi Liu]

Hi, Masayuki,
1. I agree with your explanation, although I'd say that it still makes sense to cancel the keyup if keydown is cancelled.
2. You're right - somehow I remembered keydown is fired only once when held (and auto repeat kicks in). I guess that's not why I did not use keydown then. Probably keycode/IME issues.
3. OK, I'll see if that helps. Not sure it'd solve the issue though.
4. I might need to go back to my emails too, as I remember some users of different keyboard layout might have had issues if I don't use keypress. It's been so long I don't remember the details.
5. True, so far I only found news.google.com does keydown and cancels it for certain keys, and there's a workaround for FAYT on that page. Maybe I shouldn't worry too much about it, although I know some users will complain about it.

Comment 97

[Mime Čuvalo]

This also bit the FireSSH add-on btw (https://addons.mozilla.org/en-US/firefox/addon/firessh/ )
I'm working to push out a fix to it today - just a heads up that there might be other extensions affected as well.

Comment 98

[Tim Buckingham]

Is this still considered resolved/fixed? I'm experiencing a regression in Firefox 25/26 with this code snippet:

http://jsfiddle.net/DwKSa/

Firefox 24 properly prevents the default change in selectedIndex on the select box when catching the keydown and throwing preventDefault/stopPropagation (or just returning false in the keydown event). Firefox 25/26 ignore it and proceed to throw a keyup event and change selectedIndex.

This behavior breaks custom Javascript <select> boxes and is inconsistent with other browsers.

Comment 99

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

See bug 935876. I'll fix it in this cycle (29).

Comment 100

[Tim Buckingham]

This still does not behave properly in Firefox 29.
http://jsfiddle.net/DwKSa/

The select box is still able to have its value changed through keyboard commands while it should be stopped as it is in IE/Safari/Chrome/Firefox < 25.