The new Photon sidebar header includes provision for an icon (#sidebar-icon) which works for built-in sidebars but not extension sidebars.

andy looking for dupe

Was fixed in bug 1365637, but then re-occurred and bug 1370686 is now tracking that.

Bug 1370686 only relates to the sidebar panel icons on Windows. This bug is for the sidebar header icon across all platforms. The panel also appears to be positioned incorrectly when there is no icon in the header.

Ok, moving over to the photon team.

(I've talked with aosmond about this in the Toronto office a little while ago, so hopefully he can figure out why the icon doesn't show until we hover over it in the inspector…)

Comment on attachment 8893085 [details] Bug 1374048 - Show the sidebar extension icon in the header. , ui-r=shorlander. https://reviewboard.mozilla.org/r/164082/#review169734 ::: browser/components/extensions/ext-sidebarAction.js:170 (Diff revision 1) > > - // oncommand gets attached to menuitem, so we use the observes attribute to > - // get the command id we pass to SidebarUI. > - broadcaster.setAttribute("oncommand", "SidebarUI.show(this.getAttribute('observes'))"); > + let commandHandler = () => { > + let header = document.getElementById('sidebar-switcher-target'); > + this.setMenuIcon(header, details); > + SidebarUI.show(this.id); > + } Nit: semicolon, I think? Or does eslint want that gone? ::: browser/components/extensions/ext-sidebarAction.js:192 (Diff revision 1) > > document.getElementById("mainBroadcasterSet").appendChild(broadcaster); > document.getElementById("viewSidebarMenu").appendChild(menuitem); > document.getElementById("sidebar-extensions").appendChild(toolbarbutton); > + if (SidebarUI.currentID === this.id) { > + // @todo: Figure out why this doesn't show on first run… Maybe `currentID` isn't set right on first run yet? ::: browser/themes/shared/sidebar.inc.css:150 (Diff revision 1) > + background-image: var(--webextension-menuitem-image, inherit); > + background-size: contain; > + -moz-context-properties: fill; > + fill: currentColor; Sorry, why can't we use list-style-image like we do for the other things?

Comment on attachment 8893085 [details] Bug 1374048 - Show the sidebar extension icon in the header. , ui-r=shorlander. https://reviewboard.mozilla.org/r/164082/#review169880 ::: browser/components/extensions/ext-sidebarAction.js:170 (Diff revision 1) > > - // oncommand gets attached to menuitem, so we use the observes attribute to > - // get the command id we pass to SidebarUI. > - broadcaster.setAttribute("oncommand", "SidebarUI.show(this.getAttribute('observes'))"); > + let commandHandler = () => { > + let header = document.getElementById('sidebar-switcher-target'); > + this.setMenuIcon(header, details); > + SidebarUI.show(this.id); > + } Didn't seem to care, but I've added one. Linting also found some 's that should have been "s, and missing removeEventListener calls. ::: browser/components/extensions/ext-sidebarAction.js:192 (Diff revision 1) > > document.getElementById("mainBroadcasterSet").appendChild(broadcaster); > document.getElementById("viewSidebarMenu").appendChild(menuitem); > document.getElementById("sidebar-extensions").appendChild(toolbarbutton); > + if (SidebarUI.currentID === this.id) { > + // @todo: Figure out why this doesn't show on first run… No, it's definitely getting into this block, and the --webextension-menuitem-image vars are getting set, but it's not displaying… ::: browser/themes/shared/sidebar.inc.css:150 (Diff revision 1) > + background-image: var(--webextension-menuitem-image, inherit); > + background-size: contain; > + -moz-context-properties: fill; > + fill: currentColor; Doh! It's because I was porting the code over from a different version of the patch. Fixed.

Comment on attachment 8893085 [details] Bug 1374048 - Show the sidebar extension icon in the header. , ui-r=shorlander. https://reviewboard.mozilla.org/r/164082/#review170314 ::: browser/components/extensions/ext-sidebarAction.js:197 (Diff revision 3) > + // @todo: Figure out why this doesn't show on first run… > + let header = document.getElementById("sidebar-switcher-target"); In my testing this block doesn't get hit on first run, because `currentID` is empty, and the initial showing of the sidebar presumably doesn't hit the oncommand path. Could we make the browser-sidebar.js code fire an event in .show() that we listen for here to set the image CSS properties? Then we also don't need to add command handlers to every entry path into .show(), and hopefully that will fix this bug as well.

(In reply to Blake Winton (:bwinton) (:☕️) from comment #6) > (I've talked with aosmond about this in the Toronto office a little while > ago, so hopefully he can figure out why the icon doesn't show until we hover > over it in the inspector…) I applied the attached patch, loaded the extension bwinton shared with me and reproduced. It actually tries very hard to load the image, it is requested again and again, but it fails a security check (until it doesn't, and then it is fine). When it fails, it calls imgLoader::LoadImage, aLoadingPrincipal is a ContentPrincipal and the URI is moz-extension://b950289c-c21b-4dc5-930d-e89515a78139/icons/tabcenter.svg. This fails because we aren't allowed to access this sort of URI (due to the call tree of nsJARChannel::AsyncOpen2, CheckChannel, DoCheckLoadURIChecks, nsScriptSecurityManager::CheckLoadURIWithPrincipal, nsScriptSecurityManager::CheckLoadURIFlags, DenyAccessIfURIHasFlags(nsIProtocolHandler::URI_DANGEROUS_TO_LOAD)) unless it is whitelisted (from what I could tell). This happens when the extension sidebar option is selected on startup (e.g. quit the browser with the extension sidebar option open). Merely hovering over the drop down text (without clicking) causes it to trigger a load. When it succeeds, aLoadingPrincipal is a SystemPrincipal which works as expected. If in the failure case above, this happens as soon as the sidebar drop down is clicked -- the image doesn't appear right away, but presumably that is because it didn't trigger a paint (it got loaded). It will also show if at startup, we displayed one of the native options first. If I force it to use a SystemPrincipal in imagelib at the imgLoader::LoadImage call, it works fine. bz, any insights here? I would expect it to be using the SystemPrincipal consistently for this use case.

I would expect that too. When aLoadingPrincipal is not system, what is its URI?

Would this also affect images loaded in a document that has been loaded in the sidebar? I have noticed while messing around with Webextensions that many images loaded in a document that is loaded in the sidebar fail to display. Closing and then opening the sidebar cause the images to display properly.

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #13) > I would expect that too. > > When aLoadingPrincipal is not system, what is its URI? It is a SubstitutingURL. (In reply to Trevor Rowbotham from comment #14) > Would this also affect images loaded in a document that has been loaded in > the sidebar? I have noticed while messing around with Webextensions that > many images loaded in a document that is loaded in the sidebar fail to > display. Closing and then opening the sidebar cause the images to display > properly. My suspicion is yes. While I did not track it too closely, I have seen this happen with other extension image resources while in the debugger.

> It is a SubstitutingURL. OK, but what actual string?

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #16) > > It is a SubstitutingURL. > > OK, but what actual string? moz-extension://b950289c-c21b-4dc5-930d-e89515a78139/icons/tabcenter.svg.

Oh, I thought that was the url of the image we're trying to load. What is the url of the image being loaded?

So per IRC discussion and Andrew debugging, in the failing case the aLoadingPrincipal is a ContentPrincipal for chrome://browser/skin/browser.css and the target url is <moz-extension://b950289c-c21b-4dc5-930d-e89515a78139/icons/tabcenter.svg>. The load is coming from css::ImageValue::Initialize, which uses the principal of the sheet for the load, quite purposefully: that allows user/UA sheets applied to content documents to load images that the content document could not load. And chrome://*/skin/* URIs are not system-principal. In the success case, when aLoadingPrincipal is system, it looks like in ImageValue::Initialize the referrer is chrome://browser/content/browser.css which is a chrome://*/content/* URI and hence gets the system principal. It's not clear to me so far why both of these stylesheets are trying to load this image, at the very least...

kmag: I hear you might be able to help clear up comment 19?

So at first glance, chrome://browser/content/browser.css is loaded in browser.xul and in browser/components/syncedtabs/sidebar.xhtml "chrome://browser/skin/browser.css" is presumably being loaded as "chrome://browser/skin/" and is loaded in all sorts of places that might be relevant here... Anyway, I'd really like to understand how those sheets end up with a rule to load this extension image. That might give me more of an idea of what the right fix here might be, because that depends on what exact permissions model we're looking for here.

So, I'm not entirely sure if this is what you're asking, but… As part of the patch, https://reviewboard.mozilla.org/r/164082/diff/3#chunk2.2 sets the list-style-image to `var(--webextension-menuitem-image, inherit);` in browser/themes/shared/sidebar.inc.css. That variable gets set on #sidebar-switcher-target by the call (at https://reviewboard.mozilla.org/r/164082/diff/3/#view94 ) to setMenuIcon (defined at https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-sidebarAction.js#193 ) (If that's not what you were asking, hopefully kmag will be better equipped to answer.)

Ah, that might explain what's going on. sidebar.inc.css gets included in chrome://browser/skin/browser.css, looks like. So the upshot is that right now chrome://browser/skin/browser.css is not allowed to link to moz-extension: urls. That means we need to either change the security policies involved or link to them from some other sheet; a chrome://browser/content/something one.

I'd be happy with either solution. I don't think there's any harm in allowing chrome: skin packages to load moz-extension: resources. But the simpler short term fix is probably just to move the list-style-image: rule to one of our chrome: content sheets.

Comment on attachment 8893085 [details] Bug 1374048 - Show the sidebar extension icon in the header. , ui-r=shorlander. https://reviewboard.mozilla.org/r/164082/#review173538 Close, but see below feedback. In addition, I wonder if we need to remove the event listener on the sidebar DOM element when the header's window goes away, maybe by using `windowTracker.addCloseListener` ? ::: browser/components/extensions/ext-sidebarAction.js:97 (Diff revision 4) > let {document, SidebarUI} = window; > if (SidebarUI.currentID === this.id) { > SidebarUI.hide(); > } > + if (this.buttonUpdater) { > + SidebarUI.addRemoveListener("SidebarShown", this.buttonUpdater); I think you mean ```js let header = document.getElementById("sidebar-switcher-target"); header.removeEventListener("SidebarShown", this.buttonUpdater); ``` right? ::: browser/components/extensions/ext-sidebarAction.js:184 (Diff revision 4) > // Insert a menuitem for View->Show Sidebars. > let menuitem = document.createElementNS(XUL_NS, "menuitem"); > menuitem.setAttribute("id", this.menuId); > menuitem.setAttribute("observes", this.id); > menuitem.setAttribute("class", "menuitem-iconic webextension-menuitem"); > + menuitem.addEventListener("command", this.commandHandler); This and the same listener for the toolbarbutton seem like leftovers from the previous approach?

Comment on attachment 8893085 [details] Bug 1374048 - Show the sidebar extension icon in the header. , ui-r=shorlander. https://reviewboard.mozilla.org/r/164082/#review173538 > This and the same listener for the toolbarbutton seem like leftovers from the previous approach? Heh, I missed the new version, sorry!

Comment on attachment 8893085 [details] Bug 1374048 - Show the sidebar extension icon in the header. , ui-r=shorlander. https://reviewboard.mozilla.org/r/164082/#review173538 Since the window is going away, I assumed the event listener will also go away… Would you like me to add one just in case? Is there a leak test that would tell us one way or the other? > I think you mean > > ```js > let header = document.getElementById("sidebar-switcher-target"); > header.removeEventListener("SidebarShown", this.buttonUpdater); > ``` > > right? Yeah, I'm not sure where that previous one came from… Fixed!

Comment on attachment 8893085 [details] Bug 1374048 - Show the sidebar extension icon in the header. , ui-r=shorlander. https://reviewboard.mozilla.org/r/164082/#review173622 r=me with the two minor points below fixed. Thanks! ::: browser/components/extensions/ext-sidebarAction.js:75 (Diff revision 6) > + let details = this.tabContext.get(window.gBrowser.selectedTab); > + let header = window.document.getElementById("sidebar-switcher-target"); I would put these 2 lines inside the if statement, too. ::: browser/components/extensions/ext-sidebarAction.js:128 (Diff revision 6) > broadcaster.remove(); > } > + let header = document.getElementById("sidebar-switcher-target"); > + header.removeEventListener("SidebarShown", this.updateHeader); > } > windowTracker.removeOpenListener(this.windowOpenListener); This should probably also remove the close listener.

Comment on attachment 8893085 [details] Bug 1374048 - Show the sidebar extension icon in the header. , ui-r=shorlander. https://reviewboard.mozilla.org/r/164082/#review173622 > I would put these 2 lines inside the if statement, too. So, I didn't put them there on purpose, because we want to switch the icon if we switch to a new tab (with a different icon), but retain the same sidebar (with the same url)…

Pushed by bwinton@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/82c118f8df03 Show the sidebar extension icon in the header. r=Gijs, ui-r=shorlander.

This looks like it's regressed in today's nightly. I haven't dug into it, so I'm not sure why…

(In reply to Blake Winton (:bwinton) (:☕️) from comment #36) > This looks like it's regressed in today's nightly. I haven't dug into it, > so I'm not sure why… Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=86bdc742c111a1c0a08d9bf98a6c6d963eb6f63d&tochange=d159686d279ceead4277915b5530ee4c61848ccb Regressed by Bug 1385630.

(In reply to Kestrel from comment #37) > (In reply to Blake Winton (:bwinton) (:☕️) from comment #36) > > This looks like it's regressed in today's nightly. I haven't dug into it, > > so I'm not sure why… > > Regression window: > > https://hg.mozilla.org/integration/autoland/ > pushloghtml?fromchange=86bdc742c111a1c0a08d9bf98a6c6d963eb6f63d&tochange=d159 > 686d279ceead4277915b5530ee4c61848ccb > > Regressed by Bug 1385630. As a result, I've backed that change out, also because it doesn't look like it ended up fixing the issue in that bug, so we can revisit it there and ensure it doesn't break this fix when we re-fix it. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1470c550ab6d543cbeaeac953fba00df9cb69626

I have managed to reproduce this bug on an affected Nightly build 56.0a1 (20170618100241) using the Notes extension - https://testpilot.firefox.com/experiments/notes. Not reproducible anymore on latest Beta 57.0b9 (20171016185129) across platforms: Win 10 x64, Mac OS 10.11 and Ubuntu 16.04 x64.

Hi Ciprian, How were you able to get the Downloads onto the Sidebar? I only see the option for Bookmarks, History, and Synced Tabs.

(In reply to Grover Wimberly IV [:Grover-QA] from comment #40) > Hi Ciprian, > > How were you able to get the Downloads onto the Sidebar? I only see the > option for Bookmarks, History, and Synced Tabs. Hi Grover, I only managed to install a few extensions (as stated in comment 39) which appeared in the sidebar view with no icons after the installation. I'm not familiar with this but I think you need to write a lot of extensions. Maybe Kestrel can elaborate more about this matter on how Downloads, Bookmars, History etc. can be added in sidebar.

(In reply to Ciprian Georgiu, QA [:ciprian_georgiu] from comment #41) ... > Downloads, Bookmarks, History etc. can be added in sidebar. Need to make a correction here. Bookmarks and History, these are already in sidebar view.

That was just me experimenting with my own sidebar extensions that loaded `about` pages like `about:downloads` before Bug 1190679 made that no longer possible.

Verified on Windows 10 64bit, Mac OS X 10.11, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) with today's build of Nightly.