Provide visual indicator as to which tab is causing sound

VERIFIED FIXED in Firefox 42

Status

()

Firefox
Tabbed Browser
--
enhancement
VERIFIED FIXED
9 years ago
13 days ago

People

(Reporter: L. Mohan Arun, Assigned: Ehsan)

Tracking

(Depends on: 8 bugs, Blocks: 6 bugs, {addon-compat})

Trunk
Firefox 42
addon-compat
Points:
---

Firefox Tracking Flags

(firefox42 verified)

Details

(Whiteboard: [parity-Chrome], URL)

Attachments

(7 attachments, 23 obsolete attachments)

404.12 KB, image/png
Details
2.21 KB, image/svg+xml
Details
2.14 KB, image/svg+xml
Details
6.16 KB, patch
dao
: review+
Details | Diff | Splinter Review
20.88 KB, patch
jaws
: review+
dao
: review+
Details | Diff | Splinter Review
9.76 KB, patch
dao
: review+
Details | Diff | Splinter Review
7.76 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3

When multiple tabs are getting opened in short notice, it is difficult to determine which website in which tab is causing sound or music to play automatically.

Reproducible: Always

Steps to Reproduce:
1. Example, when you have some tabs open go to site http://cadiesingularity.blogspot.com/
2. It is not clear quickly that this tab is causing the sound



Expected Results:  
Provide visual indication in tab title that this website is causing sound, so it can be quickly muted or attended to
We only know if a plugin is used but we don't know if this plugin plays sound or not.
We know if a resource with audio/* mime-type is loaded but that is incomplete.
We are unable to detect if the flash plugin plays sound as example because the sound is embedded into the flash file. We also don't know if a video/* file contains sound or not.
(Reporter)

Comment 2

8 years ago
So it cant be done by the browser then? I would like a way to get this done so that no music plays unless the particular tab has focus, and an option in the browser to stop all sounds emanating from a particular tab in one kill operation, regardless of its source.

Comment 3

7 years ago
see bug 334987

Updated

7 years ago
Duplicate of this bug: 562323

Comment 5

7 years ago
Can we add options like turning off all sound except the foreground tab?

Comment 6

7 years ago
see bug 334987 (as told in comment 3)

Updated

7 years ago
Version: unspecified → Trunk
This is something I'd love to see us do for html5 audio and video elements, even though the flash/plugin case isn't possible per tab.  In theory this is doable at the level of the media element and decoder with some work.  I'm not sure about getting the UI done.

The simplest case would be mute/unmute a document, where the document reveals the collective state of its child media elements.

Moving to Core : Video/Audio to get this in front of more people.  We might have other bugs for this that are further along.  I couldn't find any, which isn't the same as saying they don't exist.

Also CC'ing Blair, who I've talked to about this with in the past.
Status: UNCONFIRMED → NEW
Component: General → Video/Audio
Ever confirmed: true
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → video.audio
Hardware: x86 → All
Sounds similar to bug 626586.
Duplicate of this bug: 626586
Earlier firefox used to have this feature , like if my gmail tab gets a new ping, it used to highligh the tab in orange and blink the colour

Comment 11

6 years ago
No, that was probably an add-on.
no it was not .
Same problem as I noted in bug 626586 -- users don't care _how_ sounds are generated, so we shouldn't do this if the result is seemingly-inconsistent behavior (due to plugins playing sounds). I certainly like the idea, though... I just don't know how this could be implemented in a way that makes it natural to only get the UI for pages using HTML5 media. :(

Comment 14

6 years ago
This bug should be WONTFIXed in favor of fixing the issue thay causes this, background tabs playing automatically.

Comment 15

6 years ago
(In reply to comment #13)
> Same problem as I noted in bug 626586 -- users don't care _how_ sounds are
> generated, so we shouldn't do this if the result is seemingly-inconsistent
> behavior (due to plugins playing sounds). I certainly like the idea, though...
> I just don't know how this could be implemented in a way that makes it natural
> to only get the UI for pages using HTML5 media. :(

How about making this a user setting then, disabled by default? It can’t possibly be made to work for plugins, but surely implementing this for <video> and <audio> is better than nothing.
We already have a pref to turn off autoplay, so that's not what this is about.  I very much agree with comment 15, but also understand where Justin is coming from in comment 13.  What's the process for resolving this?

Comment 17

6 years ago
I want to allow my background tabs to play sound but I want to be able to visually identify which one is generating it.

Comment 18

6 years ago
We just can't fix this because, today, most of the audio is from Flash.

Here is 2 different ideas:

1. What if we provide an API (or events) to the content:
  window.ImGeneratingSound();
  window.ImNotGeneratingSoundAnyMore();

With some evangelism, it could work.
If Video providers (youtube & co) and Music providers (Pandora & co) support it, and if we provide a little ActionScript code that binds these to Flash, that would solve this problem.

2. We talk to Adobe and ask them to fire an event from the Flash plugin when some audio is been played.

(Also, we could also send events when the user use the multimedia keys (play, pause, next song, previous song, ...) even if the document and Firefox are not focused. But this is another story)
But there are lot of other plugins wich can generate sound (eg. Silverlight, WMP, Quicktime, Realplayer, Java, etc), and lot of other webpages... It won't work properly until every single plugin does not notify the host browser about the sound.
> 1. What if we provide an API (or events) to the content:
>   window.ImGeneratingSound();
>   window.ImNotGeneratingSoundAnyMore();

You could build something like this today if you worked with the media element's muted state and listened for MozAudioAvailable events.  That won't guarantee that there is actually any sound (you could have silence), but it's a good indicator that we are playing sound.  So we could hack together an experiment pretty easily.

Comment 21

6 years ago
(In reply to comment #19)
> But there are lot of other plugins wich can generate sound (eg. Silverlight,
> WMP, Quicktime, Realplayer, Java, etc), and lot of other webpages... It won't
> work properly until every single plugin does not notify the host browser about
> the sound.

afaik, the large majority of the Audio content on the web is Flash, and hopefully soon, HTML5 Audio. We don't have to cover 100% of the different scenarios, and I guess, Flash + HTML5 cover 99%.
I think I made the discussion hot again , but all I wanted was to get botified about which tab is getting an alert , like a gmail ping , or a message , or javascript alert , now that alert() function is tab dependent.

Comment 23

6 years ago
Since this can't be done automatically (we can't know if Flash content will
play some audio), I think we should let the Web Developer call himself such
functions.

Implementing an automatic notification from the <audio> tag is not really
useful. We should just define and implement an API and let people know how to
use it.

Does it make sense?
(In reply to comment #21)
> (In reply to comment #19)
> > But there are lot of other plugins wich can generate sound (eg. Silverlight,
> > WMP, Quicktime, Realplayer, Java, etc), and lot of other webpages... It won't
> > work properly until every single plugin does not notify the host browser about
> > the sound.
> 
> afaik, the large majority of the Audio content on the web is Flash, and
> hopefully soon, HTML5 Audio. We don't have to cover 100% of the different
> scenarios, and I guess, Flash + HTML5 cover 99%.


You are right, i guess only Flash covers 99%. :I But this feature would confuse users - "there is a sound icon most of the time when audio plays, but sometimes there is not, why?". But i agree, only Flash and HTML5 coverage would be a great win for users.
(In reply to comment #19)
> But there are lot of other plugins wich can generate sound (eg. Silverlight,
> WMP, Quicktime, Realplayer, Java, etc), and lot of other webpages... It won't
> work properly until every single plugin does not notify the host browser about
> the sound.

We've got bug 593897 on file to hook all sound APIs used by plugins so we can mute all plugins, if we did that, we'd be able to detect when sound is played by a plugin using the hooked APIs.

Updated

6 years ago
Duplicate of this bug: 696600

Updated

6 years ago
Depends on: 728046
Depends on: 593897

Updated

5 years ago
Blocks: 24418
Looks like Chrome is working on doing this for html5 media, as I discussed in comment 7, see http://thenextweb.com/google/2013/02/25/google-chrome-may-soon-get-audio-indicators-to-show-you-noisy-tabs-keep-them-open-when-memory-runs-out/

Comment 28

5 years ago
(In reply to David Humphrey (:humph) from comment #27)
> Looks like Chrome is working on doing this for html5 media

...and Flash as well as other Pepper-based plugins.
(In reply to Darin Fisher from comment #28)
> (In reply to David Humphrey (:humph) from comment #27)
> > Looks like Chrome is working on doing this for html5 media
> 
> ...and Flash as well as other Pepper-based plugins.

Hi Darin -- any other Pepper-based plugins of note?

/be

Comment 30

4 years ago
Haven't seen this suggested yet:
   Tools->Page Info->Sound (or as part of Media)
displaying a list of tabs currently containing the possibility of generating sound and a "switch to tab" link.

Usually, the sound is familiar to the user, so [s]he would have a real good chance of finding the sound generating tab quickly.  This would be a reasonable solution (one I'd be happy with) until a better one is developed...
Duplicate of this bug: 900660

Updated

4 years ago
Duplicate of this bug: 675626

Comment 33

4 years ago
It's now on Google Chrome Beta. http://chrome.blogspot.ca/2013/11/track-down-those-noisy-tabs.html
Whiteboard: [parity-Chrome]

Comment 34

4 years ago
Can I ask where are we at on this?

Chrome will now have this out of the box (without a need for site developers to do anything).

Am I right that we could implement this now if we only had to manage HTML audio & video (and not plugins)?

Comment 18 suggests talking with Adobe to just have Flash notify us of playing audio.  We're not going to implement Pepper, so this would seem a prudent step.  Has anyone contacted Adobe?

On the subject of *other* plugins, are we able to detect when a plugin is loaded into a particular tab?  If so that could be how we round up stragglers in a potential audio offenders window.

Finally, not to distract, but if Shumway should actually save the day, perhaps this will all become a memory.
Bug 923247 gave us notifications that we can use to solve this for the non-plugin case.
Depends on: 923247
No longer depends on: 593897, 728046
Depends on: 728046
No longer depends on: 728046
http://imgur.com/zxCt0Ol

Comment 37

4 years ago
This has landed in Chrome stable:

http://chrome.blogspot.com/2014/01/everyone-can-now-track-down-noisy-tabs.html

Comment 38

4 years ago
>On the subject of *other* plugins, are we able to detect when a plugin is loaded into a particular tab?  If so that could be how we round up stragglers in a potential audio offenders window.

This is essentially the route that I took with the MuteTab Chrome extension.  http://www.mutetab.com/.  If someone wants to play with this at the extension level, take a look at the source code (http://www.github.com/jaredsohn/mutetab/.)  I am now building it for Chrome with trigger.io's OpenForge so with some work (start by grepping the source for 'chrome') it could be ported in a cross platform way to also work for Firefox.
Depends on: 961202

Comment 39

4 years ago
As far as I can gather from this bug, currently the only way to properly detect playing audio (for most cases) is for Adobe to work with us allowing Flash to communicate its play state.

Assuming we've approached them and they will not do this in the near future, why not create an API like this for HTML audio/video as well as indicate which tabs have plugins present?
This would allow addon authors to create a solution that is better than nothing.

Bonus addon name suggestion: 'Zero In' (or with excessive subtlety: 0N)

Updated

4 years ago
Depends on: 967627
Comment hidden (me-too)
I don't think that's likely to be useful. Try running with click-to-play plugins enabled, and you'll find a surprisingly large number of pages use Flash in non-obvious ways.

Comment 42

3 years ago
(In reply to Caspy7 from comment #39)
> As far as I can gather from this bug, currently the only way to properly
> detect playing audio (for most cases) is for Adobe to work with us allowing
> Flash to communicate its play state.
> 
> Assuming we've approached them and they will not do this in the near future,
> why not create an API like this for HTML audio/video as well as indicate
> which tabs have plugins present?
> This would allow addon authors to create a solution that is better than
> nothing.
> 
> Bonus addon name suggestion: 'Zero In' (or with excessive subtlety: 0N)

Chrome does this in some way. Which means it's possible without any kind of Adobe API.
Chrome ships with a customized version of Flash (using Pepper), which added the requisite support for per-tab indicators.

Comment 44

3 years ago
So, what are the solutions?:
1° We don't do anything, after all, we don't care about witch tab is causing sound: http://www.commitstrip.com/en/2013/11/12/la-feature-que-lon-attend-depuis-15-ans/
2° Like google, we developp our flash plugin, knowing that: many video use it to play DRM video, witch is not possible with DRMs, that it will take time and won't be perfect like our js pdf viewer
3° We don't care about flash and provide indicator for HTML5 <video>/</audio> sound (and even maybe high CPU/memory usage that slow down the computer). For flash, if user have "Click to play" enabled, we can show a "plugin" icon in tab.
Comment hidden (off-topic)

Comment 46

3 years ago
(In reply to Caspy7 from comment #39)

> As far as I can gather from this bug, currently the only way to properly
> detect playing audio (for most cases) is for Adobe to work with us allowing
> Flash to communicate its play state.
> 
> Assuming we've approached them and they will not do this in the near future,
> why not create an API like this for HTML audio/video as well as indicate
> which tabs have plugins present?
> This would allow addon authors to create a solution that is better than
> nothing.
> 
> Bonus addon name suggestion: 'Zero In' (or with excessive subtlety: 0N)

Rather "Zero Out" would make sense, no ?

Comment 47

3 years ago
Is there any reason my suggestion in comment 30 can't be an interim (or final) solution in the absence of Adobe cooperation?  Is Flash the only source of audio?

Comment 48

3 years ago
(In reply to Justin Dolske [:Dolske] from comment #43)
> Chrome ships with a customized version of Flash (using Pepper), which added
> the requisite support for per-tab indicators.

What about Shumway ?
Comment hidden (me-too)

Updated

3 years ago
Duplicate of this bug: 1031628
Comment hidden (advocacy)
There is an indicator in Nightly builds! Yeah! Is it coming from this bug? E.g. The 'play' icon in the tab shows when playing this tune: https://soundcloud.com/hugo-kant/dr-van-helsing

Comment 53

3 years ago
No, it's like the Play-icon in Youtube-tabs.
By way of a bump, I'd just add that in my opinion supporting this for HTML5 audio/video is what we should do. Flash is on the way out due to mobile, so throwing support behind this, providing an API for webdevs, and informing Adobe that it'd be great if they'd call said API, would be a big win. I'd think that it might even be a slight nudge to some to use HTML5 solutions instead of flash, though it could of course be a nudge in the other direction for a certain class of netizen as well.
Comment hidden (me-too)
Comment hidden (me-too)

Comment 57

3 years ago
I'm looking for this feature too and added thread on Flash Player forum https://forums.adobe.com/message/7049842#7049842

Maybe if people will concern about it Adobe wil add sound indicator feature (API, tag playsound=TRUE/FALSE) and it could be implmented in Firefox.
On the platform side, we're aggressively pursuing a number of moves to use native audio/video for things currently served with flash (MSE, shumway, etc). Would the visual indicator be hard to implement? If not, it seems like we should put it in the UI, so that we'll automatically benefit from it for every piece of content we get running natively.

A friend in China has told me on several occasions that the chinese web is very audio-heavy, and that this is the reason he uses Chrome. :-(
Flags: needinfo?(dolske)
Bug 923247 apparently implemented the backed to make the UI possible, so someone could prototype this today as an addon. I don't think the UI should be that difficult (as least so far as a simple indicator to expose what 923247 provides).

The problem is what I noted in comment 13. So long as Flash remains prevalent (and dare I say more widely used, especially for annoying things), an indicator that's only for HTML5 won't help a lot of the time, and leads to users seeing inconsistent UI. A UI that only works some of the time can feel pretty broken and frustrating, and actually be worse than no UI at all.
Flags: needinfo?(dolske)
(In reply to Justin Dolske [:Dolske] from comment #59)
> Bug 923247 apparently implemented the backed to make the UI possible, so
> someone could prototype this today as an addon. I don't think the UI should
> be that difficult (as least so far as a simple indicator to expose what
> 923247 provides).

I was suggesting that it might be worth for Firefox team doing, especially if it's easy.

> The problem is what I noted in comment 13. So long as Flash remains
> prevalent (and dare I say more widely used, especially for annoying things),
> an indicator that's only for HTML5 won't help a lot of the time,

The idea is that a lot of this stuff may be moving to Shumway soon.

> and leads
> to users seeing inconsistent UI. A UI that only works some of the time can
> feel pretty broken and frustrating, and actually be worse than no UI at all.

This is a fair argument.
I think implementing it for what we can is an excellent start. Even without flash support it's still beneficial.

> A UI that only works some of the time can feel pretty broken and frustrating, and actually be worse than no UI at all.

That's true but 1) flash usage is dropping day-by-day and this will become less relevant. We shouldn't wait until flash market share is 0% before doing this.

As a follow-up we could address Flash support later. We can do some tricks in the plugin process and watch the sound APIs that flash uses and report those. This is how we implemented plugin fullscreen on mac. It's not ideal but it could work if the API surface is small and simple enough. The windows flash sandboxing might make this tricky however.

Comment 62

3 years ago
With click-to-play enabled there are fewer tabs expected to have Flash running at all. To deal with Flash audio, a separate icon to indicate running Flash (or other plugins) could be applied to these tabs. It wouldn't specifically indicate audio, but it would be sufficient to help find the offending tab if there is an issue. (e.g. if you hear sound you don't want coming from a background tab, it's either the one we know has audio, or one of the ones you allowed Flash on) Alternatively, such additional icons could be show on tabs only when there's enough space and then only shown in the drop-down list of all tabs when the bar gets full.

Comment 63

3 years ago
(In reply to Dave Garrett from comment #62)
> With click-to-play enabled there are fewer tabs expected to have Flash
> running at all. To deal with Flash audio, a separate icon to indicate
> running Flash (or other plugins) could be applied to these tabs.

Reminder:  the discussion centers around TABS; but please don't forget that some of us run multiple windows with lots of tabs each; so an indicator as to which window(s) is/are generating sound should be part of the solution.  In my case, I always have 9 to 11+ FF windows1[1] open with anywhere from 1[2] to nearly 200[3] tabs each (507 tabs as I write this).

[1] Too many windows to raise/lower, so I use KDE's Window of windows grouping feature -- all these windows look like tabs in a single grouping window.
[2] some websites manage to open a new window with no tabs/toolbars; even with "Open new windows in a new tab instead" set -- although sound is rare on these, they could have sound without tabs.
[3] As I write this, 507 tabs over 11 windows -- so I'm VERY interested in this issue. 

On other desktops, I have: 
- a virtually duplicate FF (without add-ons) for debugging (no sound -- not enabled from other userids)
- Windows-FF/Wine on main userid which allows sound and separate .mozilla/* under .wine 

Being able to see which tabs _and_ windows are generating sound is what I'm looking for -- just being explicit rather than hoping it's implicit...  :)

Comment 64

3 years ago
It seems that many users are missing this feature. See discussion/replys on http://gizmodo.com/but-this-tho-1685659126
Depends on: 1018504

Comment 65

2 years ago
These might help a bit in the interim:

https://addons.mozilla.org/en-US/firefox/addon/noise-control/
https://addons.mozilla.org/en-US/firefox/addon/youtube-autoplay-stopper/

Updated

2 years ago
Duplicate of this bug: 845517

Comment 67

2 years ago
According to the release notes[1] for Flash 18, it now contains the necessary APIs to make this work with Flash.

Taken from [1]:
> Audio APIs added to Flash Player NPAPI
> We have added the ability for NPAPI compatible browsers and applications to query the player and
> detect if audio is currently being played and the ability to mute.  This new feature will allow
> applications to alert the user if audio is being played and give them the ability to mute, even if the
> Flash content does not.

[1] http://labsdownload.adobe.com/pub/labs/flashruntimes/shared/air18_flashplayer18_releasenotes.pdf

Comment 68

2 years ago
> According to the release notes[1] for Flash 18, it now contains the
> necessary APIs to make this work with Flash.

Wow, that is great news!  (And surprising!)
===
For those not clicking all links (I don't), the first one in comment 65 is to the Noise Control addon, which is essentially an addon that could mark this bug as fixed [if it were in the browser].

Noise Control places an audio indicator in tabs with audio-playing HTML5 video or audio (just like Chrome). Clicking the indicator mutes the audio.
Now that Youtube is sending HTML5 video to Firefox for Windows Vista and above, this covers a lot of users as well as influencing the rest of the Net to relegate Flash.
The addon also inserts a Flash/plugin indicator in tabs with plugins loaded.  

So we can consider this addon a proof of concept, if not the beginnings of the code that could go into the browser.
Now that Adobe has made the proper Flash API available this will cover the audio for most users!

If you'd like this functionality now (with HTML video) you can get the addon here:
https://addons.mozilla.org/en-US/firefox/addon/noise-control/

Updated

2 years ago
Duplicate of this bug: 1162651
"Audio APIs added to Flash Player NPAPI
We have added the ability for NPAPI compatible browsers and applications to query the player and detect if audio is currently being played and the ability to mute.  This new feature will allow applications to alert the user if audio is being played and give them the ability to mute, even if the
Flash content does not."

This is from the flashplayer 18 release notes. I'll do some experiment today to test this new Audio APIs.
Philipp, doslke, we can now implement this for flash audio as well as normal HTML audio. Would that be something we'd be interested in?

As mentioned earlier this has been prototyped using a jetpack addon (https://addons.mozilla.org/en-US/firefox/addon/noise-control/), which I found to be very reliable (in fact, perfectly reliable). It's built upon the Gecko API baku wrote somewhat recently, of course.

I was thinking it could be a fun thing to work on all together at Whistler.
Flags: needinfo?(philipp)
Flags: needinfo?(dolske)
Alright I did some quick poking here. There's no official way in NPAPI to get the audio out. The closest thing is this -draft-, which shouldn't be implemented ATM for binary compatibility:
https://wiki.mozilla.org/NPAPI:AudioControl

But it doesn't look like they implemented that directly. It might of been done via get/setvalue. I'm contacting Adobe and I'll see if we can get a proper proposal revived and officially extend NPAPI.
Just talked about this with Stephen, who might already have designs for this ready.
Flags: needinfo?(philipp)

Updated

2 years ago
Depends on: 1167690
(In reply to Benoit Girard (On Leave Until June 1st!) from comment #72)
> Alright I did some quick poking here. There's no official way in NPAPI to
> get the audio out. The closest thing is this -draft-, which shouldn't be
> implemented ATM for binary compatibility:
> https://wiki.mozilla.org/NPAPI:AudioControl
> 
> But it doesn't look like they implemented that directly. It might of been
> done via get/setvalue. I'm contacting Adobe and I'll see if we can get a
> proper proposal revived and officially extend NPAPI.

That's now moved to bug 1167690.
(In reply to Paul Adenot (:padenot) from comment #71)
> Philipp, doslke, we can now implement this for flash audio as well as normal
> HTML audio. Would that be something we'd be interested in?

Yes!

> I was thinking it could be a fun thing to work on all together at Whistler.

Fantastic idea.
Flags: needinfo?(dolske)
Depends on: 1180347
Depends on: 1180355
Depends on: 1180421, 1180423
Created attachment 8629602 [details]
UX spec by shorlander
Created attachment 8629603 [details] [diff] [review]
Provide a visual indicator for tabs that are playing audio (WIP)

Dao, can you please provide some feedback here?  I'm not implementing the muting or the context menu for now.  One problem that I ran into which I don't know how to solve is how to overlay the icon on top of the tab icon for pinned tabs.  I tried to use a trick similar to the one used for .tab-icon-overlay but that doesn't seem to work.  Also, obviously I'm using the wrong icons for now...  ;-)
Attachment #8629603 - Flags: feedback?(dao)
Created attachment 8629604 [details]
Screenshot of the current WIP patch
Depends on: 1180448
Created attachment 8629618 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon; r=dao
Created attachment 8629620 [details] [diff] [review]
Part 3: Implement a context menu for muting and unmuting a tab

Using a dummy icon for now...
Created attachment 8629621 [details]
Screenshot of the muting menu, and the muted icon and tooltip
One scenario should be clarified in the UX spec: When the user mutes a tab, what should happen when the audio playback stops?  Should the mute state (and icon) persist?  What about when we browse to a new page?  Should the mute state (and icon) persist?
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
Assignee: nobody → ehsan
David, what should be done here for accessibility?
Flags: needinfo?(dbolter)
Component: Video/Audio → Tabbed Browser
Product: Core → Firefox

Comment 84

2 years ago
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #82)
> What about when we browse to a new page?  Should the
> mute state (and icon) persist?

Just to chime in on this specific case: I think the state needs to persist, at least. YouTube, for example, can have multiple videos in a playlist on different pages, switching to the next page at the end of each. If a user directed Firefox to mute that tab, it should do so regardless of playlists or anything else the site does. You could argue that the state should persist per-domain, but per-tab-session makes sense too.

Comment 85

2 years ago
Just attempted to test this on my computer. Applied the patches from bug 1180347, bug 1180421, bug 1180448 and this bug then did a full build. The indicator didn't show up for me unfortunately (testcases : HTML5 youtube and http://nt1m.github.io/HTML5AudioPlayer/ ). e10s doesn't seem to matter. Is there a dependency I'm missing here ?
Flags: needinfo?(ehsan)
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #82)
> One scenario should be clarified in the UX spec: When the user mutes a tab,
> what should happen when the audio playback stops?  Should the mute state
> (and icon) persist?  What about when we browse to a new page?  Should the
> mute state (and icon) persist?

The tab should stay muted (otherwise, stuff like switching from one video to another might bypass the mute?)
There should probably be a distinct icon state for that case though...
Flags: needinfo?(philipp)
(In reply to Tim Nguyen [:ntim] from comment #85)
> Just attempted to test this on my computer. Applied the patches from bug
> 1180347, bug 1180421, bug 1180448 and this bug then did a full build. The
> indicator didn't show up for me unfortunately (testcases : HTML5 youtube and
> http://nt1m.github.io/HTML5AudioPlayer/ ). e10s doesn't seem to matter. Is
> there a dependency I'm missing here ?

You need to set the media.useAudioChannelService pref to true in order to test this with the patches attached to these bugs.  The UI bits should all work fine in e10s and non-e10s.

(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #86)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #82)
> > One scenario should be clarified in the UX spec: When the user mutes a tab,
> > what should happen when the audio playback stops?  Should the mute state
> > (and icon) persist?  What about when we browse to a new page?  Should the
> > mute state (and icon) persist?
> 
> The tab should stay muted (otherwise, stuff like switching from one video to
> another might bypass the mute?)
> There should probably be a distinct icon state for that case though...

Just to confirm my understanding, that means that the muted icon should continue to display in the tab, right clicking it should let you unmute the tab, and once the tab has been unmuted, the icon will go back to playing if the tab is playing audio, or disappear if it's not playing audio.  Is that correct?

FWIW, I agree that this is probably the more desirable behavior than what my patches implement right now (the muting will only work until the next document has been navigated to.)
Flags: needinfo?(ehsan) → needinfo?(philipp)
Depends on: 1113086

Comment 88

2 years ago
Created attachment 8629691 [details] [diff] [review]
Styling patch

This adds some styling love (and tries to match the UX spec too).
The icons are from the noun project, so we'll need to replace them once UX provides some.

Comment 89

2 years ago
Created attachment 8629692 [details] [diff] [review]
Styling patch
Attachment #8629691 - Attachment is obsolete: true

Comment 90

2 years ago
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #87)
> (In reply to Tim Nguyen [:ntim] from comment #85)
> > Just attempted to test this on my computer. Applied the patches from bug
> > 1180347, bug 1180421, bug 1180448 and this bug then did a full build. The
> > indicator didn't show up for me unfortunately (testcases : HTML5 youtube and
> > http://nt1m.github.io/HTML5AudioPlayer/ ). e10s doesn't seem to matter. Is
> > there a dependency I'm missing here ?
> 
> You need to set the media.useAudioChannelService pref to true in order to
> test this with the patches attached to these bugs.  The UI bits should all
> work fine in e10s and non-e10s.
Thanks ! worked great for me.
Here's some feedback :
- The context menu with mute/unmute should probably appear when left clicking the tab audio icon
- That context menu item could also be added to the normal tab context menu

Comment 91

2 years ago
Comment on attachment 8629692 [details] [diff] [review]
Styling patch

This still needs some love for [brighttext] themes (devedition dark theme, and dark lw-themes) and Windows High contrast themes. But the overall styling is there.
Depends on: 1180523
Depends on: 1180535
Depends on: 1180539
(In reply to Tim Nguyen [:ntim] from comment #90)
> Thanks ! worked great for me.

Happy to hear that!

> Here's some feedback :
> - The context menu with mute/unmute should probably appear when left
> clicking the tab audio icon
> - That context menu item could also be added to the normal tab context menu

Both of these suggestions go against the current UX spec...  Whatever the UX team decides is fine by me.
Comment on attachment 8629692 [details] [diff] [review]
Styling patch

Thanks!  I will probably steal some of your CSS here, but I'll wait for Stephen to provide the actual icons...
Created attachment 8629742 [details] [diff] [review]
Part 3: Implement a context menu for muting and unmuting a tab

The previous patch had a bug where no matter which icon you clicked on, the current tab would be muted/unmuted.  This version of the patch fixes this.

With this, and the rest of the patches in the blocking bugs, everything seems to be working well according to my local tests.  I'll still need to get some feedback on my front-end changes, and also write tests for the front-end bits.  I'll focus on the tests when I have received the initial code feedback.
Attachment #8629620 - Attachment is obsolete: true
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #83)
> David, what should be done here for accessibility?

Thanks for the ping. We'll need to ensure visibility as per comment 91. We'll want keyboard-only users to be able to operate the functionality. MarcoZ can assess for screen reader usage.
Flags: needinfo?(dbolter) → needinfo?(mzehe)
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #87)
> (In reply to Philipp Sackl [:phlsa] please use needinfo from comment #86)
> > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> > needinfo? me!) from comment #82)
> > > One scenario should be clarified in the UX spec: When the user mutes a tab,
> > > what should happen when the audio playback stops?  Should the mute state
> > > (and icon) persist?  What about when we browse to a new page?  Should the
> > > mute state (and icon) persist?
> > 
> > The tab should stay muted (otherwise, stuff like switching from one video to
> > another might bypass the mute?)
> > There should probably be a distinct icon state for that case though...
> 
> Just to confirm my understanding, that means that the muted icon should
> continue to display in the tab, right clicking it should let you unmute the
> tab, and once the tab has been unmuted, the icon will go back to playing if
> the tab is playing audio, or disappear if it's not playing audio.  Is that
> correct?
> 
> FWIW, I agree that this is probably the more desirable behavior than what my
> patches implement right now (the muting will only work until the next
> document has been navigated to.)

Yep! Exactly!
Flags: needinfo?(philipp)
Dao: ping?
Depends on: 1182164
Comment on attachment 8629603 [details] [diff] [review]
Provide a visual indicator for tabs that are playing audio (WIP)

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #77)
> Created attachment 8629603 [details] [diff] [review]
> Provide a visual indicator for tabs that are playing audio (WIP)
> 
> Dao, can you please provide some feedback here?  I'm not implementing the
> muting or the context menu for now.  One problem that I ran into which I
> don't know how to solve is how to overlay the icon on top of the tab icon
> for pinned tabs.  I tried to use a trick similar to the one used for
> .tab-icon-overlay but that doesn't seem to work.

If you want to follow tab-icon-overlay (which I think you should), you'll actually need to reuse that element, i.e. add another state to that along with "crashed".

If you want this to be spawn a menu, you'll also need to think about keyboard access.

>+      <handler event="DOMMediaPlaybackStarted">

This will need some e10s love.
Attachment #8629603 - Flags: feedback?(dao) → feedback-
(In reply to Dão Gottwald [:dao] from comment #98)
> 
> >+      <handler event="DOMMediaPlaybackStarted">
> 
> This will need some e10s love.

I reviewed the dependent patches - we use messages to tell the parent's browser bindings to fire those events.

So I think we're good here, e10s-wise.
(In reply to Dão Gottwald [:dao] from comment #98)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #77)
> > Created attachment 8629603 [details] [diff] [review]
> > Provide a visual indicator for tabs that are playing audio (WIP)
> > 
> > Dao, can you please provide some feedback here?  I'm not implementing the
> > muting or the context menu for now.  One problem that I ran into which I
> > don't know how to solve is how to overlay the icon on top of the tab icon
> > for pinned tabs.  I tried to use a trick similar to the one used for
> > .tab-icon-overlay but that doesn't seem to work.
> 
> If you want to follow tab-icon-overlay (which I think you should), you'll
> actually need to reuse that element, i.e. add another state to that along
> with "crashed".

I thought about that.  The issue is that for the non-pinned case, the behavior is different than tab-icon-overlay (the icon is put next to the tab close icon, it doesn't overlay the favicon, and it lines up with the favicon, the text and the close button vertically), but for the pinned case, the behavior is exactly similar to tab-icon-overlay.

What makes this difficult to handle is that the element needs to live in a different position in the DOM for those two cases.  I thought about moving it, but I'm not sure if that's OK since nothing else in the tab moves in the DOM as far as I can tell.  Is moving it in the DOM okay, or do you prefer something else?

> If you want this to be spawn a menu, you'll also need to think about
> keyboard access.

I talked to davidb about this.  Since the only item in the context menu is for toggling the mute state, he said that we should probably expose that as a keyboard shortcut and not worry about making the menu item keyboard accessible, which is on par with the keyboard accessibility of the tab close icon, for example.

Do you mind if the keyboard shorcut moves to its own bug?  The implementation is very simple, but we need to decide what keyboard shortcut to use!

> >+      <handler event="DOMMediaPlaybackStarted">
> 
> This will need some e10s love.

It works well in e10s, as mconley mentioned.
Flags: needinfo?(dao)
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #100)
> I thought about that.  The issue is that for the non-pinned case, the
> behavior is different than tab-icon-overlay (the icon is put next to the tab
> close icon, it doesn't overlay the favicon, and it lines up with the
> favicon, the text and the close button vertically), but for the pinned case,
> the behavior is exactly similar to tab-icon-overlay.

It might be worth reconsidering the non-pinned case to use the same approach as pinned tabs. Our tabs get as narrow as 100px at which point we only show five to six characters of their label (two to three for the selected tab). With another icon in there, we'd drop another ~three character, making those tab labels close to useless.

Besides I think having the indicator in one place is desirable in itself in terms of UX consistency.

> What makes this difficult to handle is that the element needs to live in a
> different position in the DOM for those two cases.  I thought about moving
> it, but I'm not sure if that's OK since nothing else in the tab moves in the
> DOM as far as I can tell.  Is moving it in the DOM okay, or do you prefer
> something else?

You could use two DOM nodes and show only the one you want. But like I said above there a more fundamental UX questions.

> Do you mind if the keyboard shorcut moves to its own bug?  The
> implementation is very simple, but we need to decide what keyboard shortcut
> to use!

Fine as long as we get it done for the same release.
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #101)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #100)
> > I thought about that.  The issue is that for the non-pinned case, the
> > behavior is different than tab-icon-overlay (the icon is put next to the tab
> > close icon, it doesn't overlay the favicon, and it lines up with the
> > favicon, the text and the close button vertically), but for the pinned case,
> > the behavior is exactly similar to tab-icon-overlay.
> 
> It might be worth reconsidering the non-pinned case to use the same approach
> as pinned tabs. Our tabs get as narrow as 100px at which point we only show
> five to six characters of their label (two to three for the selected tab).
> With another icon in there, we'd drop another ~three character, making those
> tab labels close to useless.
> 
> Besides I think having the indicator in one place is desirable in itself in
> terms of UX consistency.

That's a good point.

Phillip, what do you think?

> > What makes this difficult to handle is that the element needs to live in a
> > different position in the DOM for those two cases.  I thought about moving
> > it, but I'm not sure if that's OK since nothing else in the tab moves in the
> > DOM as far as I can tell.  Is moving it in the DOM okay, or do you prefer
> > something else?
> 
> You could use two DOM nodes and show only the one you want. But like I said
> above there a more fundamental UX questions.

OK, thanks, I will wait for UX input and pick one of the approaches based on that.

> > Do you mind if the keyboard shorcut moves to its own bug?  The
> > implementation is very simple, but we need to decide what keyboard shortcut
> > to use!
> 
> Fine as long as we get it done for the same release.

Yes, of course.  Note that this functionality is hidden behind a pref so we will only toggle it when this and the other dependencies are ready.
Flags: needinfo?(philipp)
Created attachment 8632400 [details]
tab-audio.svg
Flags: needinfo?(shorlander)
Created attachment 8632401 [details]
tab-audio-small.svg
Depends on: 1177399
Depends on: 1183356
Depends on: 1183700
Comment on attachment 8629692 [details] [diff] [review]
Styling patch

I have incorporated the changes that this patch made in a better way in my patches locally.
Attachment #8629692 - Attachment is obsolete: true
Created attachment 8633860 [details] [diff] [review]
Part 1: Provide a visual indicator for tabs that are playing audio

New versions of the patches, using the new icons.  So far, fully implement what I know of the UX spec, pending a few clarifications from the UX team.
Attachment #8629603 - Attachment is obsolete: true
Attachment #8629618 - Attachment is obsolete: true
Attachment #8629742 - Attachment is obsolete: true
Created attachment 8633862 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon
Created attachment 8633863 [details] [diff] [review]
Part 3: Implement a context menu for muting and unmuting a tab
Attachment #8629604 - Attachment is obsolete: true
Attachment #8629621 - Attachment is obsolete: true
Try server builds for this feature for all desktop platforms will be available here in the next few hours:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b5af25165f

These builds can be used to test the full UI in action.

Comment 110

2 years ago
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #109)
> Try server builds for this feature for all desktop platforms will be
> available here in the next few hours:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b5af25165f
> 
> These builds can be used to test the full UI in action.

Looks really nice ! Here are a few issues I have found though :
- The icon is hardly visible on a pinned tab if the site has a dark favicon (I'd suggest adding a white background around it as the last mockup (in the UX spec) shows)
- The icon doesn't support dark themes (lw-themes and devedition), it should turn white on background tabs for a dark theme
- The icon has an hover and active state, but clicking doesn't do anything, which is quite misleading
(In reply to Tim Nguyen [:ntim] from comment #110)
> Looks really nice ! Here are a few issues I have found though :
> - The icon is hardly visible on a pinned tab if the site has a dark favicon
> (I'd suggest adding a white background around it as the last mockup (in the
> UX spec) shows)

Yeah, good point.  I'll do that.

> - The icon doesn't support dark themes (lw-themes and devedition), it should
> turn white on background tabs for a dark theme

I should be able to modify the existing icons for that purpose...  I'll do that!

> - The icon has an hover and active state, but clicking doesn't do anything,
> which is quite misleading

Yes, I have already brought this up with the UX team and am waiting on their response.  Without changing the current UX spec, we probably want to get rid of the active state...
I've tried to see if I can get *any* kind of different behaviors with tabs with this try build, but am unable to hear a difference with my screen reader. I am not even sure I'm getting the UI, since when I open a few tabs after the browser starts, and one of them contains, for example, a Youtube video, switching through the tab bar doesn't give me any different speech. Moreover, the context menu items for each tab don't show that this tab could be muted, even on the tab where the video is playing.

Since I have noone to visually verify at the moment that I am getting the UI at all, I am not sure whether I am just not seeing it, or whether the UI is there, but doesn't have any accessibility information for screen reader users.

Note that I'll be on PTO for the next two weeks, so best to continue working on this with davidb in the meantime.
Flags: needinfo?(mzehe) → needinfo?(dbolter)
Thanks for testing this, Marco!

(In reply to Marco Zehe (:MarcoZ, on PTO until August 3) from comment #112)
> I've tried to see if I can get *any* kind of different behaviors with tabs
> with this try build, but am unable to hear a difference with my screen
> reader. I am not even sure I'm getting the UI, since when I open a few tabs
> after the browser starts, and one of them contains, for example, a Youtube
> video, switching through the tab bar doesn't give me any different speech.

You should be getting a new UI, this works with Youtube.  Not hearing anything through the a11y client is expected, given that the UI elements all have role="presentation".

> Moreover, the context menu items for each tab don't show that this tab could
> be muted, even on the tab where the video is playing.

Yes, the context menu is separate from the normal tab context menu, and currently only accessible by right clicking on the icon in the UI, so this is also expected.  We will add a keyboard shortcut for toggling the mute/unmute state in the current tab.

> Since I have noone to visually verify at the moment that I am getting the UI
> at all, I am not sure whether I am just not seeing it, or whether the UI is
> there, but doesn't have any accessibility information for screen reader
> users.

That would be what I expect.  I'm not sure what else we need to do if anything besides the keyboard shortcut.

> Note that I'll be on PTO for the next two weeks, so best to continue working
> on this with davidb in the meantime.

Sounds good.  Enjoy your vacation!

Comment 114

2 years ago
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #113)
> Yes, the context menu is separate from the normal tab context menu, and
> currently only accessible by right clicking on the icon in the UI, so this
> is also expected.  We will add a keyboard shortcut for toggling the
> mute/unmute state in the current tab.

Does that mean you can't mute a tab "in advance", because you know it will be noisy but it is not yet?
I think it could be nice to be able to mute a tab before it even begin to produce any noise.
(In reply to Clément Lefèvre from comment #114)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #113)
> > Yes, the context menu is separate from the normal tab context menu, and
> > currently only accessible by right clicking on the icon in the UI, so this
> > is also expected.  We will add a keyboard shortcut for toggling the
> > mute/unmute state in the current tab.
> 
> Does that mean you can't mute a tab "in advance", because you know it will
> be noisy but it is not yet?

Yes.

> I think it could be nice to be able to mute a tab before it even begin to
> produce any noise.

Please file a follow-up for that, so that we can discuss it with the UX team.  Thanks!
Depends on: 1185303
Let's try to land the implementation based on the current UX spec.  I'm adding a pref for the UI so that we can keep it turned on for non-release builds until we're satisfied with the design.
Created attachment 8635803 [details] [diff] [review]
Part 1: Provide a visual indicator for tabs that are playing audio
Attachment #8633860 - Attachment is obsolete: true
Attachment #8633862 - Attachment is obsolete: true
Attachment #8633863 - Attachment is obsolete: true
Attachment #8635803 - Flags: review?(dao)
Created attachment 8635804 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon
Created attachment 8635805 [details] [diff] [review]
Part 3: Implement a context menu for muting and unmuting a tab
Created attachment 8635806 [details] [diff] [review]
Part 4: Add automated tests
Attachment #8635804 - Flags: review?(dao)
Attachment #8635805 - Flags: review?(dao)
Attachment #8635806 - Flags: review?(dao)
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #116)
> Let's try to land the implementation based on the current UX spec.  I'm
> adding a pref for the UI so that we can keep it turned on for non-release
> builds until we're satisfied with the design.

I'd rather go with the overlay for all tabs for now, given the inherent shortcomings of showing a separate icon I pointed out earlier.

Comment 122

2 years ago
Comment on attachment 8635803 [details] [diff] [review]
Part 1: Provide a visual indicator for tabs that are playing audio

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

::: browser/themes/shared/tabs.inc.css
@@ +108,5 @@
> +}
> +
> +.tab-icon-overlay[playing][pinned]:hover {
> +  /* This rule must be important in order to override the more specific one in devedition.inc.css. */
> +  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio-small.svg#tab-audio-hover") !important;

Instead of setting !important, you can simply use :not(:hover):not(:active) in the devedition.inc.css rule

@@ +110,5 @@
> +.tab-icon-overlay[playing][pinned]:hover {
> +  /* This rule must be important in order to override the more specific one in devedition.inc.css. */
> +  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio-small.svg#tab-audio-hover") !important;
> +  border: 1px solid transparent;
> +  border-radius: 8px;

Maybe it's better to apply both of these border properties on the normal state ?
It would reduce code duplication, and would avoid reflows when hovering the icon.

@@ +116,5 @@
> +}
> +
> +.tab-icon-overlay[playing][pinned]:active {
> +  /* This rule must be important in order to override the more specific one in devedition.inc.css. */
> +  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio-small.svg#tab-audio-pressed") !important;

Same comment as above

@@ +122,5 @@
> +  border-radius: 8px;
> +  background-color: white;
> +}
> +
> +.tab-icon-overlay[playing][pinned]:-moz-lwtheme {

The :-moz-lwtheme selector doesn't cover Windows and Linux High Contrast themes.

I recommend using the brighttext attribute instead, covers all of these cases + DevEdition :
#TabsToolbar[brighttext] .tab-icon-overlay[playing][pinned]
Sorry for the super-late response, but you're already making all the right decisions :)

As for the hover effect: Could we just make a left click on the icon mute/unmute the tab directly? That would make that functionality more discoverable as well.
Flags: needinfo?(philipp)
(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #123)
> As for the hover effect: Could we just make a left click on the icon
> mute/unmute the tab directly? That would make that functionality more
> discoverable as well.

Yes, that is definitely possible.  But should I keep the context menu around as well?

Also, are you and Stephen fine with switching to only using icons that overlay the tab icon?  Dao in comment 121 is asking me to do that, but that would kind of ignore the current UX spec.
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)

Comment 125

2 years ago
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #115)
> (In reply to Clément Lefèvre from comment #114)
> > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> > needinfo? me!) from comment #113)
> > > Yes, the context menu is separate from the normal tab context menu, and
> > > currently only accessible by right clicking on the icon in the UI, so this
> > > is also expected.  We will add a keyboard shortcut for toggling the
> > > mute/unmute state in the current tab.
> > 
> > Does that mean you can't mute a tab "in advance", because you know it will
> > be noisy but it is not yet?
> 
> Yes.
> 
> > I think it could be nice to be able to mute a tab before it even begin to
> > produce any noise.
> 
> Please file a follow-up for that, so that we can discuss it with the UX
> team.  Thanks!

I filled it here https://bugzilla.mozilla.org/show_bug.cgi?id=1185502
Making this one depends on it.
Depends on: 1185502
(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #123)
> Sorry for the super-late response, but you're already making all the right
> decisions :)
> 
> As for the hover effect: Could we just make a left click on the icon
> mute/unmute the tab directly? That would make that functionality more
> discoverable as well.

I agree - I was just playing with Ehsan's build and regular left-clicking seems very natural here. It's seemed pretty clear that clicking it would do something, based on the hover effect.

The only reason I can think of that we wouldn't make it left-clickable is just to leave more click target for just focusing the tab, when the clicked tab is not a foreground tab. You _could_ consider making the icon only actionable when the tab is in the foreground, though that would mean not letting you mute background tabs. It's probably not that problematic either way -- something we can watch.

BTW - unrelated, but a thought: maybe we should put the audio playing indicator next to items in the tab overflow list? That would be a quicker way to track down an audio-playing background tab than scrolling through all my visible tabs.
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #124)
> Also, are you and Stephen fine with switching to only using icons that
> overlay the tab icon?  Dao in comment 121 is asking me to do that, but that
> would kind of ignore the current UX spec.

I would rather keep it on the tab where we will often have more room and we won't have to worry about overlaying things. I don't think overlaying the tab/favicon is optimal but makes sense in the pinned tab case where we don't have any other real estate to work with.
Flags: needinfo?(shorlander)
Comment on attachment 8635805 [details] [diff] [review]
Part 3: Implement a context menu for muting and unmuting a tab

Speaking today on IRC, Stepehen asked me to change the muting behavior to be triggered by left clicking on the icon instead of using the context menu.  This will change part 3 and part 4 patches, the rest should be ready for review.
Attachment #8635805 - Attachment is obsolete: true
Attachment #8635805 - Flags: review?(dao)
Attachment #8635806 - Attachment is obsolete: true
Attachment #8635806 - Flags: review?(dao)
Blocks: 1186267
Created attachment 8636948 [details] [diff] [review]
Part 1: Provide a visual indicator for tabs that are playing audio
Attachment #8635803 - Attachment is obsolete: true
Attachment #8635804 - Attachment is obsolete: true
Attachment #8635803 - Flags: review?(dao)
Attachment #8635804 - Flags: review?(dao)
Created attachment 8636949 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon
Attachment #8636948 - Flags: review?(dao)
Created attachment 8636950 [details] [diff] [review]
Part 3: Allow muting and unmuting a tab by clicking on the playing icon
Attachment #8636949 - Flags: review?(dao)
Created attachment 8636951 [details] [diff] [review]
Part 4: Add automated tests
Attachment #8636950 - Flags: review?(dao)
Attachment #8636951 - Flags: review?(dao)
Attachment #8636950 - Attachment description: Part 3: Implement a context menu for muting and unmuting a tab → Part 3: Allow muting and unmuting a tab by clicking on the playing icon
(In reply to Stephen Horlander [:shorlander] (Away until 7/20) from comment #127)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #124)
> > Also, are you and Stephen fine with switching to only using icons that
> > overlay the tab icon?  Dao in comment 121 is asking me to do that, but that
> > would kind of ignore the current UX spec.
> 
> I would rather keep it on the tab where we will often have more room and we
> won't have to worry about overlaying things. I don't think overlaying the
> tab/favicon is optimal but makes sense in the pinned tab case where we don't
> have any other real estate to work with.

I realize that we often have more room. What are your thoughts on the minority case that many power users will run into? This is basically the default case for me thanks to session restore.
Flags: needinfo?(shorlander)
What about a compromise where we switch to the overlay if the tabs get less than, say, 60 or 70 pixels wide?
(That should also automatically handle the pinned-tab case, and will make the switch-over point be reasonably easily changeable if we decide to make it larger or smaller in the future…)

Comment 135

2 years ago
(In reply to Blake Winton (:bwinton) from comment #134)
> What about a compromise where we switch to the overlay if the tabs get less
> than, say, 60 or 70 pixels wide?
They'll never reach that width thanks to our awesome overflow handling. The tab min-width is 100px. 

(In reply to Dão Gottwald [:dao] from comment #133)
> I realize that we often have more room. What are your thoughts on the
> minority case that many power users will run into? This is basically the
> default case for me thanks to session restore.

We could increase the tab min-width to 116px (100 + iconsize).
In order to keep this bug sane, please let's not even consider doing things such as changing the default tab width here.  If we decide to do that, that work needs to go in its own bug.  Our tab titles are already pretty useless when you reach the tab min-width threshold, and since we're not adding the audio playback icon to every single tab, we're not making the existing issue much worse.
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #136)
> Our tab titles are already pretty useless
> when you reach the tab min-width threshold, and since we're not adding the
> audio playback icon to every single tab, we're not making the existing issue
> much worse.

Tab titles still give a hint that kind of works with some luck when reaching the min-width. Displaying only two characters instead of five can easily push that hint over to being useless much more often. Obviously we're only talking about tabs with audio playback here, but that doesn't mean it's a nonissue; users will still want to identify those tabs and favicons aren't always helpful enough.
(In reply to Tim Nguyen [:ntim] from comment #135)
> (In reply to Blake Winton (:bwinton) from comment #134)
> > What about a compromise where we switch to the overlay if the tabs get less
> > than, say, 60 or 70 pixels wide?
> They'll never reach that width thanks to our awesome overflow handling. The
> tab min-width is 100px. 

This completely misses the gist of Blake's idea. Fwiw, there's already browser.tabs.tabClipWidth, it's 140 by default, and we remove the close button from background tabs when tabs shrink to that size.
(In reply to Dão Gottwald [:dao] from comment #137)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #136)
> > Our tab titles are already pretty useless
> > when you reach the tab min-width threshold, and since we're not adding the
> > audio playback icon to every single tab, we're not making the existing issue
> > much worse.
> 
> Tab titles still give a hint that kind of works with some luck when reaching
> the min-width. Displaying only two characters instead of five can easily
> push that hint over to being useless much more often. Obviously we're only
> talking about tabs with audio playback here, but that doesn't mean it's a
> nonissue; users will still want to identify those tabs and favicons aren't
> always helpful enough.

I agree it is an issue, but I don't think it will be a serious detriment here. Tabs playing audio will (hopefully) remain rare. I think the functionality of being able to identify and mute noisy background tabs is worth the trade-off.

I do think it would be worth a follow-up to see if we can mitigate the character loss more though. Also for the best ways to handle tabs in overflow playing audio.
Flags: needinfo?(shorlander)
Blocks: 1186632
Blocks: 1186633
(In reply to Stephen Horlander [:shorlander] (Away until 7/20) from comment #139)
> (In reply to Dão Gottwald [:dao] from comment #137)
> > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> > needinfo? me!) from comment #136)
> > > Our tab titles are already pretty useless
> > > when you reach the tab min-width threshold, and since we're not adding the
> > > audio playback icon to every single tab, we're not making the existing issue
> > > much worse.
> > 
> > Tab titles still give a hint that kind of works with some luck when reaching
> > the min-width. Displaying only two characters instead of five can easily
> > push that hint over to being useless much more often. Obviously we're only
> > talking about tabs with audio playback here, but that doesn't mean it's a
> > nonissue; users will still want to identify those tabs and favicons aren't
> > always helpful enough.
> 
> I agree it is an issue, but I don't think it will be a serious detriment
> here. Tabs playing audio will (hopefully) remain rare. I think the
> functionality of being able to identify and mute noisy background tabs is
> worth the trade-off.

Thanks!  Dao, can we please move forward with reviewing the code here now?  Thank you!

> I do think it would be worth a follow-up to see if we can mitigate the
> character loss more though. Also for the best ways to handle tabs in
> overflow playing audio.

Filed bug 1186632 and bug 1186633.
No longer depends on: 1018504
Blocks: 1185502
No longer depends on: 1185502
No longer depends on: 1182164
Comment on attachment 8636948 [details] [diff] [review]
Part 1: Provide a visual indicator for tabs that are playing audio

>+      <handler event="DOMMediaPlaybackStarted">
>+        <![CDATA[
>+          if (!Services.prefs.getBoolPref("browser.tabs.showAudioPlayingIcon") ||
>+              !event.isTrusted)
>+            return;
>+
>+          var browser = event.originalTarget;
>+          var tab = this.getTabForBrowser(browser);
>+          if (!tab || tab.hasAttribute("pending"))
>+            return;

I don't understand the 'pending' attribute check. How can a pending browser send this event?

>+          tab.setAttribute("playing", true);
>+          this._tabAttrModified(tab, ["playing"]);

nit: please rename this to soundplaying for clarity

>+          <xul:image xbl:inherits="playing,pinned"
>+                     class="tab-icon-media"

nit: tab-icon-sound please

>-.tab-icon-overlay {
>+.tab-icon-overlay,
>+.tab-icon-media[pinned] {
>   opacity: 0.9999;

I don't understand this change.

>+.tab-icon-overlay[playing][pinned] {
>+  display: -moz-box;
>+  margin-top: -12px;

.tab-icon-overlay has margin-top: 10px. Let's please settle on one location and not randomly change it for different icons.

>+.tab-icon-overlay[playing][pinned]:active {

>+#TabsToolbar[brighttext] .tab-icon-overlay[playing][pinned]:active {

:hover:active

>+.tab-icon-media {
>+  -moz-margin-start: 4px;
>+  -moz-margin-end: -2px;

Can you get rid of the negative margin here?

>+  display: none;

>+.tab-icon-media[playing] {
>+  display: -moz-box;

>+.tab-icon-media[playing][pinned] {
>+  display: none !important;

Please consolidate this as follows:

.tab-icon-media:not([playing])
.tab-icon-media[pinned] {
  display: none;
}

> .tab-background-middle,
> .tabs-newtab-button,
>+.tab-icon-overlay[playing][pinned],
>+.tab-icon-media,
> .tab-close-button {
>   pointer-events: auto;
> }

Why [pinned]? The overlay isn't displayed in the first place when the tab isn't pinned.
Attachment #8636948 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #141)
> Comment on attachment 8636948 [details] [diff] [review]
> Part 1: Provide a visual indicator for tabs that are playing audio
> 
> >+      <handler event="DOMMediaPlaybackStarted">
> >+        <![CDATA[
> >+          if (!Services.prefs.getBoolPref("browser.tabs.showAudioPlayingIcon") ||
> >+              !event.isTrusted)
> >+            return;
> >+
> >+          var browser = event.originalTarget;
> >+          var tab = this.getTabForBrowser(browser);
> >+          if (!tab || tab.hasAttribute("pending"))
> >+            return;
> 
> I don't understand the 'pending' attribute check. How can a pending browser
> send this event?

This is me cargo-culting, sorry.  What's a pending browser?  If this doesn't make sense, I'll remove it.

> >-.tab-icon-overlay {
> >+.tab-icon-overlay,
> >+.tab-icon-media[pinned] {
> >   opacity: 0.9999;
> 
> I don't understand this change.

Oops, sorry, this is a remnant of previous iterations.  Will remove.

> >+.tab-icon-overlay[playing][pinned] {
> >+  display: -moz-box;
> >+  margin-top: -12px;
> 
> .tab-icon-overlay has margin-top: 10px. Let's please settle on one location
> and not randomly change it for different icons.

Should I re-use 10px here?

> >+.tab-icon-media {
> >+  -moz-margin-start: 4px;
> >+  -moz-margin-end: -2px;
> 
> Can you get rid of the negative margin here?

Sure.  What do I replace them with?  Just remove them?

> > .tab-background-middle,
> > .tabs-newtab-button,
> >+.tab-icon-overlay[playing][pinned],
> >+.tab-icon-media,
> > .tab-close-button {
> >   pointer-events: auto;
> > }
> 
> Why [pinned]? The overlay isn't displayed in the first place when the tab
> isn't pinned.

This is another remnant, sorry.

Will address the rest of the nits.
Flags: needinfo?(dao)
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #142)
> > I don't understand the 'pending' attribute check. How can a pending browser
> > send this event?
> 
> This is me cargo-culting, sorry.  What's a pending browser?  If this doesn't
> make sense, I'll remove it.

It's a browser that hasn't been restored yet by session restore. In other words it doesn't have content loaded.

> > >+.tab-icon-overlay[playing][pinned] {
> > >+  display: -moz-box;
> > >+  margin-top: -12px;
> > 
> > .tab-icon-overlay has margin-top: 10px. Let's please settle on one location
> > and not randomly change it for different icons.
> 
> Should I re-use 10px here?

.tab-icon-overlay already has that by default, so you shouldn't have to specify any margin here.

> > >+.tab-icon-media {
> > >+  -moz-margin-start: 4px;
> > >+  -moz-margin-end: -2px;
> > 
> > Can you get rid of the negative margin here?
> 
> Sure.  What do I replace them with?  Just remove them?

There's probably some room for cleaning up the margins used within tabs, but for now you can just omit -moz-margin-end here.
Flags: needinfo?(dao)
Comment on attachment 8636949 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon

This will likely need to be updated along with part 1, thus cancelling review request.

nit: we're not using the "member" variable name prefix anymore, please use _foo instead of mFoo
Attachment #8636949 - Flags: review?(dao)
Comment on attachment 8636950 [details] [diff] [review]
Part 3: Allow muting and unmuting a tab by clicking on the playing icon

>+      <method name="toggleMuteAudio">

this should be private, i.e. _toggleMuteAudio
Attachment #8636950 - Flags: review?(dao)

Updated

2 years ago
Attachment #8636951 - Flags: review?(dao)
Flags: needinfo?(philipp)
(In reply to Dão Gottwald [:dao] from comment #143)
> > > >+.tab-icon-overlay[playing][pinned] {
> > > >+  display: -moz-box;
> > > >+  margin-top: -12px;
> > > 
> > > .tab-icon-overlay has margin-top: 10px. Let's please settle on one location
> > > and not randomly change it for different icons.
> > 
> > Should I re-use 10px here?
> 
> .tab-icon-overlay already has that by default, so you shouldn't have to
> specify any margin here.

OK, I looked into this, but we actually do want this margin here, since the tab crashed icon appears at the bottom overlaying the tab icon, but the sound icon needs to appear at the top according to the UX spec, so I can't just rely on the 10px margin here.
Created attachment 8639019 [details] [diff] [review]
Part 1: Provide a visual indicator for tabs that are playing audio
Attachment #8636948 - Attachment is obsolete: true
Attachment #8636949 - Attachment is obsolete: true
Attachment #8636950 - Attachment is obsolete: true
Attachment #8636951 - Attachment is obsolete: true
Created attachment 8639020 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon
Created attachment 8639021 [details] [diff] [review]
Part 3: Allow muting and unmuting a tab by clicking on the playing icon
Created attachment 8639022 [details] [diff] [review]
Part 4: Add automated tests
Attachment #8639019 - Flags: review?(dao)
Attachment #8639020 - Flags: review?(dao)
Attachment #8639022 - Flags: review?(dao)
Attachment #8639021 - Flags: review?(dao)

Comment 151

2 years ago
Comment on attachment 8639020 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon

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

::: browser/locales/en-US/chrome/browser/tabbrowser.dtd
@@ +3,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!ENTITY  closeTab.label         "Close Tab">
> +<!ENTITY  playingAudio.label     "This tab is playing audio">
> +<!ENTITY  mutedAudio.label       "This tab has been muted">

Now that clicking the icon mutes/unmutes the tab, it's worth changing the tooltips to reflect what clicking will actually do.
Please file a follow-up bug for any requests to change the UX spec, let's keep this bug restricted to implementing the current spec.  Thanks!
Depends on: 1188095

Updated

2 years ago
Depends on: 1188114
Blocks: 1188114
No longer depends on: 1188114
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #146)
> (In reply to Dão Gottwald [:dao] from comment #143)
> > > > >+.tab-icon-overlay[playing][pinned] {
> > > > >+  display: -moz-box;
> > > > >+  margin-top: -12px;
> > > > 
> > > > .tab-icon-overlay has margin-top: 10px. Let's please settle on one location
> > > > and not randomly change it for different icons.
> > > 
> > > Should I re-use 10px here?
> > 
> > .tab-icon-overlay already has that by default, so you shouldn't have to
> > specify any margin here.
> 
> OK, I looked into this, but we actually do want this margin here, since the
> tab crashed icon appears at the bottom overlaying the tab icon, but the
> sound icon needs to appear at the top according to the UX spec, so I can't
> just rely on the 10px margin here.

If the design spec isn't in line with existing icon overlays, that seems like a bug. So please don't randomly change the position for this particular indicator. We can however just change the default tab-icon-overlay position if that's wanted.
Comment on attachment 8639021 [details] [diff] [review]
Part 3: Allow muting and unmuting a tab by clicking on the playing icon

>+      <handler event="click">
>+      <![CDATA[
>+        if (event.button != 0) {
>+          return;
>+        }
>+
>+        let anonid = event.originalTarget.getAttribute("anonid");
>+        if (anonid != "soundplaying-icon" && anonid != "overlay-icon") {
>+          return;
>+        }
>+
>+        let tab = event.originalTarget.parentNode.parentNode.parentNode;
>+        this._toggleMuteAudio(tab);

You need to check that overlay-icon is actually being used as the sound-playing icon rather than something else. On a related note, it might make sense for overlay-icon to have status attribute (e.g. status="crashed", status="soundplaying", status="muted") rather than a bunch of boolean attributes.
Attachment #8639021 - Flags: review?(dao) → review-
Blocks: 1188548
(In reply to Dão Gottwald [:dao] from comment #154)
> On a related note, it might
> make sense for overlay-icon to have status attribute (e.g. status="crashed",
> status="soundplaying", status="muted") rather than a bunch of boolean
> attributes.

Filed follow-up bug 1188548 for this.
(In reply to Dão Gottwald [:dao] from comment #153)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #146)
> > (In reply to Dão Gottwald [:dao] from comment #143)
> > > > > >+.tab-icon-overlay[playing][pinned] {
> > > > > >+  display: -moz-box;
> > > > > >+  margin-top: -12px;
> > > > > 
> > > > > .tab-icon-overlay has margin-top: 10px. Let's please settle on one location
> > > > > and not randomly change it for different icons.
> > > > 
> > > > Should I re-use 10px here?
> > > 
> > > .tab-icon-overlay already has that by default, so you shouldn't have to
> > > specify any margin here.
> > 
> > OK, I looked into this, but we actually do want this margin here, since the
> > tab crashed icon appears at the bottom overlaying the tab icon, but the
> > sound icon needs to appear at the top according to the UX spec, so I can't
> > just rely on the 10px margin here.
> 
> If the design spec isn't in line with existing icon overlays, that seems
> like a bug. So please don't randomly change the position for this particular
> indicator. We can however just change the default tab-icon-overlay position
> if that's wanted.

Talking to Stephen on IRC, he asked me to use the top-right position for both icons.
Created attachment 8640055 [details] [diff] [review]
Part 1: Provide a visual indicator for tabs that are playing audio
Attachment #8639019 - Attachment is obsolete: true
Attachment #8639019 - Flags: review?(dao)
Attachment #8640055 - Flags: review?(dao)
Created attachment 8640064 [details] [diff] [review]
Part 3: Allow muting and unmuting a tab by clicking on the playing icon
Attachment #8639021 - Attachment is obsolete: true
Attachment #8640064 - Flags: review?(dao)
Comment on attachment 8639020 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon

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

::: browser/base/content/tabbrowser.xml
@@ +5808,5 @@
>          </setter>
>        </property>
>  
>        <field name="mOverCloseButton">false</field>
> +      <field name="_overPlayingIcon">false</field>

Please use mOverPlayingIcon to be consistent with these other two fields.
Attachment #8639020 - Flags: review?(dao) → review+
Comment on attachment 8640055 [details] [diff] [review]
Part 1: Provide a visual indicator for tabs that are playing audio

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

::: browser/base/content/tabbrowser.xml
@@ +4256,5 @@
> +          if (!tab)
> +            return;
> +
> +          tab.removeAttribute("soundplaying");
> +          this._tabAttrModified(tab, ["soundplaying"]);

We should only call _tabAttrModified if the soundplaying attribute was present before it was removed.

if (tab.hasAttribute("soundplaying")) {
  tab.removeAttribute("soundplaying");
  this._tabAttrModified(tab, ["soundplaying"]);
}

::: browser/themes/shared/tabs.inc.css
@@ +90,5 @@
>  
>  .tab-icon-overlay {
>    width: 16px;
>    height: 16px;
> +  margin-top: -12px;

As this was discussed on IRC, we should be consistent with the location of the icon overlays (tab crashed and soundplaying).

Shorlander said that he wants the icon in the top right for both cases. You can either make the change so that the tabcrashed icon is moved to the top right, or not adjust the CSS in this bug and file a followup to get them both moved to the top right.
Attachment #8640055 - Flags: review?(dao) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #160)
> ::: browser/themes/shared/tabs.inc.css
> @@ +90,5 @@
> >  
> >  .tab-icon-overlay {
> >    width: 16px;
> >    height: 16px;
> > +  margin-top: -12px;
> 
> As this was discussed on IRC, we should be consistent with the location of
> the icon overlays (tab crashed and soundplaying).

Yes.

> Shorlander said that he wants the icon in the top right for both cases. You
> can either make the change so that the tabcrashed icon is moved to the top
> right, or not adjust the CSS in this bug and file a followup to get them
> both moved to the top right.

This change moves both icons to the top right, as shorlander requested.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #159)
> ::: browser/base/content/tabbrowser.xml
> @@ +5808,5 @@
> >          </setter>
> >        </property>
> >  
> >        <field name="mOverCloseButton">false</field>
> > +      <field name="_overPlayingIcon">false</field>
> 
> Please use mOverPlayingIcon to be consistent with these other two fields.

I agree with you which is why I called it mOverPlayingIcon, but Dao asked me to use an underscore prefix in comment 144.  What gives?  :-)
Flags: needinfo?(jaws)
This seems like a small point to go back and forth over. But I am fine with landing this using the 'member' prefix and filing a follow-up bug to rename all of the members to use underscores.
Flags: needinfo?(jaws)
Blocks: 1188718
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #163)
> filing a follow-up bug to rename all of the members to use underscores.

I suspect that may break a bunch of add-ons.
Comment on attachment 8639020 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon

>-    <content context="tabContextMenu" closetabtext="&closeTab.label;">
>+    <content context="tabContextMenu"
>+             closetabtext="&closeTab.label;"
>+             playingtext="&playingAudio.label;"
>+             mutedtext="&mutedAudio.label;">

Please don't add the playingtext/mutedtext attributes. Instead, these two strings should be in tabbrowser.properties (as foo.tooltip, not foo.label). This isn't as suitable as a followup because temporary strings that ride the trains are bad for localizers.

closeTab.label should be in the properties file too as far as I can tell, but you don't need to take care of that here. You can if you want though, since it should be trivial to do along with the other two strings. If you do, please change "Close Tab" to "Close tab" while you're at it.

(In reply to Marco Bonardo [::mak] from comment #164)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #163)
> > filing a follow-up bug to rename all of the members to use underscores.
> 
> I suspect that may break a bunch of add-ons.

Probably. In any case, please do stick with _overPlayingIcon here.
Attachment #8639020 - Flags: review-
Comment on attachment 8640064 [details] [diff] [review]
Part 3: Allow muting and unmuting a tab by clicking on the playing icon

>+      <method name="_toggleMuteAudio">
>+        <parameter name="aTab"/>
>+        <body>
>+        <![CDATA[
>+          let tabContainer = this.parentNode;
>+          let browser = aTab.linkedBrowser;
>+          if (browser.audioMuted) {
>+            browser.unmute();
>+            aTab.removeAttribute("muted");
>+          } else {
>+            browser.mute();
>+            aTab.setAttribute("muted", "true");
>+          }
>+          tabContainer.tabbrowser._tabAttrModified(aTab, ["muted"]);
>+        ]]>
>+        </body>
>+      </method>

You're implementing this on the tab binding, right? So get rid of the aTab parameter and use 'this' instead.

>+        let soundPlaying = event.originalTarget.hasAttribute("soundplaying");

Just use this.hasAttribute("soundplaying") here.

>+        let anonid = event.originalTarget.getAttribute("anonid");
>+        if ((anonid == "soundplaying-icon") ||
>+            ((anonid == "overlay-icon") && soundPlaying)) {
>+          let tab = event.originalTarget.parentNode.parentNode.parentNode;
>+          this._toggleMuteAudio(tab);

And obviously get rid of 'tab' here too. r+ with these changes
Attachment #8640064 - Flags: review?(dao) → review+
Comment on attachment 8640055 [details] [diff] [review]
Part 1: Provide a visual indicator for tabs that are playing audio

>+.tab-icon-overlay[soundplaying][pinned] {
>+  display: -moz-box;
>+  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio-small.svg#tab-audio");
>+  border: 1px solid transparent;
>+  border-radius: 8px;

What's the transparent border needed for? Can you use padding instead or completely drop this? r+ with this explained / fixed
Attachment #8640055 - Flags: review+
(In reply to Dão Gottwald [:dao] from comment #167)
> Comment on attachment 8640055 [details] [diff] [review]
> Part 1: Provide a visual indicator for tabs that are playing audio
> 
> >+.tab-icon-overlay[soundplaying][pinned] {
> >+  display: -moz-box;
> >+  list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio-small.svg#tab-audio");
> >+  border: 1px solid transparent;
> >+  border-radius: 8px;
> 
> What's the transparent border needed for? Can you use padding instead or
> completely drop this? r+ with this explained / fixed

Seems to be unneeded...  I'll remove it.
Created attachment 8640456 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon
Attachment #8639020 - Attachment is obsolete: true
Created attachment 8640457 [details] [diff] [review]
Part 3: Allow muting and unmuting a tab by clicking on the playing icon
Attachment #8640064 - Attachment is obsolete: true
Attachment #8640456 - Flags: review?(jaws)
Attachment #8640456 - Flags: review?(dao)
Comment on attachment 8640456 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon

>+          var name, label;
>+          if (tab.mOverCloseButton) {
>+            name = "tabs.closeTab.tooltip";
>+          } else if (tab._overPlayingIcon) {
>+            name = tab.linkedBrowser.audioMuted ?
>+              "tabs.mutedAudio.tooltip" :
>+              "tabs.playingAudio.tooltip";
>+          } else {
>+            label = tab.getAttribute("label") +
>+                      (this.AppConstants.E10S_TESTING_ONLY && tab.linkedBrowser && tab.linkedBrowser.isRemoteBrowser ? " - e10s" : "");
>+          }
>+          if (name && !label) {
>+            label = this.mStringBundle.getString(name);
>+          }
>+          event.target.setAttribute("label", label);

nit: I think stringID would be clearer than 'name'

>     <handlers>
>       <handler event="mouseover"><![CDATA[
>         let anonid = event.originalTarget.getAttribute("anonid");
>+        let soundPlaying = event.originalTarget.hasAttribute("soundplaying");

this.hasAttribute("soundplaying")

>       <handler event="mouseout"><![CDATA[
>         let anonid = event.originalTarget.getAttribute("anonid");
>+        let soundPlaying = event.originalTarget.hasAttribute("soundplaying");

ditto
Attachment #8640456 - Flags: review?(dao) → review+

Updated

2 years ago
Attachment #8639022 - Flags: review?(dao) → review+
Blocks: 1188860
Note that I will remove the test for bug 405137 since it is now pointless without closetabtext.
Comment on attachment 8640456 [details] [diff] [review]
Part 2: Implement a tooltip for the tab playing icon

Dao's review is sufficient here.
Attachment #8640456 - Flags: review?(jaws)
Blocks: 1188978

Comment 174

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/af50345620f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/66d93422384f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd6a0558682
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5137e5dc19d
https://hg.mozilla.org/mozilla-central/rev/af50345620f8
https://hg.mozilla.org/mozilla-central/rev/66d93422384f
https://hg.mozilla.org/mozilla-central/rev/5cd6a0558682
https://hg.mozilla.org/mozilla-central/rev/e5137e5dc19d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Flags: needinfo?(dbolter)
Tab audio indicators are probably a feature worth highlighting in the Firefox 42 release notes. :)
Keywords: feature, relnote

Comment 177

2 years ago
Yes, it's worth highlighting , and we should remember to add that it's not only an indicator but also a way to mute a tab easily (Chrome just indicate that sound is playing)

Also, I didn't found how to test the functionality (not implemented in 42.0a1 2015-07-30, I think I will have to wait tomorrow.) The last version I tested were only working with HTML5 video and not flash (so it weren't using flash NPAPI mentioned in comment 67).

There's also one feature that would be great to implement: a check-box that set the default status to "muted" (and maybe a dialog box like flash click to play that allow playing sound for now, or always for actual website). Maybe I should make a enhancement bug request.
Adding addon-compat flag because this changes tab structure and at least one add-on has been affected.
Keywords: addon-compat
Release Note Request (optional, but appreciated)
[Why is this notable]: No more searching for the noisy tab. Tab audio indicators with mute button. https://twitter.com/mmmandel/status/626874461686497280
[Suggested wording]: New: Easily identify tabs playing audio and mute audio per tab
[Links (documentation, blog post, etc)]:
http://ehsanakhgari.org/blog/2015-07-30/tab-audio-indicators-and-muting-in-firefox-nightly

Wording of the note can use a little help.
relnote-firefox: --- → ?
Keywords: relnote

Comment 180

2 years ago
When Australis was being worked on there was a great deal of effort (and tests) to ensure that tab performance was not effected adversely.

Are these tests being run on this and we're assured that we're not regressing those performance metrics?
(In reply to Caspy7 from comment #180)
> When Australis was being worked on there was a great deal of effort (and
> tests) to ensure that tab performance was not effected adversely.
> 
> Are these tests being run on this and we're assured that we're not
> regressing those performance metrics?

The tab animation tests (TART) run regularly so if this regresses performance, it should show up. Automated tests wouldn't catch a regressions that only happens when a tab has the speaker icon, but that seems unlikely.
Would be worth it to manually test this quickly. The TART test is just a browser extension so it's easy to run. Contact Avi Halachmi for details
Depends on: 1190040
Depends on: 1190034
(In reply to Chris Peterson [:cpeterson] from comment #176)
> Tab audio indicators are probably a feature worth highlighting in the
> Firefox 42 release notes. :)

Those belong to bug 1188860.
relnote-firefox: ? → ---
Keywords: feature
Blocks: 1189759
Duplicate of this bug: 827059
Depends on: 1190096
No longer depends on: 1190034
Depends on: 1189434
No longer depends on: 1190096
Depends on: 1190023
Virtual_ManPL, I would appreciate if you stop changing the dependencies on this bug and the related ones.  I am using these dependencies to track the remaining issues, and people changing them from under my nose makes it really difficult to ensure that nothing is falling through the cracks.
I removed only duplicate bugs, which are still on dependencies list. It's to only make it cleaner and make not fixed bugs more visible.
Depends on: 1191491
No longer depends on: 1191491
Depends on: 1191437
Depends on: 1192614
No longer depends on: 1192614
Depends on: 1193191

Updated

2 years ago
Depends on: 1193749

Comment 186

2 years ago
I wonder if anyone mentioned this scenario:
Imagine that you have 1280x720 screen and 60 tabs opened (with overflow ofc). One of them is causing sound for ~0.5s once per ~2 minutes (random interval). The task is to locate the tab.
I believe that:
1) Indicator should fade within 2-3 seconds   (even in overflow list)
2) Additional indicator should be displayed for scrollbuttons in TabsToolbar if there's noisy tab in the respective direction (just like the blue background is displayed for them when new tab is opened)

Comment 187

2 years ago
(In reply to arni2033 from comment #186)

My screen is 1920x1080. When I [re]start FF, there are sometimes several noisy tabs.  What makes them difficult to find is that I have multiple windows with many tabs each.  For example, I currently have the following window tab counts: 95, 31, 14, 42, 49, 9, 21, 43, 3, 18, 13, and 11  m= 349. At least I've been whittling them down from over 500...  :)  So it's fairly likely and in addition to knowing which window is noisy, the noisy tab(s) may not be visible.

> 1) Indicator should fade within 2-3 seconds   (even in overflow list)
Fade time should be configurable.

> 2) Additional indicator should be displayed for scrollbuttons in TabsToolbar
> if there's noisy tab in the respective direction (just like the blue
> background is displayed for them when new tab is opened)

A readily accessible list of tabs (links) would also be appreciated, even as about:noisy_tabs
(In reply to Pierre Fortin from comment #187)
> > 1) Indicator should fade within 2-3 seconds   (even in overflow list)

The indicator being visible when the tab is not playing sound is even more confusing than what you're trying to solve.  We will not do this.

This feature isn't really designed to help you find tabs that make 0.5 second bursts of audio.

> > 2) Additional indicator should be displayed for scrollbuttons in TabsToolbar
> > if there's noisy tab in the respective direction (just like the blue
> > background is displayed for them when new tab is opened)

This has been discussed, but we don't have any current plans to do this.  What we have done so far is adding an icon to the list of tabs from the "List all tabs" button next to the tab bar.

> A readily accessible list of tabs (links) would also be appreciated, even as
> about:noisy_tabs

This should be something that an add-on can do.

Comment 189

2 years ago
(In reply to Ehsan Akhgari (don't ask for review please) from comment #188)
> The indicator being visible when the tab is not playing sound is even more confusing
Ok, I see you've already taken the decision but that's not a reason to call my suggestion (1) illogical
Smooth transition from opacity:0 to 1 and back is very different from "The indicator being visible"

Updated

2 years ago
Depends on: 1196056

Comment 190

2 years ago
(In reply to Ehsan Akhgari (don't ask for review please) from comment #188)
> (In reply to Pierre Fortin from comment #187)
> > A readily accessible list of tabs (links) would also be appreciated, even as
> > about:noisy_tabs
> 
> This should be something that an add-on can do.

Having the indicator icons in both the tabs and the drop-down menu that lists all tabs is probably a good idea. A full about page is overkill.

Comment 191

2 years ago
(In reply to Dave Garrett from comment #190)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #188)
> > (In reply to Pierre Fortin from comment #187)
> > > A readily accessible list of tabs (links) would also be appreciated, even as
> > > about:noisy_tabs
> > 
> > This should be something that an add-on can do.
> 
> Having the indicator icons in both the tabs and the drop-down menu that
> lists all tabs is probably a good idea. A full about page is overkill.

This was already added, I don't remember the bug though.
(In reply to arni2033 from comment #189)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #188)
> > The indicator being visible when the tab is not playing sound is even more confusing
> Ok, I see you've already taken the decision but that's not a reason to call
> my suggestion (1) illogical
> Smooth transition from opacity:0 to 1 and back is very different from "The
> indicator being visible"

You should file a bug about that.

Updated

2 years ago
Depends on: 1197569
Depends on: 1198771

Updated

2 years ago
Depends on: 1199929

Comment 193

2 years ago
Will this also be on Firefox Android?
(In reply to donrhummy from comment #193)
> Will this also be on Firefox Android?

Yes, see bug 1190301.

Updated

2 years ago
Blocks: 1199989

Comment 195

2 years ago
Depends on: 1200236
Depends on: 1200422

Updated

2 years ago
Depends on: 1200236

Updated

2 years ago
Depends on: 1200422

Updated

2 years ago
Depends on: 1199928

Updated

2 years ago
No longer depends on: 1199928

Updated

2 years ago
Depends on: 1207167

Comment 196

2 years ago
Successfully reproduce the bug on Firefox 41.0 , Build ID 20150917150946 on youtube. 

The fixe works for me on Firefox 42.0b2, Build ID 20150928102225 on mac OSX 10.10.4 on youtube.
QA Whiteboard: [bugday-20150930]
I have reproduced this bug on Nightly 37.0a1 (2014-12-28) on windows 7, 64 bit!

The bug's fix is now verified on Latest Beta 42.0b2!

Build ID: 	20150928102225
User Agent: 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
QA Whiteboard: [bugday-20150930]
I have reproduced this bug on Nightly 35.0a1 (2014-10-04) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Beta 42.0b3!

Build ID: 20151001142456
User Agent: Mozilla/5.0 (X11; Linux i686; rv:42.0) Gecko/20100101 Firefox/42.0

[testday-20151002]
QA Whiteboard: [testday-20151002]

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified

Updated

2 years ago
Depends on: 1211314

Updated

2 years ago
Depends on: 1211511
No longer depends on: 1211314
See Also: → bug 1221002

Comment 199

2 years ago
no audio indicator, if using themes.

Comment 200

2 years ago
(In reply to Gilbert Consellado from comment #199)
> no audio indicator, if using themes.

Can you file a new bug, please. This one is closed.

Updated

2 years ago
Depends on: 1224936

Updated

2 years ago
Depends on: 1225000
Depends on: 1225342

Updated

2 years ago
Depends on: 1225770

Updated

2 years ago
Depends on: 1232267

Updated

2 years ago
No longer depends on: 1232267
Depends on: 1232357
Depends on: 1235612
Depends on: 1237159

Updated

2 years ago
Blocks: 1200801

Comment 201

2 years ago
I'm just saying thanks to the crew that came up with this long desired and much needed enhancement
Depends on: 1239372

Updated

2 years ago
Depends on: 1240358
Depends on: 1241756
Blocks: 1249281

Updated

2 years ago
Depends on: 1249502

Updated

2 years ago
Depends on: 1251004
Blocks: 1254262

Updated

a year ago
Depends on: 1266139

Updated

7 months ago
Depends on: 1331461

Updated

7 months ago
Depends on: 1332371

Updated

13 days ago
Depends on: 1388660
You need to log in before you can comment on or make changes to this bug.