1295947 - Use filter and fill properties instead of duplicating lots of SVG and CSS for the tab audio icon

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[:Gijs (he/him)]

This review would be a lot quicker if you can give me a paragraph of description of what the thrust of the tab-audio related changes is, instead of me trying to reverse-engineer it from the patch, given that neither the patch nor the bug has any real description beyond "refactor styling for X" which might as well be "change the code involving X", which isn't very useful.

Comment 2

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #1)
> patch nor the bug has any real description beyond "refactor styling for X"
> which might as well be "change the code involving X", which isn't very
> useful.

It was supposed to mean "clean up and simplify the code while maintaining the outcome." The outcome isn't actually the same, but should be roughly as good as it was or better (e.g. in the selected tab on Linux).

I didn't expand on how I simplified the code since I thought a look at the patch would make it obvious that I'm deleting most of the icon variants and use filter and fill instead.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #2)
> (In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #1)
> > patch nor the bug has any real description beyond "refactor styling for X"
> > which might as well be "change the code involving X", which isn't very
> > useful.
> 
> It was supposed to mean "clean up and simplify the code while maintaining
> the outcome." The outcome isn't actually the same, but should be roughly as
> good as it was or better (e.g. in the selected tab on Linux).
> 
> I didn't expand on how I simplified the code since I thought a look at the
> patch would make it obvious that I'm deleting most of the icon variants and
> use filter and fill instead.

I also got rid of the "pressed" states since the slight opacity change seemed basically invisible anyway.

Comment 4

[:Gijs (he/him)]

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

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

r- for regressing the outline/shadow on the icon.

::: browser/themes/shared/tabs.inc.css
@@ +145,5 @@
>  
>  .tab-icon-overlay[soundplaying],
>  .tab-icon-overlay[muted]:not([crashed]) {
> +  list-style-image: url(chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio-small);
> +  filter: url(chrome://browser/skin/filters.svg#fill) drop-shadow(1px 1px 1px white);

This doesn't work as well as the actual white outline that's in the SVG:

http://imgur.com/a/iuwY3

(top is current beta, bottom is nightly with your patch)

@@ +156,5 @@
>  
>  .tab-icon-overlay[soundplaying]:hover,
>  .tab-icon-overlay[muted]:not([crashed]):hover {
>    background-color: white;
> +  border-radius: 10px;

If I do this:

http://jsbin.com/yijimoxidi/edit?html,css,output

I see flickering when hovering the corners that would be out of the 'hover'able area when the border radius is applied.

It's harder to reproduce on a pinned tab but I do see oddness about the hover region. Can we just keep the border-radius in the non-hover rule?

::: browser/themes/windows/browser.css
@@ +2225,5 @@
> +  fill: currentColor;
> +}
> +
> +.alltabs-endimage[muted] {
> +  list-style-image: url(chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio-muted);

Can we move this into tabs.inc.css as well, as it's now identical between the 3 OS themes?

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #4)
> @@ +156,5 @@
> >  
> >  .tab-icon-overlay[soundplaying]:hover,
> >  .tab-icon-overlay[muted]:not([crashed]):hover {
> >    background-color: white;
> > +  border-radius: 10px;
> 
> If I do this:
> 
> http://jsbin.com/yijimoxidi/edit?html,css,output
> 
> I see flickering when hovering the corners that would be out of the
> 'hover'able area when the border radius is applied.
> 
> It's harder to reproduce on a pinned tab but I do see oddness about the
> hover region. Can we just keep the border-radius in the non-hover rule?

I can but the behavior you're seeing doesn't seem to make sense, because the background and border-radius changes should apply simultaneously, and would be a Gecko bug (which I don't think I see in your jsbin example.)

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #2)
> (In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #1)
> > patch nor the bug has any real description beyond "refactor styling for X"
> > which might as well be "change the code involving X", which isn't very
> > useful.
> 
> It was supposed to mean "clean up and simplify the code while maintaining
> the outcome." The outcome isn't actually the same, but should be roughly as
> good as it was or better (e.g. in the selected tab on Linux).
> 
> I didn't expand on how I simplified the code since I thought a look at the
> patch would make it obvious that I'm deleting most of the icon variants and
> use filter and fill instead.

I mostly noticed most of the icons were gone and you were unifying soundplaying and muting rules, and then also touched unrelated things like the crashed icon, so I was confused. IMHO, it would be more precise to title the patch/bug/description with something along the lines of "Use filter and fill properties instead of duplicating lots of SVG and CSS for the tab audio icon".

Comment 7

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #4)
> > @@ +156,5 @@
> > >  
> > >  .tab-icon-overlay[soundplaying]:hover,
> > >  .tab-icon-overlay[muted]:not([crashed]):hover {
> > >    background-color: white;
> > > +  border-radius: 10px;
> > 
> > If I do this:
> > 
> > http://jsbin.com/yijimoxidi/edit?html,css,output
> > 
> > I see flickering when hovering the corners that would be out of the
> > 'hover'able area when the border radius is applied.
> > 
> > It's harder to reproduce on a pinned tab but I do see oddness about the
> > hover region. Can we just keep the border-radius in the non-hover rule?
> 
> I can but the behavior you're seeing doesn't seem to make sense, because the
> background and border-radius changes should apply simultaneously, and would
> be a Gecko bug (which I don't think I see in your jsbin example.)

Clarified this on IRC.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch v2

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #4)
> Comment on attachment 8781970 [details] [diff] [review]
> patch
> 
> Review of attachment 8781970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for regressing the outline/shadow on the icon.
> 
> ::: browser/themes/shared/tabs.inc.css
> @@ +145,5 @@
> >  
> >  .tab-icon-overlay[soundplaying],
> >  .tab-icon-overlay[muted]:not([crashed]) {
> > +  list-style-image: url(chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio-small);
> > +  filter: url(chrome://browser/skin/filters.svg#fill) drop-shadow(1px 1px 1px white);
> 
> This doesn't work as well as the actual white outline that's in the SVG:
> 
> http://imgur.com/a/iuwY3
> 
> (top is current beta, bottom is nightly with your patch)

Replicating this with drop-shadow seems tricky since the spread radius parameter isn't supported. I removed this from the patch for now.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8782021 [details] [diff] [review]
patch v2

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

Closer, but I noticed some issues on devedition and would like to know more about the tab-icon-overlay selector changes.

::: browser/themes/shared/tabs.inc.css
@@ +161,5 @@
>    list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio-small.svg#tab-audio-muted");
>  }
>  
> +.tab-icon-overlay[soundplaying]:-moz-lwtheme-brighttext:not(:hover),
> +#TabsToolbar[brighttext] .tab-icon-overlay[soundplaying]:not([selected]):not(:hover) {

I confess I don't quite follow why these changes are necessary. Can you clarify?

@@ +209,5 @@
>  }
>  
> +.tab-icon-sound:-moz-lwtheme-darktext[soundplaying],
> +.tab-icon-sound:-moz-lwtheme-darktext[muted] {
> +  filter: url(chrome://browser/skin/filters.svg#fill) drop-shadow(1px 1px 1px white);

This looks bad on the selected tab on light devedition, where the background is blue, the icon is white, and now the dropshadow is also white (as your patch is replacing the dark stroke with the white dropshadow). It also looks bad on both selected and unselected tabs on dark devedition. Seems like maybe we could put the color in a CSS variable and set it for dark/light lwthemes and then override it for light/dark deveditions? That or just write several extra rules for devedition (both light and dark devedition match -moz-lwtheme-darktext because they're the same lwtheme).

Comment 10

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #9)
> ::: browser/themes/shared/tabs.inc.css
> @@ +161,5 @@
> >    list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio-small.svg#tab-audio-muted");
> >  }
> >  
> > +.tab-icon-overlay[soundplaying]:-moz-lwtheme-brighttext:not(:hover),
> > +#TabsToolbar[brighttext] .tab-icon-overlay[soundplaying]:not([selected]):not(:hover) {
> 
> I confess I don't quite follow why these changes are necessary. Can you
> clarify?

The selected tab is opaque and connects with the nav bar, so #TabsToolbar[brighttext] isn't a useful hint in that context.

> (both light and dark devedition match
> -moz-lwtheme-darktext because they're the same lwtheme).

*sigh* Someone should probably fix that.

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb97f65f521d
Use filter and fill properties instead of duplicating lots of SVG and CSS for the tab audio icon. r=gijs

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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