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.

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

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.

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

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?

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.

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

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.

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.

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

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.

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

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.

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

i would like to take this issue. Thank you

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

Depends on D128280

Depends on D128281

Depends on D128282

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

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

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