1370686 - Extensions sidebar icon doesn't show up in sidebar switcher panel

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Mike Kaply [:mkaply]]

Currently if you add an icon to a sidebar, it's shown in the dropdown for the sidebar button (where there are no other icons).

If you then open the sidebar and click the title, you'll see a dropdown where Firefox shows icons for Bookmarks/History/Synced Tabs, but the WebExtension sidebar does not have an icon.

We should match Firefox in both cases.

Comment 1

[Shane Caraveo (:mixedpuppy)]

The toolbarbutton above will loose the dropdown, but the icon shouldn't be missing from the menu in the sidebar titlebar.

Comment 2

[Andy McKay]

In my latest nightly, the button has no icon. The sidebar menu that photon adds has an icon. All looks consistent and nice to me.

Comment 3

[Kestrel]

Attachment: sidebar_icons.png

I'm not seeing the sidebar icon anywhere at all with the example sidebar extension on latest Nightly. Could this please be reopened?

Comment 4

[Andy McKay]

Sure, it was there a couple of days ago, now it is gone. I'd guess this is related to all the photon changes.

Comment 5

[:Gijs (he/him)]

Can someone check if this is working in 55 beta, and, if not, set the relevant release tracking flags?

Comment 6

[Andy McKay]

Works in beta 55. Couldn't reproduce with or without the photon tracking flag.

Comment 7

[:Gijs (he/him)]

I can't reproduce on OS X when loading the manifest.json for annotate-page from https://github.com/mdn/webextensions-examples (which I assume is what folks mean by 'example add-on'?) as a temporary add-on from about:debugging in today's nightly.

Can someone provide more detailed STR? Is this Windows- or Linux-specific, or does it only happen after a restart, or ...?

Comment 8

[Kestrel]

It's due to the combination of these two styles on Windows 10, remove one or the other and the icon displays:

#sidebarMenu-popup .subviewbutton-iconic > .toolbarbutton-icon {
    padding-inline-start: 16px;
}

toolbarbutton.webextension-menuitem > .toolbarbutton-icon {
    width: 16px;
    height: 16px;
}

Comment 9

[:Gijs (he/him)]

(In reply to Kestrel from comment #8)
> It's due to the combination of these two styles on Windows 10, remove one or
> the other and the icon displays:
> 
> #sidebarMenu-popup .subviewbutton-iconic > .toolbarbutton-icon {
>     padding-inline-start: 16px;
> }

Ah, and this rule is %ifndef XP_MACOSX ( https://dxr.mozilla.org/mozilla-central/rev/bb8eab3c3ac4147848c4c85d628ba72029978665/browser/themes/shared/sidebar.inc.css#112-120 ), which explains why I couldn't reproduce on OS X...

Comment 11

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

Attachment: Bug 1370686 - Show the WebExtensions icon in the sidebar dropdown. , ui-r=shorlander

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

Comment 12

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

For add-ons with icons, it looks like https://cl.ly/2C323N0s3S0q and for add-ons without icons, it looks like https://cl.ly/460S3h1C3k3e

Comment 13

[Aaron Benson]

This looks great! I would recommend using the default icon color (see Bookmarks/History/Synced Tabs) when we use the add-ons icon and not the custom icon.

Comment 14

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

Comment on attachment 8887158 [details]
Bug 1370686 - Show the WebExtensions icon in the sidebar dropdown. , ui-r=shorlander

https://reviewboard.mozilla.org/r/157904/#review163076

::: browser/components/extensions/ext-sidebarAction.js:204
(Diff revision 1)
> +
> +    let sidebar = menuitem.ownerDocument.getElementById('sidebar-header');
> +    sidebar.setAttribute("style", `
> +      --webextension-menuitem-image: url("${getIcon(16)}");
> +      --webextension-menuitem-image-2x: url("${getIcon(32)}");
> +    `);

This is wrong.  I should be setting this when the panel changes, not when it's built.  (But I can't seem to find the event that occurs when the panel is shown…)

::: browser/themes/shared/sidebar.inc.css:149
(Diff revision 1)
>    -moz-context-properties: fill;
>    fill: currentColor;
>    opacity: 0.8;
>  }
> +
> +#sidebar-box[sidebarcommand$="-sidebar-action"] > #sidebar-header > #sidebar-switcher-target > #sidebar-icon {

We want a little more stuff here, to handle the case where we don't have an icon, and use the default one.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8887158 [details]
Bug 1370686 - Show the WebExtensions icon in the sidebar dropdown. , ui-r=shorlander

https://reviewboard.mozilla.org/r/157906/#review163404

Clearing review for now given the self-review. I expect you should be able to use a popupshowing listener on the popup (`#sidebarMenu-popup`) to attach when the popup is being shown rather than when it is built?

::: browser/base/content/browser.css
(Diff revision 1)
>      list-style-image: var(--webextension-menuitem-image-2x, inherit);
>    }
>  }
>  
>  toolbarbutton.webextension-menuitem > .toolbarbutton-icon {
> -  width: 16px;

Out of curiosity, why remove this? I rather expect it should be kept, so that webextensions that ship a 32x16px (or 16x32px) icon still end up with a 'square' slot for their icon.

Comment 16

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

Comment on attachment 8887158 [details]
Bug 1370686 - Show the WebExtensions icon in the sidebar dropdown. , ui-r=shorlander

https://reviewboard.mozilla.org/r/157906/#review163404

> Out of curiosity, why remove this? I rather expect it should be kept, so that webextensions that ship a 32x16px (or 16x32px) icon still end up with a 'square' slot for their icon.

Without this, there's no space for the checkmark beside the icon.  Perhaps there's a better way to do that, by adding a margin for the checkmark instead of padding?  I'll look into it.

Comment 18

[erosman]

On FF56.0b1 (Win 64), I also noticed icons are missing from the new Sidebar popup as well as Menu -> View -> Sidebar

Comment 19

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

Comment on attachment 8887158 [details]
Bug 1370686 - Show the WebExtensions icon in the sidebar dropdown. , ui-r=shorlander

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

Comment 20

[:Gijs (he/him)]

Comment on attachment 8887158 [details]
Bug 1370686 - Show the WebExtensions icon in the sidebar dropdown. , ui-r=shorlander

https://reviewboard.mozilla.org/r/157906/#review174034

Fantastic, A++, would review again.

Comment 21

[Pulsebot]

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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b33b2839e981

Comment 23

[u554753]

Hey Gijs, can you clarify the STR for this?

Comment 24

[:Gijs (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #23)
> Hey Gijs, can you clarify the STR for this?

STR:

1. install https://addons.mozilla.org/en-US/firefox/addon/tree-style-tab/ (or another add-on that provides a sidebar)
2. open the sidebar
3. click the side bar header to get the dropdown with a list of sidebars you might want to open

ER:
the entry for tree style tabs has an icon

AR (pre-patch):
no icon on that entry


Does that help?

Comment 25

[u554753]

Verified on Windows 10 64bit, Mac OS X 10.11, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) as of this date.

Comment 26

[Ciprian Georgiu, Desktop QA]

Verified fixed on 57 Beta 6 (20171005195903) by following the STR from comment 24. Tested on Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.