Update the tab audio icons

VERIFIED FIXED in Firefox 42

Status

()

VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
Firefox 43
Points:
---

Firefox Tracking Flags

(firefox42 verified, firefox43 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Blocks: 1188860
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1190192
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1190103
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1191437
(Assignee)

Comment 4

3 years ago
Created attachment 8645392 [details] [diff] [review]
Update the tab audio icons
Attachment #8645392 - Flags: review?(jaws)
(Assignee)

Comment 5

3 years ago
Created attachment 8645397 [details] [diff] [review]
Part 2: Use the new tab audio icon in the all-tabs menu
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-
(Assignee)

Comment 8

3 years ago
(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.
(Assignee)

Comment 9

3 years ago
Created attachment 8645442 [details] [diff] [review]
Update the tab audio icons
Attachment #8645392 - Attachment is obsolete: true
Attachment #8645397 - Attachment is obsolete: true
Attachment #8645442 - Flags: review?(jaws)
(Assignee)

Comment 10

3 years ago
(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+
(Assignee)

Comment 13

3 years ago
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
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
status-firefox42: --- → affected
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+

Comment 18

3 years ago
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
status-firefox42: fixed → verified
status-firefox43: fixed → verified
You need to log in before you can comment on or make changes to this bug.