Closed Bug 1522092 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Site Identity, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 67
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- disabled
firefox65 --- disabled
firefox66 + verified
firefox67 --- verified

People

(Reporter: tbabos, Assigned: Jamie)

References

Details

(Keywords: access, regression)

Attachments

(2 files, 1 obsolete file)

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

Rank: 25
Priority: -- → P3

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

Flags: needinfo?(timea.babos)

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

Flags: needinfo?(timea.babos)

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.

Component: Audio/Video: Playback → Site Identity and Permission Panels
Keywords: access
Priority: P3 → P2
Product: Core → Firefox

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

Flags: needinfo?(timea.babos)

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

Flags: needinfo?(timea.babos)

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.

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.

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?

Flags: needinfo?(timea.babos)

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.

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.

Flags: needinfo?(timea.babos)
Attached image 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.

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

(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

Thank you so much for finding the regression range!

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

Flags: needinfo?(bugs)

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.

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

Flags: needinfo?(jhofmann)

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.

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)

Flags: needinfo?(bugs)

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)

No longer blocks: 1413834

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

(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

(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

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.

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

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.

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.

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?

Flags: needinfo?(drno)

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

Flags: needinfo?(enndeakin)

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

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

Flags: needinfo?(enndeakin)

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

Flags: needinfo?(enndeakin)

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?

Flags: needinfo?(enndeakin)

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

Flags: needinfo?(drno)

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

Depends on: 1530594

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.

FYI Asa for a11y product team.

Flags: needinfo?(asa)

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.

Flags: needinfo?(jhofmann) → needinfo?(jteh)

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.

Flags: needinfo?(jteh)

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.

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.

Assignee: nobody → jteh
Status: NEW → ASSIGNED
Priority: P2 → P1

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() ?

Attachment #9047404 - Attachment is obsolete: true

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.

Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20027ade109f
Disable PanelMultiView's keyboard navigation code for the Site Identity panel. r=johannh
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Flags: needinfo?(asa)

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.

Flags: needinfo?(jteh)

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

Flags: needinfo?(jteh)

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

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.
Attachment #9047274 - Flags: approval-mozilla-beta?

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.

Attachment #9047274 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

QA Whiteboard: [qa-triaged]

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!

Flags: needinfo?(jteh)

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.

Flags: needinfo?(jteh) → needinfo?(ciprian.georgiu)

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

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

Status: RESOLVED → VERIFIED

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

Flags: qe-verify+
Flags: needinfo?(ciprian.georgiu)
Depends on: 1539976
Depends on: 1539984
Regressions: 1539976
Regressions: 1539984
No longer depends on: 1539976
You need to log in before you can comment on or make changes to this bug.