1522092 - The Allow/Block dropdown box from Site Information panel can't be selected using keyboard only navigation

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Timea Cernea [:tbabos]]

[Affected versions]:
Nightly 66.0a1 - Build ID: 20190123070847

[Affected platforms]:
Windows 7/10 x64
Mac OS 10.14
Ubuntu 18.04

[Steps to reproduce]:

1. Go to cnn.com/videos
2. Open the Site Information panel
3. Select the Allow/Block dropdown using keyboard only navigation

[Expected]:
The dropdown should be selectable with keyboard only navigation

[Actual Results]:
The dropdown box can't be selected, thus the allow/block state can't be changed using keyboard only navigation.

Comment 1

[Alastor Wu [:alwu]]

How do you use keyboard only navigation? Is there any hotkey for controlling Site information panel?

Comment 2

[Timea Cernea [:tbabos]]

Open the Site Information panel (use the mouse for that) and you can navigate using TAB and select options with Space or Enter.

Comment 3

[Johann Hofmann [:johannh]]

Stealing this into the front-end component. The identity popup can be reached by tabbing to the identity block and hitting space/enter. It should be fully keyboard-navigable as it provides important functionality to the user.

It would be great if you team could take a look at this.

AFAIK the popup dropdown can be selected with the keyboard, so this feels a little weird to me. Would be good to know what's going on.

Comment 4

[Dale Harvey (:daleharvey)]

I can look into this, Timea we are having some different results testing this locally

Both Johann and I are able to select the dropdown / menupopup but are not able to change its values, we are both on OSX (personally on 10.14)

Can you verify what behaviour you see here on which platforms, many thanks

Comment 5

[Timea Cernea [:tbabos]]

MacOS and Ubuntu have the behavior you mentioned Dale, the dropdown can be selected but the values can't be changed.

On Windows there may be a different bug that affects this, the highlights on the Site Information panel appear only for the buttons that lead to the preferences page. The user is not able to select/highlight any other option using keyboard navigation.

I can select some of them by guessing which one should be highlighted. The values for the dropdown still can't be changed as on the other OS.

Here is a recording for the behavior on Windows: https://streamable.com/d83lj - used TAB to navigate.
I believe we logged a bug for this before but I can't find it right now..

Comment 6

[Marco Zehe (:MarcoZ) [out sick]]

Is this a visual issue, or does focus actually not land on these? With NVDA, I feel like I can tab to the controls that I also hear when exploring the accessibility tree. So this might be a visual issue, e. g. you can tab to it, but theming is wrong so you don't see the focus appear. Just wanting to make sure we're all on the same page, e. g. not focusable versus no visible focus outline.

Comment 7

[Timea Cernea [:tbabos]]

There is no visible focus outline, seems to be indeed a visual issue as I can advance for example to the blocked trackers list by guessing when the focus is on "Trackers" on the Site Information panel.

Comment 8

[Johann Hofmann [:johannh]]

That's strange because Dale and I are seeing the exact opposite. We have visual focus on the menulist but can't interact with it. I'm not sure what to make of this.

Timea, can you please try to find a regression window for this issue?

Comment 9

[Marco Zehe (:MarcoZ) [out sick]]

There are two separate issues here:

1. The visual focus is not always visible on Windows. That's a theming problem.

2. The interaction of the permissions dropdown doesn't work because the keyboard handler intercepts up and down arrows and moves focus to different buttons, as I explained in #fx-team on IRC on Friday. The keystrokes simply never get through to the actual dropdown/menu popup widget, but are intercepted some time before to move focus among the buttons.

Comment 10

[Timea Cernea [:tbabos]]

I can reproduce this only on Windows - I don't have visual focus and can't interact with the menulist.

Tried mozzregresion but I'm always getting different good versions and pushlogs, doesn't seem to be accurate at all. The tracking protection options changed a lot so that might have something to do with this.

Johann, can you try this out on Windows? On Mac and Ubuntu I have visual focus but I can't interact with the dropdown menu.

Comment 11

[Alice0775 White]

Attachment: image.png

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c0900754337e0568fd2d9896e950ecf9a77e4351&tochange=92b46ae620367b5891588e68adc1ac6ae3e1900d

Regressed by : 92b46ae62036 Dale Harvey — Bug 1517526 - Let user allow blocked autoplay. r=johannh

The dropdown menu was introduced by Bug 1517526.

Comment 12

[Dale Harvey (:daleharvey)]

I think Johann was meaning a regression range on accessing the drop down in the control centre, not just for autoplay, it can also be accessed on http://www.popuptest.com/popuptest1.html

Comment 13

[Alice0775 White]

Regression window using http://www.popuptest.com/popuptest1.html:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=48a5d87cf9bdb59e39653ab331df04ea2a04267d&tochange=20d536fd0f2a02bd4527044d367cf98bebbb358d

Regressed by: 20d536fd0f2a Olli Pettay — Bug 1460069 - enable Shadow DOM in Nightly, r=emilio

Comment 14

[Alice0775 White]

(In reply to Alice0775 White from comment #13)

Regression window using http://www.popuptest.com/popuptest1.html:
https://hg.mozilla.org/integration/mozilla-inbound/
pushloghtml?fromchange=48a5d87cf9bdb59e39653ab331df04ea2a04267d&tochange=20d5
36fd0f2a02bd4527044d367cf98bebbb358d

Regressed by: 20d536fd0f2a Olli Pettay — Bug 1460069 - enable Shadow DOM in
Nightly, r=emilio

And further bisection with
user_pref("dom.webcomponents.enabled", true);
user_pref("dom.webcomponents.customelements.enabled", true);
user_pref("dom.webcomponents.shadowdom.enabled", true);

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c637da65c1fec33fc36a014485acad5413e09ea&tochange=232dbd5acc3f2be2be59eddb3e7ef1d00d9111a0

RSuspect:Bug 1413834

Comment 15

[Johann Hofmann [:johannh]]

Thank you so much for finding the regression range!

Smaug, can you take a look at this, please? Thanks!

Comment 16

[James Teh [:Jamie]]

I guess the menulist keypress handler should take precedence because it's in the system group. I guess that's the regression here?

That said:

1. The PanelMultiView keydown handler probably shouldn't handle the up/down arrow keys when a menulist has focus. Certainly, if this were an HTML <select>, this would be a problem because the keydown handler would override the default behaviour of the control.
2. To prevent the user from being confused/getting trapped/making unintentional modifications when using up/down arrows, I think up/down arrows in PanelMultiView should skip menulist controls; i.e. you can only tab to those.

Comment 17

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

We could still get this into 66 if anyone can work on it.

Comment 18

[Olli Pettay [:smaug]]

I couldn't see any issue with http://www.popuptest.com/popuptest1.html case, but I can see the issue with site information panel.
I'll try to look at this asap.

My guess is that the issue is in nested xul popups. XUL popups have very special focus handling.

Comment 19

[Olli Pettay [:smaug]]

ok, the regression range wasn't right, as far as I see.
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=49efc43b14387db6c68d70bbf5e9b8195dd7f80f&tochange=0344af5522398276a98629b66df90cca6f395245 looks more correct to me.
So, bug 1462470?

The testcase is a tad different. Just focus the popup blocking "Block"/"Allow" button so that it isn't opened and then trying to change the value using keyboard.
In current Nightly tab key can be used to focus that button, but arrow keys don't change the value.

(there were other issues related to shadow DOM and popups)

Comment 20

[Olli Pettay [:smaug]]

Not being able to change the button value using arrow keys is there with or without shadow DOM enabled at that time (shadow dom pref doesn't exist anymore)

Comment 21

[Alice0775 White]

(In reply to Olli Pettay [:smaug] (massive needinfo queue, ping on IRC on anything urgent) from comment #19)

In current Nightly tab key can be used to focus that button, but arrow keys
don't change the value.

I cannot focus the button with Tab key on Nightly67.0a1.

(In reply to Olli Pettay [:smaug] (massive needinfo queue, ping on IRC on anything urgent) from comment #20)

Not being able to change the button value using arrow keys is there with or
without shadow DOM enabled at that time (shadow dom pref doesn't exist
anymore)

Before Bug 1413834, Tab key can focus the button. And Alt+arrowKey can change the value.

Comment 22

[Alice0775 White]

(In reply to Alice0775 White from comment #21)

(In reply to Olli Pettay [:smaug] (massive needinfo queue, ping on IRC on
anything urgent) from comment #19)

In current Nightly tab key can be used to focus that button, but arrow keys
don't change the value.

I cannot focus the button with Tab key on Nightly67.0a1.

(In reply to Olli Pettay [:smaug] (massive needinfo queue, ping on IRC on
anything urgent) from comment #20)

Not being able to change the button value using arrow keys is there with or
without shadow DOM enabled at that time (shadow dom pref doesn't exist
anymore)

Before Bug 1413834, Tab key can focus the button. And Alt+arrowKey can
change the value.

tested on Windows10

Comment 23

[Olli Pettay [:smaug]]

(In reply to Alice0775 White from comment #21)

(In reply to Olli Pettay [:smaug] (massive needinfo queue, ping on IRC on
anything urgent) from comment #19)

In current Nightly tab key can be used to focus that button, but arrow keys
don't change the value.

I cannot focus the button with Tab key on Nightly67.0a1.
Weird, I certainly can, using up-to-date Nightly, on Linux and Win10

Before Bug 1413834, Tab key can focus the button. And Alt+arrowKey can
change the value.
Sure, there were shadow DOM related issues too. But if you put focus to the button and try to change the value it doesn't seem to work after bug 1462470, or around that time

Comment 24

[Olli Pettay [:smaug]]

FWIW, I started to look at other regression range when the normal sequential navigation code didn't seem to be used at all for the allow/block selecting, or other stuff within identity popup.

Comment 25

[Alice0775 White]

(In reply to Olli Pettay [:smaug] (massive needinfo queue, ping on IRC on anything urgent) from comment #23)

Weird, I certainly can, using up-to-date Nightly, on Linux and Win10

I found, if I open the arrow panel with keyboard, then I can focus the button until browser closing.
However, if I open the arrow panel with mouse click, then I cannot focus the button.

Comment 26

[James Teh [:Jamie]]

TL;DR: This never worked after we started using PanelMultiView keyboard navigation in this panel.

The history of keyboard navigation in that panel is a maze. The PanelMultiView code was only used in this panel in Firefox 61 and later (some time after 2018-03-12). We still don't quite understand why, as it was supposed to be used before that. See bug 1467385 comment 8. The permission controls then only became tabbable again with the fix for bug 1473199. However, I just tested with the 2018-07-07 build (where the bug 1473199 fix landed) and it seems you could never use the arrows to make a selection in these menulists.

Given all of this, I think the correct solution is what I suggested in comment 16.

Comment 27

[James Teh [:Jamie]]

I have a working (though not yet polished) patch to have arrow keys skip menulist. Unfortunately, I then hit another bug.

Even if I disable the keydown handler, the menulist won't change the selection unless I open it and close it (e.g. with f4 then escape). After I do this once while the panel is open, the arrow keys continue to work while the menulist is closed, even if I tab away and back again. I tracked this down to setting .activeChild on the menulist failing:
https://searchfox.org/mozilla-central/source/toolkit/content/widgets/menulist.js#50
this.activeChild = this.mSelectedInternal;
Even though mSelectedInternal is correct, getting activeChild immediately afterwards returns null. I haven't worked out why yet. I guess nsMenuPopupFrame::ChangeMenuItem doesn't like this for some reason, but I can't work out why. It behaves just fine for the menulists in the Options dialog, for example.

Comment 28

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Sounds like we may end up shipping block autoplay with this issue.
But, we could still potentially take a patch in 66 beta.
drno, what do you think? Is it something you can help with? Do you think we should note it as a known issue?

Comment 29

[James Teh [:Jamie]]

Neil, do you have any thoughts/hunches on comment 27?

Comment 30

[Nils Ohlmeier [:drno]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #28)

Sounds like we may end up shipping block autoplay with this issue.
But, we could still potentially take a patch in 66 beta.
drno, what do you think? Is it something you can help with? Do you think we should note it as a known issue?

This is far outside the Media teams expertise. But I'll bring it up in the block auto play meeting and I'll report back our conclusion.

Comment 31

[Neil Deakin]

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

Neil, do you have any thoughts/hunches on comment 27?

Without seeing this patch, I'm can't really say.

I'd personally wonder why the arrow keys are doing anything at all in that panel since the right way to move between fields in a dropdown that isn't a menu is to use the tab key.

Comment 32

[James Teh [:Jamie]]

(In reply to Neil Deakin from comment #31)

Without seeing this patch, I'm can't really say.

Sorry; I was referring to the menulist behaviour:

Even if I disable the keydown handler,

so that PanelMultiView isn't overriding any keys at all; you can do this by commenting out addEventListener for keydown here:
https://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#1117

the menulist won't change the selection unless I open it and close it (e.g. with f4 then escape). After I do this once while the panel is open, the arrow keys continue to work while the menulist is closed, even if I tab away and back again. I tracked this down to setting .activeChild on the menulist failing:
https://searchfox.org/mozilla-central/source/toolkit/content/widgets/menulist.js#50

this.activeChild = this.mSelectedInternal;

Even though mSelectedInternal is correct, getting activeChild immediately afterwards returns null. I haven't worked out why yet. I guess nsMenuPopupFrame::ChangeMenuItem doesn't like this for some reason, but I can't work out why. It behaves just fine for the menulists in the Options dialog, for example.

Any thoughts on that specifically?

As to your other question:

I'd personally wonder why the arrow keys are doing anything at all in that panel since the right way to move between fields in a dropdown that isn't a menu is to use the tab key.

Agreed. I think the problem is that the idea of a "menu" is somewhat blurry now. The Firefox hamburger menu is arguably a menu and keyboard users expect it to behave like one, even though it's not a XUL menu. PanelMultiView is used for things like the hamburger menu but also for things like the Site Identity panel, and it doesn't differentiate between things that are more menu-like (the former) and things that are more panel-like (the latter). (I guess that distinction is irrelevant except for keyboard usage.) To make matters more complicated, you can now put complex controls like text boxes in the "menu-like" panels.

Comment 33

[Neil Deakin]

Any thoughts on that specifically?

Considering that when I try it the menulist size jumps about as I change the current selection, it looks like it might not be picking up the sizetopopup properly. Could be a regression from 1519917. Can you please file a new bug?

Comment 34

[Nils Ohlmeier [:drno]]

(In reply to Nils Ohlmeier [:drno] from comment #30)

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #28)

Sounds like we may end up shipping block autoplay with this issue.
But, we could still potentially take a patch in 66 beta.
drno, what do you think? Is it something you can help with? Do you think we should note it as a known issue?

This is far outside the Media teams expertise. But I'll bring it up in the block auto play meeting and I'll report back our conclusion.

Since this appears to affect other things in the Site Information panel we concluded this is not a blocker for block auto play. But I don't think we have the right people in the block auto play team to be able to offer help here.

Comment 35

[James Teh [:Jamie]]

(In reply to Nils Ohlmeier [:drno] from comment #34)

Since this appears to affect other things in the Site Information panel we concluded this is not a blocker for block auto play.

It does affect other things, yes, but those things were existing features. What this essentially means for block autoplay is that from a user perspective, we're shipping a new feature which is partially inaccessible. I'd argue that a11y should be a blocker on any new feature so as to prevent further decline in feature a11y.

Comment 36

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

I'll mention this concern in the feature review and product cross-functional meetings. I agree with Jamie that we shouldn't let accessibility slip when shipping new features. But, that is likely a decision for the product team.

Comment 37

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

FYI Asa for a11y product team.

Comment 38

[Johann Hofmann [:johannh]]

I don't think I can add much to this, it's been investigated quite properly by Jamie et al already, thank you for that.

I would say that for both popups and block autoplay this bug has quite a high priority. Jamie, is it okay to assign you this bug?

FWIW I'm happy to disable arrow-key navigation in the identity popup entirely if it helps us get to a solution faster. I think there's an attribute for that already.

Comment 39

[James Teh [:Jamie]]

I don't really have the cycles at present to get a full solution working, including tests (which tend to be difficult to get right for such things). However, I looked into setting disablekeynav and I think that is an acceptable short-term solution for 66. This is more like a dialog, so I would expect users to tab through it rather than arrowing through it anyway.

Longer term, I think the PanelMultiView key nav code needs quite a bit of reworking.

Comment 40

[James Teh [:Jamie]]

Attachment: Bug 1522092: Disable PanelMultiView's keyboard navigation code for the Site Identity panel.

PanelMultiView's keyboard navigation code currently overrides the arrow keys in menulists.
This breaks the permission selectors in the Site Identity panel.
For now, just disable this keyboard navigation code.
DOM will then handle tabbing as it normally would.
This panel is more like a dialog than a menu, so users will generally navigate it with the tab key rather than the arrow keys anyway.
Note that the code in PanelMultiView which makes controls focusable still runs even with disablekeynav set, which is what we want.

Comment 41

[James Teh [:Jamie]]

Note that we still need the (not yet submitted) patch for bug 1530594 to fully fix this issue. The patch here allows arrow keys to get through to the menulist. The patch in bug 1530594 will fix the menulist so the arrow keys actually work.

Comment 42

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

Attachment: Bug 1522092 - Investigate and debug why keyboard navigation of menulist isn't working.

This patch allows for keyboard controls to change the selected item in the menulist, but only after the menulist is opened. The user would have to Tab to the menulist, then hit Space or Enter to open the popup. The user can then use Up or Down to change the selected menuitem. If they hit Escape to close the popup they can continue to use Up or Down to change the selection.

The curious part is why that popup needs to be opened the first time. Debugging this code, I see that in nsXULPopupManager::HandleKeyboardNavigationInPopup(), aFrame->GetCurrentMenuItem() is returning NULL. I believe this might be caused by the popup frame not being created at this point. Could this happen because the panel that holds the popup was hidden? Can we create the frame on demand? Would it make more sense to call nsXULPopupManager::HandleKeyboardNavigation() instead (not the *InPopup variation) from XULMenuElement::HandleKeyPress() ?

Comment 43

[Neil Deakin]

The curious part is why that popup needs to be opened the first time. Debugging this code, I see that in nsXULPopupManager::HandleKeyboardNavigationInPopup(), aFrame->GetCurrentMenuItem() is returning NULL. I believe this might be caused by the popup frame not being created at this point. Could this happen because the panel that holds the popup was hidden? Can we create the frame on demand? Would it make more sense to call nsXULPopupManager::HandleKeyboardNavigation() instead (not the *InPopup variation) from XULMenuElement::HandleKeyPress() ?

All of that is dealt with in bug 1530594.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20027ade109f
Disable PanelMultiView's keyboard navigation code for the Site Identity panel. r=johannh

Comment 45

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/20027ade109f

Comment 46

[Nils Ohlmeier [:drno]]

Jamie, as we need this for block auto play in 66 can you please request uplift to Beta, or let me know when I should do it.

Comment 47

[James Teh [:Jamie]]

I haven't requested uplift yet because the solution for the issue described here still depends on bug 1530594, which unfortunately got backed out. I was hoping to verify the fix with both patches in Nightly before requesting uplift, as we don't want to uplift patches which don't actually solve the problem. (I'm 99% sure that these two patches combined will solve it, but I'd prefer to be certain.)

Comment 48

[James Teh [:Jamie]]

I verified that the issue described here is fixed in Nightly 20190305214137.

Comment 49

[James Teh [:Jamie]]

Comment on attachment 9047274 [details]
Bug 1522092: Disable PanelMultiView's keyboard navigation code for the Site Identity panel.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Unknown.
• User impact if declined: Permission selectors in the Site identity panel will not be adjustable via the keyboard, which makes them inaccessible to screen reader users, keyboard only users, etc. This is particularly relevant given that block autoplay needs this for full functionality.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
  1. Open https://www.youtube.com/watch?v=dQw4w9WgXcQ
  2. Open the Site Information panel.
  3. Tab to the dropdown for the Autoplay sound permission.
  4. Use the up and down arrows to adjust the value.
  • Expected: The value should change.
• List of other uplifts needed: Bug 1530594
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simply sets a supported attribute to disable arrow key navigation. Based on the appearance of this panel, users generally wouldn't attempt to move through controls with the arrow keys.
• String changes made/needed: None.

Comment 50

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9047274 [details]
Bug 1522092: Disable PanelMultiView's keyboard navigation code for the Site Identity panel.

Fix for keyboard nav for block autoplay.
OK for uplift for beta 14.

Comment 51

[Asa Dotzler [:asa]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #50)

Comment on attachment 9047274 [details]
Bug 1522092: Disable PanelMultiView's keyboard navigation code for the Site Identity panel.

Fix for keyboard nav for block autoplay.
OK for uplift for beta 14.

We also need bug 1530594 to go along with this.

Comment 52

[Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA]

I have reproduced this bug on an affected Nightly build from 2019-01-03, and by using the STR from comment 0.

This issue is not reproducing anymore on latest Nightly 67.0a1 (2019-03-07) under Windows 10 x64 and Ubuntu 18.04 x64.

However, on macOS 10.13 and 10.12 I cannot seem to navigate to the "Permissions" section in the door hanger; the focus stops at the "Report a problem" link after pressing the tab key, and it is stuck there even though the key is pressed multiple times.

James, is this bug supposed to be fixed on mac as well, any thoughts regarding the above? Thank you!

Comment 53

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c4d0a83454a2

Comment 54

[James Teh [:Jamie]]

I don't have access to a Mac, so I can't test this. Does this behave as expected if you set Mac keyboard access to all controls? See https://support.apple.com/en-au/HT204434#fullkeyboard . If so, this is probably expected behaviour for Mac.

Comment 55

[Neil Deakin]

Yes, I just tested this and the Mac behaviour is correct, although it the link probably shouldn't be tabbable either.

Comment 56

[Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA]

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

I don't have access to a Mac, so I can't test this. Does this behave as expected if you set Mac keyboard access to all controls? See https://support.apple.com/en-au/HT204434#fullkeyboard . If so, this is probably expected behaviour for Mac.

Yes, the problem goes away after setting the Mac keyboard to access all controls. Marking this as verified fixed on 67.0a1; tested with macOS 10.13.

Leaving the ni? in place for now to verify it on 66 RC as well, once is available.

Comment 57

[Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA]

This is also verified fixed on 66.0 (20190311211048) running Windows 10 x64, macOS 10.14 and Ubuntu 18.04 x64.