Closed
Bug 1370686
Opened 7 years ago
Closed 7 years ago
Extensions sidebar icon doesn't show up in sidebar switcher panel
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
People
(Reporter: mkaply, Assigned: bwinton)
References
(Blocks 1 open bug, )
Details
(Whiteboard: [reserve-photon-structure] [sidebar])
Attachments
(2 files)
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•7 years ago
|
Flags: needinfo?(amckay)
Whiteboard: [sidebar]
Comment 1•7 years ago
|
||
The toolbarbutton above will loose the dropdown, but the icon shouldn't be missing from the menu in the sidebar titlebar.
Comment 2•7 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
Closed: 7 years ago
Flags: needinfo?(amckay)
Resolution: --- → WORKSFORME
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•7 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•7 years ago
|
||
Can someone check if this is working in 55 beta, and, if not, set the relevant release tracking flags?
Comment 6•7 years ago
|
||
Works in beta 55. Couldn't reproduce with or without the photon tracking flag.
Comment 7•7 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)
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•7 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•7 years ago
|
Status: REOPENED → NEW
Updated•7 years ago
|
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-structure][triage][sidebar] → [photon-structure] [sidebar]
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
Updated•7 years ago
|
QA Contact: gwimberly
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Priority: P2 → P1
Assignee | ||
Comment 12•7 years ago
|
||
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•7 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•7 years ago
|
Attachment #8887158 -
Flags: review?(shorlander)
Assignee | ||
Comment 14•7 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•7 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•7 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•7 years ago
|
Attachment #8887158 -
Flags: review?(shorlander) → review+
Updated•7 years ago
|
Whiteboard: [photon-structure] [sidebar] → [reserve-photon-structure] [sidebar]
Comment 18•7 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•7 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•7 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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•7 years ago
|
Iteration: --- → 57.2 - Aug 29
Comment 23•7 years ago
|
||
Hey Gijs, can you clarify the STR for this?
Flags: needinfo?(gijskruitbosch+bugs)
Comment 24•7 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•7 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.
status-firefox58:
--- → verified
Flags: needinfo?(gwimberly)
Comment 26•7 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•