Skip to content
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

sidePanel API: support "one instance per tab" in openPanelOnActionClick #515

Open
fregante opened this issue Jan 5, 2024 · 10 comments
Open
Labels
proposal Proposal for a change or new feature

Comments

@fregante
Copy link

fregante commented Jan 5, 2024

The default behavior of the sidePanel API seems to be to share a single document per window. You open the sidebar and it persists while switching tabs. ⚠️

This is great for "global" extensions or those that have a UI with limited complexity (i.e. they don't require fetching and displaying data based on the current page)

However in our extension the sidebar shows content related to the current page/contentScript, so we'd rather have a dedicated sidebar per tab. This means we prefer the sidebar to disappear when switching to tabs that don't need it.

The good news

Chrome actually already allows "one sidebar instance per tab", but it requires calling setOptions on every tab, with some complexity and drawbacks.

🖼️ Demo gif

all

Proposal

chrome.sidePanel.setPanelBehavior({
	openPanelOnActionClick: true,
+	linkToTab: true,
});

Current solution

void chrome.sidePanel.setPanelBehavior({ openPanelOnActionClick: false });

// Disable by default, so that it can be enabled on a per-tab basis
void chrome.sidePanel.setOptions({
  enabled: false,
});

chrome.action.onClicked.addListener(async (tab) => {
  // Simultaneously define, enable, and open the side panel
  void chrome.sidePanel.setOptions({
    enabled: true,

    // Link to tab
    tabId,

    // Help sidebar.html know which tab it's connected to
    path: "sidebar.html?tabId=" + tab.id,
  });
  await chrome.sidePanel.open({ tabId });
});

Drawbacks for the current solution

This is somewhat feasible but it comes with some drawbacks and complexity:

  • to create one-sidepanel-per-tab, we can't use the native openPanelOnActionClick: true
  • we have to listen to the action.onClick event and manually open the sidebar
  • there is no way to easily determine whether the sidePanel is open and therefore should closed
    • this is an issue because open() must come immediately, in order not to lose the "user gesture", so doing async work ("should open or close it?") might delay this just enough to break it
  • there's no sidePanel.close() API, so to close the sidebar in a tab, you'd have to set enabled: false, which removes it from the dropdown
  • there's no sidePanel.onOpen event, so the logic must either be in action.onClick or in the sidePanel document itself
@tophf
Copy link

tophf commented Jan 6, 2024

Did you try chrome.tabs.onActivated inside the side panel and add/remove DOM elements or toggle visibility accordingly? If it doesn't flicker, the overall memory consumption would be less by ~20MB per tab probably. As for the other issues you mentioned, they should be fixed anyway regardless of this proposal.

@fregante
Copy link
Author

fregante commented Jan 6, 2024

I think you're referring to having one sidebar instance and then changing its content depending on the tab.

It can work in some cases but here preserving the state between tab changes can be difficult; the side panel here contains a complex app (think multi-step forms, iframes, etc) that can't be easily unloaded and restored without side effects.

@tophf
Copy link

tophf commented Jan 6, 2024

AFAIK it should be possible. Which side effects specifically?

@fregante
Copy link
Author

fregante commented Jan 7, 2024

Which side effects specifically?

What happens when you remove these elements from the DOM:

  • iframe
  • audio
  • video
  • img

Removing/reattaching them on tab change will trigger HTTP requests and it will lose state, e.g. navigation in the iframe. These are just the most prominent examples and not an exhaustive list.

AFAIK it should be possible

It is possible, but it moves the complexity to the app itself, for example in a React app you'd have to specifically try to preserve the entire DOM for every relevant tab.

Even for simple sidebars, having a dedicated document/instance per tab greatly simplifies management.

Since having multiple sidebars is already possible, I'd always prefer that over having to deal with tab/content changes.

@tophf
Copy link

tophf commented Jan 7, 2024

Well, don't remove them, simply toggle their visibility. I don't see anything complex in having multiple apps in one document.

@tophf
Copy link

tophf commented Jan 7, 2024

You can also put the entire app inside an iframe to isolate the globals/whatever and have an iframe per tab, then toggle the iframe visibility. You'll be able to reuse resources and data between all of them through parent, so it'll consume less resources than separate pages.

@tophf
Copy link

tophf commented Jan 7, 2024

But if your proposal is easy to implement for browser makers I agree it makes sense for the sake of consistency of the API.

@fregante
Copy link
Author

fregante commented Jan 7, 2024

Let's stick to the proposal, because that solution is in no way better than just loading dedicated sidebars (which already works, without altering the app)

Also what you're suggesting just doesn't match the requirement: the sidebar must close when switching to tab that didn't open it.

@dotproto
Copy link
Member

  • there's no sidePanel.close() API, so to close the sidebar in a tab, you'd have to set enabled: false, which removes it from the dropdown

Have you tried calling window.close() inside the side panel window?

@fregante
Copy link
Author

@dotproto That's really useful, thanks! I opened a dedicated issue for that:

@fregante fregante changed the title sidePanel API: optionally create one sidebar instance per tab sidePanel API: support "one instance per tab" in openPanelOnActionClick Jan 12, 2024
@xeenon xeenon added proposal Proposal for a change or new feature and removed needs-triage labels Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposal for a change or new feature
Projects
None yet
Development

No branches or pull requests

4 participants