James Teh [:Jamie] Reporter
	Description • 6 years ago

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.

	James Teh [:Jamie] Reporter
	Comment 1 • 6 years ago

 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.

	James Teh [:Jamie] Reporter
	Comment 2 • 6 years ago

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?

	Marco Zehe (:MarcoZ)
	Comment 3 • 6 years ago

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.

	Marco Zehe (:MarcoZ)
	Comment 4 • 6 years ago

(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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 5 • 6 years ago

Aha, this is exactly the code I was thinking...
Taking

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 6 • 6 years ago

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.)

	Marco Zehe (:MarcoZ)
	Comment 7 • 6 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 8 • 6 years ago

The test is not testing any tab key handling.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 9 • 6 years ago

Or I guess we could do this. SubtreeRoot is in general O(1), so this should optimize out FindOwner loop in normal cases.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 10 • 6 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 11 • 6 years ago

(in general the issue is that xul:panel's focus handling is special, so it is not part of any specification.)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 12 • 6 years ago

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.

	James Teh [:Jamie] Reporter
	Comment 13 • 6 years ago

(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.

	James Teh [:Jamie] Reporter
	Comment 14 • 6 years ago

(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.

	Johann Hofmann [:johannh]
	Comment 15 • 6 years ago

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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 16 • 6 years ago

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"

	Pulsebot
	Comment 18 • 6 years ago

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

	Andreea Pavel [:apavel]
	Comment 20 • 6 years ago
bugherder

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

	Sajedul Islam
	Comment 21 • 6 years ago

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]

	Mohammad Maruf Rahman
	Comment 22 • 6 years ago

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