Use CSS variables for dimmed elements in arrow panels

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Theme
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
Created attachment 8789894 [details] [diff] [review]
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.
Attachment #8789894 - Flags: review?(past)
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.
Attachment #8789894 - Flags: review?(past) → review+
(Assignee)

Comment 2

a year ago
(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.
(Assignee)

Updated

a year ago
Assignee: nobody → dao+bmo
(Assignee)

Comment 3

a year ago
Created attachment 8790284 [details] [diff] [review]
patch v2

review comments addressed
Attachment #8789894 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8790284 - Attachment description: patch → patch v2

Comment 4

a year ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c58144c28d79
Use CSS variables for dimmed elements in arrow panels. r=past
(Assignee)

Updated

a year ago
Blocks: 1302355
(Assignee)

Updated

a year ago
Priority: -- → P2

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c58144c28d79
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.