1301758 - Use CSS variables for dimmed elements in arrow panels

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P2)

Description

[Dão Gottwald [:dao]]

Attachment: patch

This is inspired by your patch in bug 1282768 but goes a bit further. I'd like to land this separately since it's generally useful. I intentionally didn't touch downloads stuff since I assume you're working on elm and this is based on mozilla-central.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8789894 [details] [diff] [review]
patch

Review of attachment 8789894 [details] [diff] [review]:
-----------------------------------------------------------------

Apart from a couple of questions I have, this looks good to me. Since I will have to rebase my patch anyway to use your variable names (which I prefer by the way), why not modify the downloads panel as well? My work is not really relevant to that in any way.

::: browser/themes/linux/searchbar.css
@@ -300,5 @@
>    min-height: 32px;
>  }
>  
>  .search-setting-button[selected] {
> -  background-color: hsla(210,4%,10%,.15);

Wouldn't --arrowpanel-dimmed-even-further be closer to .15 as it dims by .17? And semantically, shouldn't we treat a selected state as an active state (for which we use --arrowpanel-dimmed-even-further)?

::: browser/themes/shared/customizableui/panelUI.inc.css
@@ +940,5 @@
>  }
>  
>  #PanelUI-footer-fxa[fxastatus="error"] > #PanelUI-fxa-status:hover:active {
> +  background-color: hsl(42,94%,82%);
> +  box-shadow: 0 1px 0 hsl(210,4%,10%) inset;

I realize an opacity amount of 5% is tiny, but I'm curious as to why you are removing it entirely? Do we have a (even informal) policy about the amounts we use?

Also, if the original intent was to have an almost invisible shadow, by removing it entirely you are effectively making it 100%, right? Did you mean to use --arrowpanel-dimmed here instead?

@@ +1111,5 @@
>  menuitem.subviewbutton@menuStateActive@,
>  .share-provider-button:-moz-any(@buttonStateActive@,[checked=true]),
>  .widget-overflow-list .toolbarbutton-1@buttonStateActive@,
>  .toolbaritem-combined-buttons@inAnyPanel@ > toolbarbutton@buttonStateActive@ {
> +  background-color: var(--arrowpanel-dimmed-further);

I understand that you are not changing the value here, but perhaps we should, given that we associate the active state with the --arrowpanel-dimmed-even-further color? I guess this is the same question as in my first comment.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Panos Astithas [:past] from comment #1)
> Apart from a couple of questions I have, this looks good to me. Since I will
> have to rebase my patch anyway to use your variable names (which I prefer by
> the way), why not modify the downloads panel as well? My work is not really
> relevant to that in any way.

Ahem, yes, I can do that. I confused the downloads work with the control center work.

> ::: browser/themes/linux/searchbar.css
> @@ -300,5 @@
> >    min-height: 32px;
> >  }
> >  
> >  .search-setting-button[selected] {
> > -  background-color: hsla(210,4%,10%,.15);
> 
> Wouldn't --arrowpanel-dimmed-even-further be closer to .15 as it dims by
> .17? And semantically, shouldn't we treat a selected state as an active
> state (for which we use --arrowpanel-dimmed-even-further)?

I intentionally didn't name it "active", because that's not how we use these colors. If an element is normally transparent but should be dimmed on hover, you'd use --arrowpanel-dimmed for that (and --arrowpanel-dimmed-further for :active). If an element is normally dimmed (such as panel footers), you'd use --arrowpanel-dimmed for that, --arrowpanel-dimmed-further on hover and --arrowpanel-dimmed-even-further for :active.

> ::: browser/themes/shared/customizableui/panelUI.inc.css
> @@ +940,5 @@
> >  }
> >  
> >  #PanelUI-footer-fxa[fxastatus="error"] > #PanelUI-fxa-status:hover:active {
> > +  background-color: hsl(42,94%,82%);
> > +  box-shadow: 0 1px 0 hsl(210,4%,10%) inset;
> 
> I realize an opacity amount of 5% is tiny, but I'm curious as to why you are
> removing it entirely? Do we have a (even informal) policy about the amounts
> we use?
> 
> Also, if the original intent was to have an almost invisible shadow, by
> removing it entirely you are effectively making it 100%, right? Did you mean
> to use --arrowpanel-dimmed here instead?

This was a typo or oversight, I'll keep the box-shadow as it was.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch v2

review comments addressed

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c58144c28d79
Use CSS variables for dimmed elements in arrow panels. r=past

Comment 5

[Carsten Book [:Tomcat]]

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