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

#7228: chrome.sidePanel POC #7232

Closed
wants to merge 14 commits into from
Closed

#7228: chrome.sidePanel POC #7232

wants to merge 14 commits into from

Conversation

fregante
Copy link
Contributor

Findings:

  • the sidebar appears to be loaded once per window
  • while the docs suggest that the document can be linked via tabId, that does not seem to make any difference (at least not automatically)
  • the docs mention you can have "multiple documents", but each load replaces the old one, so there's no way to have a sidebar.html in two tabs without unloading the previous one
  • I haven't tried, but I think Edge supports the same API, to the point that its docs are nearly copy-pasted: https://learn.microsoft.com/en-us/microsoft-edge/extensions-chromium/developer-guide/sidebar

Thoughts:

  • this requires a complete review of how the sidebar is seen within PB, because:
    • the document survives tab changes
    • the document survives navigations
    • it's completely unrelated to and independent from the content script
    • multiple content scripts/tabs can connect to the single open sidebar
  • since it's MV3-only, I don’t think it's a good time to explore this API because it's probably best to "start from scratch" rather than have two sidebar implementations coexisting (MV2 in content script, MV2 in sidePanel)

This PR accomplishes:

  • ✅ allows the side panel to load without error, fixing messenger/framing API inconsistencies
  • ⚠️ loads the content SOMETIMES — I don't know why sometimes it loads a panel, sometimes it stays on the welcome screen, others it stays on the loading
  • ✅ opens the sidebar on action click
  • ✅ ℹ️ closes the sidebar on action click SORT OF — possible natively, but not if we use chrome.sidePanel.open(specific tab ID). There is no .close() API but there might be other hackyish solutions.
  • ❌ responds to the page editor — currently it still opens the in-page sidebar, which still works
  • ⚠️ the messenger already supports this, but due to the changes required with these constraints, it doesn't really work as is. This is only because the target expects sidebar.html to be associated with a specific tabId, which may not be possible.

Demos

Screenshot 16 Screenshot 17

Next

Any other questions you'd like answered? Want me to work on this further? The message

@fregante fregante added the mv3 label Dec 29, 2023
@fregante fregante requested a review from twschiller December 29, 2023 16:20
@fregante
Copy link
Contributor Author

@twschiller
Copy link
Contributor

twschiller commented Dec 29, 2023

I don’t think it's a good time to explore this API because it's probably best

What do you mean by "I don't think it's a good time to explore this API"? Do you mean the open method call specifically as opposed to declaring the panel in the manifest as being connected to the action button?

this requires a complete review of how the sidebar is seen within PB

Correct. We're assembling that here (main discussion in Phase 1): https://www.notion.so/pixiebrix/Use-Chromium-Side-Panel-for-Sidebar-985eedfb2d62450584c2d03cab7bfb12?pvs=4

while the docs suggest that the document can be linked via tabId, that does not seem to make any difference (at least not automatically)

This is an area I want to dig into. From your investigation, it sounds like there's a single sidebar instance shared across tabs. It would be good to know what the tabId does? It might not link but instead indicate where it would show up?

I will look at how Chrome's built-in sidebars handle tab change UX. For example, they have a shopping assistance sidebar that I'm curious how it handles changing product tabs vs. going to a tab where no product is located

since it's MV3-only, I don’t think it's a good time to explore this API because it's probably best to "start from scratch" rather than have two sidebar implementations

I agree for MV3 it's not worth the two methods co-exist

A gotcha is that mod-variables and mod state are tried to the content script. (See consideration writeup here: https://www.notion.so/pixiebrix/Phase-1-PixieBrix-sidebar-in-Chromium-Side-Panel-d95485473005421db945142bfa792568?pvs=4). If the sidebar is global, we can still just have it re-render whenever the focused tab changes to keep the content in sync with the page

loads the content SOMETIMES

There's probably a race condition with the controller on the page, or which content script gets to message it?

Any other questions you'd like answered? Want me to work on this further?

I'll check out the PR this afternoon and have some more questions from that

Some additional questions off the top of my head

  • Is the icon/name of the panel customizable? Our enterprise customers will want to white-label

@fregante
Copy link
Contributor Author

It would be good to know what the tabId does? It might not link but instead indicate where it would show up?

This PR in the current state uses that parameter so you can try it as well. In short it doesn't seem to have any behavioral change whatsoever. What might change is that this link can be verified via sidePanel.getOptions({tabId})… if we had different options per tab.

I'll investigate further. But I still think that even the sidebar were to be actually associated to a tab, it would have to unload/load at every tab switch.

I will look at how Chrome's built-in sidebars handle tab change UX

I assume that the sidebar frame has a SPA and a tabs.onUpdated listener inside, that's what the docs show in their demos.

  • Is the icon/name of the panel customizable?

It doesn't look like it:

There's a small chance that the icon follows the browseAction's, but I doubt it.

@fregante
Copy link
Contributor Author

fregante commented Dec 30, 2023

I have some good news.

I created a standalone extension for ease of testing, and indeed:

  • ✅ Each tab can have a dedicated document
  • ✅ Switching tab does not unload the document

This simplifies things greatly. Now the two major differences left are:

  • the sidebar survives navigation and therefore can connect to a new document / content script
  • the sidebar is not an iframe on the same tabId (but it is associated to a single tabId)

Next tasks:

  • Link the sidebar to the current tab in this PR
  • Fix messaging to the sidebar
  • Have the page editor connect to the sidePanel
  • See if the sidePanel can be opened from the content script

Questions:

  • Have you considered finally moving PB out of the dev tools and into the sidebar? Would it be a good task for 2025? 😃

Demo

The code for this is:

chrome.tabs.onUpdated.addListener(async (tabId, info, tab) => {
  if (!tab.url) return;
  await chrome.sidePanel.setOptions({
    tabId,
    path: 'sidepanel.html?tabId=' + tabId,
    enabled: true
  });
});
Screen.Recording.3.mov

@twschiller
Copy link
Contributor

twschiller commented Jan 3, 2024

I pushed a commit which:

  • Uses webpack alias to swap out sidebarControllerDomLite when using MV3 to use the sidebar panel API
  • Adds background handler for showing/hiding panel on a tab
  • Uses tab-specific URL when opening via browser action
  • Use runtime messaging for background and content script to know if the sidebar is visible or not

Things I've confirmed

  • Gestures in the content script can open the sidebar to the correct URL. Tested via create Quick Bar + Show Panel brick
  • Gestures in the content script can hide the whole sidebar by disabling it for the tab. Tested via Quick Bar + Hide Sidebar Brick
  • If you switch to a different panel and then click the browser icon, it will switch back to PixieBrix panel
  • If you are on a different panel and use the Hide Sidebar brick, the panel will stay showing, but PixieBrix will not appear in the dropdown. If we wanted to hide the entire sidebar, we could force switch to the PixieBrix panel and then disable it

New Questions (see comments in code)

  • How to implement isSidebarFrameVisible. Required to get mods to run, because they don't run when not visible
  • How to implement toggle behavior for the toolbar icon. Similar solution likely to isSidebarFrameVisible
  • Figure out why insertSidebarFrame loses the user gesture when using "Render Panel" from the dev tools. My current theory is a bug linked in the commit

I've implemented a POC of https://stackoverflow.com/a/77106777/402560 which seems to work for the 1 tab case for using the browser action button. It also uses the Page Visibility API to determine when the user has switched to a different panel in the sidebar. I tried briefly to also use that in the sidebar dom contoller, but it wasn't working as expected. The same approach is working for the content script to know if the sidebar is visible

Working:

  • Render panel when toggled via the browser action

Still Broken:

image

@twschiller
Copy link
Contributor

twschiller commented Jan 3, 2024

@fregante what's an ETA on looking into the messenger to the sidebar? It seems to be working when going in the opposite direction (sidebar to content script).

Probably a matter of fixing the target to no depend on tabId, but instead to pass the tabId via the page?:

const target = { tabId: "this", page: "/sidebar.html" } as const;

@@ -52,6 +52,12 @@ function updateManifestToV3(manifestV2) {
manifest.permissions = [...permissions, "scripting"];
manifest.host_permissions = origins;

// Add sidePanel
manifest.permissions.push("sidePanel");
manifest.side_panel = {
Copy link
Contributor

@twschiller twschiller Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: need a "generic" sidebar that indicates it can only be used in the context of a web page tab?

@fregante
Copy link
Contributor Author

fregante commented Jan 4, 2024

Testing out the current solution, I added the "default" page to the manifest just help see issues better. I see one with the current logic: the sidebar does update with the correct URL on tab change (sidebar.html?tab=123)

Screen.Recording.mov

@twschiller
Copy link
Contributor

twschiller commented Jan 4, 2024

The new commit:

  • Fixes sidebarInThisTab to work with in-page and MV3 side panel
  • Fixes createFrameSource to handle forms in MV3 side panel

Remaining Work

  • Figure out why panel lifecycle not working on initial toggle

/**
* The Chrome Side Panel API doesn't have a method yet for checking if the sidebar is open, so track it ourselves.
* @see init
*/
let sidePanelOpen = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when the page reloads and this data is lost? sidePanelOpen is false but the sidebar is open.

In #7266 I'm currently just pinging the sidebar itself to check if it responds. It works 100% of the times, but we can verify later whether their callers actually need to have this information the same way.

Copy link
Contributor

@twschiller twschiller Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when the page reloads and this data is lost? sidePanelOpen is false but the sidebar is open.

I believe the sidebar should reconnect after navigation (because the port would disconnect on navigation):

tabPort.onDisconnect.addListener(() => {
. I haven't fully tested it though. Looks like it might not be working

@fregante fregante changed the title chrome.sidePanel POC #7228: chrome.sidePanel POC Jan 7, 2024
@fregante fregante mentioned this pull request Jan 11, 2024
7 tasks
@twschiller
Copy link
Contributor

Closing in favor of: #7320

@twschiller twschiller closed this Jan 13, 2024
@fregante fregante deleted the F/mv3/sidePanel branch January 25, 2024 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants