Closed Bug 792324 Opened 7 years ago Closed 7 years ago

Access keys are fired for non-open popups

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox17 + verified

People

(Reporter: jaws, Assigned: markh)

References

Details

Attachments

(1 file, 1 obsolete file)

STR using Firefox Nightly 9/18/2012:
Click the bookmark star to bookmark the current page
Click the bookmark star again to show the bookmarks popup
Press tab to move focus to a button
Press 's'

Expected:
no actions

Actual:
The "Show all downloads" command from the Downloads panel is executed and the Downloads window is shown.
You'll have to open the Downloads Panel before running the STR above.
This is because there is a special accesskey handling case for buttons in the keypress handler in button.xml that doesn't go through the normal accesskey codepath (which skips accesskeys in hidden popups for example). Ideally it should do the same checks as nsEventStateManager::IsAccessKeyTarget.
The following patch attempts to perform the same basic logic as IsAccessKeyTarget, but given the different context it works differently.  For each element, we get the computed style and handle visibility!="visible", display=="none", and a special case for display=="-moz-popup", in which case we check the 'state' property.

There is a test which was stuck in content/events/test/, simply as that is where I found a similar accesskey test - that might not be the best place though.

FWIW, a try run is at https://tbpl.mozilla.org/?tree=Try&rev=a6e8f12e9a6d
Attachment #662811 - Flags: feedback?(enndeakin)
Comment on attachment 662811 [details] [diff] [review]
Only target visible items when handling an access key

>+function test() {
>+  let panel = document.getElementById("panel-1");
>+  panel.addEventListener("popupshown", function onpopupshown() {
>+    panel.removeEventListener("popupshown", onpopupshown);
>+    panel.firstChild.focus();
>+    after_click = function(event) {
>+      is(event.target.id, "button-1", "Accesskey was directed to the button in panel 1.");
>+      panel.hidePopup();
>+      panel = document.getElementById("panel-2");
>+      panel.addEventListener("popupshown", function onpopupshown() {
>+        panel.removeEventListener("popupshown", onpopupshown);
>+        panel.firstChild.focus();
>+        after_click = function(event) {
>+          is(event.target.id, "button-2", "Accesskey was directed to the button in panel 2.");
>+          panel.hidePopup();
>+          SimpleTest.finish();
>+        };
>+        synthesizeKey("X", {});
>+      });
>+      panel.openPopup(null, "", 200, 200, false, false);
>+    }
>+    synthesizeKey("X", {});
>+  });
>+ panel.openPopup(null, "", 100, 100, false, false);
>+}

Since both checks only differ by which popup you use, you could combine them into a single block.

The test should go in toolkit/content/tests

>+          // OK - the node seems visible, so it is a candidate.
>           if (node.localName == "button" && node.accessKey &&
>             !node.disabled && !node.collapsed && !node.hidden)

You could also remove the !node.collapsed && !node.hidden checks as you've now covered them.

This patch doesn't cover all cases but it's better than what is there now.
Attachment #662811 - Flags: feedback?(enndeakin) → feedback+
Updated based on feedback.  New try run at https://tbpl.mozilla.org/?tree=Try&rev=3bf493d3ebdc
Assignee: nobody → mhammond
Attachment #662811 - Attachment is obsolete: true
Attachment #665250 - Flags: review?(enndeakin)
Comment on attachment 665250 [details] [diff] [review]
updated based on feedback

+<panel id="panel-1">
+  <button label="just a normal button"/>
+  <button id="button-1"
+          accesskey="X"
+          oncommand="dump('click on 1\n'); clicked(event)"

Remove the dump here and in the other button.

+          label="Button in panel 1"
+          noautohide="true"

The noautohide attribute should be removed.
Attachment #665250 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7c07373996

do we want to track this for branches other than trunk?
Status: NEW → ASSIGNED
Flags: in-testsuite+
OS: Windows 7 → All
Hardware: x86_64 → All
Yeah, I think we need this for Social API a11y.
Duplicate of this bug: 790820
https://hg.mozilla.org/mozilla-central/rev/eb7c07373996
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
I think this is Aurora-suitable for me, I think we should uplift it ASAP.
Comment on attachment 665250 [details] [diff] [review]
updated based on feedback

[Approval Request Comment]
Bug caused by (feature/regressing bug #): always been this way
User impact if declined: access keys in popups are handled when popups aren't open
Testing completed (on m-c, etc.): been on m-c and aurora for a while
Risk to taking this patch (and alternatives if risky): no regressions have been reported while this has been baking on trunk and aurora
String or UUID changes made by this patch: none

This is a general fix for access keys in panels. It was discovered while working on Social API. If the share/recommend panel is open and the user presses the 'u' key, then the Social API functionality will be removed as it will be treated as an "Undo" on the social activation panel.
Attachment #665250 - Flags: approval-mozilla-beta?
Attachment #665250 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Could reproduce the steps from comment 0 with the help from comment 1 on Ubuntu 12.04 for 9/18/2012 Nightly.

Not reproducible for 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 beta 4
Build ID: 20121031065642

Also not reproducible for Firefox 17.0 Beta 4 on Ubuntu 12.04 and Mac OS X 10.7.5.
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.