Closed Bug 1377655 Opened 2 years ago Closed 2 years ago

Remove controlcenter-specific subview arrows

Categories

(Firefox :: Theme, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: dao, Assigned: dao)

Details

Attachments

(1 file)

No description provided.
Comment on attachment 8882785 [details]
Bug 1377655 - Remove controlcenter-specific subview arrows.

https://reviewboard.mozilla.org/r/153892/#review159046

I'm generally in favor of this kind of optimization, but we have to be careful here. I'm seeing a noticeable lag/stutter on my 2014 Windows laptop when switching between the main view and the subview on the identity popup after this change. It's buttery smooth without the patch.

Can you confirm the stutter on your computer?

::: browser/themes/shared/controlcenter/panel.inc.css:132
(Diff revision 1)
>    -moz-appearance: none;
> -  background: url("chrome://browser/skin/controlcenter/arrow-subview.svg") center no-repeat;
> +  background: url("chrome://browser/skin/arrow-left.svg") center no-repeat;
>    background-size: 16px, auto;
> +  -moz-context-properties: fill;
> +  fill: currentColor;
> +  color: inherit;

This doesn't work for me (on Windows). The icon is black in both the expanded and non-expanded state. 

I'm having a hard time validating that this pattern works at all, since all usages I've found so far are simply black icons.

Explicitly setting fill: GrayText and fill: HighlightText works, and I don't really see how it's worse than this :)
Attachment #8882785 - Flags: review?(jhofmann) → review-
(In reply to Johann Hofmann [:johannh] from comment #2)
> I'm generally in favor of this kind of optimization, but we have to be
> careful here. I'm seeing a noticeable lag/stutter on my 2014 Windows laptop
> when switching between the main view and the subview on the identity popup
> after this change. It's buttery smooth without the patch.
> 
> Can you confirm the stutter on your computer?

No, I cannot. Seems to work fine on Ubuntu and Windows 10.

> ::: browser/themes/shared/controlcenter/panel.inc.css:132
> (Diff revision 1)
> >    -moz-appearance: none;
> > -  background: url("chrome://browser/skin/controlcenter/arrow-subview.svg") center no-repeat;
> > +  background: url("chrome://browser/skin/arrow-left.svg") center no-repeat;
> >    background-size: 16px, auto;
> > +  -moz-context-properties: fill;
> > +  fill: currentColor;
> > +  color: inherit;
> 
> This doesn't work for me (on Windows). The icon is black in both the
> expanded and non-expanded state. 

This worked on Ubuntu, just checked Windows 10 too.

> I'm having a hard time validating that this pattern works at all, since all
> usages I've found so far are simply black icons.

We do use fill: currentColor for non-black icons, for instance http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/themes/shared/contextmenu.inc.css#16 on Ubuntu.
Comment on attachment 8882785 [details]
Bug 1377655 - Remove controlcenter-specific subview arrows.

https://reviewboard.mozilla.org/r/153892/#review166710

Sorry for the delay. This works fine for me now, in all regards. I'm really not sure what was breaking it before, but I can't reproduce it.
Attachment #8882785 - Flags: review?(jhofmann) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/efd5c608d474
Remove controlcenter-specific subview arrows. r=johannh
https://hg.mozilla.org/mozilla-central/rev/efd5c608d474
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.