1921631 - window.close() does not close sidebar

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[Rob Wu [:robwu]]

Attachment: sidebar-close-demo.zip

STR:

1. Load the attached extension at about:debugging.
2. Click on View > Sidebar > "Sidebar (close demo)" to open the sidebar.
3. Click on "sidebarAction.close()" button in the sidebar.
4. Repeat step 2 to re-open the sidebar.
5. Click on "window.close()" button in the sidebar.

Expected:

• After step 5, the sidebar should close.

Actual:

• After step 5, the sidebar stays open.

Note: fixing this is easy: we should add a DOMWindowClose event listener to the sidebar's <browser> and trigger the same logic as sidebarAction.close().

Comment 1

[Rob Wu [:robwu]]

Note: fixing this is easy: we should add a DOMWindowClose event listener to the sidebar's <browser> and trigger the same logic as sidebarAction.close().

Some more context on what specifically to change:

The <browser> is created at https://searchfox.org/mozilla-central/rev/9fe73403523732f57cd82d30590ecc272fb0b165/browser/base/content/webext-panels.js

Here is an example of a patch that added the DOMWindowClose handler in the parent: https://hg.mozilla.org/mozilla-central/rev/c7d802a739c6

In response to the DOMWindowClose listener, the implementation would have to do something similar to the close handler in ext-sidebarAction.js, which calls window.SidebarController.hide(). Since webext-panels.js is actually embedded in a child <browser> instead of the main browser window, I think that it should actually be windowRoot.ownerGlobal.SidebarController.hide().

Comment 2

[Rob Wu [:robwu]]

I got a question about how to test this.

The desired test coverage is to confirm that the sidebar closes as expected when window.close() is called from the sidebar panel. For that you need to create a test case that has a sidebar and calls window.close() from it. The initial comment in this bug has a zip file with a test case for manual testing.

For further inspiration you can also take a look at an existing test that checks the behavior of closing the sidebar.
There is an existing unit test that checks multiple behaviors around opening/closing the sidebar: https://searchfox.org/mozilla-central/rev/1b90936792b2c71ef931cb1b8d6baff9d825592e/browser/components/extensions/test/browser/browser_ext_openPanel.js#44-151

To verify behavior, you could modify it to:

• add a browser.test.onMessage listener in (panel.js) that confirms that the received message is window_close (which is what we send later) and then call window.close().
• open the sidebar again, after line 148. Wait for the sidebar to be opened (await extension.awaitMessage("panel-opened");)
• call extension.sendMessage("window_close"). This should notify the panel.js code in the extension from the first step that calls window.close().
• Wait until the sidebar is closed.

If the bug is not fixed, the last step will time out.
If the bug is fixed, the last step will complete soon and the test will run to completion.

Comment 3

[Becca]

Thank you! One question- which panel.js file are you referring to when you say to add a browser.test.onMessage listener? I see quite a few files named panel.js in different locations, and the file I worked in is browser\base\content\webext-panels.js.

Comment 4

[Rob Wu [:robwu]]

(I already answered in chat, pasting my response for future reference)

The ExtensionTestUtils.loadExtension test helper function generates an extension package based on the given parameters. Some of the input can be functions, which are then converted to a string that resembles an immediately-invoked function expression, and used as the content of the file.

In the test file I linked, line 107-109 defines the content of the panel.js file, which is referenced through panel.html, at line 74: https://searchfox.org/mozilla-central/rev/1b90936792b2c71ef931cb1b8d6baff9d825592e/browser/components/extensions/test/browser/browser_ext_openPanel.js#71,74,107-109
This panel.html file is referenced in the manifest, via sidebar_action's default_panel (line 54).

Comment 5

[Becca]

Attachment: Bug 1921631 - window.close does not close sidebar - r=robwu

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: test-sidebar-and-devtools-panel.zip

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcebd2888274
window.close does not close sidebar - r=rpl

Comment 8

[agoloman]

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

Comment 9

[Rob Wu [:robwu]]

We should document this in the Firefox 133 for Developers release notes:

• Calling window.close() from a sidebar now closes the sidebar instead of doing nothing.

(with window.close() linking to the MDN article of it, and sidebar pointing to https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/user_interface/Sidebars

Comment 10

[Becca]

Suggested wording:

Calling window.close() from a sidebar now closes the sidebar instead of doing nothing.

Comment 11

[Donal Meehan [:dmeehan]]

Added to the Fx133 release notes, please allow 30 mins for the page to update.
https://www.mozilla.org/firefox/133.0/releasenotes/

dev-doc-needed keyword is used for the mdn notes
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases

Comment 12

[Rob Wu [:robwu]]

This is not sufficiently notable to include in the release notes for users. Please drop it from the release notes and let the dev-doc-needed process take care of creating the relnotes. I've already pinged rbloor about this.

Comment 13

[Donal Meehan [:dmeehan]]

This is not sufficiently notable to include in the release notes for users.
Note this is in the Developers section of the release notes, but happy to remove it

Comment 14

[Rob Wu [:robwu]]

Let's remove it from the release notes page. MDN suffices.

MDN docs has been added at https://github.com/mdn/content/pull/37055