Closed Bug 1018504 Opened 6 years ago Closed 4 years ago

Show which tab is playing sound (pref'd off by default)

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed
relnote-firefox --- 43+

People

(Reporter: ywang, Assigned: Margaret)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: productwanted)

Attachments

(5 files, 3 obsolete files)

Attached image i01- layered on top.jpg (obsolete) —
A indicator of active media(sound,web cam, casting) to help the users locate the tab. 

This bug is created for sound indicator, the most common scenario.

A sound indicator
*  is ACTIVE when one or more tabs (including pinned tabs) are playing sound
*  is INACTIVE when media player is paused or muted by the users
*  can not be affected by OS level sound actions, for example “F12/mute”.

I explored two approaches, see attachments for details.
i01: An indicator layered on top of favicon
i02: Stand alone indicator

The pros and cons are listed in the attachment. Personally I think stand alone indicator makes more sense given the current structure of our tab strip and panel.
How do we want to treat pages with Flash which may be creating sound?
(In reply to Kevin Brosnan [:kbrosnan] from comment #3)
> How do we want to treat pages with Flash which may be creating sound?

From the users' perspective, they just want to find the tab that is playing.
With no technical constraints, ideally the browser should be smart enough to detect sound that is creating by Flash player.
As far as I know we are under the same restrictions as the desktop version of this bug. That we won't be able to tell that Flash is playing sound.
(In reply to Kevin Brosnan [:kbrosnan] from comment #5)
> As far as I know we are under the same restrictions as the desktop version
> of this bug. That we won't be able to tell that Flash is playing sound.

We actually can know on Android. i.e. we wrap the Android plugin system with NPAPI there, but the Android system does know about sound:

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/android/ANPAudio.cpp
Duplicate of this bug: 958525
Assignee: nobody → scleymans
Assignee: scleymans → esawin
Blocks: 486262
Is Fennec still interested in this?  My patch in bug 1180421 should give you the API that you need...
I believe we are.
tracking-fennec: --- → ?
Assignee: esawin → nobody
In order to make it easier to track what blocks bug 486262, I am removing the dependency on this bug.  All of the Gecko and toolkit related changed have landed, and bug 486262 is specific to Firefox Desktop.
No longer blocks: 486262
antlam, do you have some mocks for this?
Flags: needinfo?(alam)
tracking-fennec: ? → 42+
Assignee: nobody → margaret.leibovic
I found these ones for Mobile. I can attach the asset too.

Specs for mobile:
Horizontal - centered, same padding from the 'x' as the 'x' is from the right edge. I think it's 8 dp? (If not, we can set it at 8 for now).

Vertical - 4 dp padding from the bottom, same left padding as the title has from the tab preview.
Attachment #8431989 - Attachment is obsolete: true
Attachment #8431993 - Attachment is obsolete: true
Flags: needinfo?(alam)
I also found this for tablets tab strip. the icon does need to be tinted to our Placeholder grey (#777777) and it's padded 8 dp from both sides.

I'll keep looking for the ones we did in the tray as well. 

But this is a good place to start I think. Given that we might also want to do some updates to the mobile tabs tray.
Fx42 seems very optimistic
(In reply to Mark Finkle (:mfinkle) from comment #15)
> Fx42 seems very optimistic

We discussed this in triage... we want to try to be optimistic here, although we're totally prepared for this to slip if necessary.
Bug 1018504 - (Part 1) Add indicator to show which tab is playing sound. r=mhaigh
Attachment #8642079 - Flags: review?(mhaigh)
Bug 1018504 - (Part 2) Mute/unmute tab when tapping audio indicator. r=mhaigh
Attachment #8642080 - Flags: review?(mhaigh)
These patches still need some UX tweaks, so this is more of a feedback request than a review request, but I wanted to get some eyes on them sooner rather than later.

antlam, do we want to allow tapping on this icon to toggle mute/unmute? That's what my second patch here does (this would still need another icon), but the UX is actually pretty poor because the tap area is so small. I was modeling this after desktop, but maybe we don't need this fine-grained control on mobile as well? Although if you're annoyed by the sound it would be nice to automatically mute it.
Flags: needinfo?(alam)
Comment on attachment 8642079 [details]
MozReview Request: Bug 1018504 - (Part 1) Add indicator to show which tab is playing sound. r=mhaigh

RB not working for me atm :/

LGTM
Attachment #8642079 - Flags: review?(mhaigh) → review+
Comment on attachment 8642080 [details]
MozReview Request: Bug 1018504 - (Part 2) Mute/unmute tab when tapping audio indicator. r=mhaigh

Is there a bug number for the TODOs?  

Looking good.
Attachment #8642080 - Flags: review?(mhaigh) → review+
(In reply to Martyn Haigh (:mhaigh) from comment #21)
> Comment on attachment 8642080 [details]
> MozReview Request: Bug 1018504 - (Part 2) Mute/unmute tab when tapping audio
> indicator. r=mhaigh
> 
> Is there a bug number for the TODOs?  
> 
> Looking good.

I was planning to address them here once I get some more feedback from antlam on the UX. But maybe I should just land this pref'd off, and we can iterate on it in a separate bug.
Depends on: 1190301
Filed bug 1190301. Let's use that to refine this an enable it (if we don't fix that before the merge, this can just be disabled on 42).
Flags: needinfo?(alam)
url:        https://hg.mozilla.org/integration/fx-team/rev/d065e876a83b7903f87b54cb2f1ea7c6fd157ba7
changeset:  d065e876a83b7903f87b54cb2f1ea7c6fd157ba7
user:       Margaret Leibovic <margaret.leibovic@gmail.com>
date:       Fri Jul 31 11:08:26 2015 -0700
description:
Bug 1018504 - (Part 1) Add indicator to show which tab is playing sound. r=mhaigh

url:        https://hg.mozilla.org/integration/fx-team/rev/23e480296ddcfdea0cf7950b22d88d3c70a29297
changeset:  23e480296ddcfdea0cf7950b22d88d3c70a29297
user:       Margaret Leibovic <margaret.leibovic@gmail.com>
date:       Fri Jul 31 11:32:46 2015 -0700
description:
Bug 1018504 - (Part 2) Mute/unmute tab when tapping audio indicator. r=mhaigh
https://hg.mozilla.org/mozilla-central/rev/d065e876a83b
https://hg.mozilla.org/mozilla-central/rev/23e480296ddc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Yay!  Thanks, Margaret.  :-)

I'm landing a follow-up to this to rename the events used here since it mid-aired with bug 1190082.
Depends on: 1191159
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #27)
> Yay!  Thanks, Margaret.  :-)
> 
> I'm landing a follow-up to this to rename the events used here since it
> mid-aired with bug 1190082.

Thanks for the follow-up! That rename works well with the variable names I chose for the rest of the patch :)
Duplicate of this bug: 1200002
In order for a polished version of this to land both bug 1190301 and bug 1198084 need to be uplifted. However, bug 1190301 no longer applies cleanly because of my new_tablet_* -> tablet_* filename changes (bug 1150742 or one of the other similar ones) and Martyn's tabs tray changes, where he removed 2 of the 3 tabs tray layouts.

I could figure out the details and come up with some solid patches, but it'll take longer than I think I should be spending here.

Anthony, Finkle – to save time, is there any reason this can't be backed out of 42 and pushed off until 43?
Flags: needinfo?(mark.finkle)
Flags: needinfo?(alam)
I'm ok with holding this for 43. Up to you guys!
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #33)
> I'm ok with holding this for 43. Up to you guys!

I am also fine with that.
Flags: needinfo?(mark.finkle)
(In reply to Mark Finkle (:mfinkle) from comment #34)
> (In reply to Anthony Lam (:antlam) from comment #33)
> > I'm ok with holding this for 43. Up to you guys!
> 
> I am also fine with that.

But I am also grumpy with that. It would be nice to figure out why we got into this situation and work to avoid it.
(In reply to Mark Finkle (:mfinkle) from comment #35)
> But I am also grumpy with that. It would be nice to figure out why we got
> into this situation and work to avoid it.

Note: If Margaret could finish this off, it probably wouldn't take as long as me trying – missing the development context means I have to be very careful about catching all of the loose ends.

My analysis on how to avoid this (disclaimer: missing context because I am just here to finish off the remaining work):

The visual refinements, assuming we'd refuse to ship without them, should been been pushed to land in the same fx-team/mozilla-central tree as the feature. If the initial bug lands and there is a change in that same version that will cause an uplift to require a branch patch, working in the next version causes a workload increase (nevermind the other overhead surrounding uplifts). In this case, to reduce the workload, it seems it's best to prioritize a feature and nail it out quickly before any patches have to be uplifted (or wait to initially land it until after merge). It's worth noting that a quick turn-around time on reviews is helpful here.

For this feature specifically, this bug landed on 8/4, merge was 8/10, and the visual refinements patches were initially posted on 8/24.
---

Alternatively, we could try to be more disciplined about how we land breaking changes (e.g. at the start of the cycle?), or be communicative about what features are expected to land and work around them (e.g. if Margaret knew the tabs tray is going to change, wait to work on this feature until the changes land). However, I don't have any good solutions here.
(In reply to Michael Comella (:mcomella) from comment #32)
> Anthony, Finkle – to save time, is there any reason this can't be backed out
> of 42 and pushed off until 43?

Oh, it seems there is a boolean preference that prevents this from showing by default on 42 [1] – guess a backout isn't needed after all. Nice Margaret!

Clearing tracking and updating title so it's less confusing.

re comment 36: boolean prefs seem like a good way to avoid having to rush features.

[1]: https://mxr.mozilla.org/mozilla-aurora/source/mobile/android/app/mobile.js?rev=3a38dba3692d#933
tracking-fennec: 42+ → ---
Summary: Show which tab is playing sound → Show which tab is playing sound (pref'd off by default)
Yes, my intention was to keep this disabled until it was ready, otherwise I wouldn't have landed my original patches right before the merge.

The reason that I'm fine with holding this back for 43 instead of trying to rush for 42 is that this isn't a feature we tried to plan for 42, it just happened to land during that window.
Release Note Request (optional, but appreciated)
[Why is this notable]: user-facing feature, suggested by dev team
[Suggested wording]:  Tab audio indicator is on by default
[Links (documentation, blog post, etc)]:

Noted for 43 aurora.
Can we mute/unmute tab audio indicator?
Flags: needinfo?(margaret.leibovic)
(In reply to Biraj Karmakar [:biraj] from comment #40)
> Can we mute/unmute tab audio indicator?

No, we haven't implemented that. That's bug 1191159.
Flags: needinfo?(margaret.leibovic)
You need to log in before you can comment on or make changes to this bug.