Closed
Bug 486262
Opened 14 years ago
Closed 8 years ago
Provide visual indicator as to which tab is causing sound
Categories
(Firefox :: Tabbed Browser, enhancement)
Firefox
Tabbed Browser
Tracking
()
VERIFIED
FIXED
Firefox 42
Tracking | Status | |
---|---|---|
firefox42 | --- | verified |
People
(Reporter: marun2, Assigned: ehsan.akhgari)
References
(Depends on 7 open bugs, Blocks 4 open bugs, )
Details
(Keywords: addon-compat, Whiteboard: [parity-Chrome])
Attachments
(7 files, 23 obsolete files)
404.12 KB,
image/png
|
Details | |
2.21 KB,
image/svg+xml
|
Details | |
2.14 KB,
image/svg+xml
|
Details | |
6.16 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
20.88 KB,
patch
|
jaws
:
review+
dao
:
review+
|
Details | Diff | Splinter Review |
9.76 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
7.76 KB,
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
We only know if a plugin is used but we don't know if this plugin plays sound or not. We know if a resource with audio/* mime-type is loaded but that is incomplete. We are unable to detect if the flash plugin plays sound as example because the sound is embedded into the flash file. We also don't know if a video/* file contains sound or not.
Reporter | ||
Comment 2•14 years ago
|
||
So it cant be done by the browser then? I would like a way to get this done so that no music plays unless the particular tab has focus, and an option in the browser to stop all sounds emanating from a particular tab in one kill operation, regardless of its source.
Comment 3•13 years ago
|
||
see bug 334987
Comment 5•13 years ago
|
||
Can we add options like turning off all sound except the foreground tab?
Comment 6•13 years ago
|
||
see bug 334987 (as told in comment 3)
Updated•12 years ago
|
Version: unspecified → Trunk
Comment 7•12 years ago
|
||
This is something I'd love to see us do for html5 audio and video elements, even though the flash/plugin case isn't possible per tab. In theory this is doable at the level of the media element and decoder with some work. I'm not sure about getting the UI done. The simplest case would be mute/unmute a document, where the document reveals the collective state of its child media elements. Moving to Core : Video/Audio to get this in front of more people. We might have other bugs for this that are further along. I couldn't find any, which isn't the same as saying they don't exist. Also CC'ing Blair, who I've talked to about this with in the past.
Status: UNCONFIRMED → NEW
Component: General → Video/Audio
Ever confirmed: true
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → video.audio
Hardware: x86 → All
Comment 8•12 years ago
|
||
Sounds similar to bug 626586.
Comment 10•12 years ago
|
||
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•12 years ago
|
||
No, that was probably an add-on.
Comment 12•12 years ago
|
||
no it was not .
Comment 13•12 years ago
|
||
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•12 years ago
|
||
This bug should be WONTFIXed in favor of fixing the issue thay causes this, background tabs playing automatically.
Comment 15•12 years ago
|
||
(In reply to comment #13) > Same problem as I noted in bug 626586 -- users don't care _how_ sounds are > generated, so we shouldn't do this if the result is seemingly-inconsistent > behavior (due to plugins playing sounds). I certainly like the idea, though... > I just don't know how this could be implemented in a way that makes it natural > to only get the UI for pages using HTML5 media. :( How about making this a user setting then, disabled by default? It can’t possibly be made to work for plugins, but surely implementing this for <video> and <audio> is better than nothing.
Comment 16•12 years ago
|
||
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•12 years ago
|
||
I want to allow my background tabs to play sound but I want to be able to visually identify which one is generating it.
Comment 18•12 years ago
|
||
We just can't fix this because, today, most of the audio is from Flash. Here is 2 different ideas: 1. What if we provide an API (or events) to the content: window.ImGeneratingSound(); window.ImNotGeneratingSoundAnyMore(); With some evangelism, it could work. If Video providers (youtube & co) and Music providers (Pandora & co) support it, and if we provide a little ActionScript code that binds these to Flash, that would solve this problem. 2. We talk to Adobe and ask them to fire an event from the Flash plugin when some audio is been played. (Also, we could also send events when the user use the multimedia keys (play, pause, next song, previous song, ...) even if the document and Firefox are not focused. But this is another story)
Comment 19•12 years ago
|
||
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•12 years ago
|
||
> 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•12 years ago
|
||
(In reply to comment #19) > But there are lot of other plugins wich can generate sound (eg. Silverlight, > WMP, Quicktime, Realplayer, Java, etc), and lot of other webpages... It won't > work properly until every single plugin does not notify the host browser about > the sound. afaik, the large majority of the Audio content on the web is Flash, and hopefully soon, HTML5 Audio. We don't have to cover 100% of the different scenarios, and I guess, Flash + HTML5 cover 99%.
Comment 22•12 years ago
|
||
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•12 years ago
|
||
Since this can't be done automatically (we can't know if Flash content will play some audio), I think we should let the Web Developer call himself such functions. Implementing an automatic notification from the <audio> tag is not really useful. We should just define and implement an API and let people know how to use it. Does it make sense?
Comment 24•12 years ago
|
||
(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•12 years ago
|
||
(In reply to comment #19) > But there are lot of other plugins wich can generate sound (eg. Silverlight, > WMP, Quicktime, Realplayer, Java, etc), and lot of other webpages... It won't > work properly until every single plugin does not notify the host browser about > the sound. 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•10 years ago
|
||
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•10 years ago
|
||
(In reply to David Humphrey (:humph) from comment #27) > Looks like Chrome is working on doing this for html5 media ...and Flash as well as other Pepper-based plugins.
Comment 29•10 years ago
|
||
(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•10 years ago
|
||
Haven't seen this suggested yet: Tools->Page Info->Sound (or as part of Media) displaying a list of tabs currently containing the possibility of generating sound and a "switch to tab" link. Usually, the sound is familiar to the user, so [s]he would have a real good chance of finding the sound generating tab quickly. This would be a reasonable solution (one I'd be happy with) until a better one is developed...
![]() |
||
Comment 33•10 years ago
|
||
It's now on Google Chrome Beta. http://chrome.blogspot.ca/2013/11/track-down-those-noisy-tabs.html
Whiteboard: [parity-Chrome]
Comment 34•10 years ago
|
||
Can I ask where are we at on this? Chrome will now have this out of the box (without a need for site developers to do anything). Am I right that we could implement this now if we only had to manage HTML audio & video (and not plugins)? Comment 18 suggests talking with Adobe to just have Flash notify us of playing audio. We're not going to implement Pepper, so this would seem a prudent step. Has anyone contacted Adobe? On the subject of *other* plugins, are we able to detect when a plugin is loaded into a particular tab? If so that could be how we round up stragglers in a potential audio offenders window. Finally, not to distract, but if Shumway should actually save the day, perhaps this will all become a memory.
Comment 35•10 years ago
|
||
Bug 923247 gave us notifications that we can use to solve this for the non-plugin case.
Comment 36•10 years ago
|
||
http://imgur.com/zxCt0Ol
Comment 37•10 years ago
|
||
This has landed in Chrome stable: http://chrome.blogspot.com/2014/01/everyone-can-now-track-down-noisy-tabs.html
Comment 38•10 years ago
|
||
>On the subject of *other* plugins, are we able to detect when a plugin is loaded into a particular tab? If so that could be how we round up stragglers in a potential audio offenders window. This is essentially the route that I took with the MuteTab Chrome extension. http://www.mutetab.com/. If someone wants to play with this at the extension level, take a look at the source code (http://www.github.com/jaredsohn/mutetab/.) I am now building it for Chrome with trigger.io's OpenForge so with some work (start by grepping the source for 'chrome') it could be ported in a cross platform way to also work for Firefox.
Comment 39•10 years ago
|
||
As far as I can gather from this bug, currently the only way to properly detect playing audio (for most cases) is for Adobe to work with us allowing Flash to communicate its play state. Assuming we've approached them and they will not do this in the near future, why not create an API like this for HTML audio/video as well as indicate which tabs have plugins present? This would allow addon authors to create a solution that is better than nothing. Bonus addon name suggestion: 'Zero In' (or with excessive subtlety: 0N)
Comment hidden (me-too) |
Comment 41•9 years ago
|
||
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•9 years ago
|
||
(In reply to Caspy7 from comment #39) > As far as I can gather from this bug, currently the only way to properly > detect playing audio (for most cases) is for Adobe to work with us allowing > Flash to communicate its play state. > > Assuming we've approached them and they will not do this in the near future, > why not create an API like this for HTML audio/video as well as indicate > which tabs have plugins present? > This would allow addon authors to create a solution that is better than > nothing. > > Bonus addon name suggestion: 'Zero In' (or with excessive subtlety: 0N) Chrome does this in some way. Which means it's possible without any kind of Adobe API.
Comment 43•9 years ago
|
||
Chrome ships with a customized version of Flash (using Pepper), which added the requisite support for per-tab indicators.
Comment 44•9 years ago
|
||
So, what are the solutions?: 1° We don't do anything, after all, we don't care about witch tab is causing sound: http://www.commitstrip.com/en/2013/11/12/la-feature-que-lon-attend-depuis-15-ans/ 2° Like google, we developp our flash plugin, knowing that: many video use it to play DRM video, witch is not possible with DRMs, that it will take time and won't be perfect like our js pdf viewer 3° We don't care about flash and provide indicator for HTML5 <video>/</audio> sound (and even maybe high CPU/memory usage that slow down the computer). For flash, if user have "Click to play" enabled, we can show a "plugin" icon in tab.
Comment hidden (off-topic) |
Comment 46•9 years ago
|
||
(In reply to Caspy7 from comment #39) > As far as I can gather from this bug, currently the only way to properly > detect playing audio (for most cases) is for Adobe to work with us allowing > Flash to communicate its play state. > > Assuming we've approached them and they will not do this in the near future, > why not create an API like this for HTML audio/video as well as indicate > which tabs have plugins present? > This would allow addon authors to create a solution that is better than > nothing. > > Bonus addon name suggestion: 'Zero In' (or with excessive subtlety: 0N) Rather "Zero Out" would make sense, no ?
Comment 47•9 years ago
|
||
Is there any reason my suggestion in comment 30 can't be an interim (or final) solution in the absence of Adobe cooperation? Is Flash the only source of audio?
Comment 48•9 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #43) > Chrome ships with a customized version of Flash (using Pepper), which added > the requisite support for per-tab indicators. What about Shumway ?
Comment hidden (me-too) |
Comment hidden (advocacy) |
Comment 52•9 years ago
|
||
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•9 years ago
|
||
No, it's like the Play-icon in Youtube-tabs.
Comment 54•9 years ago
|
||
By way of a bump, I'd just add that in my opinion supporting this for HTML5 audio/video is what we should do. Flash is on the way out due to mobile, so throwing support behind this, providing an API for webdevs, and informing Adobe that it'd be great if they'd call said API, would be a big win. I'd think that it might even be a slight nudge to some to use HTML5 solutions instead of flash, though it could of course be a nudge in the other direction for a certain class of netizen as well.
Comment hidden (me-too) |
Comment hidden (me-too) |
Comment 57•9 years ago
|
||
I'm looking for this feature too and added thread on Flash Player forum https://forums.adobe.com/message/7049842#7049842 Maybe if people will concern about it Adobe wil add sound indicator feature (API, tag playsound=TRUE/FALSE) and it could be implmented in Firefox.
Comment 58•9 years ago
|
||
On the platform side, we're aggressively pursuing a number of moves to use native audio/video for things currently served with flash (MSE, shumway, etc). Would the visual indicator be hard to implement? If not, it seems like we should put it in the UI, so that we'll automatically benefit from it for every piece of content we get running natively. A friend in China has told me on several occasions that the chinese web is very audio-heavy, and that this is the reason he uses Chrome. :-(
Flags: needinfo?(dolske)
Comment 59•9 years ago
|
||
Bug 923247 apparently implemented the backed to make the UI possible, so someone could prototype this today as an addon. I don't think the UI should be that difficult (as least so far as a simple indicator to expose what 923247 provides). The problem is what I noted in comment 13. So long as Flash remains prevalent (and dare I say more widely used, especially for annoying things), an indicator that's only for HTML5 won't help a lot of the time, and leads to users seeing inconsistent UI. A UI that only works some of the time can feel pretty broken and frustrating, and actually be worse than no UI at all.
Flags: needinfo?(dolske)
Comment 60•9 years ago
|
||
(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•9 years ago
|
||
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•9 years ago
|
||
With click-to-play enabled there are fewer tabs expected to have Flash running at all. To deal with Flash audio, a separate icon to indicate running Flash (or other plugins) could be applied to these tabs. It wouldn't specifically indicate audio, but it would be sufficient to help find the offending tab if there is an issue. (e.g. if you hear sound you don't want coming from a background tab, it's either the one we know has audio, or one of the ones you allowed Flash on) Alternatively, such additional icons could be show on tabs only when there's enough space and then only shown in the drop-down list of all tabs when the bar gets full.
Comment 63•9 years ago
|
||
(In reply to Dave Garrett from comment #62) > With click-to-play enabled there are fewer tabs expected to have Flash > running at all. To deal with Flash audio, a separate icon to indicate > running Flash (or other plugins) could be applied to these tabs. Reminder: the discussion centers around TABS; but please don't forget that some of us run multiple windows with lots of tabs each; so an indicator as to which window(s) is/are generating sound should be part of the solution. In my case, I always have 9 to 11+ FF windows1[1] open with anywhere from 1[2] to nearly 200[3] tabs each (507 tabs as I write this). [1] Too many windows to raise/lower, so I use KDE's Window of windows grouping feature -- all these windows look like tabs in a single grouping window. [2] some websites manage to open a new window with no tabs/toolbars; even with "Open new windows in a new tab instead" set -- although sound is rare on these, they could have sound without tabs. [3] As I write this, 507 tabs over 11 windows -- so I'm VERY interested in this issue. On other desktops, I have: - a virtually duplicate FF (without add-ons) for debugging (no sound -- not enabled from other userids) - Windows-FF/Wine on main userid which allows sound and separate .mozilla/* under .wine Being able to see which tabs _and_ windows are generating sound is what I'm looking for -- just being explicit rather than hoping it's implicit... :)
![]() |
||
Comment 64•8 years ago
|
||
It seems that many users are missing this feature. See discussion/replys on http://gizmodo.com/but-this-tho-1685659126
Depends on: 1018504
![]() |
||
Comment 65•8 years ago
|
||
These might help a bit in the interim: https://addons.mozilla.org/en-US/firefox/addon/noise-control/ https://addons.mozilla.org/en-US/firefox/addon/youtube-autoplay-stopper/
Comment 67•8 years ago
|
||
According to the release notes[1] for Flash 18, it now contains the necessary APIs to make this work with Flash. Taken from [1]: > Audio APIs added to Flash Player NPAPI > We have added the ability for NPAPI compatible browsers and applications to query the player and > detect if audio is currently being played and the ability to mute. This new feature will allow > applications to alert the user if audio is being played and give them the ability to mute, even if the > Flash content does not. [1] http://labsdownload.adobe.com/pub/labs/flashruntimes/shared/air18_flashplayer18_releasenotes.pdf
Comment 68•8 years ago
|
||
> According to the release notes[1] for Flash 18, it now contains the > necessary APIs to make this work with Flash. Wow, that is great news! (And surprising!) === For those not clicking all links (I don't), the first one in comment 65 is to the Noise Control addon, which is essentially an addon that could mark this bug as fixed [if it were in the browser]. Noise Control places an audio indicator in tabs with audio-playing HTML5 video or audio (just like Chrome). Clicking the indicator mutes the audio. Now that Youtube is sending HTML5 video to Firefox for Windows Vista and above, this covers a lot of users as well as influencing the rest of the Net to relegate Flash. The addon also inserts a Flash/plugin indicator in tabs with plugins loaded. So we can consider this addon a proof of concept, if not the beginnings of the code that could go into the browser. Now that Adobe has made the proper Flash API available this will cover the audio for most users! If you'd like this functionality now (with HTML video) you can get the addon here: https://addons.mozilla.org/en-US/firefox/addon/noise-control/
Comment 70•8 years ago
|
||
"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•8 years ago
|
||
Philipp, doslke, we can now implement this for flash audio as well as normal HTML audio. Would that be something we'd be interested in? As mentioned earlier this has been prototyped using a jetpack addon (https://addons.mozilla.org/en-US/firefox/addon/noise-control/), which I found to be very reliable (in fact, perfectly reliable). It's built upon the Gecko API baku wrote somewhat recently, of course. I was thinking it could be a fun thing to work on all together at Whistler.
Flags: needinfo?(philipp)
Flags: needinfo?(dolske)
Comment 72•8 years ago
|
||
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•8 years ago
|
||
Just talked about this with Stephen, who might already have designs for this ready.
Flags: needinfo?(philipp)
Assignee | ||
Comment 74•8 years ago
|
||
(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•8 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #71) > Philipp, doslke, we can now implement this for flash audio as well as normal > HTML audio. Would that be something we'd be interested in? Yes! > I was thinking it could be a fun thing to work on all together at Whistler. Fantastic idea.
Flags: needinfo?(dolske)
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 76•8 years ago
|
||
Assignee | ||
Comment 77•8 years ago
|
||
Dao, can you please provide some feedback here? I'm not implementing the muting or the context menu for now. One problem that I ran into which I don't know how to solve is how to overlay the icon on top of the tab icon for pinned tabs. I tried to use a trick similar to the one used for .tab-icon-overlay but that doesn't seem to work. Also, obviously I'm using the wrong icons for now... ;-)
Attachment #8629603 -
Flags: feedback?(dao)
Assignee | ||
Comment 78•8 years ago
|
||
Assignee | ||
Comment 79•8 years ago
|
||
Assignee | ||
Comment 80•8 years ago
|
||
Using a dummy icon for now...
Assignee | ||
Comment 81•8 years ago
|
||
Assignee | ||
Comment 82•8 years ago
|
||
One scenario should be clarified in the UX spec: When the user mutes a tab, what should happen when the audio playback stops? Should the mute state (and icon) persist? What about when we browse to a new page? Should the mute state (and icon) persist?
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ehsan
Assignee | ||
Comment 83•8 years ago
|
||
David, what should be done here for accessibility?
Flags: needinfo?(dbolter)
Assignee | ||
Updated•8 years ago
|
Component: Video/Audio → Tabbed Browser
Product: Core → Firefox
Comment 84•8 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #82) > What about when we browse to a new page? Should the > mute state (and icon) persist? Just to chime in on this specific case: I think the state needs to persist, at least. YouTube, for example, can have multiple videos in a playlist on different pages, switching to the next page at the end of each. If a user directed Firefox to mute that tab, it should do so regardless of playlists or anything else the site does. You could argue that the state should persist per-domain, but per-tab-session makes sense too.
Comment 85•8 years ago
|
||
Just attempted to test this on my computer. Applied the patches from bug 1180347, bug 1180421, bug 1180448 and this bug then did a full build. The indicator didn't show up for me unfortunately (testcases : HTML5 youtube and http://nt1m.github.io/HTML5AudioPlayer/ ). e10s doesn't seem to matter. Is there a dependency I'm missing here ?
Flags: needinfo?(ehsan)
Comment 86•8 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #82) > One scenario should be clarified in the UX spec: When the user mutes a tab, > what should happen when the audio playback stops? Should the mute state > (and icon) persist? What about when we browse to a new page? Should the > mute state (and icon) persist? The tab should stay muted (otherwise, stuff like switching from one video to another might bypass the mute?) There should probably be a distinct icon state for that case though...
Flags: needinfo?(philipp)
Assignee | ||
Comment 87•8 years ago
|
||
(In reply to Tim Nguyen [:ntim] from comment #85) > Just attempted to test this on my computer. Applied the patches from bug > 1180347, bug 1180421, bug 1180448 and this bug then did a full build. The > indicator didn't show up for me unfortunately (testcases : HTML5 youtube and > http://nt1m.github.io/HTML5AudioPlayer/ ). e10s doesn't seem to matter. Is > there a dependency I'm missing here ? You need to set the media.useAudioChannelService pref to true in order to test this with the patches attached to these bugs. The UI bits should all work fine in e10s and non-e10s. (In reply to Philipp Sackl [:phlsa] please use needinfo from comment #86) > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, > needinfo? me!) from comment #82) > > One scenario should be clarified in the UX spec: When the user mutes a tab, > > what should happen when the audio playback stops? Should the mute state > > (and icon) persist? What about when we browse to a new page? Should the > > mute state (and icon) persist? > > The tab should stay muted (otherwise, stuff like switching from one video to > another might bypass the mute?) > There should probably be a distinct icon state for that case though... Just to confirm my understanding, that means that the muted icon should continue to display in the tab, right clicking it should let you unmute the tab, and once the tab has been unmuted, the icon will go back to playing if the tab is playing audio, or disappear if it's not playing audio. Is that correct? FWIW, I agree that this is probably the more desirable behavior than what my patches implement right now (the muting will only work until the next document has been navigated to.)
Flags: needinfo?(ehsan) → needinfo?(philipp)
Comment 88•8 years ago
|
||
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•8 years ago
|
||
Attachment #8629691 -
Attachment is obsolete: true
Comment 90•8 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #87) > (In reply to Tim Nguyen [:ntim] from comment #85) > > Just attempted to test this on my computer. Applied the patches from bug > > 1180347, bug 1180421, bug 1180448 and this bug then did a full build. The > > indicator didn't show up for me unfortunately (testcases : HTML5 youtube and > > http://nt1m.github.io/HTML5AudioPlayer/ ). e10s doesn't seem to matter. Is > > there a dependency I'm missing here ? > > You need to set the media.useAudioChannelService pref to true in order to > test this with the patches attached to these bugs. The UI bits should all > work fine in e10s and non-e10s. Thanks ! worked great for me. Here's some feedback : - The context menu with mute/unmute should probably appear when left clicking the tab audio icon - That context menu item could also be added to the normal tab context menu
Comment 91•8 years ago
|
||
Comment on attachment 8629692 [details] [diff] [review] Styling patch This still needs some love for [brighttext] themes (devedition dark theme, and dark lw-themes) and Windows High contrast themes. But the overall styling is there.
Assignee | ||
Comment 92•8 years ago
|
||
(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.
Assignee | ||
Comment 93•8 years ago
|
||
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...
Assignee | ||
Comment 94•8 years ago
|
||
The previous patch had a bug where no matter which icon you clicked on, the current tab would be muted/unmuted. This version of the patch fixes this. With this, and the rest of the patches in the blocking bugs, everything seems to be working well according to my local tests. I'll still need to get some feedback on my front-end changes, and also write tests for the front-end bits. I'll focus on the tests when I have received the initial code feedback.
Attachment #8629620 -
Attachment is obsolete: true
Comment 95•8 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #83) > David, what should be done here for accessibility? Thanks for the ping. We'll need to ensure visibility as per comment 91. We'll want keyboard-only users to be able to operate the functionality. MarcoZ can assess for screen reader usage.
Flags: needinfo?(dbolter) → needinfo?(mzehe)
Comment 96•8 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #87) > (In reply to Philipp Sackl [:phlsa] please use needinfo from comment #86) > > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, > > needinfo? me!) from comment #82) > > > One scenario should be clarified in the UX spec: When the user mutes a tab, > > > what should happen when the audio playback stops? Should the mute state > > > (and icon) persist? What about when we browse to a new page? Should the > > > mute state (and icon) persist? > > > > The tab should stay muted (otherwise, stuff like switching from one video to > > another might bypass the mute?) > > There should probably be a distinct icon state for that case though... > > Just to confirm my understanding, that means that the muted icon should > continue to display in the tab, right clicking it should let you unmute the > tab, and once the tab has been unmuted, the icon will go back to playing if > the tab is playing audio, or disappear if it's not playing audio. Is that > correct? > > FWIW, I agree that this is probably the more desirable behavior than what my > patches implement right now (the muting will only work until the next > document has been navigated to.) Yep! Exactly!
Flags: needinfo?(philipp)
Assignee | ||
Comment 97•8 years ago
|
||
Dao: ping?
Depends on: 1182164
Comment 98•8 years ago
|
||
Comment on attachment 8629603 [details] [diff] [review] Provide a visual indicator for tabs that are playing audio (WIP) (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #77) > Created attachment 8629603 [details] [diff] [review] > Provide a visual indicator for tabs that are playing audio (WIP) > > Dao, can you please provide some feedback here? I'm not implementing the > muting or the context menu for now. One problem that I ran into which I > don't know how to solve is how to overlay the icon on top of the tab icon > for pinned tabs. I tried to use a trick similar to the one used for > .tab-icon-overlay but that doesn't seem to work. If you want to follow tab-icon-overlay (which I think you should), you'll actually need to reuse that element, i.e. add another state to that along with "crashed". If you want this to be spawn a menu, you'll also need to think about keyboard access. >+ <handler event="DOMMediaPlaybackStarted"> This will need some e10s love.
Attachment #8629603 -
Flags: feedback?(dao) → feedback-
Comment 99•8 years ago
|
||
(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.
Assignee | ||
Comment 100•8 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #98) > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, > needinfo? me!) from comment #77) > > Created attachment 8629603 [details] [diff] [review] > > Provide a visual indicator for tabs that are playing audio (WIP) > > > > Dao, can you please provide some feedback here? I'm not implementing the > > muting or the context menu for now. One problem that I ran into which I > > don't know how to solve is how to overlay the icon on top of the tab icon > > for pinned tabs. I tried to use a trick similar to the one used for > > .tab-icon-overlay but that doesn't seem to work. > > If you want to follow tab-icon-overlay (which I think you should), you'll > actually need to reuse that element, i.e. add another state to that along > with "crashed". I thought about that. The issue is that for the non-pinned case, the behavior is different than tab-icon-overlay (the icon is put next to the tab close icon, it doesn't overlay the favicon, and it lines up with the favicon, the text and the close button vertically), but for the pinned case, the behavior is exactly similar to tab-icon-overlay. What makes this difficult to handle is that the element needs to live in a different position in the DOM for those two cases. I thought about moving it, but I'm not sure if that's OK since nothing else in the tab moves in the DOM as far as I can tell. Is moving it in the DOM okay, or do you prefer something else? > If you want this to be spawn a menu, you'll also need to think about > keyboard access. I talked to davidb about this. Since the only item in the context menu is for toggling the mute state, he said that we should probably expose that as a keyboard shortcut and not worry about making the menu item keyboard accessible, which is on par with the keyboard accessibility of the tab close icon, for example. Do you mind if the keyboard shorcut moves to its own bug? The implementation is very simple, but we need to decide what keyboard shortcut to use! > >+ <handler event="DOMMediaPlaybackStarted"> > > This will need some e10s love. It works well in e10s, as mconley mentioned.
Flags: needinfo?(dao)
Comment 101•8 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #100) > I thought about that. The issue is that for the non-pinned case, the > behavior is different than tab-icon-overlay (the icon is put next to the tab > close icon, it doesn't overlay the favicon, and it lines up with the > favicon, the text and the close button vertically), but for the pinned case, > the behavior is exactly similar to tab-icon-overlay. It might be worth reconsidering the non-pinned case to use the same approach as pinned tabs. Our tabs get as narrow as 100px at which point we only show five to six characters of their label (two to three for the selected tab). With another icon in there, we'd drop another ~three character, making those tab labels close to useless. Besides I think having the indicator in one place is desirable in itself in terms of UX consistency. > What makes this difficult to handle is that the element needs to live in a > different position in the DOM for those two cases. I thought about moving > it, but I'm not sure if that's OK since nothing else in the tab moves in the > DOM as far as I can tell. Is moving it in the DOM okay, or do you prefer > something else? You could use two DOM nodes and show only the one you want. But like I said above there a more fundamental UX questions. > Do you mind if the keyboard shorcut moves to its own bug? The > implementation is very simple, but we need to decide what keyboard shortcut > to use! Fine as long as we get it done for the same release.
Flags: needinfo?(dao)
Assignee | ||
Comment 102•8 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #101) > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, > needinfo? me!) from comment #100) > > I thought about that. The issue is that for the non-pinned case, the > > behavior is different than tab-icon-overlay (the icon is put next to the tab > > close icon, it doesn't overlay the favicon, and it lines up with the > > favicon, the text and the close button vertically), but for the pinned case, > > the behavior is exactly similar to tab-icon-overlay. > > It might be worth reconsidering the non-pinned case to use the same approach > as pinned tabs. Our tabs get as narrow as 100px at which point we only show > five to six characters of their label (two to three for the selected tab). > With another icon in there, we'd drop another ~three character, making those > tab labels close to useless. > > Besides I think having the indicator in one place is desirable in itself in > terms of UX consistency. That's a good point. Phillip, what do you think? > > What makes this difficult to handle is that the element needs to live in a > > different position in the DOM for those two cases. I thought about moving > > it, but I'm not sure if that's OK since nothing else in the tab moves in the > > DOM as far as I can tell. Is moving it in the DOM okay, or do you prefer > > something else? > > You could use two DOM nodes and show only the one you want. But like I said > above there a more fundamental UX questions. OK, thanks, I will wait for UX input and pick one of the approaches based on that. > > Do you mind if the keyboard shorcut moves to its own bug? The > > implementation is very simple, but we need to decide what keyboard shortcut > > to use! > > Fine as long as we get it done for the same release. Yes, of course. Note that this functionality is hidden behind a pref so we will only toggle it when this and the other dependencies are ready.
Flags: needinfo?(philipp)
Comment 103•8 years ago
|
||
Flags: needinfo?(shorlander)
Comment 104•8 years ago
|
||
Assignee | ||
Comment 105•8 years ago
|
||
Comment on attachment 8629692 [details] [diff] [review] Styling patch I have incorporated the changes that this patch made in a better way in my patches locally.
Attachment #8629692 -
Attachment is obsolete: true
Assignee | ||
Comment 106•8 years ago
|
||
New versions of the patches, using the new icons. So far, fully implement what I know of the UX spec, pending a few clarifications from the UX team.
Attachment #8629603 -
Attachment is obsolete: true
Attachment #8629618 -
Attachment is obsolete: true
Attachment #8629742 -
Attachment is obsolete: true
Assignee | ||
Comment 107•8 years ago
|
||
Assignee | ||
Comment 108•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8629604 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8629621 -
Attachment is obsolete: true
Assignee | ||
Comment 109•8 years ago
|
||
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•8 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #109) > Try server builds for this feature for all desktop platforms will be > available here in the next few hours: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b5af25165f > > These builds can be used to test the full UI in action. Looks really nice ! Here are a few issues I have found though : - The icon is hardly visible on a pinned tab if the site has a dark favicon (I'd suggest adding a white background around it as the last mockup (in the UX spec) shows) - The icon doesn't support dark themes (lw-themes and devedition), it should turn white on background tabs for a dark theme - The icon has an hover and active state, but clicking doesn't do anything, which is quite misleading
Assignee | ||
Comment 111•8 years ago
|
||
(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•8 years ago
|
||
I've tried to see if I can get *any* kind of different behaviors with tabs with this try build, but am unable to hear a difference with my screen reader. I am not even sure I'm getting the UI, since when I open a few tabs after the browser starts, and one of them contains, for example, a Youtube video, switching through the tab bar doesn't give me any different speech. Moreover, the context menu items for each tab don't show that this tab could be muted, even on the tab where the video is playing. Since I have noone to visually verify at the moment that I am getting the UI at all, I am not sure whether I am just not seeing it, or whether the UI is there, but doesn't have any accessibility information for screen reader users. Note that I'll be on PTO for the next two weeks, so best to continue working on this with davidb in the meantime.
Flags: needinfo?(mzehe) → needinfo?(dbolter)
Assignee | ||
Comment 113•8 years ago
|
||
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•8 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #113) > Yes, the context menu is separate from the normal tab context menu, and > currently only accessible by right clicking on the icon in the UI, so this > is also expected. We will add a keyboard shortcut for toggling the > mute/unmute state in the current tab. Does that mean you can't mute a tab "in advance", because you know it will be noisy but it is not yet? I think it could be nice to be able to mute a tab before it even begin to produce any noise.
Assignee | ||
Comment 115•8 years ago
|
||
(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!
Assignee | ||
Comment 116•8 years ago
|
||
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.
Assignee | ||
Comment 117•8 years ago
|
||
Attachment #8633860 -
Attachment is obsolete: true
Attachment #8633862 -
Attachment is obsolete: true
Attachment #8633863 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8635803 -
Flags: review?(dao)
Assignee | ||
Comment 118•8 years ago
|
||
Assignee | ||
Comment 119•8 years ago
|
||
Assignee | ||
Comment 120•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8635804 -
Flags: review?(dao)
Assignee | ||
Updated•8 years ago
|
Attachment #8635805 -
Flags: review?(dao)
Assignee | ||
Updated•8 years ago
|
Attachment #8635806 -
Flags: review?(dao)
Comment 121•8 years ago
|
||
(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•8 years ago
|
||
Comment on attachment 8635803 [details] [diff] [review] Part 1: Provide a visual indicator for tabs that are playing audio Review of attachment 8635803 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/tabs.inc.css @@ +108,5 @@ > +} > + > +.tab-icon-overlay[playing][pinned]:hover { > + /* This rule must be important in order to override the more specific one in devedition.inc.css. */ > + list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio-small.svg#tab-audio-hover") !important; Instead of setting !important, you can simply use :not(:hover):not(:active) in the devedition.inc.css rule @@ +110,5 @@ > +.tab-icon-overlay[playing][pinned]:hover { > + /* This rule must be important in order to override the more specific one in devedition.inc.css. */ > + list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio-small.svg#tab-audio-hover") !important; > + border: 1px solid transparent; > + border-radius: 8px; Maybe it's better to apply both of these border properties on the normal state ? It would reduce code duplication, and would avoid reflows when hovering the icon. @@ +116,5 @@ > +} > + > +.tab-icon-overlay[playing][pinned]:active { > + /* This rule must be important in order to override the more specific one in devedition.inc.css. */ > + list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio-small.svg#tab-audio-pressed") !important; Same comment as above @@ +122,5 @@ > + border-radius: 8px; > + background-color: white; > +} > + > +.tab-icon-overlay[playing][pinned]:-moz-lwtheme { The :-moz-lwtheme selector doesn't cover Windows and Linux High Contrast themes. I recommend using the brighttext attribute instead, covers all of these cases + DevEdition : #TabsToolbar[brighttext] .tab-icon-overlay[playing][pinned]
Comment 123•8 years ago
|
||
Sorry for the super-late response, but you're already making all the right decisions :) As for the hover effect: Could we just make a left click on the icon mute/unmute the tab directly? That would make that functionality more discoverable as well.
Flags: needinfo?(philipp)
Assignee | ||
Comment 124•8 years ago
|
||
(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #123) > As for the hover effect: Could we just make a left click on the icon > mute/unmute the tab directly? That would make that functionality more > discoverable as well. Yes, that is definitely possible. But should I keep the context menu around as well? Also, are you and Stephen fine with switching to only using icons that overlay the tab icon? Dao in comment 121 is asking me to do that, but that would kind of ignore the current UX spec.
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
Comment 125•8 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #115) > (In reply to Clément Lefèvre from comment #114) > > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, > > needinfo? me!) from comment #113) > > > Yes, the context menu is separate from the normal tab context menu, and > > > currently only accessible by right clicking on the icon in the UI, so this > > > is also expected. We will add a keyboard shortcut for toggling the > > > mute/unmute state in the current tab. > > > > Does that mean you can't mute a tab "in advance", because you know it will > > be noisy but it is not yet? > > Yes. > > > I think it could be nice to be able to mute a tab before it even begin to > > produce any noise. > > Please file a follow-up for that, so that we can discuss it with the UX > team. Thanks! I filled it here https://bugzilla.mozilla.org/show_bug.cgi?id=1185502 Making this one depends on it.
Depends on: 1185502
Comment 126•8 years ago
|
||
(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•8 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #124) > Also, are you and Stephen fine with switching to only using icons that > overlay the tab icon? Dao in comment 121 is asking me to do that, but that > would kind of ignore the current UX spec. I would rather keep it on the tab where we will often have more room and we won't have to worry about overlaying things. I don't think overlaying the tab/favicon is optimal but makes sense in the pinned tab case where we don't have any other real estate to work with.
Flags: needinfo?(shorlander)
Assignee | ||
Comment 128•8 years ago
|
||
Comment on attachment 8635805 [details] [diff] [review] Part 3: Implement a context menu for muting and unmuting a tab Speaking today on IRC, Stepehen asked me to change the muting behavior to be triggered by left clicking on the icon instead of using the context menu. This will change part 3 and part 4 patches, the rest should be ready for review.
Attachment #8635805 -
Attachment is obsolete: true
Attachment #8635805 -
Flags: review?(dao)
Assignee | ||
Updated•8 years ago
|
Attachment #8635806 -
Attachment is obsolete: true
Attachment #8635806 -
Flags: review?(dao)
Assignee | ||
Comment 129•8 years ago
|
||
Attachment #8635803 -
Attachment is obsolete: true
Attachment #8635804 -
Attachment is obsolete: true
Attachment #8635803 -
Flags: review?(dao)
Attachment #8635804 -
Flags: review?(dao)
Assignee | ||
Comment 130•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8636948 -
Flags: review?(dao)
Assignee | ||
Comment 131•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8636949 -
Flags: review?(dao)
Assignee | ||
Comment 132•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8636950 -
Flags: review?(dao)
Assignee | ||
Updated•8 years ago
|
Attachment #8636951 -
Flags: review?(dao)
Assignee | ||
Updated•8 years ago
|
Attachment #8636950 -
Attachment description: Part 3: Implement a context menu for muting and unmuting a tab → Part 3: Allow muting and unmuting a tab by clicking on the playing icon
Comment 133•8 years ago
|
||
(In reply to Stephen Horlander [:shorlander] (Away until 7/20) from comment #127) > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, > needinfo? me!) from comment #124) > > Also, are you and Stephen fine with switching to only using icons that > > overlay the tab icon? Dao in comment 121 is asking me to do that, but that > > would kind of ignore the current UX spec. > > I would rather keep it on the tab where we will often have more room and we > won't have to worry about overlaying things. I don't think overlaying the > tab/favicon is optimal but makes sense in the pinned tab case where we don't > have any other real estate to work with. I realize that we often have more room. What are your thoughts on the minority case that many power users will run into? This is basically the default case for me thanks to session restore.
Flags: needinfo?(shorlander)
Comment 134•8 years ago
|
||
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•8 years ago
|
||
(In reply to Blake Winton (:bwinton) from comment #134) > What about a compromise where we switch to the overlay if the tabs get less > than, say, 60 or 70 pixels wide? They'll never reach that width thanks to our awesome overflow handling. The tab min-width is 100px. (In reply to Dão Gottwald [:dao] from comment #133) > I realize that we often have more room. What are your thoughts on the > minority case that many power users will run into? This is basically the > default case for me thanks to session restore. We could increase the tab min-width to 116px (100 + iconsize).
Assignee | ||
Comment 136•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #137) > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, > needinfo? me!) from comment #136) > > Our tab titles are already pretty useless > > when you reach the tab min-width threshold, and since we're not adding the > > audio playback icon to every single tab, we're not making the existing issue > > much worse. > > Tab titles still give a hint that kind of works with some luck when reaching > the min-width. Displaying only two characters instead of five can easily > push that hint over to being useless much more often. Obviously we're only > talking about tabs with audio playback here, but that doesn't mean it's a > nonissue; users will still want to identify those tabs and favicons aren't > always helpful enough. I agree it is an issue, but I don't think it will be a serious detriment here. Tabs playing audio will (hopefully) remain rare. I think the functionality of being able to identify and mute noisy background tabs is worth the trade-off. I do think it would be worth a follow-up to see if we can mitigate the character loss more though. Also for the best ways to handle tabs in overflow playing audio.
Flags: needinfo?(shorlander)
Assignee | ||
Comment 140•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Comment 141•8 years ago
|
||
Comment on attachment 8636948 [details] [diff] [review] Part 1: Provide a visual indicator for tabs that are playing audio >+ <handler event="DOMMediaPlaybackStarted"> >+ <![CDATA[ >+ if (!Services.prefs.getBoolPref("browser.tabs.showAudioPlayingIcon") || >+ !event.isTrusted) >+ return; >+ >+ var browser = event.originalTarget; >+ var tab = this.getTabForBrowser(browser); >+ if (!tab || tab.hasAttribute("pending")) >+ return; I don't understand the 'pending' attribute check. How can a pending browser send this event? >+ tab.setAttribute("playing", true); >+ this._tabAttrModified(tab, ["playing"]); nit: please rename this to soundplaying for clarity >+ <xul:image xbl:inherits="playing,pinned" >+ class="tab-icon-media" nit: tab-icon-sound please >-.tab-icon-overlay { >+.tab-icon-overlay, >+.tab-icon-media[pinned] { > opacity: 0.9999; I don't understand this change. >+.tab-icon-overlay[playing][pinned] { >+ display: -moz-box; >+ margin-top: -12px; .tab-icon-overlay has margin-top: 10px. Let's please settle on one location and not randomly change it for different icons. >+.tab-icon-overlay[playing][pinned]:active { >+#TabsToolbar[brighttext] .tab-icon-overlay[playing][pinned]:active { :hover:active >+.tab-icon-media { >+ -moz-margin-start: 4px; >+ -moz-margin-end: -2px; Can you get rid of the negative margin here? >+ display: none; >+.tab-icon-media[playing] { >+ display: -moz-box; >+.tab-icon-media[playing][pinned] { >+ display: none !important; Please consolidate this as follows: .tab-icon-media:not([playing]) .tab-icon-media[pinned] { display: none; } > .tab-background-middle, > .tabs-newtab-button, >+.tab-icon-overlay[playing][pinned], >+.tab-icon-media, > .tab-close-button { > pointer-events: auto; > } Why [pinned]? The overlay isn't displayed in the first place when the tab isn't pinned.
Attachment #8636948 -
Flags: review?(dao) → review-
Assignee | ||
Comment 142•8 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #141) > Comment on attachment 8636948 [details] [diff] [review] > Part 1: Provide a visual indicator for tabs that are playing audio > > >+ <handler event="DOMMediaPlaybackStarted"> > >+ <![CDATA[ > >+ if (!Services.prefs.getBoolPref("browser.tabs.showAudioPlayingIcon") || > >+ !event.isTrusted) > >+ return; > >+ > >+ var browser = event.originalTarget; > >+ var tab = this.getTabForBrowser(browser); > >+ if (!tab || tab.hasAttribute("pending")) > >+ return; > > I don't understand the 'pending' attribute check. How can a pending browser > send this event? This is me cargo-culting, sorry. What's a pending browser? If this doesn't make sense, I'll remove it. > >-.tab-icon-overlay { > >+.tab-icon-overlay, > >+.tab-icon-media[pinned] { > > opacity: 0.9999; > > I don't understand this change. Oops, sorry, this is a remnant of previous iterations. Will remove. > >+.tab-icon-overlay[playing][pinned] { > >+ display: -moz-box; > >+ margin-top: -12px; > > .tab-icon-overlay has margin-top: 10px. Let's please settle on one location > and not randomly change it for different icons. Should I re-use 10px here? > >+.tab-icon-media { > >+ -moz-margin-start: 4px; > >+ -moz-margin-end: -2px; > > Can you get rid of the negative margin here? Sure. What do I replace them with? Just remove them? > > .tab-background-middle, > > .tabs-newtab-button, > >+.tab-icon-overlay[playing][pinned], > >+.tab-icon-media, > > .tab-close-button { > > pointer-events: auto; > > } > > Why [pinned]? The overlay isn't displayed in the first place when the tab > isn't pinned. This is another remnant, sorry. Will address the rest of the nits.
Flags: needinfo?(dao)
Comment 143•8 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #142) > > I don't understand the 'pending' attribute check. How can a pending browser > > send this event? > > This is me cargo-culting, sorry. What's a pending browser? If this doesn't > make sense, I'll remove it. It's a browser that hasn't been restored yet by session restore. In other words it doesn't have content loaded. > > >+.tab-icon-overlay[playing][pinned] { > > >+ display: -moz-box; > > >+ margin-top: -12px; > > > > .tab-icon-overlay has margin-top: 10px. Let's please settle on one location > > and not randomly change it for different icons. > > Should I re-use 10px here? .tab-icon-overlay already has that by default, so you shouldn't have to specify any margin here. > > >+.tab-icon-media { > > >+ -moz-margin-start: 4px; > > >+ -moz-margin-end: -2px; > > > > Can you get rid of the negative margin here? > > Sure. What do I replace them with? Just remove them? There's probably some room for cleaning up the margins used within tabs, but for now you can just omit -moz-margin-end here.
Flags: needinfo?(dao)
Comment 144•8 years ago
|
||
Comment on attachment 8636949 [details] [diff] [review] Part 2: Implement a tooltip for the tab playing icon This will likely need to be updated along with part 1, thus cancelling review request. nit: we're not using the "member" variable name prefix anymore, please use _foo instead of mFoo
Attachment #8636949 -
Flags: review?(dao)
Comment 145•8 years ago
|
||
Comment on attachment 8636950 [details] [diff] [review] Part 3: Allow muting and unmuting a tab by clicking on the playing icon >+ <method name="toggleMuteAudio"> this should be private, i.e. _toggleMuteAudio
Attachment #8636950 -
Flags: review?(dao)
Updated•8 years ago
|
Attachment #8636951 -
Flags: review?(dao)
Updated•8 years ago
|
Flags: needinfo?(philipp)
Assignee | ||
Comment 146•8 years ago
|
||
(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.
Assignee | ||
Comment 147•8 years ago
|
||
Attachment #8636948 -
Attachment is obsolete: true
Attachment #8636949 -
Attachment is obsolete: true
Attachment #8636950 -
Attachment is obsolete: true
Attachment #8636951 -
Attachment is obsolete: true
Assignee | ||
Comment 148•8 years ago
|
||
Assignee | ||
Comment 149•8 years ago
|
||
Assignee | ||
Comment 150•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8639019 -
Flags: review?(dao)
Assignee | ||
Updated•8 years ago
|
Attachment #8639020 -
Flags: review?(dao)
Assignee | ||
Updated•8 years ago
|
Attachment #8639022 -
Flags: review?(dao)
Assignee | ||
Updated•8 years ago
|
Attachment #8639021 -
Flags: review?(dao)
Comment 151•8 years ago
|
||
Comment on attachment 8639020 [details] [diff] [review] Part 2: Implement a tooltip for the tab playing icon Review of attachment 8639020 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/locales/en-US/chrome/browser/tabbrowser.dtd @@ +3,5 @@ > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > > <!ENTITY closeTab.label "Close Tab"> > +<!ENTITY playingAudio.label "This tab is playing audio"> > +<!ENTITY mutedAudio.label "This tab has been muted"> Now that clicking the icon mutes/unmutes the tab, it's worth changing the tooltips to reflect what clicking will actually do.
Assignee | ||
Comment 152•8 years ago
|
||
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!
Assignee | ||
Updated•8 years ago
|
Comment 153•8 years ago
|
||
(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•8 years ago
|
||
Comment on attachment 8639021 [details] [diff] [review] Part 3: Allow muting and unmuting a tab by clicking on the playing icon >+ <handler event="click"> >+ <![CDATA[ >+ if (event.button != 0) { >+ return; >+ } >+ >+ let anonid = event.originalTarget.getAttribute("anonid"); >+ if (anonid != "soundplaying-icon" && anonid != "overlay-icon") { >+ return; >+ } >+ >+ let tab = event.originalTarget.parentNode.parentNode.parentNode; >+ this._toggleMuteAudio(tab); You need to check that overlay-icon is actually being used as the sound-playing icon rather than something else. On a related note, it might make sense for overlay-icon to have status attribute (e.g. status="crashed", status="soundplaying", status="muted") rather than a bunch of boolean attributes.
Attachment #8639021 -
Flags: review?(dao) → review-
Assignee | ||
Comment 155•8 years ago
|
||
(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.
Assignee | ||
Comment 156•8 years ago
|
||
(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.
Assignee | ||
Comment 157•8 years ago
|
||
Attachment #8639019 -
Attachment is obsolete: true
Attachment #8639019 -
Flags: review?(dao)
Assignee | ||
Updated•8 years ago
|
Attachment #8640055 -
Flags: review?(dao)
Assignee | ||
Comment 158•8 years ago
|
||
Attachment #8639021 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8640064 -
Flags: review?(dao)
Comment 159•8 years ago
|
||
Comment on attachment 8639020 [details] [diff] [review] Part 2: Implement a tooltip for the tab playing icon Review of attachment 8639020 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabbrowser.xml @@ +5808,5 @@ > </setter> > </property> > > <field name="mOverCloseButton">false</field> > + <field name="_overPlayingIcon">false</field> Please use mOverPlayingIcon to be consistent with these other two fields.
Attachment #8639020 -
Flags: review?(dao) → review+
Comment 160•8 years ago
|
||
Comment on attachment 8640055 [details] [diff] [review] Part 1: Provide a visual indicator for tabs that are playing audio Review of attachment 8640055 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabbrowser.xml @@ +4256,5 @@ > + if (!tab) > + return; > + > + tab.removeAttribute("soundplaying"); > + this._tabAttrModified(tab, ["soundplaying"]); We should only call _tabAttrModified if the soundplaying attribute was present before it was removed. if (tab.hasAttribute("soundplaying")) { tab.removeAttribute("soundplaying"); this._tabAttrModified(tab, ["soundplaying"]); } ::: browser/themes/shared/tabs.inc.css @@ +90,5 @@ > > .tab-icon-overlay { > width: 16px; > height: 16px; > + margin-top: -12px; As this was discussed on IRC, we should be consistent with the location of the icon overlays (tab crashed and soundplaying). Shorlander said that he wants the icon in the top right for both cases. You can either make the change so that the tabcrashed icon is moved to the top right, or not adjust the CSS in this bug and file a followup to get them both moved to the top right.
Attachment #8640055 -
Flags: review?(dao) → review+
Assignee | ||
Comment 161•8 years ago
|
||
(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.
Assignee | ||
Comment 162•8 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #159) > ::: browser/base/content/tabbrowser.xml > @@ +5808,5 @@ > > </setter> > > </property> > > > > <field name="mOverCloseButton">false</field> > > + <field name="_overPlayingIcon">false</field> > > Please use mOverPlayingIcon to be consistent with these other two fields. I agree with you which is why I called it mOverPlayingIcon, but Dao asked me to use an underscore prefix in comment 144. What gives? :-)
Flags: needinfo?(jaws)
Comment 163•8 years ago
|
||
This seems like a small point to go back and forth over. But I am fine with landing this using the 'member' prefix and filing a follow-up bug to rename all of the members to use underscores.
Flags: needinfo?(jaws)
Comment 164•8 years ago
|
||
(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•8 years ago
|
||
Comment on attachment 8639020 [details] [diff] [review] Part 2: Implement a tooltip for the tab playing icon >- <content context="tabContextMenu" closetabtext="&closeTab.label;"> >+ <content context="tabContextMenu" >+ closetabtext="&closeTab.label;" >+ playingtext="&playingAudio.label;" >+ mutedtext="&mutedAudio.label;"> Please don't add the playingtext/mutedtext attributes. Instead, these two strings should be in tabbrowser.properties (as foo.tooltip, not foo.label). This isn't as suitable as a followup because temporary strings that ride the trains are bad for localizers. closeTab.label should be in the properties file too as far as I can tell, but you don't need to take care of that here. You can if you want though, since it should be trivial to do along with the other two strings. If you do, please change "Close Tab" to "Close tab" while you're at it. (In reply to Marco Bonardo [::mak] from comment #164) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #163) > > filing a follow-up bug to rename all of the members to use underscores. > > I suspect that may break a bunch of add-ons. Probably. In any case, please do stick with _overPlayingIcon here.
Attachment #8639020 -
Flags: review-
Comment 166•8 years ago
|
||
Comment on attachment 8640064 [details] [diff] [review] Part 3: Allow muting and unmuting a tab by clicking on the playing icon >+ <method name="_toggleMuteAudio"> >+ <parameter name="aTab"/> >+ <body> >+ <![CDATA[ >+ let tabContainer = this.parentNode; >+ let browser = aTab.linkedBrowser; >+ if (browser.audioMuted) { >+ browser.unmute(); >+ aTab.removeAttribute("muted"); >+ } else { >+ browser.mute(); >+ aTab.setAttribute("muted", "true"); >+ } >+ tabContainer.tabbrowser._tabAttrModified(aTab, ["muted"]); >+ ]]> >+ </body> >+ </method> You're implementing this on the tab binding, right? So get rid of the aTab parameter and use 'this' instead. >+ let soundPlaying = event.originalTarget.hasAttribute("soundplaying"); Just use this.hasAttribute("soundplaying") here. >+ let anonid = event.originalTarget.getAttribute("anonid"); >+ if ((anonid == "soundplaying-icon") || >+ ((anonid == "overlay-icon") && soundPlaying)) { >+ let tab = event.originalTarget.parentNode.parentNode.parentNode; >+ this._toggleMuteAudio(tab); And obviously get rid of 'tab' here too. r+ with these changes
Attachment #8640064 -
Flags: review?(dao) → review+
Comment 167•8 years ago
|
||
Comment on attachment 8640055 [details] [diff] [review] Part 1: Provide a visual indicator for tabs that are playing audio >+.tab-icon-overlay[soundplaying][pinned] { >+ display: -moz-box; >+ list-style-image: url("chrome://browser/skin/tabbrowser/tab-audio-small.svg#tab-audio"); >+ border: 1px solid transparent; >+ border-radius: 8px; What's the transparent border needed for? Can you use padding instead or completely drop this? r+ with this explained / fixed
Attachment #8640055 -
Flags: review+
Assignee | ||
Comment 168•8 years ago
|
||
(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.
Assignee | ||
Comment 169•8 years ago
|
||
Attachment #8639020 -
Attachment is obsolete: true
Assignee | ||
Comment 170•8 years ago
|
||
Attachment #8640064 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8640456 -
Flags: review?(jaws)
Attachment #8640456 -
Flags: review?(dao)
Comment 171•8 years ago
|
||
Comment on attachment 8640456 [details] [diff] [review] Part 2: Implement a tooltip for the tab playing icon >+ var name, label; >+ if (tab.mOverCloseButton) { >+ name = "tabs.closeTab.tooltip"; >+ } else if (tab._overPlayingIcon) { >+ name = tab.linkedBrowser.audioMuted ? >+ "tabs.mutedAudio.tooltip" : >+ "tabs.playingAudio.tooltip"; >+ } else { >+ label = tab.getAttribute("label") + >+ (this.AppConstants.E10S_TESTING_ONLY && tab.linkedBrowser && tab.linkedBrowser.isRemoteBrowser ? " - e10s" : ""); >+ } >+ if (name && !label) { >+ label = this.mStringBundle.getString(name); >+ } >+ event.target.setAttribute("label", label); nit: I think stringID would be clearer than 'name' > <handlers> > <handler event="mouseover"><![CDATA[ > let anonid = event.originalTarget.getAttribute("anonid"); >+ let soundPlaying = event.originalTarget.hasAttribute("soundplaying"); this.hasAttribute("soundplaying") > <handler event="mouseout"><![CDATA[ > let anonid = event.originalTarget.getAttribute("anonid"); >+ let soundPlaying = event.originalTarget.hasAttribute("soundplaying"); ditto
Attachment #8640456 -
Flags: review?(dao) → review+
Updated•8 years ago
|
Attachment #8639022 -
Flags: review?(dao) → review+
Assignee | ||
Updated•8 years ago
|
Blocks: tab-sound-indicator
Assignee | ||
Comment 172•8 years ago
|
||
Note that I will remove the test for bug 405137 since it is now pointless without closetabtext.
Comment 173•8 years ago
|
||
Comment on attachment 8640456 [details] [diff] [review] Part 2: Implement a tooltip for the tab playing icon Dao's review is sufficient here.
Attachment #8640456 -
Flags: review?(jaws)
Comment 174•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/af50345620f8 https://hg.mozilla.org/integration/mozilla-inbound/rev/66d93422384f https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd6a0558682 https://hg.mozilla.org/integration/mozilla-inbound/rev/e5137e5dc19d
Comment 175•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/af50345620f8 https://hg.mozilla.org/mozilla-central/rev/66d93422384f https://hg.mozilla.org/mozilla-central/rev/5cd6a0558682 https://hg.mozilla.org/mozilla-central/rev/e5137e5dc19d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Updated•8 years ago
|
Flags: needinfo?(dbolter)
Comment 176•8 years ago
|
||
Tab audio indicators are probably a feature worth highlighting in the Firefox 42 release notes. :)
Comment 177•8 years ago
|
||
Yes, it's worth highlighting , and we should remember to add that it's not only an indicator but also a way to mute a tab easily (Chrome just indicate that sound is playing) Also, I didn't found how to test the functionality (not implemented in 42.0a1 2015-07-30, I think I will have to wait tomorrow.) The last version I tested were only working with HTML5 video and not flash (so it weren't using flash NPAPI mentioned in comment 67). There's also one feature that would be great to implement: a check-box that set the default status to "muted" (and maybe a dialog box like flash click to play that allow playing sound for now, or always for actual website). Maybe I should make a enhancement bug request.
Comment 178•8 years ago
|
||
Adding addon-compat flag because this changes tab structure and at least one add-on has been affected.
Keywords: addon-compat
Comment 179•8 years ago
|
||
Release Note Request (optional, but appreciated) [Why is this notable]: No more searching for the noisy tab. Tab audio indicators with mute button. https://twitter.com/mmmandel/status/626874461686497280 [Suggested wording]: New: Easily identify tabs playing audio and mute audio per tab [Links (documentation, blog post, etc)]: http://ehsanakhgari.org/blog/2015-07-30/tab-audio-indicators-and-muting-in-firefox-nightly Wording of the note can use a little help.
relnote-firefox:
--- → ?
Keywords: relnote
Comment 180•8 years ago
|
||
When Australis was being worked on there was a great deal of effort (and tests) to ensure that tab performance was not effected adversely. Are these tests being run on this and we're assured that we're not regressing those performance metrics?
Comment 181•8 years ago
|
||
(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
Assignee | ||
Comment 182•8 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #176) > Tab audio indicators are probably a feature worth highlighting in the > Firefox 42 release notes. :) Those belong to bug 1188860.
relnote-firefox:
? → ---
Keywords: feature
Assignee | ||
Comment 184•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
I wonder if anyone mentioned this scenario: Imagine that you have 1280x720 screen and 60 tabs opened (with overflow ofc). One of them is causing sound for ~0.5s once per ~2 minutes (random interval). The task is to locate the tab. I believe that: 1) Indicator should fade within 2-3 seconds (even in overflow list) 2) Additional indicator should be displayed for scrollbuttons in TabsToolbar if there's noisy tab in the respective direction (just like the blue background is displayed for them when new tab is opened)
Comment 187•8 years ago
|
||
(In reply to arni2033 from comment #186) My screen is 1920x1080. When I [re]start FF, there are sometimes several noisy tabs. What makes them difficult to find is that I have multiple windows with many tabs each. For example, I currently have the following window tab counts: 95, 31, 14, 42, 49, 9, 21, 43, 3, 18, 13, and 11 m= 349. At least I've been whittling them down from over 500... :) So it's fairly likely and in addition to knowing which window is noisy, the noisy tab(s) may not be visible. > 1) Indicator should fade within 2-3 seconds (even in overflow list) Fade time should be configurable. > 2) Additional indicator should be displayed for scrollbuttons in TabsToolbar > if there's noisy tab in the respective direction (just like the blue > background is displayed for them when new tab is opened) A readily accessible list of tabs (links) would also be appreciated, even as about:noisy_tabs
Assignee | ||
Comment 188•8 years ago
|
||
(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•8 years ago
|
||
(In reply to Ehsan Akhgari (don't ask for review please) from comment #188) > The indicator being visible when the tab is not playing sound is even more confusing Ok, I see you've already taken the decision but that's not a reason to call my suggestion (1) illogical Smooth transition from opacity:0 to 1 and back is very different from "The indicator being visible"
Comment 190•8 years ago
|
||
(In reply to Ehsan Akhgari (don't ask for review please) from comment #188) > (In reply to Pierre Fortin from comment #187) > > A readily accessible list of tabs (links) would also be appreciated, even as > > about:noisy_tabs > > This should be something that an add-on can do. Having the indicator icons in both the tabs and the drop-down menu that lists all tabs is probably a good idea. A full about page is overkill.
Comment 191•8 years ago
|
||
(In reply to Dave Garrett from comment #190) > (In reply to Ehsan Akhgari (don't ask for review please) from comment #188) > > (In reply to Pierre Fortin from comment #187) > > > A readily accessible list of tabs (links) would also be appreciated, even as > > > about:noisy_tabs > > > > This should be something that an add-on can do. > > Having the indicator icons in both the tabs and the drop-down menu that > lists all tabs is probably a good idea. A full about page is overkill. This was already added, I don't remember the bug though.
Assignee | ||
Comment 192•8 years ago
|
||
(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•8 years ago
|
||
Will this also be on Firefox Android?
Assignee | ||
Comment 194•8 years ago
|
||
(In reply to donrhummy from comment #193) > Will this also be on Firefox Android? Yes, see bug 1190301.
Comment 195•8 years ago
|
||
Depends on: 1200236 Depends on: 1200422
Comment 196•8 years ago
|
||
Successfully reproduce the bug on Firefox 41.0 , Build ID 20150917150946 on youtube. The fixe works for me on Firefox 42.0b2, Build ID 20150928102225 on mac OSX 10.10.4 on youtube.
QA Whiteboard: [bugday-20150930]
Comment 197•8 years ago
|
||
I have reproduced this bug on Nightly 37.0a1 (2014-12-28) on windows 7, 64 bit! The bug's fix is now verified on Latest Beta 42.0b2! Build ID: 20150928102225 User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
QA Whiteboard: [bugday-20150930]
Comment 198•8 years ago
|
||
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]
Updated•8 years ago
|
QA Whiteboard: [testday-20151002]
![]() |
||
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Comment 199•8 years ago
|
||
no audio indicator, if using themes.
Comment 200•8 years ago
|
||
(In reply to Gilbert Consellado from comment #199) > no audio indicator, if using themes. Can you file a new bug, please. This one is closed.
Comment 201•8 years ago
|
||
I'm just saying thanks to the crew that came up with this long desired and much needed enhancement
Comment hidden (offtopic) |
Comment 203•5 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•