-
Notifications
You must be signed in to change notification settings - Fork 74
Proposal: close() Method for the sidePanel API #837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
dotproto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes requested in this review are either internal styling consistency or GFM rendering fixes.
Developers can currently close the document in the sidebar or sidepanel ("sidebar" for simplicity) by calling There are times when it might make sense to diverge or to change existing behavior to also require a user activation, but I don't think this is such a case. As I see it, there's minimal risk to allowing an extension to close it's own sidebar, especially since both supporting browsers currently require user activation to open a sidebar. The main risk I see is that an extension might try to close a side panel right before a user clicks in order to cause the user to click something they didn't intend to in the main document, but I'm not sure how practical that kind of attack would be. At the moment I'm comfortable with not requiring user activation, but I'll defer to @Rob--W for a more official position. |
oliverdunk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment, but otherwise this seems like a good proposal.
|
@carlosjeurissen While preparing the proposal, I was not in favor of throwing an error when the side panel is already closed for the given context. To keep things simple, an error should only be thrown if the context is invalid. Otherwise, it should simply do nothing and resolve, indicating that the panel is already closed for that context. If the side panel is opened globally for a window and we try to close it for just one tab, it will close for all tabs in that window. |
oliverdunk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure to align on the behavior when both windowId and tabId are specified. Otherwise, LGTM.
|
Currently, the focus is on using "close" when the intention is to remove the side panel from the tab registry entirely. The concept of "hide"—making the side panel inactive but not removing it—could be considered for future enhancements.
What I thought was -
|
If If there is a global panel open, and you call close() with a @kiaraarose Do you have a preference from the Safari side? |
The sidePanel.close() method allows extensions to programmatically close the side panel for a specified tabId or windowId. Reference: w3c/webextensions#837 Bug: 403765214 Change-Id: I16b1c05869a1f4707bf107d4eff1d085235b906b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6638129 Reviewed-by: Devlin Cronin <[email protected]> Commit-Queue: Oliver Dunk <[email protected]> Reviewed-by: Oliver Dunk <[email protected]> Cr-Commit-Position: refs/heads/main@{#1499029}
I agree with @carlosjeurissen that the behavior of closing a global sidePanel for all tabs if both Update: For Safari, we agree that having the sidePanel close for all tabs in this scenario wouldn't be ideal. I think an error for this case is sufficient. |
Thanks for following up. To confirm:
Is that correct? Also, an additional question: If you try to open a side panel for a |
|
Hi @kiaraarose / @carlosjeurissen - we have been looking at updating Let's plan on discussing next steps at TPAC? Based on this I am once again leaning towards supporting a fallback to the global panel for both the |
|
I'm taking over the review from @zombie on behalf of Mozilla, since I have been actively involved in discussions on the design of sidepanel/sidebar APIs at TPAC 2025. Those meeting notes will be published soon. |
carlosjeurissen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggested changes apply what has been discussed during TPAC2025. There is one open end:
Should we specify a global side panel will be reopened when closing a contextual one when switching tabs and returning to the tab with contextual side panel? As this is current Chrome behaviour.
| - If `windowId` or `tabId` is invalid, rejects with an error. | ||
| - If both `windowId` and `tabId` are provided, the method will verify | ||
| that the tab belongs to the specified window. If not, it rejects with an error. | ||
| - If both `windowId` and `tabId` are provided: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - If both `windowId` and `tabId` are provided: | |
| - If `tabId` is provided: |
Calling only with a tabId should be possible
| - If the panel is already closed or does not exist in the given | ||
| context, the method does nothing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - If the panel is already closed or does not exist in the given | |
| context, the method does nothing. |
Move this to the two different scenarios, in which the method is called with a tabId or without a tabId
| 1. If a contextual side panel is visible on the specified `tabId`, it will be closed. | ||
| 2. If a global side panel is visible on the given `tabId`, | ||
| it will be closed for all tabs in that window, not just the specified tab. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 1. If a contextual side panel is visible on the specified `tabId`, it will be closed. | |
| 2. If a global side panel is visible on the given `tabId`, | |
| it will be closed for all tabs in that window, not just the specified tab. | |
| 1. If no contextual side panel is configured for the specified `tabId`, reject with an error. | |
| 2. If the contextual side panel is currently not open, the method does nothing. | |
| 3. If the contextual side panel is currently open, it will close the contextual side panel. |
| 1. If a contextual side panel is visible on the specified `tabId`, it will be closed. | ||
| 2. If a global side panel is visible on the given `tabId`, | ||
| it will be closed for all tabs in that window, not just the specified tab. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - If `tabId` is not provided: | |
| 1. If no global side panel is currently open in the window, this method does nothing. | |
| 2. If a global side panel is currently open in the window, close just the global side panel. |
The sidePanel.close() call with tabId no longer falls back to the global panel. It now exclusively closes tab-specific panels and returns an error if one is not enabled for that tab. Reference: w3c/webextensions#837 Bug: 403765214 Change-Id: I6d4fe1b1f2382a7d81bc2216418c9e4e513f66ef Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7317143 Reviewed-by: Oliver Dunk <[email protected]> Commit-Queue: Oliver Dunk <[email protected]> Reviewed-by: Solomon Kinard <[email protected]> Commit-Queue: Harsh Singh <[email protected]> Cr-Commit-Position: refs/heads/main@{#1566376}
This proposal introduces a sidePanel.close() method (among work items for Summer of Code).
While not fully fleshed out, it outlines the core idea. Initial feedback is welcome. I would also like your thoughts on the need for a user gesture for this. #521 #chromium