1192568 - Update the tab audio icons

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Comment 4

[(no longer active)]

Attachment: Update the tab audio icons

Comment 5

[(no longer active)]

Attachment: Part 2: Use the new tab audio icon in the all-tabs menu

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8645392 [details] [diff] [review]
Update the tab audio icons

> .tab-icon-sound[soundplaying] {
>-  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio");
>+  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio.svg");
>+  -moz-image-region: rect(0, 112px, 16px, 96px);
> }

Please don't make these changes. IDs are more human-friendly and easier to maintain than image regions, and we use them in all our SVGs containing multiple images. The flash (bug 1191437) wasn't considered a blocker for those, so I don't think it should be a blocker here. We need to fix the underlying bug.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8645397 [details] [diff] [review]
Part 2: Use the new tab audio icon in the all-tabs menu

>+#alltabs-popup > menuitem[endimage] > .menu-iconic-right > .menu-iconic-icon {
>+  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio.svg");

This doesn't make much sense semantically. You should set an attribute indicating that sound is playing rather than relying on your code being the only place setting the endimage attribute. If nothing else, extensions might want to set it too.

nit: you can drop " > .menu-iconic-icon" from the selector

Comment 8

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 8645392 [details] [diff] [review]
> Update the tab audio icons
> 
> > .tab-icon-sound[soundplaying] {
> >-  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio");
> >+  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio.svg");
> >+  -moz-image-region: rect(0, 112px, 16px, 96px);
> > }
> 
> Please don't make these changes. IDs are more human-friendly and easier to
> maintain than image regions, and we use them in all our SVGs containing
> multiple images. The flash (bug 1191437) wasn't considered a blocker for
> those, so I don't think it should be a blocker here. We need to fix the
> underlying bug.

OK, although this basically means shipping 42 with this bug since the underlying bug will not be fixed in the one day we have left before the uplift, and I would personally not let the perfect be the enemy of the good here, but no time for arguing!  Luckily I have the old changes in my reflog.

Comment 9

[(no longer active)]

Attachment: Update the tab audio icons

Comment 10

[(no longer active)]

(The second part of the patch isn't needed using IDs since they remain the same.)

Comment 11

[Tim Nguyen :ntim]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #8)
> OK, although this basically means shipping 42 with this bug since the
> underlying bug will not be fixed in the one day we have left before the
> uplift, and I would personally not let the perfect be the enemy of the good
> here, but no time for arguing!  Luckily I have the old changes in my reflog.

Considering that we already ship other parts of the UI that already have this bug (iconic context menu, svg in Firefox DevTools, ...), we should simply wait for platform work to be done instead of introducing such hacks. Also, this bug only appears the first time the user clicks the audio icon, which makes it even lower priority.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8645442 [details] [diff] [review]
Update the tab audio icons

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

::: browser/themes/shared/tabbrowser/tab-audio.svg
@@ +11,2 @@
>      .icon {
> +      fill: #333333;

nit, fill: #333;

Comment 13

[(no longer active)]

Comment on attachment 8645442 [details] [diff] [review]
Update the tab audio icons

I'm requesting Aurora approval for 42 for this since this will definitely miss the uplift.

Approval Request Comment
[Feature/regressing bug #]: This is required for shipping tab audio indicators.
[User impact if declined]: This fixes bug 1190103 and bug 1190192.
[Describe test coverage new/current, TreeHerder]: Locally.
[Risks and why]: Low risk, purely visual change.
[String/UUID change made/needed]: none.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3df76ef5e44

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8645442 [details] [diff] [review]
Update the tab audio icons

Polishing of a new feature, taking it.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1afd8bcd692

Comment 18

[Rezaul Huque Nayeem]

Found this issue on Firefox nightly 42.0a1 (2015-08-08) on windows 10 x64

Verified as fixed with 

Latest Firefox Aurora 43.0a2 (Build ID: 20151009004029)
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0

Latest Firefox Beta 42.0b5 (Build ID: 20151008162217)
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0

Comment 19

[Nazir Ahmed Sabbir [:NaSb]]

Seen this issue on Nightly 42.0a1 (2015-08-08) in Linux,64 bit

This Bug is now verified as fixed on 

Latest Developer Edition 43.0a2 (2015-10-09) (Build ID: 20151009004029)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0 and

Latest Firefox Beta 42.0b5 (Build ID: 20151008162217)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0

As it is also verified on Windows (Comment 18), Marking it as verified!