1154183 - nsXBLWindowKeyHandler shouldn't fire keydown events in content when following keypress event is "reserved" by XUL <command> element

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

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

Currently, XUL <key> event handles keypress events only when they are not consumed by a call of preventDefault(). In other words, <key> cannot handle key events when (1) preceding keydown events are consumed by preventDefault() or (2) keypress events are consumed by preventDefault(). This allows web pages can prevent some important shortcut keys like Alt(Cmd) + ArrowLeft(ArrowRight).

I think that XUL <key> element should have "ignoredefaultprevented" attribute. If it's true, the <key> element should handle "keydown" event instead of "keypress" event even if its defaultPrevented is true.

The most important behavior change by this, keypress event won't be fired after that. However, on other browsers, keypress events are fired only when a key press causes text input. I.e., web pages shouldn't handle key events with accel modifier with keypress event handler.


Guys, do you have some suggestions or something?

Comment 1

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

XUL <key>:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/key

Comment 2

[Neil Deakin]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #0)
> In other words, <key> cannot handle
> key events when (1) preceding keydown events are consumed by
> preventDefault() or (2) keypress events are consumed by preventDefault().
> This allows web pages can prevent some important shortcut keys like Alt(Cmd)
> + ArrowLeft(ArrowRight).

Usually, we do this by putting the event listener in the system group. In xbl we can do:
  <handler event="keypress" keycode="VK_ESCAPE" group="system">

Do we just need something similar for <key> or are you referring to something specific related to keydown/keypress?

Comment 3

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

Yeah, there is no way to handle events whose defaultPrevented is true because they are ignored before walking all registered handlers. Therefore, we need a new flag to ignore it.
http://mxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLWindowKeyHandler.cpp#264

Additionally, keypress isn't fired if preceding keydown is consumed, therefore, if some <key> shouldn't be blocked by content, they should be handled at keydown event.
http://mxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLPrototypeHandler.cpp#735

But I don't think that it's worthwhile to add a way to specify which key event should be the target.

Now, looks like we already listen the key events in system event group, so, we can just avoid from blocking by stopPropagation() in normal event group.
http://mxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLService.cpp#563

Comment 4

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

Attachment: part.1 Implement ignoredefaultprevented attribute of XUL <key> element

This supports ignoredefaultprevented attribute. However, this is not keyhell-aware because keydown events don't have alternative charcode values.

So, before landing this patch, we need to fix in widget side. I think that I should do it in bug 1137560.

Comment 5

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

Attachment: part.2 Reduce virtual calls while nsXBLWindowKeyHandler::WalkHandlersAndExecute() looks for a matched key handler

Comment 6

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

I realized that bug 1074971 already added "reserved" attribute to XUL <command> which I want. So, we don't need to add new attribute to XUL <key> anymore.

But we still need to make nsXBLWindowKeyHandler handle keydown events if <command> has "reserved" attribute.

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fa8d5f49227

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d06c6a08e41

Comment 9

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

Okay, according to the new approach of bug 1203059 and investigation of new orange of comment 8, we shouldn't handle keydown event for reserved XUL command elements because killing keypress events requires a lot of changes in chrome and breaks compatibility of addons.

So, instead, we should just stop dispatching keydown events only in content.

Comment 10

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b80bf1bbcc7

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f20c979dbf7d

Comment 12

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

Attachment: part.1 Move shortcut/access key candidate list creators from nsContentUtils to WidgetKeyboardEvent

Now, we have WidgetEventImpl.cpp. Therefore, it doesn't make sense nsContentUtils to have utility methods which retrieves shortcut/access key candidates from a keyboard event. So, let's move them to WidgetKeyboardEvent.

Comment 13

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

Attachment: part.2 eKeyDown event should have charCode value of following keypress event

We need to store charCode value of following keypress event into keydown events.

There are two options:

1. Using first of alternativeCharCodes and set charCode 0.
2. Using charCode member simply.
3. Adding new member like |mPseudoCharCode|.

I don't like to take #2 since it may cause some regressions if somebody checks charCode value of eKeyDown directly.

I was thinking to use #2 (the code was implemented already). However, this means that adding complicated rule to developers to write widget.

Currently, my most favorite approach is #3, although, there might be better member name.

Comment 14

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

Attachment: part.3 Clean up some variable names in nsXBLWindowKeyHandler::WalkHandlersAndExecute()

Before changing nsXBLWindowKeyHandler::WalkHandlersAndExecute(), I'd like to separate some parts of it to independent methods since the method is too long and messy. So, not easy to read.

This patch renames variable names and change the early return timing of |if (!aExecute)|.

Comment 15

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

Attachment: part.4 Implement nsXBLWindowKeyHandler::GetElementForHandler()

Just separating a part of the method, I'll rewrite this method with early return style in the following patch.

Comment 16

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

Attachment: part.5 Make nsXBLWindowKeyHandler::GetElementForHandler() use early return style

Comment 17

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

Attachment: part.6 Add nsXBLWindowKeyHandler::IsExecuteableElement()

Comment 18

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

Attachment: part.7 Don't dispatch preceding keydown events of reserved keypress events on content in the default event group

* nsXBLPrototypeHandler::KeyEventMatched() should use WidgetKeyboardEvent::PseudoCharCode() for making it possible to check charCode of following keypress event of a keydown event.
* nsXBLWindowKeyHandler should check keydown event if the key combination is reserved at keypress.

Comment 19

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

Comment on attachment 8731060 [details] [diff] [review]
part.1 Move shortcut/access key candidate list creators from nsContentUtils to WidgetKeyboardEvent



>+typedef nsTArray<ShortcutKeyCandidate> ShortcutKeyCandidateArray;
>+typedef AutoTArray<ShortcutKeyCandidate, 10> AutoShortcutKeyCandidateArray;
Not sure these make the code any easier to read.
But I guess if you prefer having them, fine.

Comment 20

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

Comment on attachment 8731064 [details] [diff] [review]
part.4 Implement nsXBLWindowKeyHandler::GetElementForHandler()

I would have made the method to return already_AddRefed<Element> (or null) but up to you.

Comment 21

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

(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 8731064 [details] [diff] [review]
> part.4 Implement nsXBLWindowKeyHandler::GetElementForHandler()
> 
> I would have made the method to return already_AddRefed<Element> (or null)
> but up to you.

Yeah, I wrote it as so first. However, I realized that we need to return bool for calling continue. For consistency with other methods around it, I used this old style...

Comment 22

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

Comment on attachment 8731061 [details] [diff] [review]
part.2 eKeyDown event should have charCode value of following keypress event

>+  if (keyEvent.mKeyNameIndex != KEY_NAME_INDEX_USE_STRING) {
>     MOZ_ASSERT(!aIndexOfKeypress,
>       "aIndexOfKeypress must be 0 for eKeyPress of non-printable key");
>     // If keypress event isn't caused by printable key, its charCode should
>     // be 0.
So the assert message isn't quite right anymore, since this code maybe executed for keydown and up too. Want to fix that and also the comment after the assertion.
Or if something guarantees that aMessage is in fact aKeyPress here, then need to add an assertion for that.


>-    keyEvent.charCode = 0;
>+    keyEvent.SetCharCode(0);
>   } else {
>-    MOZ_RELEASE_ASSERT(
>-      !aIndexOfKeypress || aIndexOfKeypress < keyEvent.mKeyValue.Length(),
>-      "aIndexOfKeypress must be 0 - mKeyValue.Length() - 1");
>-    keyEvent.keyCode = 0;
>+    if (aMessage == eKeyDown || aMessage == eKeyUp) {
>+      MOZ_RELEASE_ASSERT(!aIndexOfKeypress,
>+        "aIndexOfKeypress must be 0 for either eKeyDown or eKeyUp");
>+    } else {
>+      MOZ_RELEASE_ASSERT(
>+        !aIndexOfKeypress || aIndexOfKeypress < keyEvent.mKeyValue.Length(),
>+        "aIndexOfKeypress must be 0 - mKeyValue.Length() - 1");
>+    }
understanding these assertions is getting really hard, even though they just moved. Maybe they are right.

>   // If the instance is a keypress event of a printable key, this is a UTF-16
>   // value of the key.  Otherwise, 0.  This value must not be a control
>   // character when some modifiers are active.  Then, this value should be an
>   // unmodified value except Shift and AltGr.
>   uint32_t charCode;
>+  // mPseudoCharCode is valid only when mMessage is an eKeyDown event.
>+  // This stores charCode value of keypress event which is fired with same
>+  // key value and same modifier state.
>+  uint32_t mPseudoCharCode;
It is a bit error prone that in some cases we call SetPseudoCode and in some cases we just set charCode.
But looks like we actually need to do. Error prone anyhow. I don't have suggestions how to fix this.




> TISInputSourceWrapper::WillDispatchKeyboardEvent(
>                          NSEvent* aNativeKeyEvent,
>                          const nsAString* aInsertString,
>                          WidgetKeyboardEvent& aKeyEvent)
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> 
>+  // Nothing to do here if the native key event is neither NSKeyDown nor
>+  // NSKeyUp because accessing [aNativeKeyEvent characters] causes throwing
>+  // an exception.
>+  if ([aNativeKeyEvent type] != NSKeyDown &&
>+      [aNativeKeyEvent type] != NSKeyUp) {
>+    return;
>+  }
>+
>   UInt32 kbType = GetKbdType();
> 
>   if (MOZ_LOG_TEST(gLog, LogLevel::Info)) {
>     nsAutoString chars;
>     nsCocoaUtils::GetStringForNSString([aNativeKeyEvent characters], chars);
>     NS_ConvertUTF16toUTF8 utf8Chars(chars);
>     char16_t uniChar = static_cast<char16_t>(aKeyEvent.charCode);
>     MOZ_LOG(gLog, LogLevel::Info,
>@@ -1076,30 +1084,19 @@ TISInputSourceWrapper::WillDispatchKeybo
>        kbType, TrueOrFalse(IsOpenedIMEMode())));
>   }
> 
>   nsAutoString insertStringForCharCode;
>   ComputeInsertStringForCharCode(aNativeKeyEvent, aKeyEvent, aInsertString,
>                                  insertStringForCharCode);
>   uint32_t charCode =
>     insertStringForCharCode.IsEmpty() ? 0 : insertStringForCharCode[0];
>-  if (aKeyEvent.mMessage == eKeyPress) {
>-    aKeyEvent.charCode = charCode;
>-    aKeyEvent.isChar = true; // this is not a special key  XXX not used in XP
>-  } else if (charCode) {
>-    // If it's not a keypress event, we need to set alternative char code
>-    // to charCode value for shortcut key event handlers.
>-    AlternativeCharCode altCharCodes(0, 0);
>-    if (!aKeyEvent.IsShift()) {
>-      altCharCodes.mUnshiftedCharCode = charCode;
>-    } else {
>-      altCharCodes.mShiftedCharCode = charCode;
>-    }
>-    aKeyEvent.alternativeCharCodes.AppendElement(altCharCodes);
>-  }
>+  aKeyEvent.SetCharCode(charCode);
>+  // this is not a special key  XXX not used in XP
>+  aKeyEvent.isChar = (aKeyEvent.mMessage == eKeyPress);
I don't understand this change.

>diff --git a/widget/gtk/nsGtkKeyUtils.cpp b/widget/gtk/nsGtkKeyUtils.cpp
>--- a/widget/gtk/nsGtkKeyUtils.cpp
>+++ b/widget/gtk/nsGtkKeyUtils.cpp
>@@ -1336,38 +1336,21 @@ KeymapWrapper::WillDispatchKeyboardEvent
>     if (!charCode) {
>         MOZ_LOG(gKeymapWrapperLog, LogLevel::Info,
>             ("KeymapWrapper(%p): WillDispatchKeyboardEventInternal, "
>              "keyCode=0x%02X, charCode=0x%08X",
>              this, aKeyEvent.keyCode, aKeyEvent.charCode));
>         return;
>     }
> 
>-    AlternativeCharCode* firstAltCharCodes = nullptr;
>-    if (aKeyEvent.mMessage == eKeyPress) {
>-        // charCode of aKeyEvent is set from mKeyValue.  However, for backward
>-        // compatibility, we may need to set it to other value, e.g., when
>-        // Ctrl key is pressed.  Therefore, we need to overwrite the charCode
>-        // here.
>-        aKeyEvent.charCode = charCode;
>-        MOZ_ASSERT(!aKeyEvent.keyCode);
>-    } else {
>-        MOZ_ASSERT(charCode);
>-        // If it's not a keypress event, we need to set alternative char code
>-        // to charCode value for shortcut key event handlers.
>-        AlternativeCharCode altCharCodes(0, 0);
>-        if (!aKeyEvent.IsShift()) {
>-            altCharCodes.mUnshiftedCharCode = charCode;
>-        } else {
>-            altCharCodes.mShiftedCharCode = charCode;
>-        }
>-        MOZ_ASSERT(aKeyEvent.alternativeCharCodes.IsEmpty());
>-        firstAltCharCodes =
>-            aKeyEvent.alternativeCharCodes.AppendElement(altCharCodes);
>-    }
>+    // charCode of aKeyEvent is set from mKeyValue.  However, for backward
>+    // compatibility, we may need to set it to other value, e.g., when
>+    // Ctrl key is pressed.  Therefore, we need to overwrite the charCode
>+    // here.
>+    aKeyEvent.SetCharCode(charCode);
Same here.

>     uint16_t uniChar =
>       mInputtingStringAndModifiers.mChars[aIndex - skipUniChars];
>-    if (aKeyboardEvent.mMessage == eKeyPress) {
>-      // charCode is set from mKeyValue but e.g., when Ctrl key is pressed,
>-      // the value should indicate an ASCII character for backward
>-      // compatibility instead of inputting character without the modifiers.
>-      aKeyboardEvent.charCode = uniChar;
>-      MOZ_ASSERT(!aKeyboardEvent.keyCode);
>-    } else if (uniChar) {
>-      // If the event is not keypress event, we should set charCode as
>-      // first alternative char code since the char code value is necessary
>-      // for shortcut key handlers at looking for a proper handler.
>-      AlternativeCharCode chars(0, 0);
>-      if (!aKeyboardEvent.IsShift()) {
>-        chars.mUnshiftedCharCode = uniChar;
>-      } else {
>-        chars.mShiftedCharCode = uniChar;
>-      }
>-      altArray.AppendElement(chars);
>-    }
>+
>+    // charCode is set from mKeyValue but e.g., when Ctrl key is pressed,
>+    // the value should indicate an ASCII character for backward
>+    // compatibility instead of inputting character without the modifiers.
>+    aKeyboardEvent.SetCharCode(uniChar);
And here.

What happens to all those alternativecharcode handling?

Comment 23

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

Comment on attachment 8731072 [details] [diff] [review]
part.7 Don't dispatch preceding keydown events of reserved keypress events on content in the default event group


> nsXBLPrototypeHandler::KeyEventMatched(
>                          nsIDOMKeyEvent* aKeyEvent,
>                          uint32_t aCharCode,
>                          const IgnoreModifierState& aIgnoreModifierState)
> {
>+  if (!EventTypeEquals(nsGkAtoms::keydown) &&
>+      !EventTypeEquals(nsGkAtoms::keypress) &&
>+      !EventTypeEquals(nsGkAtoms::keyup)) {
>+    return false;
>+  }
How could we ever enter here when event type isn't keydown/press/up?
If that is the case, sounds like a bug in calling code.

>+
>   if (mDetail != -1) {
>     // Get the keycode or charcode of the key event.
>     uint32_t code;
> 
>     if (mMisc) {
>       if (aCharCode)
>         code = aCharCode;
>-      else
>-        aKeyEvent->GetCharCode(&code);
>+      else {
>+        WidgetKeyboardEvent* widgetKeyboardEvent =
>+          aKeyEvent->AsEvent()->WidgetEventPtr()->AsKeyboardEvent();
>+        MOZ_ASSERT(widgetKeyboardEvent);
>+        code = widgetKeyboardEvent->PseudoCharCode();
>+      }
So this changes behavior of all the keydown handlers in XBL. I don't think we want that.
Need to somehow special case this for those "reserved" cases.

Comment 24

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

(In reply to Olli Pettay [:smaug] from comment #23)
> Comment on attachment 8731072 [details] [diff] [review]
> part.7 Don't dispatch preceding keydown events of reserved keypress events
> on content in the default event group
> 
> 
> > nsXBLPrototypeHandler::KeyEventMatched(
> >                          nsIDOMKeyEvent* aKeyEvent,
> >                          uint32_t aCharCode,
> >                          const IgnoreModifierState& aIgnoreModifierState)
> > {
> >+  if (!EventTypeEquals(nsGkAtoms::keydown) &&
> >+      !EventTypeEquals(nsGkAtoms::keypress) &&
> >+      !EventTypeEquals(nsGkAtoms::keyup)) {
> >+    return false;
> >+  }
> How could we ever enter here when event type isn't keydown/press/up?
> If that is the case, sounds like a bug in calling code.

I think that it's possible case. nsXBLPrototypeHandler may be a mouse event handler (Although, I'm not sure if it's actually called when the event handler is a mouse event handler).

> >   if (mDetail != -1) {
> >     // Get the keycode or charcode of the key event.
> >     uint32_t code;
> > 
> >     if (mMisc) {
> >       if (aCharCode)
> >         code = aCharCode;
> >-      else
> >-        aKeyEvent->GetCharCode(&code);
> >+      else {
> >+        WidgetKeyboardEvent* widgetKeyboardEvent =
> >+          aKeyEvent->AsEvent()->WidgetEventPtr()->AsKeyboardEvent();
> >+        MOZ_ASSERT(widgetKeyboardEvent);
> >+        code = widgetKeyboardEvent->PseudoCharCode();
> >+      }
> So this changes behavior of all the keydown handlers in XBL. I don't think
> we want that.
> Need to somehow special case this for those "reserved" cases.

Hmm, by the design of retrieving "reserved" attribute, we need to pass if the handler is "reserved" with a bool argument... But for now, I think that we should *can* use mOnlySystemGroupDispatchInContent, so, for now I use it instead.

Comment 25

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

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24)
> (In reply to Olli Pettay [:smaug] from comment #23)
> > Comment on attachment 8731072 [details] [diff] [review]
> > part.7 Don't dispatch preceding keydown events of reserved keypress events
> > on content in the default event group
> > 
> > 
> > > nsXBLPrototypeHandler::KeyEventMatched(
> > >                          nsIDOMKeyEvent* aKeyEvent,
> > >                          uint32_t aCharCode,
> > >                          const IgnoreModifierState& aIgnoreModifierState)
> > > {
> > >+  if (!EventTypeEquals(nsGkAtoms::keydown) &&
> > >+      !EventTypeEquals(nsGkAtoms::keypress) &&
> > >+      !EventTypeEquals(nsGkAtoms::keyup)) {
> > >+    return false;
> > >+  }
> > How could we ever enter here when event type isn't keydown/press/up?
> > If that is the case, sounds like a bug in calling code.
> 
> I think that it's possible case. nsXBLPrototypeHandler may be a mouse event
> handler (Although, I'm not sure if it's actually called when the event
> handler is a mouse event handler).
But this code path should be executed only with key events, no? nsIDOMKeyEvent as a param.
Though, I wonder if this gets executed if one has handler for "foo" and dispatchEvent(new KeyboardEvent("foo")); is called.
Would need to read where this code gets called.

Comment 26

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

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24)
> (In reply to Olli Pettay [:smaug] from comment #23)
> > >   if (mDetail != -1) {
> > >     // Get the keycode or charcode of the key event.
> > >     uint32_t code;
> > > 
> > >     if (mMisc) {
> > >       if (aCharCode)
> > >         code = aCharCode;
> > >-      else
> > >-        aKeyEvent->GetCharCode(&code);
> > >+      else {
> > >+        WidgetKeyboardEvent* widgetKeyboardEvent =
> > >+          aKeyEvent->AsEvent()->WidgetEventPtr()->AsKeyboardEvent();
> > >+        MOZ_ASSERT(widgetKeyboardEvent);
> > >+        code = widgetKeyboardEvent->PseudoCharCode();
> > >+      }
> > So this changes behavior of all the keydown handlers in XBL. I don't think
> > we want that.
> > Need to somehow special case this for those "reserved" cases.
> 
> Hmm, by the design of retrieving "reserved" attribute, we need to pass if
> the handler is "reserved" with a bool argument... But for now, I think that
> we should *can* use mOnlySystemGroupDispatchInContent, so, for now I use it
> instead.

Oops, this is wrong because KeyEventMatched() is called for a pre-check of if it's reserved by linked command element. So, I need to keep this code. Does this sound bad to you?

Comment 27

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

(In reply to Olli Pettay [:smaug] from comment #25)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24)
> > (In reply to Olli Pettay [:smaug] from comment #23)
> > > Comment on attachment 8731072 [details] [diff] [review]
> > > part.7 Don't dispatch preceding keydown events of reserved keypress events
> > > on content in the default event group
> > > 
> > > 
> > > > nsXBLPrototypeHandler::KeyEventMatched(
> > > >                          nsIDOMKeyEvent* aKeyEvent,
> > > >                          uint32_t aCharCode,
> > > >                          const IgnoreModifierState& aIgnoreModifierState)
> > > > {
> > > >+  if (!EventTypeEquals(nsGkAtoms::keydown) &&
> > > >+      !EventTypeEquals(nsGkAtoms::keypress) &&
> > > >+      !EventTypeEquals(nsGkAtoms::keyup)) {
> > > >+    return false;
> > > >+  }
> > > How could we ever enter here when event type isn't keydown/press/up?
> > > If that is the case, sounds like a bug in calling code.
> > 
> > I think that it's possible case. nsXBLPrototypeHandler may be a mouse event
> > handler (Although, I'm not sure if it's actually called when the event
> > handler is a mouse event handler).
> But this code path should be executed only with key events, no?
> nsIDOMKeyEvent as a param.
> Though, I wonder if this gets executed if one has handler for "foo" and
> dispatchEvent(new KeyboardEvent("foo")); is called.
> Would need to read where this code gets called.

The callers are:
* nsXBLWindowKeyHandler::WalkHandlersAndExecute() 
* nsXBLKeyEventHandler::ExecuteMatchedHandlers()

I don't know if they may have mouse event handlers. They just look for a handler which matches with the keyboard event. So, even if they include some mouse event handlers, they won't be hit with this method.

Comment 28

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

Attachment: part.2 eKeyDown event should have charCode value of following keypress event

>>@@ -1076,30 +1084,19 @@ TISInputSourceWrapper::WillDispatchKeybo
>>        kbType, TrueOrFalse(IsOpenedIMEMode())));
>>   }
>> 
>>   nsAutoString insertStringForCharCode;
>>   ComputeInsertStringForCharCode(aNativeKeyEvent, aKeyEvent, aInsertString,
>>                                  insertStringForCharCode);
>>   uint32_t charCode =
>>     insertStringForCharCode.IsEmpty() ? 0 : insertStringForCharCode[0];
>>-  if (aKeyEvent.mMessage == eKeyPress) {
>>-    aKeyEvent.charCode = charCode;
>>-    aKeyEvent.isChar = true; // this is not a special key  XXX not used in XP
>>-  } else if (charCode) {
>>-    // If it's not a keypress event, we need to set alternative char code
>>-    // to charCode value for shortcut key event handlers.
>>-    AlternativeCharCode altCharCodes(0, 0);
>>-    if (!aKeyEvent.IsShift()) {
>>-      altCharCodes.mUnshiftedCharCode = charCode;
>>-    } else {
>>-      altCharCodes.mShiftedCharCode = charCode;
>>-    }
>>-    aKeyEvent.alternativeCharCodes.AppendElement(altCharCodes);
>>-  }
>>+  aKeyEvent.SetCharCode(charCode);
>>+  // this is not a special key  XXX not used in XP
>>+  aKeyEvent.isChar = (aKeyEvent.mMessage == eKeyPress);
> I don't understand this change.

As I explained in the comment at attached, the change was preparations for this bug. However, the rule to add pseudo charCode value at first alternaitveCharCodes of eKeyDown and eKeyUp is very complicated. Therefore, I rewrote with SetCharCode() and mPseudoCharCode in this patch.

The reason of why WillDispatchKeyboardEvent() needs to overwrite the charCode value is, TextEventDispatcher sets charCode from mKeyValue. However, mKeyValue stores characters which are inputted without Ctrl/Alt modifiers if the key combination doesn't cause text input. On the other hand, we set an ASCII code if Ctrl key is pressed since an ASCII character is wanted by shortcut key listeners.

Comment 29

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

For example, here checks if an event handler instance is for key event...
http://mxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLBinding.cpp?rev=114ec4ff6d65#490

Hmm, perhaps, I can move the check to nsXULWindowKeyHandler.

Comment 30

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

Ah, okay, perhaps, I don't need to change nsXBLPrototypeHandler::KeyEventMatched() because nsXBLWindowKeyHandler always calls it with aCharCode.

Comment 31

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

Attachment: part.7 Don't dispatch preceding keydown events of reserved keypress events on content in the default event group

This must be what you want.

Comment 32

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

Comment on attachment 8732519 [details] [diff] [review]
part.7 Don't dispatch preceding keydown events of reserved keypress events on content in the default event group

Ugh, failed to merge...

Comment 33

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

Attachment: part.7 Don't dispatch preceding keydown events of reserved keypress events on content in the default event group

Comment 34

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f497fa0faf05

Comment 35

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18de8691b09a

Comment 36

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

Comment on attachment 8732511 [details] [diff] [review]
part.2 eKeyDown event should have charCode value of following keypress event

>+++ b/widget/cocoa/TextInputHandler.mm
...
>-  } else if (charCode) {
>-    // If it's not a keypress event, we need to set alternative char code
>-    // to charCode value for shortcut key event handlers.
>-    AlternativeCharCode altCharCodes(0, 0);
>-    if (!aKeyEvent.IsShift()) {
>-      altCharCodes.mUnshiftedCharCode = charCode;
>-    } else {
>-      altCharCodes.mShiftedCharCode = charCode;
>-    }
>-    aKeyEvent.alternativeCharCodes.AppendElement(altCharCodes);
>-  }
So this change is still total mystery to me

>+++ b/widget/gtk/nsGtkKeyUtils.cpp
...
>-    AlternativeCharCode* firstAltCharCodes = nullptr;
...
>-    } else {
>-        MOZ_ASSERT(charCode);
>-        // If it's not a keypress event, we need to set alternative char code
>-        // to charCode value for shortcut key event handlers.
>-        AlternativeCharCode altCharCodes(0, 0);
>-        if (!aKeyEvent.IsShift()) {
>-            altCharCodes.mUnshiftedCharCode = charCode;
>-        } else {
>-            altCharCodes.mShiftedCharCode = charCode;
>-        }
>-        MOZ_ASSERT(aKeyEvent.alternativeCharCodes.IsEmpty());
>-        firstAltCharCodes =
>-            aKeyEvent.alternativeCharCodes.AppendElement(altCharCodes);
>-    }
...
>-        } else {
>-            MOZ_RELEASE_ASSERT(firstAltCharCodes);
>-            if (!aKeyEvent.IsShift()) {
>-                firstAltCharCodes->mUnshiftedCharCode = ch;
>-            } else {
>-                firstAltCharCodes->mShiftedCharCode = ch;
>-            }
>-        }
as are these changes

>+++ b/widget/windows/KeyboardLayout.cpp
...
>-    } else if (uniChar) {
>-      // If the event is not keypress event, we should set charCode as
>-      // first alternative char code since the char code value is necessary
>-      // for shortcut key handlers at looking for a proper handler.
>-      AlternativeCharCode chars(0, 0);
>-      if (!aKeyboardEvent.IsShift()) {
>-        chars.mUnshiftedCharCode = uniChar;
>-      } else {
>-        chars.mShiftedCharCode = uniChar;
>-      }
>-      altArray.AppendElement(chars);
>-    }
and these.


What is this code currently for and how is that handled after the patches in this bug have landed?
GetShortcutKeyCandidates uses that stuff, so why it is ok to not append new entries to the alternativeCharCodes array?
Please explain.

Comment 37

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

Comment on attachment 8732511 [details] [diff] [review]
part.2 eKeyDown event should have charCode value of following keypress event

> What is this code currently for and how is that handled after the patches in
> this bug have landed?
> GetShortcutKeyCandidates uses that stuff, so why it is ok to not append new
> entries to the alternativeCharCodes array?
> Please explain.

So, the removing code does nothing now because WillDispatchKeyboardEvent() is never called for non-eKeyPress events. They were appended for this bug at modifying each widget to use TextEventDispatcher.

That was done for making this bug's patches simpler. However, I changed my mind during coding the patches since the "rule" is too complicated for other developers because alternativeCharCodes should be only for one purpose for easier to understand.

Now, WidgetKeyboardEvent has new member to store charCode value for eKeyDown (and eKeyUp). Therefore, we need to remove the unnecessary code from each widget. So, I'm removing the unnecessary preparation for this bug in this patch.

# Asking review again

Comment 38

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

Comment on attachment 8732523 [details] [diff] [review]
part.7 Don't dispatch preceding keydown events of reserved keypress events on content in the default event group

>+    } else {
>+      if (handler->EventTypeEquals(nsGkAtoms::keypress)) {
>+        // If the handler is a keypress event handler, we also need to check
>+        // if coming keydown event is a preceding event of the key combination.
>+        if (aEventType != nsGkAtoms::keydown &&
>+            aEventType != nsGkAtoms::keypress) {
>+          continue;
>+        }
I think the comment could be improved a bit. It should say why we care about keydown here.


>+
>+      nsAutoString value;
>+      commandElement->GetAttribute(NS_LITERAL_STRING("reserved"), value);
>+      isReserved = value.EqualsLiteral("true");
>       if (aOutReservedForChrome) {
>-        nsAutoString value;
>-        // The caller wants to know if this is a reserved command
>-        commandElement->GetAttribute(NS_LITERAL_STRING("reserved"), value);
>-        *aOutReservedForChrome = value.EqualsLiteral("true");
>+        *aOutReservedForChrome = isReserved;
>       }
I don't understand this change. You don't use isReserved elsewhere but inside that 'if', so why you move GetAttribute call outside the 'if'?
Because of this, r-

(and that would be faster if "reserved" was added to nsGkAtomList.h and then
isReserved = commandElement->AttrValueIs(kNameSpaceID_None, nsGkAtoms::reserved,
                                         nsGkAtoms::_true, eCaseMatters)))

Comment 39

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

> I don't understand this change. You don't use isReserved elsewhere but inside that 'if', so why
> you move GetAttribute call outside the 'if'?

Oh, isReserved should be checked in |if (!aExecute) {| block. It was accidentally removed by at merging the patches :-(

Comment 40

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0bc2a0a27bc

Comment 41

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

Attachment: part.7 Don't dispatch preceding keydown events of reserved keypress events on content in the default event group

isReserved should be changed at checking if the key event is preceding keydown event of a reserved key combination.

Comment 42

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

Comment on attachment 8733272 [details] [diff] [review]
part.7 Don't dispatch preceding keydown events of reserved keypress events on content in the default event group

>+    bool isReserved = false;
>     if (commandElement) {
>       if (!IsExecutableElement(commandElement)) {
>         continue;
>       }
>+
>+      isReserved =
>+        commandElement->AttrValueIs(kNameSpaceID_None, nsGkAtoms::reserved,
>+                                    nsGkAtoms::_true, eCaseMatters);
>       if (aOutReservedForChrome) {
>-        nsAutoString value;
>-        // The caller wants to know if this is a reserved command
>-        commandElement->GetAttribute(NS_LITERAL_STRING("reserved"), value);
>-        *aOutReservedForChrome = value.EqualsLiteral("true");
>+        *aOutReservedForChrome = isReserved;
So just to make sure, we want to change the behavior here so that even if event is for keydown and listener for keypress, we want to start report
*aOutReservedForChrome == true here? (assuming reserved is true.)

Comment 43

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

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #37)
> So, the removing code does nothing now because WillDispatchKeyboardEvent()
> is never called for non-eKeyPress events. They were appended for this bug at
> modifying each widget to use TextEventDispatcher.
So can we actually assert that only eKeyPress calls WillDispatchKeyboardEvent?

Comment 44

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

Comment on attachment 8732511 [details] [diff] [review]
part.2 eKeyDown event should have charCode value of following keypress event

I think I'll need some picture soon to explain what the setup looks like for all the key handling. Getting harder to understand this all (for someone who doesn't actually write the code here ;) ).
Thanks for bearing with me here.

Comment 45

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

(In reply to Olli Pettay [:smaug] from comment #42)
> Comment on attachment 8733272 [details] [diff] [review]
> part.7 Don't dispatch preceding keydown events of reserved keypress events
> on content in the default event group
> 
> >+    bool isReserved = false;
> >     if (commandElement) {
> >       if (!IsExecutableElement(commandElement)) {
> >         continue;
> >       }
> >+
> >+      isReserved =
> >+        commandElement->AttrValueIs(kNameSpaceID_None, nsGkAtoms::reserved,
> >+                                    nsGkAtoms::_true, eCaseMatters);
> >       if (aOutReservedForChrome) {
> >-        nsAutoString value;
> >-        // The caller wants to know if this is a reserved command
> >-        commandElement->GetAttribute(NS_LITERAL_STRING("reserved"), value);
> >-        *aOutReservedForChrome = value.EqualsLiteral("true");
> >+        *aOutReservedForChrome = isReserved;
> So just to make sure, we want to change the behavior here so that even if
> event is for keydown and listener for keypress, we want to start report
> *aOutReservedForChrome == true here? (assuming reserved is true.)

Exactly. Then, the caller (handler of capture phase in the default group can mark the keyboard event as reserved).

(In reply to Olli Pettay [:smaug] from comment #43)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #37)
> > So, the removing code does nothing now because WillDispatchKeyboardEvent()
> > is never called for non-eKeyPress events. They were appended for this bug at
> > modifying each widget to use TextEventDispatcher.
> So can we actually assert that only eKeyPress calls
> WillDispatchKeyboardEvent?

No, part.2 (https://bugzilla.mozilla.org/attachment.cgi?id=8732511&action=diff#a/widget/TextEventDispatcher.cpp_sec3) makes TextEventDispatcher call WillDispatchKeyboardEvent() with eKeyDown.

I'm still not sure for eKeyUp. According to the MDN, <key> cannot specify the event type.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/key
(And it has phase attribute but current nsXBLWindowKeyHandler doesn't try to execute any handlers at capturing phase, though)

But the design of nsXBLPrototypeHandler allows specifying the event type. We need to investigate and clean up the document later...

(In reply to Olli Pettay [:smaug] from comment #44)
> Comment on attachment 8732511 [details] [diff] [review]
> part.2 eKeyDown event should have charCode value of following keypress event
> 
> I think I'll need some picture soon to explain what the setup looks like for
> all the key handling. Getting harder to understand this all (for someone who
> doesn't actually write the code here ;) ).
> Thanks for bearing with me here.

Thank *you* for your review. Pointing wrong design of my hack very helps me!

I'd like to update IME event handling document in MDN. Then, I need to include keyboard event handling into there because those handling are now related in TextEventDispatcher.

Comment 46

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e01f6bf3b982735e4c51ce0ed60862091648145
Bug 1154183 part.1 Move shortcut/access key candidate list creators from nsContentUtils to WidgetKeyboardEvent r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bf385a7c2a84e72ce0caab657c082625381045
Bug 1154183 part.2 eKeyDown event should have charCode value of following keypress event r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/024017da65648b6393ddf01126139bd7cb1b172d
Bug 1154183 part.3 Clean up some variable names in nsXBLWindowKeyHandler::WalkHandlersAndExecute() r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c650c359c8e65904805108b1996a56f08c2a372
Bug 1154183 part.4 Implement nsXBLWindowKeyHandler::GetElementForHandler() r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0733a06e7f14481ced69b8e3690d79ba7a2c88a
Bug 1154183 part.5 Make nsXBLWindowKeyHandler::GetElementForHandler() use early return style r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/a12f5a5fff6789823af34028da2d4a6bb62b2e70
Bug 1154183 part.6 Add nsXBLWindowKeyHandler::IsExecuteableElement() r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ef9818a44c721778263259641de8259f443d765
Bug 1154183 part.7 Don't dispatch preceding keydown events of reserved keypress events on content in the default event group r=smaug

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e01f6bf3b98
https://hg.mozilla.org/mozilla-central/rev/b6bf385a7c2a
https://hg.mozilla.org/mozilla-central/rev/024017da6564
https://hg.mozilla.org/mozilla-central/rev/6c650c359c8e
https://hg.mozilla.org/mozilla-central/rev/b0733a06e7f1
https://hg.mozilla.org/mozilla-central/rev/a12f5a5fff67
https://hg.mozilla.org/mozilla-central/rev/6ef9818a44c7

Comment 49

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336