Last Comment Bug 501496 - preventDefault on keydown does not cancel following keypress
: preventDefault on keydown does not cancel following keypress
Status: RESOLVED FIXED
[parity-IE][parity-Chrome][parity-Saf...
: addon-compat, dev-doc-needed, site-compat
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla25
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Andrew Overholt [:overholt]
Mentors:
http://emil.eae.net/stuff/preventdefa...
: 888204 (view as bug list)
Depends on: 903925 933470 1019630 622245 CVE-2013-1723 899105 903715 903815 935876 940843 950373 966612 967945 1008772
Blocks: 887695
  Show dependency treegraph
 
Reported: 2009-06-30 14:44 PDT by Emil A Eklund
Modified: 2016-06-24 10:57 PDT (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part.1 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Windows (17.74 KB, patch)
2013-07-04 00:20 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.2 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on GTK (2.15 KB, patch)
2013-07-04 00:21 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
karlt: review+
Details | Diff | Splinter Review
part.3 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Cocoa (10.84 KB, patch)
2013-07-04 00:22 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
Details | Diff | Splinter Review
part.4 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Qt (4.39 KB, patch)
2013-07-04 00:22 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
romaxa: review+
Details | Diff | Splinter Review
part.5 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on OS/2 (2.08 KB, patch)
2013-07-04 00:23 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
Details | Diff | Splinter Review
part.6 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Android (2.02 KB, patch)
2013-07-04 00:24 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
nchen: review+
Details | Diff | Splinter Review
part.7 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Gonk (1.94 KB, patch)
2013-07-04 00:25 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
mwu.code: review+
Details | Diff | Splinter Review
part.8 Native key event tests should prevent default only when the event is keypress (1.10 KB, patch)
2013-07-04 00:25 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
Details | Diff | Splinter Review
part.9 Remove nsIDOMWindowUtils::KEY_FLAG_PREVENT_DEFAULT and don't synthesize keypress event if precede keydown event is consumed (9.73 KB, patch)
2013-07-04 00:30 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
bugs: superreview-
Details | Diff | Splinter 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 (11.29 KB, patch)
2013-07-04 00:38 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.11 <browser> should handle keydown event instead of keypress event during auto scroll (1.59 KB, patch)
2013-07-04 00:41 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.12 tilt-visualizer should use keydown event handler instead of keypress event handler for Escape key (3.95 KB, patch)
2013-07-04 00:43 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
vporof: review+
Details | Diff | Splinter Review
part.1 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Windows (19.14 KB, patch)
2013-07-09 04:27 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
jmathies: review+
Details | Diff | Splinter Review
part.3 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Cocoa (r=smaug) (11.18 KB, patch)
2013-07-16 07:39 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Splinter Review
part.9 EventUtils and some tests shouldn't synthesize keypress event if precede keydown event is consumed (r=smaug) (7.86 KB, patch)
2013-07-16 07:41 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.10 nsXULPopupManager should stop propagation of keydown event while menu is open (12.07 KB, patch)
2013-07-18 05:58 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.11 <browser> should not handle keydown and keypress event when escape key is pressed during auto scroll (1.92 KB, patch)
2013-07-18 05:59 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.10 nsXULPopupManager should stop propagation of keydown event while menu is open (12.54 KB, patch)
2013-07-18 06:09 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.10 nsXULPopupManager should handle keydown events for non-printable keys (9.81 KB, patch)
2013-07-23 01:42 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
enndeakin: review+
Details | Diff | Splinter Review
part.11 <browser> should not handle keydown event when escape key is pressed during auto scroll (1.66 KB, patch)
2013-07-23 01:42 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
enndeakin: review+
Details | Diff | Splinter Review
part.13 nsListControlFrame should handle keydown event for non-printable keys (25.75 KB, patch)
2013-07-23 07:24 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.13 (-w) (22.78 KB, patch)
2013-07-23 07:25 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.13 nsListControlFrame should handle key navigation at keydown event handler (25.49 KB, patch)
2013-07-24 00:29 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.13 (-w) (22.79 KB, patch)
2013-07-24 00:30 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.13 nsListControlFrame should handle key navigation at keydown event handler (25.40 KB, patch)
2013-07-24 04:46 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
neil: review+
Details | Diff | Splinter Review

Description Emil A Eklund 2009-06-30 14:44:02 PDT
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] 2009-06-30 15:00:58 PDT
This is probably a dup of some other bug, and note, there
isn't any specification for key events!
Comment 2 Emil A Eklund 2009-06-30 15:29:16 PDT
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] (Mozilla Japan) 2011-02-02 00:37:12 PST
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 2011-11-30 08:00:52 PST
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] (Mozilla Japan) 2011-11-30 17:37:21 PST
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 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-06-28 03:22:53 PDT
*** Bug 888204 has been marked as a duplicate of this bug. ***
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-04 00:20:28 PDT
Created attachment 771200 [details] [diff] [review]
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] (Mozilla Japan) 2013-07-04 00:21:20 PDT
Created attachment 771201 [details] [diff] [review]
part.2 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on GTK
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-04 00:22:15 PDT
Created attachment 771202 [details] [diff] [review]
part.3 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Cocoa
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-04 00:22:58 PDT
Created attachment 771203 [details] [diff] [review]
part.4 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Qt
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-04 00:23:31 PDT
Created attachment 771204 [details] [diff] [review]
part.5 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on OS/2
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-04 00:24:28 PDT
Created attachment 771205 [details] [diff] [review]
part.6 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Android
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-04 00:25:03 PDT
Created attachment 771206 [details] [diff] [review]
part.7 Don't dispatch keypress events if defaultPrevent() of the keydown event is called on Gonk
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-04 00:25:59 PDT
Created attachment 771207 [details] [diff] [review]
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] (Mozilla Japan) 2013-07-04 00:30:04 PDT
Created 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

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] (Mozilla Japan) 2013-07-04 00:38:09 PDT
Created 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

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] (Mozilla Japan) 2013-07-04 00:41:29 PDT
Created attachment 771213 [details] [diff] [review]
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] (Mozilla Japan) 2013-07-04 00:43:37 PDT
Created attachment 771215 [details] [diff] [review]
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] (Mozilla Japan) 2013-07-04 01:07:19 PDT
You can get test builds from here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=354d8d102bbc
Comment 20 Neil Deakin 2013-07-05 05:07:05 PDT
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] (Mozilla Japan) 2013-07-05 09:15:13 PDT
(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 2013-07-08 06:07:06 PDT
(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] (Mozilla Japan) 2013-07-08 07:25:20 PDT
(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] 2013-07-08 08:15:16 PDT
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] (Mozilla Japan) 2013-07-08 23:44:04 PDT
(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] (Mozilla Japan) 2013-07-09 04:27:47 PDT
Created attachment 772603 [details] [diff] [review]
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] 2013-07-10 12:31:58 PDT
Reviews coming today or tomorrow.
Comment 28 Olli Pettay [:smaug] 2013-07-11 12:33:46 PDT
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] 2013-07-11 12:35:39 PDT
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] 2013-07-12 13:23:01 PDT
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] 2013-07-12 14:06:45 PDT
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] 2013-07-12 14:13:02 PDT
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] 2013-07-12 14:15:20 PDT
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] 2013-07-12 14:28:01 PDT
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] (Mozilla Japan) 2013-07-16 04:31:06 PDT
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] (Mozilla Japan) 2013-07-16 04:38:39 PDT
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] (Mozilla Japan) 2013-07-16 04:40:14 PDT
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] (Mozilla Japan) 2013-07-16 04:45:10 PDT
(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] (Mozilla Japan) 2013-07-16 04:49:30 PDT
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] 2013-07-16 07:22:59 PDT
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] (Mozilla Japan) 2013-07-16 07:39:33 PDT
Created attachment 776395 [details] [diff] [review]
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] (Mozilla Japan) 2013-07-16 07:41:39 PDT
Created attachment 776396 [details] [diff] [review]
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] 2013-07-16 16:40:33 PDT
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 2013-07-17 05:17:38 PDT
(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] (Mozilla Japan) 2013-07-17 07:40:52 PDT
(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] (Mozilla Japan) 2013-07-17 07:45:44 PDT
> 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] (Mozilla Japan) 2013-07-17 07:54:06 PDT
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] (Mozilla Japan) 2013-07-17 08:06:30 PDT
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] (Mozilla Japan) 2013-07-17 08:18:16 PDT
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] (Mozilla Japan) 2013-07-18 05:58:08 PDT
Created attachment 777754 [details] [diff] [review]
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] (Mozilla Japan) 2013-07-18 05:59:46 PDT
Created attachment 777755 [details] [diff] [review]
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] (Mozilla Japan) 2013-07-18 06:02:48 PDT
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] (Mozilla Japan) 2013-07-18 06:09:56 PDT
Created attachment 777761 [details] [diff] [review]
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] (Mozilla Japan) 2013-07-18 06:59:17 PDT
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] (Mozilla Japan) 2013-07-18 19:42:01 PDT
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) 2013-07-21 22:37:19 PDT
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] (Mozilla Japan) 2013-07-21 23:32:03 PDT
(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 2013-07-22 13:28:45 PDT
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] (Mozilla Japan) 2013-07-23 01:27:59 PDT
(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] (Mozilla Japan) 2013-07-23 01:34:26 PDT
jimm: ping
Comment 61 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-23 01:42:00 PDT
Created attachment 779665 [details] [diff] [review]
part.10 nsXULPopupManager should handle keydown events for non-printable keys
Comment 62 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-23 01:42:52 PDT
Created attachment 779666 [details] [diff] [review]
part.11 <browser> should not handle keydown event when escape key is pressed during auto scroll
Comment 63 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-23 01:44:46 PDT
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] 2013-07-23 06:03:28 PDT
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] (Mozilla Japan) 2013-07-23 07:23:09 PDT
Thank you, Enn and Jim!
Comment 66 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-23 07:24:53 PDT
Created attachment 779757 [details] [diff] [review]
part.13 nsListControlFrame should handle keydown event for non-printable keys
Comment 67 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-23 07:25:29 PDT
Created attachment 779758 [details] [diff] [review]
part.13 (-w)
Comment 68 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-23 09:28:35 PDT
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] (Mozilla Japan) 2013-07-23 09:29:43 PDT
Neil:

And see attachment 779758 [details] [diff] [review] for the differences without only whitespace changes.
Comment 70 neil@parkwaycc.co.uk 2013-07-23 14:38:05 PDT
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 neil@parkwaycc.co.uk 2013-07-23 14:44:01 PDT
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] (Mozilla Japan) 2013-07-24 00:29:55 PDT
Created attachment 780233 [details] [diff] [review]
part.13 nsListControlFrame should handle key navigation at keydown event handler

(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 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] (Mozilla Japan) 2013-07-24 00:30:37 PDT
Created attachment 780235 [details] [diff] [review]
part.13 (-w)
Comment 74 neil@parkwaycc.co.uk 2013-07-24 02:21:47 PDT
(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 neil@parkwaycc.co.uk 2013-07-24 02:28:53 PDT
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] (Mozilla Japan) 2013-07-24 02:41:45 PDT
(In reply to neil@parkwaycc.co.uk 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] (Mozilla Japan) 2013-07-24 03:29:36 PDT
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 neil@parkwaycc.co.uk 2013-07-24 03:39:09 PDT
(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] (Mozilla Japan) 2013-07-24 03:41:10 PDT
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] (Mozilla Japan) 2013-07-24 03:42:11 PDT
(In reply to neil@parkwaycc.co.uk 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] (Mozilla Japan) 2013-07-24 04:46:28 PDT
Created attachment 780342 [details] [diff] [review]
part.13 nsListControlFrame should handle key navigation at keydown event handler
Comment 82 neil@parkwaycc.co.uk 2013-07-24 14:21:07 PDT
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 84 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-24 23:15:31 PDT
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] (Mozilla Japan) 2013-07-24 23:30:48 PDT
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 87 Kohei Yoshino [:kohei] 2013-07-27 13:02:16 PDT
Added to the site-compat doc.
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25/Site_Compatibility
Comment 88 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-29 03:21:40 PDT
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 Ben Hearsum (:bhearsum) 2013-07-29 08:18:34 PDT
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 2013-09-21 13:43:47 PDT
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] 2013-09-21 23:54:57 PDT
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] (Mozilla Japan) 2013-09-22 00:23:02 PDT
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 2013-09-22 06:41:15 PDT
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 2013-09-22 06:42:31 PDT
I meant ghost keyup.
Comment 95 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-09-22 20:09:57 PDT
(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 2013-09-22 20:56:59 PDT
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 2013-10-30 08:32:09 PDT
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 2014-01-10 09:30:59 PST
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] (Mozilla Japan) 2014-01-10 15:16:15 PST
See bug 935876. I'll fix it in this cycle (29).
Comment 100 Tim Buckingham 2014-04-28 08:16:44 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.