Closed Bug 1101975 Opened 5 years ago Closed 3 years ago

Alt+Shift+F no longer focuses on Wikipedia search box

Categories

(Core :: User events and focus handling, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s m9+ ---
firefox48 --- wontfix
firefox49 --- fixed

People

(Reporter: botond, Assigned: enndeakin)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

STR:
  1. In today's Nightly, load any Wikipedia page, such as [1].    
  2. Press Alt+Shift+F.

Expected results:
  The Wikipedia search box is given focus.
  This is a feature of Wikipedia that's documented here [2].

Actual results:
  The File menu is activated, as if one had pressed just
  Alt+F instead.

This works fine in Aurora (and as far as I can remember,
worked fine with the nightlies of a few days ago), leading
me to conclude that this is a Firefox regression.
 
[1] http://en.wikipedia.org/wiki/Firefox
[2] http://en.wikipedia.org/wiki/Wikipedia:Keyboard_shortcuts
I can reproduce the problem on e10s window, but not on non-e10s window.

https://hg.mozilla.org/mozilla-central/rev/b4fbeba78a7d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141119030200
https://hg.mozilla.org/mozilla-central/rev/b4fbeba78a7d
Mozilla/5.0 (X11; Linux i686; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141119030200
tracking-e10s: --- → ?
OS: Linux → All
maybe dup of Bug 1060643
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1060643
Actually, I just realized we still experience this problem even though the duplicate bug 1060643 has been fixed. Unduping and reopening.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Hey Neil - any idea what's going on here?
Flags: needinfo?(enndeakin)
Likely, the menu listener (nsMenuBarListener.cpp) is being invoked before the content process receives the keypress event.
Flags: needinfo?(enndeakin)
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1203059
This is not a dup of bug 1203059. Bug 1203059 tries to fix non-e10s mode's shortcut key behavior. However, this bug is a bug of access key handling in content process.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
jeff would like to block on this. isn't this a dupe?
FWIW, I'd like to block on this too. It's one of the most noticeable remaining e10s regressions in my experience.
Neil, is this something you can pick up?
Flags: needinfo?(enndeakin)
Duplicate of this bug: 1212080
Assignee: nobody → enndeakin
Status: REOPENED → ASSIGNED
Flags: needinfo?(enndeakin)
Hey Neil, can we get a status update on this work?
Flags: needinfo?(enndeakin)
Attached patch Handle accesskeys (obsolete) — Splinter Review
This patch works when there is a child remote tab. It breaks other cases.

The key event needs to be handled by the child and nsMenuBarListener::KeyPress should be the default behaviour.
Flags: needinfo?(enndeakin)
Attachment #8738552 - Flags: feedback?(masayuki)
Comment on attachment 8738552 [details] [diff] [review]
Handle accesskeys

>@@ -188,16 +189,30 @@ nsMenuBarListener::KeyPress(nsIDOMEvent*

This is now handled only in bubbling phase of the system event group. For setting mWantReplyFromContentProcess, you should add another event listener which listens to capture phase in the default event group around here:
https://dxr.mozilla.org/mozilla-central/rev/68c0b7d6f16ce5bb023e08050102b5f2fe4aacd8/layout/xul/nsMenuBarFrame.cpp#63,79-81

Then, (!keyEvent->mFlags.mInSystemGroup && keyEvent->mFlags.mInCapturePhase) case is the timing to set mWantReplyFromContentProcess.

>   if (aKeyEvent) {
>     bool eventHandled = false;
>     aKeyEvent->GetDefaultPrevented(&eventHandled);
>     if (eventHandled) {
>       return NS_OK;       // don't consume event
>     }
>   }
> 
>+  // Check if the key is set to the modifiers for keys within the content
>+  // area. If so, the content could override the access key, so set the
>+  // mWantReplyFromContentProcess flag and check for menus when the key
>+  // event gets resent.
>+  WidgetKeyboardEvent* keyEvent =
>+    aKeyEvent->WidgetEventPtr()->AsKeyboardEvent();
>+  if (keyEvent && !keyEvent->mFlags.mNoCrossProcessBoundaryForwarding) {
>+    EventStateManager* esm = mMenuBarFrame->PresContext()->EventStateManager();
>+    if (esm && esm->MatchAccessKeyModifiers(keyEvent, false)) {

And also you shouldn't do nothing if the event target is not a remote process. See here:
https://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLWindowKeyHandler.cpp#430,441-445

>+      keyEvent->mFlags.mWantReplyFromContentProcess = true;
>+      return NS_OK;

And you should stop propagation for the first event propagation in the chrome process since it will be dispatched later. I.g., every key event listener will receive same events twice per key press.
Attachment #8738552 - Flags: feedback?(masayuki)
I tried a number of different combinations of this but couldn't find any that worked. Could you be more specific?


> And also you shouldn't do nothing if the event target is not a remote
> process. See here:
> https://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLWindowKeyHandler.
> cpp#430,441-445
> 

I'm not sure why I want this. Accesskey behaviour should be the same matter what is focused or whatever the target of the key event is.


> >+      keyEvent->mFlags.mWantReplyFromContentProcess = true;
> >+      return NS_OK;

As far as I can tell, mWantReplyFromContentProcess only does anything when a child process responds to the event. I need some mechanism to work when there isn't a content process as well.
Flags: needinfo?(masayuki)
(In reply to Neil Deakin from comment #16)
> > And also you shouldn't do nothing if the event target is not a remote
> > process. See here:
> > https://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLWindowKeyHandler.
> > cpp#430,441-445
> > 
> 
> I'm not sure why I want this. Accesskey behaviour should be the same matter
> what is focused or whatever the target of the key event is.

Oh, right.

> > >+      keyEvent->mFlags.mWantReplyFromContentProcess = true;
> > >+      return NS_OK;
> 
> As far as I can tell, mWantReplyFromContentProcess only does anything when a
> child process responds to the event. I need some mechanism to work when
> there isn't a content process as well.

In my understanding, keyboard events are propagated in following order:

1. in the default event group in the chrome process (capture -> target -> bubble)
2. nsIFrame::HandleEvent() is called in the chrome process
3. in the system event group in the chrome process (capture -> target -> bubble)
4. in the default event group in the content process (capture -> target -> bubble)
5. nsIFrame::HandleEvent() is called in the content process
6. in the system event group in the content process

Additionally, if mWantReplyFromContentProcess is set to true,

7. in the default event group in the chrome process (capture -> target -> bubble)
8. nsIFrame::HandleEvent() is called in the chrome process
9. in the system event group in the chrome process (capture -> target -> bubble)

So, if you need the response from content process, you should stop the propagation #1 - #3 for preventing double action as soon as possible.
Flags: needinfo?(masayuki)
This patch isn't too elegant but seems to work. The approach is to delay the menu access key handling until the content process if any has handled it. There are four situations:

1. When pressing an accesskey when content is focused, key events also get sent to the content process.
2. When pressing an accesskey when chrome is focused, key events don't get sent to the content process. Instead, the HandleAccessKey message is called in TabChild.
3. When no content process is focused, the same codepath is used as now.
4. If another listener is listening for a keyboard shortcut (a <key> element for example), the mWantReplyFromContentProcess flag will already be set so an extra flag is added to key events to check for this case. The test browser_ext_commands_onCommand.js checks for this.

I am going to write a test or adapt an existing test to test all of this.
Attachment #8738552 - Attachment is obsolete: true
Attachment #8743791 - Flags: review?(masayuki)
Sorry for the delay to review this. I'm reviewing the patch, but I still want to understand around that deeper. Perhaps, I can finish the review tomorrow.
Comment on attachment 8743791 [details] [diff] [review]
Handle accesskeys in content process before menus

>@@ -712,16 +712,19 @@ EventStateManager::PreHandleEvent(nsPres
>     // eDrop is fired before eLegacyDragDrop so send the enter/exit events
>     // before eDrop.
>     GenerateDragDropEnterExit(aPresContext, aEvent->AsDragEvent());
>     break;
> 
>   case eKeyPress:
>     {
>       WidgetKeyboardEvent* keyEvent = aEvent->AsKeyboardEvent();
>+      if (keyEvent->mFlags.mNoCrossProcessBoundaryForwarding) {

This must mean that the key event comes from the remote process. So, please add a comment before here for explaining that since the meaning of mNoCrossProcessBoundaryForwarding is changed in some method like here.

> struct AccessKeyInfo
> {
>+  WidgetKeyboardEvent* event;

WidgetKeyboardEvent instance is typically created in the stack. So, creating this struct at heap isn't safe. If this is created only in the stack, please add MOZ_STACK_CLASS.  Otherwise, please copy the event with using Duplicate() or AssignKeyboardEventData().

> static void
> HandleAccessKeyInRemoteChild(TabParent* aTabParent, void* aArg)
> {
>   AccessKeyInfo* accessKeyInfo = static_cast<AccessKeyInfo*>(aArg);
> 
>   // Only forward accesskeys for the active tab.
>   bool active;
>   aTabParent->GetDocShellIsActive(&active);
>   if (active) {
>-    aTabParent->HandleAccessKey(accessKeyInfo->charCodes, accessKeyInfo->isTrusted,
>+    accessKeyInfo->event->mFlags.mWantReplyFromContentProcess = true;
>+    accessKeyInfo->event->mAccessKeyForwardedToChild = true;
>+    aTabParent->HandleAccessKey(*accessKeyInfo->event,
>+                                accessKeyInfo->charCodes,
>                                 accessKeyInfo->modifierMask);

Why don't you stop the propagation?

>   // Now try remote children
>   if (mDocument && mDocument->GetWindow()) {
>     // If the focus is currently on a node with a TabParent, the key event will
>     // get forwarded to the child process and HandleAccessKey called from there.
>     // If focus is somewhere else, then we need to check the remote children.
>     nsFocusManager* fm = nsFocusManager::GetFocusManager();
>     nsIContent* focusedContent = fm ? fm->GetFocusedContent() : nullptr;
>-    if (!TabParent::GetFrom(focusedContent)) {
>-      AccessKeyInfo accessKeyInfo(aAccessCharCodes, aIsTrusted, aModifierMask);
>+    if (TabParent::GetFrom(focusedContent)) {
>+      // A remote child process is focused. The key event should get sent to
>+      // the child process.
>+      aEvent->mFlags.mWantReplyFromContentProcess = true;
>+      aEvent->StopPropagation();

I think that this is not enough, the event will be fired in the system event group. However, you should keep this code for now. I'll fix that in follow up bug because it's EventDispatcher's problem and we already have some bugs due to same reason.

> bool
>-TabChild::RecvHandleAccessKey(nsTArray<uint32_t>&& aCharCodes,
>-                              const bool& aIsTrusted,
>+TabChild::RecvHandleAccessKey(const WidgetKeyboardEvent& aEvent,
>+                              nsTArray<uint32_t>&& aCharCodes,
>                               const int32_t& aModifierMask)
> {
>   nsCOMPtr<nsIDocument> document(GetDocument());
>   nsCOMPtr<nsIPresShell> presShell = document->GetShell();
>   if (presShell) {
>     nsPresContext* pc = presShell->GetPresContext();
>     if (pc) {
>-      pc->EventStateManager()->HandleAccessKey(pc, aCharCodes, aIsTrusted, aModifierMask);
>+      if (!pc->EventStateManager()->HandleAccessKey(&(const_cast<WidgetKeyboardEvent&>(aEvent)), pc, aCharCodes,
>+                                                    aModifierMask)) {
>+        // If no accesskey was found, resend the event back at the parent
>+        // so that accesskeys on menus can be handled.
>+        WidgetKeyboardEvent localEvent(aEvent);
>+        localEvent.mWidget = mPuppetWidget;
>+        SendReplyKeyEvent(localEvent);

I don't think that this is right thing to do here because you may post the message to HandleAccessKey() for two or more remove children (via nsContentUtils::CallOnAllRemoteChildren()). So, the parent will receive two or more times for same keyboard event. This means that same keyboard events are fired multiple times. I think that you should create a new method into TabParent to receive the access key handling result in the remote process.

However, additionally, posting same messages to multiple remote children may cause two or more web contents trying to set focus to the contents. I'm not sure how we can fix this issue...

>@@ -211,20 +211,27 @@ nsMenuBarListener::KeyPress(nsIDOMEvent*
>     bool preventDefault;
>     aKeyEvent->GetDefaultPrevented(&preventDefault);
>     if (!preventDefault) {
>       nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aKeyEvent);
>       uint32_t keyCode, charCode;
>       keyEvent->GetKeyCode(&keyCode);
>       keyEvent->GetCharCode(&charCode);
> 
>+      // If accesskey handling was forwarded to a child process, wait for
>+      // the event to be resent before handling accesskeys.
>+      WidgetKeyboardEvent* nativeKeyEvent =
>+        aKeyEvent->WidgetEventPtr()->AsKeyboardEvent();
>+      if (nativeKeyEvent->mFlags.mWantReplyFromContentProcess &&
>+          !nativeKeyEvent->mFlags.mNoCrossProcessBoundaryForwarding) {
>+        return NS_OK;
>+      }

Hmm, event listeners needing to ignore forwarded events are the bug of EventDispatcher. But for now, this is necessary...

>diff --git a/widget/TextEvents.h b/widget/TextEvents.h
>--- a/widget/TextEvents.h
>+++ b/widget/TextEvents.h
>@@ -104,16 +104,17 @@ protected:
>   WidgetKeyboardEvent()
>     : keyCode(0)
>     , charCode(0)
>     , mPseudoCharCode(0)
>     , location(nsIDOMKeyEvent::DOM_KEY_LOCATION_STANDARD)
>     , isChar(false)
>     , mIsRepeat(false)
>     , mIsComposing(false)
>+    , mAccessKeyForwardedToChild(false)

Be careful, I added a bool member here in other bug.

Sorry for the long delay to review this.
Attachment #8743791 - Flags: review?(masayuki) → review-
> I don't think that this is right thing to do here because you may post the
> message to HandleAccessKey() for two or more remove children (via
> nsContentUtils::CallOnAllRemoteChildren()). So, the parent will receive two
> or more times for same keyboard event. This means that same keyboard events
> are fired multiple times. I think that you should create a new method into
> TabParent to receive the access key handling result in the remote process.
> 
> However, additionally, posting same messages to multiple remote children may
> cause two or more web contents trying to set focus to the contents. I'm not
> sure how we can fix this issue...

Non-active tabs are ignored in HandleAccessKeyInRemoteChild so HandleAccessKey should only be called on one tab.
(In reply to Neil Deakin from comment #21)
> > I don't think that this is right thing to do here because you may post the
> > message to HandleAccessKey() for two or more remove children (via
> > nsContentUtils::CallOnAllRemoteChildren()). So, the parent will receive two
> > or more times for same keyboard event. This means that same keyboard events
> > are fired multiple times. I think that you should create a new method into
> > TabParent to receive the access key handling result in the remote process.
> > 
> > However, additionally, posting same messages to multiple remote children may
> > cause two or more web contents trying to set focus to the contents. I'm not
> > sure how we can fix this issue...
> 
> Non-active tabs are ignored in HandleAccessKeyInRemoteChild so
> HandleAccessKey should only be called on one tab.

Okay, then, please add a bool like |sentToRemote| into AccessKeyInfo and check it with MOZ_RELEASE_ASSERT() if it's never sent to two or more content processes.
> > 
> >   // Only forward accesskeys for the active tab.
> >   bool active;
> >   aTabParent->GetDocShellIsActive(&active);
> >   if (active) {
> >-    aTabParent->HandleAccessKey(accessKeyInfo->charCodes, accessKeyInfo->isTrusted,
> >+    accessKeyInfo->event->mFlags.mWantReplyFromContentProcess = true;
> >+    accessKeyInfo->event->mAccessKeyForwardedToChild = true;
> >+    aTabParent->HandleAccessKey(*accessKeyInfo->event,
> >+                                accessKeyInfo->charCodes,
> >                                 accessKeyInfo->modifierMask);
> 
> Why don't you stop the propagation?

OK, so adding this causes some tests to fail as they test shift+alt+key combinations and expect them to happen synchronously. I suppose I will need to fix these tests.
(In reply to Neil Deakin from comment #23)
> > > 
> > >   // Only forward accesskeys for the active tab.
> > >   bool active;
> > >   aTabParent->GetDocShellIsActive(&active);
> > >   if (active) {
> > >-    aTabParent->HandleAccessKey(accessKeyInfo->charCodes, accessKeyInfo->isTrusted,
> > >+    accessKeyInfo->event->mFlags.mWantReplyFromContentProcess = true;
> > >+    accessKeyInfo->event->mAccessKeyForwardedToChild = true;
> > >+    aTabParent->HandleAccessKey(*accessKeyInfo->event,
> > >+                                accessKeyInfo->charCodes,
> > >                                 accessKeyInfo->modifierMask);
> > 
> > Why don't you stop the propagation?
> 
> OK, so adding this causes some tests to fail as they test shift+alt+key
> combinations and expect them to happen synchronously. I suppose I will need
> to fix these tests.

Hmm, which tests? Currently, we have a bug to dispatch same events twice (one is original key event coming from widget, the other is reply key event from the content process) in chrome process. If you meet this bug, please wait bug 1257617, I'm working on this.
The failing tests are shown at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e691ca69eb5105d8e2bd89f9b009e54836b598c2

I haven't investigated too far, but I think tests are verifying some UI in the urlbar/devtools that expects shortcuts like ctrl+shift+letter in chrome code. Calling StopPropagation prevents them from receiving the event synchronously as the keypress event now happens later.

My understanding is that the redispatched event only resends just the keypress event again and not keydown/keyup? One complication is that the test browser_rules_edit-property-increments.js waits for the keyup event.
> My understanding is that the redispatched event only resends just the keypress event again and not keydown/keyup?

That's same as what my understanding is.
This version doesn't resend key events; it instead creates a separate event just for the menubar to respond to. It operates similar but doesn't change the way key events are fired. This allows existing key listening code and tests to work as is.
Attachment #8743791 - Attachment is obsolete: true
Attachment #8747681 - Flags: review?(masayuki)
Comment on attachment 8747681 [details] [diff] [review]
Handle accesskeys in content process before menus, v2

Almost looks good.

Look like this patch isn't aware of nested remote processes, right? But it's okay for now because we don't need to support such case for now.

>+++ b/dom/base/nsGkAtomList.h
>@@ -675,16 +675,17 @@ GK_ATOM(objectType, "object-type")
> GK_ATOM(observer, "observer")
> GK_ATOM(observes, "observes")
> GK_ATOM(odd, "odd")
> GK_ATOM(OFF, "OFF")
> GK_ATOM(ol, "ol")
> GK_ATOM(omitXmlDeclaration, "omit-xml-declaration")
> GK_ATOM(ona2dpstatuschanged, "ona2dpstatuschanged")
> GK_ATOM(onabort, "onabort")
>+GK_ATOM(onaccesskeynotfound, "onaccesskeynotfound")

Should be onmozacceskeynotfound.
            ^^^

>diff --git a/dom/events/EventNameList.h b/dom/events/EventNameList.h
>@@ -265,16 +265,20 @@ NON_IDL_EVENT(mozbrowserafterkeydown,
> NON_IDL_EVENT(mozbrowserbeforekeyup,
>               eBeforeKeyUp,
>               EventNameType_None,
>               eBeforeAfterKeyboardEventClass)
> NON_IDL_EVENT(mozbrowserafterkeyup,
>               eAfterKeyUp,
>               EventNameType_None,
>               eBeforeAfterKeyboardEventClass)
>+NON_IDL_EVENT(accesskeynotfound,

So, should be mozaccesskeynotfound

>+              eAccessKeyNotFound,

but you don't need "moz" prefix for EventMessage name because this isn't exposed to the web.

>diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp
>   case eKeyPress:
>     {
>+      // Key events can be resent to the parent process after checking
>+      // accesskeys in the content process, so check if the
>+      // mNoCrossProcessBoundaryForwarding flag is set and don't do anything the
>+      // second time.

Hmm, but you don't check it now...

>-TabChild::RecvHandleAccessKey(nsTArray<uint32_t>&& aCharCodes,
>-                              const bool& aIsTrusted,
>+TabChild::RecvHandleAccessKey(const WidgetKeyboardEvent& aEvent,
>+                              nsTArray<uint32_t>&& aCharCodes,
>                               const int32_t& aModifierMask)
> {
>   nsCOMPtr<nsIDocument> document(GetDocument());
>   nsCOMPtr<nsIPresShell> presShell = document->GetShell();
>   if (presShell) {
>     nsPresContext* pc = presShell->GetPresContext();
>     if (pc) {
>-      pc->EventStateManager()->HandleAccessKey(pc, aCharCodes, aIsTrusted, aModifierMask);
>+      if (!pc->EventStateManager()->HandleAccessKey(&(const_cast<WidgetKeyboardEvent&>(aEvent)), pc, aCharCodes,
>+                                                    aModifierMask, true)) {

too long, perhaps,

>      if (!pc->EventStateManager()->
>                 HandleAccessKey(&(const_cast<WidgetKeyboardEvent&>(aEvent)),
>                                 pc, aCharCodes, aModifierMask, true)) {

> bool
>+TabParent::RecvAccessKeyNotHandled(const WidgetKeyboardEvent& aEvent)
>+{
>+  NS_ENSURE_TRUE(mFrameElement, true);
>+
>+  WidgetKeyboardEvent localEvent(aEvent);
>+  localEvent.mMessage = eAccessKeyNotFound;
>+  localEvent.mAccessKeyForwardedToChild = false;
>+
>+  // Here we convert the WidgetEvent that we received to an nsIDOMEvent
>+  // to be able to dispatch it to the <browser> element as the target element.
>+  nsIDocument* doc = mFrameElement->OwnerDoc();
>+  nsIPresShell* presShell = doc->GetShell();
>+  NS_ENSURE_TRUE(presShell, true);

I'm not sure the presShell is always safe here... Could you check nsIPresShell::CanDispatchEvent() here?

> nsresult
>-nsMenuBarListener::KeyPress(nsIDOMEvent* aKeyEvent)
>+nsMenuBarListener::KeyPress(nsIDOMEvent* aKeyEvent, bool aIsAccessKeyNotFound)

I think that aIsAccessKeyNotFound isn't necessary because you can check it with the event message value.

>   if (mAccessKey)
>   {
>-    bool preventDefault;
>-    aKeyEvent->GetDefaultPrevented(&preventDefault);
>-    if (!preventDefault) {
>-      nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aKeyEvent);
>-      uint32_t keyCode, charCode;
>-      keyEvent->GetKeyCode(&keyCode);
>-      keyEvent->GetCharCode(&charCode);
>+    nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aKeyEvent);
>+    uint32_t keyCode, charCode;
>+    keyEvent->GetKeyCode(&keyCode);
>+    keyEvent->GetCharCode(&charCode);

Perhaps, you need to modify GetKeyCode() and GetCharCode() in dom/events/KeyboardEvent.cpp since the switch statements don't check eAccessKeyNotFound.

> 
>+    // If accesskey handling was forwarded to a child process, wait for
>+    // the accesskeynotfound event before handling accesskeys.
>+    WidgetKeyboardEvent* nativeKeyEvent =
>+      aKeyEvent->WidgetEventPtr()->AsKeyboardEvent();
>+    if (nativeKeyEvent->mAccessKeyForwardedToChild) {
>+      return NS_OK;
>+    }

This check should be done before retrieving keyCode and charCode value.

>+#ifndef XP_MACOSX
>+    // Also need to handle F10 specially on Non-Mac platform.
>+    else if (!aIsAccessKeyNotFound && keyCode == NS_VK_F10) {

Looks like tricky, but it's okay for now since aIsAccessKeyNotFound should be false with ordinal modifier state for access key.

>diff --git a/widget/EventMessageList.h b/widget/EventMessageList.h
>@@ -47,16 +47,18 @@ NS_EVENT_MESSAGE(eKeyDown)
> NS_EVENT_MESSAGE(eKeyDownOnPlugin)
> NS_EVENT_MESSAGE(eKeyUpOnPlugin)
> 
> NS_EVENT_MESSAGE(eBeforeKeyDown)
> NS_EVENT_MESSAGE(eAfterKeyDown)
> NS_EVENT_MESSAGE(eBeforeKeyUp)
> NS_EVENT_MESSAGE(eAfterKeyUp)
> 
>+NS_EVENT_MESSAGE(eAccessKeyNotFound)

Please explain the purpose of this event with comment.
Attachment #8747681 - Flags: review?(masayuki) → review-
> 
> Should be onmozacceskeynotfound.
>             ^^^
> 

This event doesn't get sent to content only to chrome. Why is the prefix needed?
(In reply to Neil Deakin from comment #29)
> > 
> > Should be onmozacceskeynotfound.
> >             ^^^
> > 
> 
> This event doesn't get sent to content only to chrome. Why is the prefix
> needed?

Indeed, I worried about non-e10s case, but it must never occur (I guess it might be problem if we will support nested remote content in the future?). However, I still would like you to use moz prefix for that. Using moz prefix is clearer for other developers, explaining what the event is created for internal use explicitly.
Attachment #8747681 - Attachment is obsolete: true
Attachment #8750745 - Flags: review?(masayuki)
Comment on attachment 8750745 [details] [diff] [review]
Handle accesskeys in content process before menus, v3

Thanks!
Attachment #8750745 - Flags: review?(masayuki) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b02f9d6c0586c7d7db405b3fc1ad4347503a180
Bug 1101975, handle access keys in content process before menus, r=masayuki
https://hg.mozilla.org/mozilla-central/rev/1b02f9d6c058
Status: ASSIGNED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Want to uplift this to 48? It is a big patch, but if you think it should uplift, I'd rather try that earlier than later in the cycle, to help smooth the way for e10s rollout.
Flags: needinfo?(enndeakin)
\o/

Neil and Masayuki, thanks a lot for fixing this!
I think this is ok as is.
Flags: needinfo?(enndeakin)
QA Whiteboard: [good first verify]
I have reproduced this bug with Firefox Nightly 36.0a1 (Build ID: 20141119030200) on Windows 8.1, 64-bit.

Verified as fixed with Latest Firefox Beta 49.0b5 (Build ID: 20160818050015)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[bugday-20160824]
I have reproduced this bug with Firefox Nightly 36.0a1 (2014-11-19) on Deepin os ,32 bit.

Verified as fixed with Latest Firefox Beta 49.0b7 (Build ID: 20160825132718)
User Agent : Mozilla/5.0 (X11; Linux i686; rv:49.0) Gecko/20100101 Firefox/49.0.

[bugday-20160824]
Duplicate of this bug: 1301239
I have reproduced this bug with Nightly 36.0a1 (2014-11-19) on Windows 7 , 64 Bit !

This bug's fix is verified with latest Beta

Build ID   20160912134115
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.