1684876 - "Mute Tab" sometimes starts playing the tab's video, with sound

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Egmont Koblinger]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:84.0) Gecko/20100101 Firefox/84.0

Steps to reproduce:

1. Have a page with a link to a youtube video (could be e.g. a search result page on youtube).
2. Open this link in a new tab (e.g. ctrl+click). Note: as per my settings, this is the default I think, the new tab is not switched to.
3. Quickly, before that page loads, right-click on that new tab's label. A menu opens, its second entry being "Mute Tab".
4. Wait until the page loads. This is seen by a "play" icon appearing on the tab. The menu entry remains "Mute Tab" (although if you closed and reopened this menu, the second entry would be a "Play Tab" now, but don't do it for this bug to occur).
5. Click on this "Mute Tab" menu entry.

Actual results:

The video starts playing, unmuted, as if I clicked on the "Play Tab" entry.

Expected results:

The "Mute Tab" menu entry should mute the tab, and not start playing the video within.

Note: Another approach that you might consider would be to immediately replace the "Mute Tab" entry with "Play Tab" whenever the page is loaded to that extent. I'd strongly be against this approach, since it might happen just before I'm about to click on "Mute Tab", too late for me to realize that it just switched to some other action, effectively resulting in the same buggy experience.

Another note: I'm not sure I agree with the approach of "Mute Tab" and "Play Tab" being mutually exclusive (and apparently the technical solution being that they grab the same menu "slot" and hence one's action is invoked instead of the other's). Currently, if a video page is fully loaded, I cannot mute that tab without switching to that tab first. I don't understand the rationale behind it, I think it would be better for the "Mute Tab" and "Play Tab" entries to coexist in that case.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Alastor Wu [:alwu]]

The last version of UX spec for play icon in bug1308399 comment 10 didn't mention that we should always keep those two option mutually exclusive, so it sounds good to me to separate the play icon and mute icon.

As a reference, here is the place we trigger mute and resume, so we should separate them into different functions.

Comment 3

[Arjun Krishna Babu]

I'd like to work on this, if it is suitable for beginners.

Comment 4

[Arjun Krishna Babu]

I looked through the source code to understand how the existing functionality works. From what I understand, I might need to work on the following 3 files (at least):

1. browser/base/content/tabbrowser-tab.js
2. browser/base/content/tabbrowser.js
3. browser/base/content/browser.xhtml

Since the plan is to make two menu items, one for Mute/Unmute, and another for Play, here are some questions that I have:

1. Shouldn't the text on the new Play option toggle between "Play" and "Pause"?

2. Do we have a pause icon?

3. I probably should implement something like the opposite of "resumeMedia()" (in toolkit/content/widgets/browser-custom-element.js). Does such a method already exist? I couldn't find any, but I probably might have missed it.

Am I on the right track?

Comment 5

[Alastor Wu [:alwu]]

Sure, I'm happy to provide some direction on this bug, but because I'm not a peer of firefox component, the actual review would need to be done by Firefox ownwers and peers, eg. :Gijs

---

Ok, so the thing is that. Currently "Play Tab" uses the same menu item with the "Mute/Unmute Tab". We adjust its label here and the actual implementation for "Play tab" is in the same function toggleMuteAudio().

Therefore, here are something we need to do.

1. create a new "menu item" for "Play tab", which should be hidden in the beginning (hidden="true"). Because this menu should only appear when we postpone the autoplay. (see UX spec for more details about why we need "Play Tab")
2. create a new function (eg, resumeDelayedMedia()) and move the "Play tab" implementation in toggleMuteAudio() to that new function. Then set it to oncommand attribute of the menu item you created in the step1.
3. Now we don't need to set the "Play tab" label to the menu item for "Mute tab", so we need to remove that. In contrary, we have to detemine if we want to show the menu item for "Play tab" or not. If tab has attribute activemedia-blocked, then set the hidden state of the menu item to false.
4. Add the function you created in the step2 to here , then
   to determine whether calling toggleMuteAudio() or the function you created in the step2 depending on if tab has attribute activemedia-blocked or not.
5. Do similar thing as the step4 here, which also determine whether calling toggleMuteAudio() or the function you created in the step2 depending on if tab has attribute activemedia-blocked or not.

So after these steps, you are able to create a new menu item for "play tab" when selecting single tab.

---

(bonus, remove handling "play tab" on multiple selected tabs)
That is slightly out of the scope of this bug, which is also more complicated. In our initial design, we didn't propose to have this feature on multiple selected tabs, because playing multiple tabs at the same time would violate the original goal we want to achieve. So I don't know why they add this on multiple tabs selection. So I personally lean to remove that, and that won't need to be done in this bug.

Comment 6

[Alastor Wu [:alwu]]

(In reply to Arjun Krishna Babu from comment #4)

1. Shouldn't the text on the new Play option toggle between "Play" and "Pause"?

No, this icon is not used for normal play & pause, it's used for resuming those delayed autoplay. (see UX spec)

2. Do we have a pause icon?

No.

3. I probably should implement something like the opposite of "resumeMedia()" (in toolkit/content/widgets/browser-custom-element.js). Does such a method already exist? I couldn't find any, but I probably might have missed it.

No, resumeMedia() is the implementation of how we resume the delayed autoplay media, so you don't need to implement that again. You need to separate this from the toggleMuteAudio().

Comment 7

[Arjun Krishna Babu]

Attachment: bug1684876_changes_so_far.patch

Thank you Alastor Wu.

I followed your instructions, and attached a patch of my changes.

As per my current implementation, once the media starts playing after clicking the Play Tab menu item, clicking the menu item again redundantly does nothing. Therefore, should this menu item be hidden once the media starts playing (perhaps, by examining whether activemedia-blocked is false)?

Other than the above, things seem to be working when I tested things locally.

Who is the right person to ask a review for? I checked this page and it seems the owner for Tabbed Browser is Dão Gottwald, hence I have requested information from Dão Gottwald as well.

Comment 8

[Alastor Wu [:alwu]]

Nice done! Yes, we should show that menu item for Play tab only when activemedia-blocked is true. So after removing activemedia-blocked, we should hide that menu item again. I think both :dao or :Gijs would be the right person to ask for a review.

Comment 9

[Alastor Wu [:alwu]]

Comment on attachment 9196251 [details] [diff] [review]
bug1684876_changes_so_far.patch

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

Some suggestions.

::: browser/base/content/tabbrowser-tab.js
@@ +626,5 @@
> +        "TAB_AUDIO_INDICATOR_USED"
> +      );
> +
> +      if (this.hasAttribute("activemedia-blocked")) {
> +        this.removeAttribute("activemedia-blocked");

hide the menu item again.

::: browser/base/content/tabbrowser.js
@@ +6643,3 @@
>  
>      // Adjust the state of the toggle mute menu item.
>      if (this.contextTab.hasAttribute("activemedia-blocked")) {

Because `play tab` is no longer sharing same menu item with `mute/unmute tab`, you should move this part out from the original if-elseif-else section.

```
if (this.contextTab.hasAttribute("activemedia-blocked") {
  // display `play tab` menu item and set its label and access key
}

if (this.contextTab.hasAttribute("muted")) {
   // keep codes unchanged  
} else {
   // keep codes unchanged    
}
```

Comment 10

[Arjun Krishna Babu]

Attachment: bug1684876_changes2.patch

(In reply to Alastor Wu [:alwu] from comment #9)

hide the menu item again.

I now hide it by checking activemedia-blocked attribute in browser/base/content/tabbrowser.js, as follows:

// Show "Play Tab" if there's a media available for playing and hide it otherwise
let playItem = document.getElementById("context_playTab");
playItem.label = gNavigatorBundle.getString("playTab.label");
playItem.accessKey = gNavigatorBundle.getString("playTab.accesskey");
playItem.hidden = !this.contextTab.hasAttribute("activemedia-blocked");

Because play tab is no longer sharing same menu item with mute/unmute tab, you should move this part out from the original if-elseif-else section.

Okay. See the final line in the snippet above in this comment.

I attached a new patch file with the mentioned changes. Does it look alright?

Comment 11

[:Gijs (out for now; he/him)]

Please use phabricator and moz-phab instead of needinfo for patch review - https://moz-conduit.readthedocs.io/en/latest/walkthrough.html . I can't see much of what the patch is doing because there is no context. The change in tabbrowser.js in the loop seems wrong, for instance (it won't do the same thing across tabs, but do different things for different tabs), but it's hard to tell because there's no context.

We should also ensure there is appropriate testing if that doesn't already exist for these items. If there is already automated test coverage, we will probably need to update it.

Comment 12

[Alastor Wu [:alwu]]

Comment on attachment 9196405 [details] [diff] [review]
bug1684876_changes2.patch

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

When you feel your patch is ready, then as what Gijs said in comment11, switch to Phabricator for a real review. 

>  The change in tabbrowser.js in the loop seems wrong, for instance (it won't do the same thing across tabs, but do different things for different tabs), but it's hard to tell because there's no context.

That change happens in ` toggleMuteAudioOnMultiSelectedTabs()` where we would filter tab by its attrbutes to make sure that we only do the following operation (`toggleMuteAudio()`) on tabs that have correct attributes (`activeMediaBlocked` or `tabMuted`)

::: browser/base/content/tabbrowser.js
@@ +6632,5 @@
>      );
>      bookmarkMultiSelectedTabs.hidden = !multiselectionContext;
>  
> +    // Show "Play Tab" if there's a media available for playing and hide it otherwise
> +    let playItem = document.getElementById("context_playTab");

This should also check if we're selecting multiple tabs like [1], because in this bug, we only separate `play tab` from `mute/unmute tab` when selecting one tab.

[1] https://searchfox.org/mozilla-central/rev/03224522336f60a1a61a87e1fcd4feb0a0315a9b/browser/base/content/tabbrowser.js#6629

Comment 13

[Arjun Krishna Babu]

Attachment: Bug 1684876 - Create a new "Play Tab" menu entry, and separate the functionality of "Mute Tab" and "Play Tab" menu items. r?Gijs

The "Play Tab" menu item would be shown only when activemedia-blocked attribute is set.

Comment 14

[Arjun Krishna Babu]

Submitted the thing to Phabricator. Followed the default steps in the docs.

However, I just noticed that in the diff at Phabricator, the changes to browser/base/content/tabbrowser-tab.js does not show up. I'm unsure if that's because I messed up something.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 16

[Dhanesh]

i would like to take this issue. Thank you

Comment 17

[Alastor Wu [:alwu]]

Feel free to take it and NI me if you get any question!

Comment 18

[kz04px]

Attachment: Bug 1684876 - Separate Play and Mute tab menu icons and functionality r?gijs,alwu

Comment 19

[kz04px]

Attachment: Bug 1684876 - Implement Play Tabs menu button r?gijs,alwu

Depends on D128280

Comment 20

[kz04px]

Attachment: Bug 1684876 - Fix play/mute tab icon and update tests r?gijs,alwu

Depends on D128281

Comment 21

[kz04px]

Attachment: Bug 1684876 - Add multiselect Play Tabs tests r?gijs,alwu

Depends on D128282

Comment 22

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 9196405 [details] [diff] [review]
bug1684876_changes2.patch

Going to obsolete this as there's a new attempt.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e27da301b07a
Separate Play and Mute tab menu icons and functionality r=Gijs,alwu,flod

Comment 24

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/e27da301b07a