1268498 - [Linux] [Developer Edition] The tab visual sound indicator is not visible enough using the light developer edition theme

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Andrei Vaida [:avaida]]

Attachment: screenshot

[Affected versions]:
- 46.0a2
- 47.0a2
- 48.0a2
- 49.0a1

[Affected platforms]:
- Ubuntu 16.04 LTS (x64)
- Ubuntu 12.04 LTS (x86)

[Steps to reproduce]:
1. Launch Firefox Developer Edition.
2. Enable the light Developer Edition theme variant by toggling developer tools → accessing Toolbox Options → selecting "Light" from the Themes section.
3. Open a new tab and access a random video, e.g. https://www.youtube.com/watch?v=-2m6JkJvv4w
4. Open a new tab.
5. Check the way the tab visual sound indicator is displayed in the background tab, that's currently playing back the YouTube video.

[Expected result]:
- The tab visual sound indicator is clearly visible, even for background tabs.

[Actual result]:
- The sound indicator is almost invisible on a background tab, while the light Developer Edition theme variant is enabled.

[Regression range]:
- This bug is _not_ a regression, as it's reproducible since the feature was first implemented.

[Additional notes]:
- This is _not_ reproducible on Mac OS X or Windows.

Comment 1

[Karl Tomlinson (:karlt)]

I guess this is a problem with the icon.

Comment 2

[:Gijs (he/him)]

Rakhi, maybe you could look into this?

Comment 3

[Rakhi(:rakhisharma)]

Yes, I will be looking at it thanks :)

Comment 4

[Rakhi(:rakhisharma)]

Attachment: MozReview Request: Bug 1268498 - [Linux] [Developer Edition] The tab visual sound indicator is not visible enough using the light developer edition theme. r=Gijs.

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

Comment 5

[:Gijs (he/him)]

Comment on attachment 8753781 [details]
MozReview Request: Bug 1268498 - [Linux] [Developer Edition] The tab visual sound indicator is not visible enough using the light developer edition theme. r=Gijs.

https://reviewboard.mozilla.org/r/53482/#review50256

::: browser/themes/shared/tabs.inc.css:156
(Diff revision 1)
>    height: 16px;
>    padding: 0;
>  }
>  
>  .tab-icon-sound[soundplaying] {
> -  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio-backgroundTab");
> +  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio");

Unfortunately these changes are not quite right.

The 'backgroundTab' icon is coloured with the fill: rule here: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/tabbrowser/tab-audio.svg#49-53

which is correct in general (certainly for Linux).

It breaks down with the devedition theme because the devedition theme forces the use of other colours. The same will happen with other (brightly- rather than darkly-colored) lightweight themes.

Unfortunately, we can't select for lightweight theme status inside SVG files.

So instead, I think the 6 selectors below the ones you changed (the visuallyselected=true ones) should be updated to add:

```
.tab-icon-sound:-moz-lwtheme,
...

.tab-icon-sound:hover:-moz-lwtheme,
...
```

etc.

Then the devedition light theme will need to be updated to use the white image for the selected tab (so copy these 6 selectors, but the list-style-image properties from the corresponding `#TabsToolbar[brighttext]` rules, and put them in the devedition stylesheet).

Does that make sense?

Comment 6

[Rakhi(:rakhisharma)]

Attachment: MozReview Request: Bug 1268498 - [Linux] [Developer Edition] The tab visual sound indicator is not visible enough using the light developer edition theme . r= Gijs.

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

Comment 7

[Rakhi(:rakhisharma)]

Comment on attachment 8754820 [details]
MozReview Request: Bug 1268498 - [Linux] [Developer Edition] The tab visual sound indicator is not visible enough using the light developer edition theme . r= Gijs.

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

Comment 8

[Rakhi(:rakhisharma)]

Comment on attachment 8754820 [details]
MozReview Request: Bug 1268498 - [Linux] [Developer Edition] The tab visual sound indicator is not visible enough using the light developer edition theme . r= Gijs.

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

Comment 9

[Rakhi(:rakhisharma)]

Comment on attachment 8754820 [details]
MozReview Request: Bug 1268498 - [Linux] [Developer Edition] The tab visual sound indicator is not visible enough using the light developer edition theme . r= Gijs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54262/diff/3-4/

Comment 10

[:Gijs (he/him)]

Comment on attachment 8754820 [details]
MozReview Request: Bug 1268498 - [Linux] [Developer Edition] The tab visual sound indicator is not visible enough using the light developer edition theme . r= Gijs.

https://reviewboard.mozilla.org/r/54262/#review51192

::: browser/themes/shared/devedition.inc.css:309
(Diff revision 4)
> +  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio-white-muted");
> +}
> +
> +.tab-icon-sound[visuallyselected=true][muted]:hover {
> +  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio-white-muted-hover");
> +}
> +
> +.tab-icon-sound[visuallyselected=true][muted]:hover:active {
> +  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio-white-muted-pressed");

These image references don't exist. You want:

tab-audio-muted-white

tab-audio-muted-white-hover

tab-audio-muted-white-pressed

instead (note the order of "muted" and "white").

With that, r=me.

Comment 11

[Rakhi(:rakhisharma)]

Comment on attachment 8754820 [details]
MozReview Request: Bug 1268498 - [Linux] [Developer Edition] The tab visual sound indicator is not visible enough using the light developer edition theme . r= Gijs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54262/diff/4-5/

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/023976c4567d

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/023976c4567d