1539984 - Some items in the Site Identity panel are not focusable/navigatable if you click the Site Identity icon and navigate to them

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Itiel]

STR:

1. Restart Nightly or start it from fresh
2. Open https://bugzilla.mozilla.org/home
3. Click the Site Identity icon in the URL bar (the {i) icon)
4. Using Tab/Arrow keys try to navigate to "Trackers", "Cookies", "Standard (cog)" and the permissions section's cog

AR:
You can't.

ER:
You should be able to.

This doesn't reproduce if you Ctrl+L > Shift+Tab and Enter, and after doing so you wouldn't be able to reproduce the STR above. Restarting Nightly will make this reproducible.

Comment 1

[James Teh [:Jamie]]

I think this occurs because we no longer use PanelMultiView keyboard navigation in the Site Identity panel as per the short-term workaround in bug 1522092. Unfortunately, that means PanelMultiView doesn't run the initialisation code which makes these controls focusable. (It does run that code if you activate the panel from the keyboard, though, because PanelMultiView has to focus the first control in that case.)

This will probably be fixed by bug 1477673, where we will fix PanelMultiView's key nav code and then switch Site Identity to use it again.

Unfortunately, this probably means this bug is a regression introduced by bug 1522092. Easiest way to work that out would be to test with Firefox 66.

Comment 2

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #1)

Unfortunately, this probably means this bug is a regression introduced by bug 1522092. Easiest way to work that out would be to test with Firefox 66.

Ug. Nope, because we uplifted that patch to 66. So we'd need to test with 65 (or just revert the patch and test with a local build).

Comment 3

[James Teh [:Jamie]]

I confirmed that this was regressed by bug 1522092. :(

Comment 4

[Neha Kochar [:neha]]

Assigning to Jamie so it can be closed after bug 1477673 gets fixed and is confirmed to fix this issue too.

Comment 5

[Neha Kochar [:neha]]

bug 1477673's fix bounced so still continuing to wait on that.

Comment 6

[Itiel]

The STR can still be reproduced after bug 1477673, though the issue is a bit different now.
You can now open all of the items in the panel, but they're not focusable (except for the 2 cog buttons), meaning there's no visual representation of them being selected.
And again, you must first click the Site Identity icon in the URL bar the first time in Nightly's session to reproduce this.

Comment 7

[James Teh [:Jamie]]

(In reply to Itiel from comment #6)

The STR can still be reproduced after bug 1477673, though the issue is a bit different now.
You can now open all of the items in the panel, but they're not focusable (except for the 2 cog buttons),

Are you still doing this with the tab key? If they're not focusable (and thus not getting focus), how are you opening them?

I can't reproduce this. Even if I click the Site identity icon, tab then gets me to all the controls. That said, I can't see the screen, so perhaps they're somehow getting focus without there being visual indication. I don't know why that would be, though.

Can you provide complete and updated STR (including exact actions and expected results at each point) for clarity? Thanks.

Comment 8

[Itiel]

(In reply to James Teh [:Jamie] from comment #7)

Are you still doing this with the tab key?

Doesn't matter if you're tabbing or using the space key.

If they're not focusable (and thus not getting focus), how are you opening them?

I meant visually focused. Behind the scenes they are actually focused.

That said, I can't see the screen, so perhaps they're somehow getting focus without there being visual indication.

Yes, that's the case.

Can you provide complete and updated STR (including exact actions and expected results at each point) for clarity? Thanks.

Sure!

1. Restart Nightly (or be in a session that you didn't open the Site Identity panel by keyboard navigation means)
2. Open bugzilla.mozilla.org
3. Click the Site Identity icon
4. Use the ArrowDown or Tab key once to navigate to the "Show connection details" (>) button -- the button is not outlined or changing to a hover state. The same issue reproduces for all buttons in this panel except for the 2 cog buttons
5. Even though it seems nothing is focused/selected, hit Enter -- will work as expected
6. Close the Site Identity panel
7. You can repeat steps 3-6 as many times as you'd like, as long as you don't open the Site Identity panel by keyboard navigation means
8. Ctrl+L > Shift+Tab > Enter, and repeat step #4 -- this will now work as expected until the next restart to Nightly.

Hope that's clear enough :)

Comment 9

[James Teh [:Jamie]]

Thanks. Since focus is being set correctly, I have no idea why it isn't being reflected visually. Given that I can't see, this is not something I can investigate. Gijs, can you help find someone to assist with this? See comment 8. (Note that a11y does report focus, so this seems to be purely visual.)

Comment 10

[:Gijs (he/him)]

This doesn't reproduce on mac. This is caused by how we track "should we show focus rings" on Windows/Linux. The elements that have working visual feedback use :focus and the elements that are broken use :-moz-focusring.

Generally speaking, I believe :-moz-focusring is preferred, to avoid showing the focused styling for users who navigate the UI by clicking things, and where showing what to them is effectively a "hover" style when the item is not actually hovered by the cursor (after pressing a button once, for instance) is confusing.

I think the bug here is that tabbing around the panel somehow doesn't trip the window-global "This user uses keyboard navigation" state flag, whereas using shift-tab + enter to open the identity popup from the navbar does do that.

It looks like the panelmultiview code that sets focused elements in response to keypresses might need to pass FLAG_BY_KEY, based on https://searchfox.org/mozilla-central/rev/d33d470140ce3f9426af523eaa8ecfa83476c806/dom/base/nsGlobalWindowInner.cpp#4023-4028 which is used in ther window's "do we need to show focus rings" tracking code.

Comment 11

[:Gijs (he/him)]

Attachment: Bug 1539984 - pass along whether a focus change was tripped by a keypress to ensure :-moz-focusring works as designed, r=Jamie

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fea732af766
pass along whether a focus change was tripped by a keypress to ensure :-moz-focusring works as designed, r=Jamie

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/291b093ea26a
Backed out changeset 6fea732af766 for browser_PanelMultiView_keyboard.js failures CLOSED TREE

Comment 14

[:Gijs (he/him)]

So the spacebar insertion instead wipes the field, indicating its contents were selected. That's why the test fails. Why only on mac? "home" on a mac does not, in fact, move focus to the start of the line (cmd-arrow does). It only happened to work because the test wasn't selecting the input before. Now that we're moving focus with the keyboard, that's totally what it does because the HTML input code checks for the last focus movement method when deciding whether to select field contents, and it makes things sad. But only on mac, because on linux/windows dispatching 'home' fixed things.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ffdb79d8075
pass along whether a focus change was tripped by a keypress to ensure :-moz-focusring works as designed, r=Jamie

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4ffdb79d8075

Comment 17

[:Gijs (he/him)]

I'm tempted to suggest we let this ride with 68 (ie wontfix on 67) as I don't think the steps in comment #0 (never using tab/shift-tab and then clicking to open the identity panel and then starting to use tab/shift-tab) are all that common. Itiel / Johann, could you share if that seems like the right decision to you?

Comment 18

[Johann Hofmann [:johannh]]

I agree, seems like an edge-case. :)

Comment 19

[Itiel]

(In reply to Johann Hofmann [:johannh] from comment #18)

I agree, seems like an edge-case. :)

Same here.
Additionally, this may give me (or others) enough time to find unforeseeable regression(s) that may be caused by this fix.

Comment 20

[:Gijs (he/him)]

Thanks!

Comment 21

[Itiel]

Fixed on latest Nightly.

Comment 22

[Camelia Badau [:cbadau], Desktop Test Engineering]

Marking VERIFIED FIXED based on comment 21.