Sidebar header icons are invisible with dark OS themes

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Theme
P1
normal
RESOLVED FIXED
6 months ago
2 months ago

People

(Reporter: dao, Assigned: dao)

Tracking

({access})

Trunk
Firefox 55
access
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 fixed, firefox58 verified)

Details

(Whiteboard: [reserve-photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

6 months ago
Created attachment 8872130 [details]
screenshot

The current sidebar's icon, the dropdown arrow and the close button are all invisible.
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED

Updated

6 months ago
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
(Assignee)

Comment 2

6 months ago
Why reserve-photon-structure? I don't think this can be shipped in its current state.
Flags: needinfo?(mmucci)

Updated

6 months ago
Flags: needinfo?(mmucci)

Comment 3

6 months ago
mozreview-review
Comment on attachment 8872132 [details]
Bug 1368311 - Fix sidebar header icon colors.

https://reviewboard.mozilla.org/r/143620/#review147956

::: browser/themes/shared/sidebar.inc.css:63
(Diff revision 1)
> -#sidebar-switcher-arrow {
> +#sidebar-icon,
> +#sidebar-switcher-arrow,
> +#sidebar-close > .toolbarbutton-icon {
>    -moz-context-properties: fill;
> -  fill: var(--icon-fill);
> +  fill: currentColor;
> +  opacity: 0.9;

We should set the opacity to .8 here to more closely match the designs and requested color from shorlander (the current value of --icon-fill).

Here's the computed colors I see for the icon with various options (measured on OSX):

--icon-fill: #3A3A3B
currentColor + .8 opacity: #303030
currentColor + .9 opacity: #171717

::: browser/themes/shared/sidebar.inc.css:82
(Diff revision 1)
>  }
>  
>  #sidebar-switcher-target {
>    -moz-appearance: none;
>    padding: 4px;
> -  margin-inline-end: 4px;
> +  color: inherit;

Why's this needed?

::: browser/themes/shared/sidebar.inc.css:107
(Diff revision 1)
>  }
>  %endif
>  
>  /* Use bookmarks star as default icon for the sidebar box (including when opening a web page) */
>  #sidebar-switcher-bookmarks > .toolbarbutton-icon,
> -#sidebar-box #sidebar-icon {
> +#sidebar-box[sidebarcommand="viewBookmarksSidebar"] > #sidebar-header > #sidebar-switcher-target > #sidebar-icon {

This was set as a default value so that it would keep the icon for web pages loaded in the sidebar.  This could also be done explicitly via:

#sidebar-box[sidebarcommand="viewWebPanelsSidebar"] > #sidebar-header > #sidebar-switcher-target > #sidebar-icon

But we shouldn't remove the bookmark star icon in this case
Attachment #8872132 - Flags: review?(bgrinstead) → review-
(Assignee)

Comment 4

6 months ago
(In reply to Brian Grinstead [:bgrins] from comment #3)
> >  #sidebar-switcher-target {
> >    -moz-appearance: none;
> >    padding: 4px;
> > -  margin-inline-end: 4px;
> > +  color: inherit;
> 
> Why's this needed?

Toolbarbuttons don't inherit color by default and the color isn't always what we need. E.g.: http://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/toolkit/themes/linux/global/toolbarbutton.css#40-42

> ::: browser/themes/shared/sidebar.inc.css:107
> (Diff revision 1)
> >  }
> >  %endif
> >  
> >  /* Use bookmarks star as default icon for the sidebar box (including when opening a web page) */
> >  #sidebar-switcher-bookmarks > .toolbarbutton-icon,
> > -#sidebar-box #sidebar-icon {
> > +#sidebar-box[sidebarcommand="viewBookmarksSidebar"] > #sidebar-header > #sidebar-switcher-target > #sidebar-icon {
> 
> This was set as a default value so that it would keep the icon for web pages
> loaded in the sidebar.

Why would we want to use the same icon for the bookmarks sidebar and these pages? This seems unnecessary and confusing, especially when you open the dropdown and see the same icon twice, used for two different things. Not showing an icon here for random pages seems strictly better to me.
Flags: needinfo?(bgrinstead)
(In reply to Dão Gottwald [::dao] from comment #4)
> > ::: browser/themes/shared/sidebar.inc.css:107
> > (Diff revision 1)
> > >  }
> > >  %endif
> > >  
> > >  /* Use bookmarks star as default icon for the sidebar box (including when opening a web page) */
> > >  #sidebar-switcher-bookmarks > .toolbarbutton-icon,
> > > -#sidebar-box #sidebar-icon {
> > > +#sidebar-box[sidebarcommand="viewBookmarksSidebar"] > #sidebar-header > #sidebar-switcher-target > #sidebar-icon {
> > 
> > This was set as a default value so that it would keep the icon for web pages
> > loaded in the sidebar.
> 
> Why would we want to use the same icon for the bookmarks sidebar and these
> pages? This seems unnecessary and confusing, especially when you open the
> dropdown and see the same icon twice, used for two different things. Not
> showing an icon here for random pages seems strictly better to me.

There is not entry in the dropdown for web pages, the icon is only next to the title in the header. I asked UX about this before doing the review, they want to keep the icon there as an anchor for the dropdown and to keep the layout consistent
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 6

6 months ago
(In reply to Brian Grinstead [:bgrins] from comment #5)
> (In reply to Dão Gottwald [::dao] from comment #4)
> > > ::: browser/themes/shared/sidebar.inc.css:107
> > > (Diff revision 1)
> > > >  }
> > > >  %endif
> > > >  
> > > >  /* Use bookmarks star as default icon for the sidebar box (including when opening a web page) */
> > > >  #sidebar-switcher-bookmarks > .toolbarbutton-icon,
> > > > -#sidebar-box #sidebar-icon {
> > > > +#sidebar-box[sidebarcommand="viewBookmarksSidebar"] > #sidebar-header > #sidebar-switcher-target > #sidebar-icon {
> > > 
> > > This was set as a default value so that it would keep the icon for web pages
> > > loaded in the sidebar.
> > 
> > Why would we want to use the same icon for the bookmarks sidebar and these
> > pages? This seems unnecessary and confusing, especially when you open the
> > dropdown and see the same icon twice, used for two different things. Not
> > showing an icon here for random pages seems strictly better to me.
> 
> There is not entry in the dropdown for web pages, the icon is only next to
> the title in the header.

And when opening the dropdown, you'll see the current title + icon above the popup. This is confusing as it makes it look as if the Bookmarks option from the popup was already picked.
Flags: needinfo?(bgrinstead)
(In reply to Dão Gottwald [::dao] from comment #6)
> (In reply to Brian Grinstead [:bgrins] from comment #5)
> > (In reply to Dão Gottwald [::dao] from comment #4)
> > > > ::: browser/themes/shared/sidebar.inc.css:107
> > > > (Diff revision 1)
> > > > >  }
> > > > >  %endif
> > > > >  
> > > > >  /* Use bookmarks star as default icon for the sidebar box (including when opening a web page) */
> > > > >  #sidebar-switcher-bookmarks > .toolbarbutton-icon,
> > > > > -#sidebar-box #sidebar-icon {
> > > > > +#sidebar-box[sidebarcommand="viewBookmarksSidebar"] > #sidebar-header > #sidebar-switcher-target > #sidebar-icon {
> > > > 
> > > > This was set as a default value so that it would keep the icon for web pages
> > > > loaded in the sidebar.
> > > 
> > > Why would we want to use the same icon for the bookmarks sidebar and these
> > > pages? This seems unnecessary and confusing, especially when you open the
> > > dropdown and see the same icon twice, used for two different things. Not
> > > showing an icon here for random pages seems strictly better to me.
> > 
> > There is not entry in the dropdown for web pages, the icon is only next to
> > the title in the header.
> 
> And when opening the dropdown, you'll see the current title + icon above the
> popup. This is confusing as it makes it look as if the Bookmarks option from
> the popup was already picked.

Sure, but this was generally true even before the sidebar updates (the sidebar popup has never included web content as an menu item). I'm sure there are plenty of options for improving the UI for pages loaded in the sidebar but it shouldn't be folded into this patch as a side effect without UX involvement. Please leave that behavior unchanged here and file a new UX bug or start a thread on the mailing list to discuss further.
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 8

6 months ago
(In reply to Brian Grinstead [:bgrins] from comment #7)
> (In reply to Dão Gottwald [::dao] from comment #6)
> > (In reply to Brian Grinstead [:bgrins] from comment #5)
> > > (In reply to Dão Gottwald [::dao] from comment #4)
> > > > > ::: browser/themes/shared/sidebar.inc.css:107
> > > > > (Diff revision 1)
> > > > > >  }
> > > > > >  %endif
> > > > > >  
> > > > > >  /* Use bookmarks star as default icon for the sidebar box (including when opening a web page) */
> > > > > >  #sidebar-switcher-bookmarks > .toolbarbutton-icon,
> > > > > > -#sidebar-box #sidebar-icon {
> > > > > > +#sidebar-box[sidebarcommand="viewBookmarksSidebar"] > #sidebar-header > #sidebar-switcher-target > #sidebar-icon {
> > > > > 
> > > > > This was set as a default value so that it would keep the icon for web pages
> > > > > loaded in the sidebar.
> > > > 
> > > > Why would we want to use the same icon for the bookmarks sidebar and these
> > > > pages? This seems unnecessary and confusing, especially when you open the
> > > > dropdown and see the same icon twice, used for two different things. Not
> > > > showing an icon here for random pages seems strictly better to me.
> > > 
> > > There is not entry in the dropdown for web pages, the icon is only next to
> > > the title in the header.
> > 
> > And when opening the dropdown, you'll see the current title + icon above the
> > popup. This is confusing as it makes it look as if the Bookmarks option from
> > the popup was already picked.
> 
> Sure, but this was generally true even before the sidebar updates (the
> sidebar popup has never included web content as an menu item).

I don't understand what you're saying here. The popup I'm talking about is the one you introduced. The issue of the header using the same icon for web content as the popup does for Bookmarks is entirely new.

> I'm sure
> there are plenty of options for improving the UI for pages loaded in the
> sidebar but it shouldn't be folded into this patch as a side effect without
> UX involvement. Please leave that behavior unchanged here and file a new UX
> bug or start a thread on the mailing list to discuss further.

Filed bug 1368945.
Comment hidden (mozreview-request)

Comment 10

6 months ago
mozreview-review
Comment on attachment 8872132 [details]
Bug 1368311 - Fix sidebar header icon colors.

https://reviewboard.mozilla.org/r/143620/#review148378
Attachment #8872132 - Flags: review?(bgrinstead) → review+

Comment 11

6 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7fdbcd53500
Fix sidebar header icon colors. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/a7fdbcd53500
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

6 months ago
Iteration: --- → 55.7 - Jun 12

Updated

5 months ago
Flags: qe-verify? → qe-verify+

Updated

5 months ago
QA Contact: gwimberly
Verfied on Windows 10 64bit, Mac OS X 10.11, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) of this this date.
status-firefox58: --- → verified
You need to log in before you can comment on or make changes to this bug.