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

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: mkaply, Assigned: bwinton)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-structure] [sidebar], URL)

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.

Updated

2 years ago
Flags: needinfo?(amckay)
Whiteboard: [sidebar]
The toolbarbutton above will loose the dropdown, but the icon shouldn't be missing from the menu in the sidebar titlebar.

Comment 2

2 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(amckay)
Resolution: --- → WORKSFORME

Comment 3

2 years ago
Posted image 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?
Flags: needinfo?(amckay)

Comment 4

2 years ago
Sure, it was there a couple of days ago, now it is gone. I'd guess this is related to all the photon changes.
Status: RESOLVED → REOPENED
Component: WebExtensions: General → General
Flags: needinfo?(amckay)
Product: Toolkit → Firefox
Resolution: WORKSFORME → ---
Summary: Sidebar icon is used in the opposite place as Firefox → Extensions sidebar icon doesn't show up

Comment 5

2 years ago
Can someone check if this is working in 55 beta, and, if not, set the relevant release tracking flags?
Blocks: 1353421, 1355324
Summary: Extensions sidebar icon doesn't show up → Extensions sidebar icon doesn't show up in sidebar switcher panel
Whiteboard: [sidebar] → [photon-structure][triage][sidebar]

Comment 6

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

Comment 7

2 years ago
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 ...?
Flags: needinfo?(kestrel)

Comment 8

2 years ago
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;
}
Flags: needinfo?(kestrel)

Comment 9

2 years ago
(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...

Updated

2 years ago
Duplicate of this bug: 1374048
Status: REOPENED → NEW
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-structure][triage][sidebar] → [photon-structure] [sidebar]

Updated

2 years ago
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
Comment hidden (mozreview-request)
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Priority: P2 → P1
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

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8887158 - Flags: review?(shorlander)
(Assignee)

Comment 14

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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.
Attachment #8887158 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 16

2 years ago
mozreview-review-reply
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.

Updated

2 years ago
Blocks: 1387512
Whiteboard: [photon-structure] [sidebar] → [reserve-photon-structure] [sidebar]
Duplicate of this bug: 1355642

Comment 18

2 years ago
On FF56.0b1 (Win 64), I also noticed icons are missing from the new Sidebar popup as well as Menu -> View -> Sidebar
Comment hidden (mozreview-request)

Comment 20

2 years ago
mozreview-review
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.
Attachment #8887158 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 21

2 years ago
Pushed by bwinton@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b33b2839e981
Show the WebExtensions icon in the sidebar dropdown. r=Gijs, ui-r=shorlander

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b33b2839e981
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.2 - Aug 29

Comment 23

2 years ago
Hey Gijs, can you clarify the STR for this?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 24

2 years ago
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gwimberly)

Comment 25

2 years ago
Verified on Windows 10 64bit, Mac OS X 10.11, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) as of this date.
Flags: needinfo?(gwimberly)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.