1374048 - Sidebar extension icon does not appear in Photon sidebar header

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Kestrel]

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

Comment 1

[:shell escalante]

andy looking for dupe

Comment 2

[Andy McKay]

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

Comment 3

[Kestrel]

Attachment: sidebar_header_comparison.png

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.

Comment 4

[Andy McKay]

Ok, moving over to the photon team.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Attachment: Bug 1374048 - Show the sidebar extension icon in the header. , ui-r=shorlander.

Review commit: https://reviewboard.mozilla.org/r/164082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164082/

Comment 6

[Blake Winton (:bwinton) (:☕️)]

(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 7

[:Gijs (he/him)]

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 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8893085 [details]
Bug 1374048 - Show the sidebar extension icon in the header.  , ui-r=shorlander.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164082/diff/1-2/

Comment 9

[Blake Winton (:bwinton) (:☕️)]

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 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8893085 [details]
Bug 1374048 - Show the sidebar extension icon in the header.  , ui-r=shorlander.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164082/diff/2-3/

Comment 11

[:Gijs (he/him)]

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.

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

(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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I would expect that too.

When aLoadingPrincipal is not system, what is its URI?

Comment 14

[Trevor Rowbotham [:rowbot]]

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.

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

(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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> It is a SubstitutingURL.

OK, but what actual string?

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

(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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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...

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Blake Winton (:bwinton) (:☕️)]

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.)

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Kris Maglione [:kmag]]

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 25

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8893085 [details]
Bug 1374048 - Show the sidebar extension icon in the header.  , ui-r=shorlander.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164082/diff/3-4/

Comment 26

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8893085 [details]
Bug 1374048 - Show the sidebar extension icon in the header.  , ui-r=shorlander.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164082/diff/4-5/

Comment 27

[:Gijs (he/him)]

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 28

[:Gijs (he/him)]

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 29

[Blake Winton (:bwinton) (:☕️)]

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 30

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8893085 [details]
Bug 1374048 - Show the sidebar extension icon in the header.  , ui-r=shorlander.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164082/diff/5-6/

Comment 31

[:Gijs (he/him)]

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 32

[Blake Winton (:bwinton) (:☕️)]

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)…

Comment 33

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8893085 [details]
Bug 1374048 - Show the sidebar extension icon in the header.  , ui-r=shorlander.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164082/diff/6-7/

Comment 34

[Pulsebot]

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

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82c118f8df03

Comment 36

[Blake Winton (:bwinton) (:☕️)]

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

Comment 37

[Kestrel]

(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.

Comment 38

[:Gijs (he/him)]

(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

Comment 39

[Ciprian Georgiu, Desktop QA]

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.

Comment 40

[u554753]

Hi Ciprian,

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

Comment 41

[Ciprian Georgiu, Desktop QA]

(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.

Comment 42

[Ciprian Georgiu, Desktop QA]

(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.

Comment 43

[Kestrel]

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.

Comment 44

[u554753]

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.