Closed Bug 1368311 Opened 3 years ago Closed 3 years ago

Sidebar header icons are invisible with dark OS themes

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.7 - Jun 12
Tracking Status
firefox55 --- fixed
firefox58 --- verified

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: access, Whiteboard: [reserve-photon-structure])

Attachments

(2 files)

Attached image screenshot
The current sidebar's icon, the dropdown arrow and the close button are all invisible.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Why reserve-photon-structure? I don't think this can be shipped in its current state.
Flags: needinfo?(mmucci)
Flags: needinfo?(mmucci)
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-
(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)
(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)
(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 on attachment 8872132 [details]
Bug 1368311 - Fix sidebar header icon colors.

https://reviewboard.mozilla.org/r/143620/#review148378
Attachment #8872132 - Flags: review?(bgrinstead) → review+
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.7 - Jun 12
Flags: qe-verify? → qe-verify+
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.
You need to log in before you can comment on or make changes to this bug.