792324 - Access keys are fired for non-open popups

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

You'll have to open the Downloads Panel before running the STR above.

Comment 2

[Neil Deakin]

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.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: Only target visible items when handling an access key

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

Comment 4

[Neil Deakin]

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.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: updated based on feedback

Updated based on feedback.  New try run at https://tbpl.mozilla.org/?tree=Try&rev=3bf493d3ebdc

Comment 6

[Neil Deakin]

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.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7c07373996

do we want to track this for branches other than trunk?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, I think we need this for Social API a11y.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eb7c07373996

Comment 11

[:Gavin Sharp [email: [e-mail]]]

I think this is Aurora-suitable for me, I think we should uplift it ASAP.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/93a02587deca

Comment 14

[Virgil Dicu [:virgil] [QA]]

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.

Comment 15

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old