486262 - Provide visual indicator as to which tab is causing sound

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

[L. Mohan Arun]

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

Comment 1

[Matthias Versen [:Matti]]

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.

Comment 2

[L. Mohan Arun]

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

[Jo Hermans]

see bug 334987

Comment 5

[Stormy Peters]

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

Comment 6

[Jo Hermans]

see bug 334987 (as told in comment 3)

Comment 7

[David Humphrey (:humph)]

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.

Comment 8

[Matthew Gregan [:kinetik]]

Sounds similar to bug 626586.

Comment 10

[Girish Sharma [:Optimizer]]

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

[Jo Hermans]

No, that was probably an add-on.

Comment 12

[Girish Sharma [:Optimizer]]

no it was not .

Comment 13

[Justin Dolske [:Dolske]]

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

[u88484]

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

Comment 15

[Mathias Bynens]

(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.

Comment 16

[David Humphrey (:humph)]

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

[Stormy Peters]

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

[Paul Rouget [:paul]]

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)

Comment 19

[Csaba Kozák [:WonderCsabo]]

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.

Comment 20

[David Humphrey (:humph)]

> 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

[Paul Rouget [:paul]]

(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%.

Comment 22

[Girish Sharma [:Optimizer]]

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

[Paul Rouget [:paul]]

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?

Comment 24

[Csaba Kozák [:WonderCsabo]]

(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.

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 27

[David Humphrey (:humph)]

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

[Darin Fisher]

(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.

Comment 29

[Brendan Eich [:brendan]]

(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

[u123541]

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...

Comment 33

[Guilherme Lima]

It's now on Google Chrome Beta. http://chrome.blogspot.ca/2013/11/track-down-those-noisy-tabs.html

Comment 34

[Caspy7]

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.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

Bug 923247 gave us notifications that we can use to solve this for the non-plugin case.

Comment 36

[Daniel Veditz [:dveditz]]

http://imgur.com/zxCt0Ol

Comment 37

[Asif Youssuff]

This has landed in Chrome stable:

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

Comment 38

[Jared Sohn]

>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.

Comment 39

[Caspy7]

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)

Comment 40

[Nicholas O'Connor]

Until we get a response from Adobe, I'd like to politely ask for a setting (perhaps in about:config) to show an indicator when a tab has loaded a plugin.

It'd at least give a hint as to where that dang noise is coming from.

Comment 41

[Justin Dolske [:Dolske]]

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

[Tim Nguyen :ntim]

(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.

Comment 43

[Justin Dolske [:Dolske]]

Chrome ships with a customized version of Flash (using Pepper), which added the requisite support for per-tab indicators.

Comment 44

[Keul]

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 45

[techsupportmoc]

Surely the

Comment 46

[Nicolas Barbulesco]

(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

[u123541]

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

[Tim Nguyen :ntim]

(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 49

[spaceshipgalore]

Please could mozilla respond to this. Having support for this for html5 video/audio would be very helpful even if we can't get support for flash and other plugins.

Comment 51

[Boris Lassort]

Oh yes please support this for HTML5 !

Comment 52

[Albert Scheiner [:alberts]]

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

[Elbart]

No, it's like the Play-icon in Youtube-tabs.

Comment 54

[Paul [:pmac] McLanahan]

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 55

[Mitar]

I see now that Chrome has something like this now. A small speaker in the tab if audio is playing.

Comment 56

[xizha127]

I really need this, being the heavy user that I am, always having more than 5+ youtube tabs open. When I have 30 tabs that isn't playing any audio, this would be a life saver, it's also one of the main reasons I've used Chrome for the past year, but as Chrome is so bad at handling long up-times and massive amounts of memory over a long time period, I switched to Firefox.

It's worth the sacrifice, but it wouldn't hurt to have it implanted, like now.

Comment 57

[SebaZ]

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.

Comment 58

[Bobby Holley (:bholley)]

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. :-(

Comment 59

[Justin Dolske [: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.

Comment 60

[Bobby Holley (:bholley)]

(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.

Comment 61

[Benoit Girard (:BenWa)]

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

[Dave Garrett]

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

[u123541]

(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

[sjw]

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

Comment 65

[nemo]

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/

Comment 67

[Trevor Rowbotham [:rowbot]]

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

[Caspy7]

> 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/

Comment 70

[Andrea Marchesini [:baku]]

"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.

Comment 71

[Paul Adenot (:padenot)]

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.

Comment 72

[Benoit Girard (:BenWa)]

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.

Comment 73

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Just talked about this with Stephen, who might already have designs for this ready.

Comment 74

[(no longer active)]

(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.

Comment 75

[Justin Dolske [:Dolske]]

(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.

Comment 76

[(no longer active)]

Attachment: UX spec by shorlander

Comment 77

[(no longer active)]

Attachment: 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...  ;-)

Comment 78

[(no longer active)]

Attachment: Screenshot of the current WIP patch

Comment 79

[(no longer active)]

Attachment: Part 2: Implement a tooltip for the tab playing icon; r=dao

Comment 80

[(no longer active)]

Attachment: Part 3: Implement a context menu for muting and unmuting a tab

Using a dummy icon for now...

Comment 81

[(no longer active)]

Attachment: Screenshot of the muting menu, and the muted icon and tooltip

Comment 82

[(no longer active)]

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?

Comment 83

[(no longer active)]

David, what should be done here for accessibility?

Comment 84

[Dave Garrett]

(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

[Tim Nguyen :ntim]

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 ?

Comment 86

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(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...

Comment 87

[(no longer active)]

(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.)

Comment 88

[Tim Nguyen :ntim]

Attachment: 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

[Tim Nguyen :ntim]

Attachment: Styling patch

Comment 90

[Tim Nguyen :ntim]

(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

[Tim Nguyen :ntim]

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.

Comment 92

[(no longer active)]

(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 93

[(no longer active)]

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...

Comment 94

[(no longer active)]

Attachment: 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.

Comment 95

[David Bolter [:davidb] (NeedInfo me for attention)]

(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.

Comment 96

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(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!

Comment 97

[(no longer active)]

Dao: ping?

Comment 98

[Dão Gottwald [::dao]]

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.

Comment 99

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 100

[(no longer active)]

(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.

Comment 101

[Dão Gottwald [::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.

Comment 102

[(no longer active)]

(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.

Comment 103

[Stephen Horlander [:shorlander]]

Attachment: tab-audio.svg

Comment 104

[Stephen Horlander [:shorlander]]

Attachment: tab-audio-small.svg

Comment 105

[(no longer active)]

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.

Comment 106

[(no longer active)]

Attachment: 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.

Comment 107

[(no longer active)]

Attachment: Part 2: Implement a tooltip for the tab playing icon

Comment 108

[(no longer active)]

Attachment: Part 3: Implement a context menu for muting and unmuting a tab

Comment 109

[(no longer active)]

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

[Tim Nguyen :ntim]

(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

Comment 111

[(no longer active)]

(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...

Comment 112

[Marco Zehe (:MarcoZ)]

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.

Comment 113

[(no longer active)]

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

[Clément Lefèvre]

(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.

Comment 115

[(no longer active)]

(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!

Comment 116

[(no longer active)]

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.

Comment 117

[(no longer active)]

Attachment: Part 1: Provide a visual indicator for tabs that are playing audio

Comment 118

[(no longer active)]

Attachment: Part 2: Implement a tooltip for the tab playing icon

Comment 119

[(no longer active)]

Attachment: Part 3: Implement a context menu for muting and unmuting a tab

Comment 120

[(no longer active)]

Attachment: Part 4: Add automated tests

Comment 121

[Dão Gottwald [::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

[Tim Nguyen :ntim]

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]

Comment 123

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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.

Comment 124

[(no longer active)]

(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.

Comment 125

[Clément Lefèvre]

(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.

Comment 126

[Madhava Enros [:madhava]]

(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.

Comment 127

[Stephen Horlander [:shorlander]]

(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.

Comment 128

[(no longer active)]

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.

Comment 129

[(no longer active)]

Attachment: Part 1: Provide a visual indicator for tabs that are playing audio

Comment 130

[(no longer active)]

Attachment: Part 2: Implement a tooltip for the tab playing icon

Comment 131

[(no longer active)]

Attachment: Part 3: Allow muting and unmuting a tab by clicking on the playing icon

Comment 132

[(no longer active)]

Attachment: Part 4: Add automated tests

Comment 133

[Dão Gottwald [::dao]]

(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.

Comment 134

[Blake Winton (:bwinton) (:☕️)]

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

[Tim Nguyen :ntim]

(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).

Comment 136

[(no longer active)]

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.

Comment 137

[Dão Gottwald [::dao]]

(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.

Comment 138

[Dão Gottwald [::dao]]

(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.

Comment 139

[Stephen Horlander [:shorlander]]

(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.

Comment 140

[(no longer active)]

(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.

Comment 141

[Dão Gottwald [::dao]]

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.

Comment 142

[(no longer active)]

(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.

Comment 143

[Dão Gottwald [::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.

Comment 144

[Dão Gottwald [::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

Comment 145

[Dão Gottwald [::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

Comment 146

[(no longer active)]

(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.

Comment 147

[(no longer active)]

Attachment: Part 1: Provide a visual indicator for tabs that are playing audio

Comment 148

[(no longer active)]

Attachment: Part 2: Implement a tooltip for the tab playing icon

Comment 149

[(no longer active)]

Attachment: Part 3: Allow muting and unmuting a tab by clicking on the playing icon

Comment 150

[(no longer active)]

Attachment: Part 4: Add automated tests

Comment 151

[Tim Nguyen :ntim]

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.

Comment 152

[(no longer active)]

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!

Comment 153

[Dão Gottwald [::dao]]

(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 154

[Dão Gottwald [::dao]]

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.

Comment 155

[(no longer active)]

(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.

Comment 156

[(no longer active)]

(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.

Comment 157

[(no longer active)]

Attachment: Part 1: Provide a visual indicator for tabs that are playing audio

Comment 158

[(no longer active)]

Attachment: Part 3: Allow muting and unmuting a tab by clicking on the playing icon

Comment 159

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

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.

Comment 160

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

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.

Comment 161

[(no longer active)]

(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.

Comment 162

[(no longer active)]

(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?  :-)

Comment 163

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

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.

Comment 164

[Marco Bonardo [:mak]]

(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 165

[Dão Gottwald [::dao]]

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.

Comment 166

[Dão Gottwald [::dao]]

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

Comment 167

[Dão Gottwald [::dao]]

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

Comment 168

[(no longer active)]

(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.

Comment 169

[(no longer active)]

Attachment: Part 2: Implement a tooltip for the tab playing icon

Comment 170

[(no longer active)]

Attachment: Part 3: Allow muting and unmuting a tab by clicking on the playing icon

Comment 171

[Dão Gottwald [::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

Comment 172

[(no longer active)]

Note that I will remove the test for bug 405137 since it is now pointless without closetabtext.

Comment 173

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

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

Dao's review is sufficient here.

Comment 174

[Pulsebot]

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

Comment 175

[Carsten Book [:Tomcat]]

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

Comment 176

[Chris Peterson [:cpeterson]]

Tab audio indicators are probably a feature worth highlighting in the Firefox 42 release notes. :)

Comment 177

[Keul]

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.

Comment 178

[Jorge Villalobos [:jorgev] (he/him)]

Adding addon-compat flag because this changes tab structure and at least one add-on has been affected.

Comment 179

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 180

[Caspy7]

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?

Comment 181

[Vladan Djeric (:vladan)]

(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

Comment 182

[(no longer active)]

(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.

Comment 184

[(no longer active)]

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.

Comment 185

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

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.

Comment 186

[arni2033]

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

[u123541]

(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

Comment 188

[(no longer active)]

(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

[arni2033]

(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"

Comment 190

[Dave Garrett]

(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

[Tim Nguyen :ntim]

(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.

Comment 192

[(no longer active)]

(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.

Comment 193

[donrhummy]

Will this also be on Firefox Android?

Comment 194

[(no longer active)]

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

Yes, see bug 1190301.

Comment 195

[magicp]

Depends on: 1200236
Depends on: 1200422

Comment 196

[Ashickur Rahman]

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.

Comment 197

[Hossain Al Ikram [:ikram] (QA Contact)]

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

Comment 198

[Khalid Syfullah Zaman [:khalid32]]

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]

Comment 199

[Gilbert]

no audio indicator, if using themes.

Comment 200

[Loic]

(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.

Comment 201

[steve hayes]

I'm just saying thanks to the crew that came up with this long desired and much needed enhancement

Comment 202

[johnnelson]

I can't Buffer Music Which Was Downloaded from mozilla browser through this website
 https://spotifypremiumapkdld.com/
can I Get any solution?

Comment 203

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to johnnelson from comment #202)
> I can't Buffer Music Which Was Downloaded from mozilla browser through this
> website
>  https://spotifypremiumapkdld.com/
> can I Get any solution?

Please go to our support site at https://support.mozilla.org.