Closed Bug 1191897 Opened 4 years ago Closed 4 years ago

[e10s] Keyboard shortcuts don't work when a select menu is open

Categories

(Core :: XUL, defect)

Unspecified
BSDI
defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
e10s m8+ ---
firefox43 --- fixed

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

1. Open a <select> menu in e10s mode.
2. Press any browser shortcut key (ctrl+t for new tab, ctrl+u for view source, ctrl+a for select all, etc)

Expected: Operation succeeds
Actual: Shortcut key is ignored.

Presumably, the popup needs to not trap certain keys when open.
tracking-e10s: --- → +
Attached patch Do this on Windows (obsolete) — Splinter Review
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Don't like the attribute value name here, but the rest seems to work ok.
Attachment #8652965 - Attachment is obsolete: true
Attachment #8654309 - Flags: review?(neil)
Comment on attachment 8654309 [details] [diff] [review]
Add a null check to previous patch

Unfortunately I found a bug in that if you press Ctrl+W while the popup is open then the page closes but the popup doesn't.

>+  // On Windows, don't check shortcuts when the accelerator key is down.
>+#ifdef XP_WIN
>+  WidgetInputEvent* evt = aKeyEvent->GetInternalNSEvent()->AsInputEvent();
>+  if (evt && evt->IsAccel()) {
>+    return false;
>+  }
>+#endif
>+
I notice that nsListControlFrame actually ignores Ctrl and Meta modifiers on all platforms.

>       aKeyEvent->PreventDefault();
>     }
>   }
> 
>   // Since a menu was open, stop propagation of the event to keep other event
>   // listeners from becoming confused.
>-  aKeyEvent->StopPropagation();
>+
>+  if (!item || item->IgnoreKeys() != eIgnoreKeys_Handled) {
>+    aKeyEvent->StopPropagation();
>+  }
I can't decide whether you should still stop propagation in the case of pressing the Alt key, even if ignorekeys="handled".
Attachment #8654309 - Flags: review?(neil) → review-
> Unfortunately I found a bug in that if you press Ctrl+W while the popup is
> open then the page closes but the popup doesn't.

I think that should be treated as a different issue. In fact, removing the <select> from the document or closing the window with script doesn't close the popup either. I filed bug 1203110 on this.


> 
> >+  // On Windows, don't check shortcuts when the accelerator key is down.
> >+#ifdef XP_WIN
> >+  WidgetInputEvent* evt = aKeyEvent->GetInternalNSEvent()->AsInputEvent();
> >+  if (evt && evt->IsAccel()) {
> >+    return false;
> >+  }
> >+#endif
> >+
> I notice that nsListControlFrame actually ignores Ctrl and Meta modifiers on
> all platforms.

I based this on the way combobox type widgets behave on Windows and Mac, as well as how Chrome does this for <select>.



> >-  aKeyEvent->StopPropagation();
> >+
> >+  if (!item || item->IgnoreKeys() != eIgnoreKeys_Handled) {
> >+    aKeyEvent->StopPropagation();
> >+  }
> I can't decide whether you should still stop propagation in the case of
> pressing the Alt key, even if ignorekeys="handled".


I will add this.
tracking-e10s: + → ---
Component: XP Toolkit/Widgets: Menus → General
OS: Unspecified → BSDI
Product: Core → Data Compliance
Component: General → XP Toolkit/Widgets: XUL
Product: Data Compliance → Core
Attached patch Minor updateSplinter Review
Addressed the third point.
Attachment #8654309 - Attachment is obsolete: true
Attachment #8660686 - Flags: review?(neil)
Attachment #8660686 - Flags: review?(neil) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/837fa5b51ffafe2816b644440561ce2ad2a50cc5
changeset:  837fa5b51ffafe2816b644440561ce2ad2a50cc5
user:       Neil Deakin <neil@mozilla.com>
date:       Thu Sep 17 11:20:33 2015 -0400
description:
Bug 1191897, add a flag for popups which allow shortcut keys to not be consumed, fixes shortcuts not working when an e10s select popup is open, r=neil
https://hg.mozilla.org/mozilla-central/rev/837fa5b51ffa
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Points: --- → 2
Depends on: 1246438
Depends on: CVE-2016-5254
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.