[a11y] Pressing View site information button with keyboard no longer focuses the Show connection details button

RESOLVED FIXED in Firefox 62

Status

()

defect
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: Jamie, Assigned: smaug)

Tracking

({access, regression})

Trunk
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 unaffected, firefox62 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

STR:
1. Open https://example.com/
2. Press alt+d to focus the address bar.
3. Press shift+tab to focus the View site information button.
4. Press space to open the site identity panel.
Expected: Focus should land on the Show connection details button.
Actual: Focus goes to limbo.

This is a recent regression (in the last week or so). We have tests for this, so I'm not sure why this wasn't picked up.
Keywords: access
 9:10.85 INFO: Last good revision: 48a5d87cf9bdb59e39653ab331df04ea2a04267d
 9:10.86 INFO: First bad revision: 20d536fd0f2a02bd4527044d367cf98bebbb358d
 9:10.86 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=48a5d87cf9bdb59e39653ab331df04ea2a04267d&tochange=20d536fd0f2a02bd4527044d367cf98bebbb358d

This suggests the culprit is bug 1460069 - enable Shadow DOM in Nightly. Indeed, setting dom.webcomponents.shadowdom.enabled to false and restarting "fixes" the regression.

@Smaug, any idea what's going on here? Some quick pointers:
1. We have a keypress event on this button:
https://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#798
2. which calls a handler which sets a keyboard flag if the event is keypress:
https://searchfox.org/mozilla-central/source/browser/base/content/browser-siteIdentity.js#830
3. If the flag is set, focus gets advanced into the subtree:
https://searchfox.org/mozilla-central/source/browser/base/content/browser-siteIdentity.js#856
I haven't checked yet whether 2) or 3) is the failing step.
Flags: needinfo?(bugs)
We also have a test which covers this, but it isn't failing for some reason:
https://searchfox.org/mozilla-central/source/browser/base/content/test/siteIdentity/browser_identityPopup_focus.js#24
Would this test get run with shadow DOM enabled?
One other bit of detail:

Once focus goes to limbo, and I press tab once, I land on the "Clear cookies and site data" button inside that security panel. But then, neither tab nor shift+tab gets me to that other button, the one with ID identity-popup-security-expander. Accessible States indicate that it is focusable, but it never receives focus.
(In reply to James Teh [:Jamie] from comment #1)
> This suggests the culprit is bug 1460069 - enable Shadow DOM in Nightly.
> Indeed, setting dom.webcomponents.shadowdom.enabled to false and restarting
> "fixes" the regression.
> 
> @Smaug, any idea what's going on here? Some quick pointers:
> 1. We have a keypress event on this button:
> https://searchfox.org/mozilla-central/source/browser/base/content/browser.
> xul#798
> 2. which calls a handler which sets a keyboard flag if the event is keypress:
> https://searchfox.org/mozilla-central/source/browser/base/content/browser-
> siteIdentity.js#830

Through some quick and dirty alert-based logging, I found that this step is still OK. The correct event type is recognized, and the nested if statement from step 3 below is entered.

> 3. If the flag is set, focus gets advanced into the subtree:
> https://searchfox.org/mozilla-central/source/browser/base/content/browser-
> siteIdentity.js#856

The call into commandDispatcher and subsequently nsFocusManager is what's failing here.
Aha, this is exactly the code I was thinking...
Taking
Assignee: nobody → bugs
Flags: needinfo?(bugs)
Posted patch shadow_tab_regression.diff (obsolete) — Splinter Review
Need a test for this.

Jamie, do you know if we have any tests for tab handling for this kind of case?
https://searchfox.org/mozilla-central/source/browser/base/content/test/siteIdentity/browser_identityPopup_focus.js#24 seems to be doing manual focus calls.
(I'm having trouble to understand what the test is actually testing.)
Flags: needinfo?(jteh)
Attachment #8984881 - Flags: review?(mrbkap)
The test is testing that, after pressing Space on the security info button in the awesome bar, popup opens and focus is automatically redirected to the first button. See EventUtils.sendString(" "); two lines above. So the test waits until the panel is shown, then tests whether the button automatically received the focus. This seems to work in the test, at least the test doesn't fail, but not in an actual user interaction condition. Or the tet is somehow not correct.
The test is not testing any tab key handling.
Posted patch shadow_tab_regression_2.diff (obsolete) — Splinter Review
Or I guess we could do this. SubtreeRoot is in general O(1), so this should optimize out FindOwner loop in normal cases.
Attachment #8984881 - Attachment is obsolete: true
Attachment #8984881 - Flags: review?(mrbkap)
Attachment #8985000 - Flags: review?(mrbkap)
Attachment #8985000 - Flags: review?(mrbkap) → review+
Comment on attachment 8985000 [details] [diff] [review]
shadow_tab_regression_2.diff

bah, realized this isn't right after all if aStartContent is a node assigned to a slot.
Attachment #8985000 - Attachment is obsolete: true
(in general the issue is that xul:panel's focus handling is special, so it is not part of any specification.)
Posted patch shadow_tab_regression_3.diff (obsolete) — Splinter Review
FindOwner returns root element if start content isn't in shadow DOM, nor is under any element which is assigned to a slot.

We may need to tweak the setup later if xul:panels start to use Shadow DOM. It is still unclear to me what kind of tab handling we'll want in that case.
Attachment #8985188 - Flags: review?(mrbkap)
(In reply to Olli Pettay [:smaug] from comment #6)
> Jamie, do you know if we have any tests for tab handling for this kind of
> case?
> https://searchfox.org/mozilla-central/source/browser/base/content/test/
> siteIdentity/browser_identityPopup_focus.js#24 seems to be doing manual
> focus calls.
> (I'm having trouble to understand what the test is actually testing.)

It's true that this test doesn't test tab key handling. However, this bug isn't about the tab key; see comment 0. Pressing space on a button programmatically advances focus into the subtree, and it is this programmatic focus advance that fails.

Comment 3 does mention the tab key, but that's a separate bug; I filed bug 1467385 for that.
Flags: needinfo?(jteh)
(In reply to James Teh [:Jamie] from comment #13)
> It's true that this test doesn't test tab key handling.

Specifically, the test covers steps 3 and 4 of the STR from comment 0:

> 3. ... focus the View site information button.
> 4. Press space to open the site identity panel.
> Expected: Focus should land on the Show connection details button.
> Actual: Focus goes to limbo.

What I don't understand is why this test passes, since it doesn't work when you do it manually.
I was on PTO while this bug happened and just noticed it now. Please note that I'm fixing this issue (and other issues with keyboard focus) separately in bug 1467385, but I don't think the two patches are in conflict with each other so please go ahead.
Blocks: 1460069
Status: NEW → ASSIGNED
Priority: -- → P1
See Also: → 1467385
Comment on attachment 8985188 [details] [diff] [review]
shadow_tab_regression_3.diff

Review of attachment 8985188 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsFocusManager.cpp
@@ +3485,5 @@
>          return NS_OK;
>        }
>      }
>  
> +    // If aStartContent is not in scope owned by root element

Nit: "not in a scope owned by the root element"
Attachment #8985188 - Flags: review?(mrbkap) → review+
Attachment #8985188 - Attachment is obsolete: true
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8a53ad30ca1
because of XUL panels, sequential focusing needs to handle also non-document-roots as roots, r=mrbkap
Duplicate of this bug: 1469473
https://hg.mozilla.org/mozilla-central/rev/e8a53ad30ca1
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
I have reproduced this bug with Nightly 62.0a1 (2018-06-05) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build   ID    20180625100047
User Agent    Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0

[bugday-20180620]
I have reproduced this bug with Nightly 62.0a1 (2018-06-05) on Manjaro Linux 17!
This bug's fi x is verified with latest Beta!


Build   ID    20180709172241
User Agent    Mozilla/5.0 (X11; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0
QA Whiteboard: [good first verify] → [good first verify] [bugday-20180711]
You need to log in before you can comment on or make changes to this bug.