Update the tab audio icons

VERIFIED FIXED in Firefox 42

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
Firefox 43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 verified, firefox43 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.
Duplicate of this bug: 1190192
Duplicate of this bug: 1190103
Duplicate of this bug: 1191437
Posted patch Update the tab audio icons (obsolete) — Splinter Review
Attachment #8645392 - Flags: review?(jaws)
Attachment #8645397 - Flags: review?(jaws)
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.
Attachment #8645392 - Flags: review?(jaws)
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
Attachment #8645397 - Flags: review?(jaws) → review-
(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.
Attachment #8645392 - Attachment is obsolete: true
Attachment #8645397 - Attachment is obsolete: true
Attachment #8645442 - Flags: review?(jaws)
(The second part of the patch isn't needed using IDs since they remain the same.)
(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 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;
Attachment #8645442 - Flags: review?(jaws) → review+
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.
Attachment #8645442 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/e3df76ef5e44
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment on attachment 8645442 [details] [diff] [review]
Update the tab audio icons

Polishing of a new feature, taking it.
Attachment #8645442 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
QA Whiteboard: [bugday-20151007]
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!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.