1561517 - The autoplay drop down options are displayed in about:preferences page when navigating with Tab+Enter while the autoplay drop down is open

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect)

Description

[Gabi Cheta [:Gabi] Desktop QA]

Attachment: autoplay-issue.gif

• [Affected versions]:
  69.0a1 (2019-06-24) (64-bit)

• [Affected platforms]:
  macOS 10.14
  macOS 10.13
  Windows 10x64
  Ubuntu 18.4x64

[Steps to reproduce]:

1. Launch Firefox
2. Navigate to a video
3. Click on the Site Information Panel and reveal the block autoplay options by clicking open the autoplay drop down
4. Start navigation through the site information panel using the Tab key
5. Press Enter key when Settings option is reached using the Tab key

• [Expected result]:
  The autoplay drop down options are displayed in about:preferences page when navigating with Tab+Enter while the autoplay drop down is open

• [Actual result]:
  Autoplay drop down options should not appear in the Preferences page

[Regression range]:
Issue introduced with the new autoplay

[Note 1]: For more information on how to reproduce this issue see the attached gif.
[Note 2}: On macOS/Ubuntu, empty/black panel is displayed in the Preferences page

Comment 1

[Gabi Cheta [:Gabi] Desktop QA]

Attachment: MacOS.png

Comment 2

[Johann Hofmann [:johannh]]

I'm not really sure where this bug should live, maybe in some Core component? Moving this to "Menus" for now...

Apparently we don't close that panel when its parent panel closes. We could probably just hack around it in the identity popup somehow, but that seems a bit brittle considering we probably have more of these dropdown menus in panels. Can we make them close automatically?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I think this is because we don't fire pagehide in the panel document when the panel goes away, so the SelectChild doesn't know to tell the parent to close the popup.

Hey Neil, is there some other mechanism we can / should use in SelectContent.jsm to notice when the panel is going away?

Comment 4

[Neil Deakin]

The popup should close at step 4 when the tab key is first pressed. It looks like some key listener for the site panel is trapping events. It's also preventing keyboard navigation in the menu.

It doesn't happen in 67.0.4

Comment 5

[Johann Hofmann [:johannh]]

This could have been introduced by one of Jamie's recent patches to improve key navigation in the identity popup, though he certainly has a better overview of those changes than me, so forwarding to him.

In any case this doesn't seem like a critical issue to me.

Comment 6

[James Teh [:Jamie]]

(In reply to Neil Deakin from comment #4)

The popup should close at step 4 when the tab key is first pressed. It looks like some key listener for the site panel is trapping events.

PanelMultiView manages the tab key itself so we have complete control of the tab order and can keep it in sync with the focus order used by the arrow keys. This was re-introduced in bug 1477673 (Firefox 68) after being disabled in bug 1522092 (Firefox 67). However, we trapped the tab key for a long time before that (Firefox 66 and earlier).

I guess we'll need to deal with closing the focused menulist when the tab key is pressed.

It's also preventing keyboard navigation in the menu.

I guess that's bug 1561581. I still can't reproduce that on Windows (I don't have a mac) and I don't understand why it's happening.

Comment 7

[James Teh [:Jamie]]

I added code to bale out of our keydown handler (without preventing or stopping the event) if tab is pressed and an open menulist is focused. It works in the sense that the menulist gets dismissed, but it also dismisses the entire Site Info panel. I then disabled the PanelMultiView keydown handler altogether in case there's some other interference. That didn't fix the problem.

Is there any reason the tab key should dismiss the outer panel as well (not just the menulist)? Tested in Windows. Note that the escape key dismisses the menulist without dismissing the outer panel.

I was able to reproduce this with a minimal test case in the Browser Toolbox:

p = document.createXULElement("panel");
document.getElementById("nav-bar").append(p);
ml = document.createXULElement("menulist");
p.append(ml);
mp = document.createXULElement("menupopup");
ml.append(mp);
mp.append(document.createXULElement("menuitem"));
p.append(document.createXULElement("button"));
setTimeout(() => p.openPopup(), 3000);

STR:

1. Run that code in the Browser Toolbox console.
2. Switch back to the main Firefox window within 3 seconds.
3. Wait for the popup to open.
4. Press tab to focus the menulist.
5. Press alt+downArrow to expand it.
6. Press tab.
   • Expected: The menulist should be closed, but the panel (p) should remain open.
   • Actual: Both the menulist and the panel are dismissed.

Comment 8

[James Teh [:Jamie]]

Attachment: Bug 1561517: PanelMultiView: Don't override the tab key when an open menulist has focus.

Tab in an open menulist should close the menulist.
Previously, we were overriding the tab key in this case, which meant that the menulist remained open while focus moved elsewhere.

Comment 9

[Neil Deakin]

Is there any reason the tab key should dismiss the outer panel as well (not just the menulist)? Tested in Windows. Note that the escape key dismisses the menulist without dismissing the outer panel.

We should ideally be running the block at https://searchfox.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp#223 to limit closing to similar-type popups only, but the passed in aLastRolledUp argument is null. Likely, passing a dummy non-null pointer for the tab key (but not F10) at https://searchfox.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp#2199 would fix this.

Comment 10

[Johann Hofmann [:johannh]]

So, can we implement the suggestion from comment 9 or are we deciding to make the trade-off that closing both panels is better than the current behavior and do comment 9 in a follow-up? I'm not sure which behavior is worse, but I'd like us to be explicit about what we are doing here.

Thanks!

Comment 11

[James Teh [:Jamie]]

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

So, can we implement the suggestion from comment 9 or are we deciding to make the trade-off that closing both panels is better than the current behavior and do comment 9 in a follow-up?

The patch I posted for your review in comment 8 makes it so that pressing tab closes both panels. While incorrect IMO, it's consistent with the behaviour of menulists inside panels when PanelMultiView is not at play; i.e. it "fixes the regression".

I don't have the cycles right now to push a polished patch for comment 9 and I don't want this to delay fixing the regression.

I'm not sure which behavior is worse,

I think the menulist staying open while Preferences is opened is worse, though I agree closing both panels is undesirable.

Comment 12

[James Teh [:Jamie]]

Filed bug 1566673 for comment 9.

Comment 13

[Johann Hofmann [:johannh]]

Ok, thanks, that seems fine to me.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3ea4567267f
PanelMultiView: Don't override the tab key when an open menulist has focus. r=johannh

Comment 15

[Cosmin Sabou [:CosminS]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Looks like a pretty simple fix. Did you want to nominate this for Beta and ESR68 approval, Jamie?

Comment 17

[James Teh [:Jamie]]

Comment on attachment 9077597 [details]
Bug 1561517: PanelMultiView: Don't override the tab key when an open menulist has focus.

Beta/Release Uplift Approval Request

• User impact if declined: Broken/confusing UI behaviour when users tab away from an open dropdown in the Site Identity panel.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial, very specific fix (open menulists in PanelMultiView panels) covered by automated tests.
• String changes made/needed: None.

Comment 18

[James Teh [:Jamie]]

IMO, this isn't "major" enough for ESR. Also, while it fixes the behaviour so it's consistent with dropdowns in some other panels, the behaviour is still buggy (bug 1566673).

Comment 19

[Gabi Cheta [:Gabi] Desktop QA]

Issue still reproducing on macOS 10.12.6 with Nightly 70.0a1 , empty panel is displayed in the Preferences page when navigating with Tab key while the block auto play drop down is opened.

Comment 20

[James Teh [:Jamie]]

(In reply to Gabi Cheta [:Gabi] Release Desktop QA from comment #19)

Issue still reproducing on macOS 10.12.6 with Nightly 70.0a1 , empty panel is displayed in the Preferences page when navigating with Tab key while the block auto play drop down is opened.

So the issue is slightly different now; i.e. an empty panel as opposed to the autoplay dropdown options described in comment 0?

The autoplay drop down options are displayed in about:preferences page when navigating with Tab+Enter while the autoplay drop down is open

Johann, is there a Mac developer who can take a look at this? I've implemented a fix, I have an automated test which proves it, yet apparently, it isn't fixed. I don't have access to a Mac, so I can't really progress this any further.

Comment 21

[James Teh [:Jamie]]

Investigation note: it'd be interesting to know whether this code is ever hit when pressing tab while the autoplay dropdown is open on Mac:
https://searchfox.org/mozilla-central/rev/4436573fa3d5c4311822090e188504c10c916c6f/browser/components/customizableui/PanelMultiView.jsm#1739
If not, why not? Is focus.localName not "menulist" for some reason?

Comment 22

[Gabi Cheta [:Gabi] Desktop QA]

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

(In reply to Gabi Cheta [:Gabi] Release Desktop QA from comment #19)

Issue still reproducing on macOS 10.12.6 with Nightly 70.0a1 , empty panel is displayed in the Preferences page when navigating with Tab key while the block auto play drop down is opened.

So the issue is slightly different now; i.e. an empty panel as opposed to the autoplay dropdown options described in comment 0?

The autoplay drop down options are displayed in about:preferences page when navigating with Tab+Enter while the autoplay drop down is open

Johann, is there a Mac developer who can take a look at this? I've implemented a fix, I have an automated test which proves it, yet apparently, it isn't fixed. I don't have access to a Mac, so I can't really progress this any further.

That's correct, an empty panel is displayed for macOS in the preferences page. You can see how it looks from the macOS.png attachment, let me know if there's anything else I should investigate/help with. Thanks

Comment 23

[Johann Hofmann [:johannh]]

I can't reproduce this anymore. Can you provide me with a video showing your STR?

Thanks!

Comment 24

[Gabi Cheta [:Gabi] Desktop QA]

Attachment: autoplay.avi

Comment 25

[Gabi Cheta [:Gabi] Desktop QA]

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

I can't reproduce this anymore. Can you provide me with a video showing your STR?

Thanks!

Added recording autoplay.avi from Nightly 70.0a1 MacOS 10.10.

Comment 26

[Johann Hofmann [:johannh]]

Ah, I see, thanks, so the issue still occurs when you mouse-click on the menupanel (as opposed to opening it via keyboard interaction) and then pressing tab will not close the menupanel. Jamie, do you think that expected? Should we file a follow-up bug? If you don't have an idea how to solve this then we might just have to set it on P3. I really don't have the resources to dig into it right now and I don't think folks on Widget side do, either.

Comment 27

[James Teh [:Jamie]]

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

Ah, I see, thanks, so the issue still occurs when you mouse-click on the menupanel (as opposed to opening it via keyboard interaction) and then pressing tab will not close the menupanel.

Do you mean you mouse click on the View Site Information button, mouse click on the permission menulist or both? And which panel doesn't it close when you press tab: the Site Information panel, the menulist or both? And what does happen when you press tab: what gets focus? It'd be really helpful if we had detailed STR and results here rather than relying on a video (which I can't see) to tell the story.

FWIW, I tested on Windows by clicking both the View Site Information button and the block autoplay menulist with the mouse. When I press tab, both panels close immediately. So either I'm missing something or this is not reproduceable on Windows. In which case I have no idea.

Comment 28

[Gabi Cheta [:Gabi] Desktop QA]

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

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

Ah, I see, thanks, so the issue still occurs when you mouse-click on the menupanel (as opposed to opening it via keyboard interaction) and then pressing tab will not close the menupanel.

Do you mean you mouse click on the View Site Information button, mouse click on the permission menulist or both? And which panel doesn't it close when you press tab: the Site Information panel, the menulist or both? And what does happen when you press tab: what gets focus? It'd be really helpful if we had detailed STR and results here rather than relying on a video (which I can't see) to tell the story.

FWIW, I tested on Windows by clicking both the View Site Information button and the block autoplay menulist with the mouse. When I press tab, both panels close immediately. So either I'm missing something or this is not reproduceable on Windows. In which case I have no idea.

Hi James,

Here are the STR:

1. Navigate to video
2. Open the Site Information Panel
3. Mouse click open the Autoplay drop down options
4. Hit the Tab key and start navigating trough the Site Information Panel until Permission/Settings button is focused
5. Hit Enter

AR: About Preferences Page is displayed with the empty panel (autoplay dropdown options opened in step 3) on macOS.

On Windows 10x64 and Ubuntu 16.04 once the Tab key is pressed the Site Information Panel is closed. This means the on Windows and Ubuntu the bug was fixed.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

It sounds like the fix that landed improved the situation even if there's still cases which need attention. Should we move the remaining issues to a follow-up bug?

Comment 30

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9077597 [details]
Bug 1561517: PanelMultiView: Don't override the tab key when an open menulist has focus.

Trivial fix, has tests, on nightly for 2 weeks and mitigates the bug with the STR provided. Approved for 69 beta 11 thanks.

Comment 31

[Oana Pop-Rus]

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

Comment 32

[Gabi Cheta [:Gabi] Desktop QA]

(In reply to Gabi Cheta [:Gabi] Release Desktop QA from comment #28)

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

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

Ah, I see, thanks, so the issue still occurs when you mouse-click on the menupanel (as opposed to opening it via keyboard interaction) and then pressing tab will not close the menupanel.

Do you mean you mouse click on the View Site Information button, mouse click on the permission menulist or both? And which panel doesn't it close when you press tab: the Site Information panel, the menulist or both? And what does happen when you press tab: what gets focus? It'd be really helpful if we had detailed STR and results here rather than relying on a video (which I can't see) to tell the story.

FWIW, I tested on Windows by clicking both the View Site Information button and the block autoplay menulist with the mouse. When I press tab, both panels close immediately. So either I'm missing something or this is not reproduceable on Windows. In which case I have no idea.

Hi James,

Here are the STR:

1. Navigate to video
2. Open the Site Information Panel
3. Mouse click open the Autoplay drop down options
4. Hit the Tab key and start navigating trough the Site Information Panel until Permission/Settings button is focused
5. Hit Enter

AR: About Preferences Page is displayed with the empty panel (autoplay dropdown options opened in step 3) on macOS.

On Windows 10x64 and Ubuntu 16.04 once the Tab key is pressed the Site Information Panel is closed. This means the on Windows and Ubuntu the bug was fixed.

Should I open a new issue for macOS only

Comment 33

[James Teh [:Jamie]]

(In reply to Gabi Cheta [:Gabi] Release Desktop QA from comment #32)

1. Navigate to video
2. Open the Site Information Panel

When you say "open", did you click or use the keyboard? I'm not sure if this is relevant, but it may be. It's worth testing both scenarios.

Should I open a new issue for macOS only

Yes, I think that makes sense. Thanks.

Comment 34

[Gabi Cheta [:Gabi] Desktop QA]

Verified as fixed with Windows 10x64 and Ubuntu 16.4 on Nightly 70.0a1 and Fx 60.0b11.
Opened Bug 1572035 for macOS issue.