1368311 - Sidebar header icons are invisible with dark OS themes

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P1)

Description

[Dão Gottwald [:dao]]

Attachment: screenshot

The current sidebar's icon, the dropdown arrow and the close button are all invisible.

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1368311 - Fix sidebar header icon colors.

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

Comment 2

[Dão Gottwald [:dao]]

Why reserve-photon-structure? I don't think this can be shipped in its current state.

Comment 3

[Brian Grinstead [:bgrins]]

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

Comment 4

[Dão Gottwald [:dao]]

(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.

Comment 5

[Brian Grinstead [:bgrins]]

(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

Comment 6

[Dão Gottwald [:dao]]

(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.

Comment 7

[Brian Grinstead [:bgrins]]

(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.

Comment 8

[Dão Gottwald [:dao]]

(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 9

[Dão Gottwald [:dao]]

Comment on attachment 8872132 [details]
Bug 1368311 - Fix sidebar header icon colors.

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

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8872132 [details]
Bug 1368311 - Fix sidebar header icon colors.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7fdbcd53500
Fix sidebar header icon colors. r=bgrins

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[u554753]

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