1580949 - Closing the overflow panel programmatically in the middle of a transition leaves a transitioning="true" attribute on the panelmultiview

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[graham.mcknight2]

Attachment: open-my-popup-button.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

1. Create an extension with a browserAction that shows a popup. I have attached an extension that does this.
2. Load the extension as a temporary extension. Note the part of the extension id before @temporary-addon.
3. Customize the toolbar and place the temporary extension into the overflow panel.
4. Open the Firefox developer tools.
5. In the console, call window.document.getElementById("nav-bar").overflowable.show() to see the open overflow panel.
6. let widget = window.document.getElementById("def6193627a17c50b38193457d9e420f84c11347_temporary-addon-browser-action");
7. Use the following function, substituting the extension id of temporary extension:

async function freezePanelMultiView() {
  widget.click();
  // The delay may need to be changed 
  await new Promise((resolve, reject) => window.setTimeout(resolve, 50));
  CustomizableUI.hidePanelForNode(widget);
}

8. Reopen the overflow panel in any way.

Actual results:

The panel is unclickable, and does not highlight when moused over. The panelmultiview element (mainViewId="widget-overflow-mainView") has the attribute transitioning="true".

Expected results:

The transitioning="true" attribute should be removed, and the panel should be clickable.

Comment 1

[graham.mcknight2]

A few notes:

• The extension id needs to be substituted into step 6, not step 7.
• Addressing this may enable the use of EventUtils.synthesizeMouseAtCenter here:
  https://searchfox.org/mozilla-central/rev/878bbf3cb8897a208454df27535f3522ab482cf2/browser/components/extensions/test/browser/head.js#365

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 3

[:Gijs (he/him)]

I can't install this add-on - the add-ons manager claims that the add-on is corrupt?

Comment 4

[:Gijs (he/him)]

Do the same steps not work with a builtin button that opens a subview, like the library button or similar?

Comment 5

[graham.mcknight2]

This is just a zip of the raw extension files. When I tried installing it as an add-on through the regular add-ons page it said that it was corrupted; is the way you tried to install it? Have you tried installing it as a temporary extension (from Nightly, go to about:debugging, click This Nightly on the left nav-bar in the page, and click Load Temporary Add-On)? I will figure out how to generate a more official extension from this tonight if that doesn't work.

I haven't been able to reproduce it with the library button, and my suspicion is that it's because we don't use a customRectGetter and check for the panel being closed with no further awaits (and therefore no opportunity to close the panel inconveniently) before setting the transitioning state?
https://searchfox.org/mozilla-central/rev/7ed8e2d3d1d7a1464ba42763a33fd2e60efcaedc/browser/components/customizableui/PanelMultiView.jsm#968-970

Comment 6

[:Gijs (he/him)]

OK, I can add the add-on via about:debugging, but this is annoying because I need to repeat it every time I debug it...

The other issue is that in step 5 you show the overflow panel, but it hides in step 6 / 7, because you focus the browser console - did you forget to mention you need to turn off popup auto-hide?

What's the normal way in which this bug happens? That is, I assume there are some other circumstances than this convoluted set of STR that is actually a problem here?

Comment 7

[:Gijs (he/him)]

(In reply to graham.mcknight2 from comment #5)

I haven't been able to reproduce it with the library button, and my suspicion is that it's because we don't use a customRectGetter and check for the panel being closed with no further awaits (and therefore no opportunity to close the panel inconveniently) before setting the transitioning state?
https://searchfox.org/mozilla-central/rev/7ed8e2d3d1d7a1464ba42763a33fd2e60efcaedc/browser/components/customizableui/PanelMultiView.jsm#968-970

I've added a fix based on this suggestion, but answers to the questions in comment #6 would still help to understand this bug and improve testing.

Comment 8

[:Gijs (he/him)]

Attachment: Bug 1580949 - avoid re-entry when showing extension subviews in the overflow panel, r?johannh

Comment 9

[graham.mcknight2]

Attachment: open_my_popup-1.0-fx.xpi

Sorry for the late reply; here is the xpi that points to this extension on AMO.

Comment 10

[graham.mcknight2]

I don't have popup auto-hide disabled. I was a little vague here, but I was using the browser console in the developer toolbox, and never focused/unfocused anything on the main browser window, so the popup never disappeared.

This bug is unlikely to be experienced by any user because of the very tight timing; it was just discovered when adding tests for an unrelated change. The test in question rapidly shows, clicks, and closes the extension panel, both from the nav-bar and overflow panel. Here's a link:
https://searchfox.org/mozilla-central/rev/7ed8e2d3d1d7a1464ba42763a33fd2e60efcaedc/browser/components/extensions/test/browser/browser_ext_browserAction_popup.js

Currently, the test works because it calls click() on the browserAction button, but that's not suitable for clicks with other keys pressed or right/middle clicks.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf84fbd266c7
avoid re-entry when showing extension subviews in the overflow panel, r=johannh

Comment 12

[Alexandru Michis [:malexandru]]

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

Comment 13

[:Gijs (he/him)]

(In reply to graham.mcknight2 from comment #10)

This bug is unlikely to be experienced by any user because of the very tight timing; it was just discovered when adding tests for an unrelated change. The test in question rapidly shows, clicks, and closes the extension panel, both from the nav-bar and overflow panel. Here's a link:
https://searchfox.org/mozilla-central/rev/7ed8e2d3d1d7a1464ba42763a33fd2e60efcaedc/browser/components/extensions/test/browser/browser_ext_browserAction_popup.js

Currently, the test works because it calls click() on the browserAction button, but that's not suitable for clicks with other keys pressed or right/middle clicks.

Sorry, so where were you adding tests (no other bugs seem to be linked to this one) ? Are you going to update this test in some other bug, or do I need to file a follow-up for it? :-)

Comment 14

[graham.mcknight2]

I was adding a test to browser/components/extensions/test/browser/browser_ext_browserAction_popup.js for Bug 1405031.

The patch for that is not yet landed, so I think I can make the change in that patch, but let's double check with Rob, who's mentoring for it.

Comment 15

[Rob Wu [:robwu]]

If not strictly necessary, I suggest to not include the test fixup in that patch, because it makes it more difficult to understand the patch. Ideally each patch would be one unit of change for a specific purpose.

If you want to add a test, please add a new patch (feel free to reuse that bug), on top of your existing patch if the change depends on your existing patch.

Comment 16

[graham.mcknight2]

That makes sense.

The test fixup in question is the one mentioned in Comment 1:

Addressing this may enable the use of EventUtils.synthesizeMouseAtCenter here:
https://searchfox.org/mozilla-central/rev/878bbf3cb8897a208454df27535f3522ab482cf2/browser/components/extensions/test/browser/head.js#365

This doesn't depend on the changes in the patch on the other bug per se, but there will be a conflict because that patch adds the ability to pass modifiers to the function.

Also, it's not totally guaranteed that this works for all tests, so I'll follow up on this bug once after running all the tests with this proposed fixup.