Implement ability to mute/unmute a selection of tabs

VERIFIED FIXED in Firefox 62

Status

()

enhancement
P3
normal
VERIFIED FIXED
Last year
8 months ago

People

(Reporter: ablayelyfondou, Assigned: ablayelyfondou, Mentored)

Tracking

(Blocks 1 bug)

Trunk
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox62 fixed, firefox64 verified, firefox65 verified)

Details

Attachments

(1 attachment)

When a group of tabs are selected:
1. Clicking on the mute/unmute button of any tab in the selection will mute/unmute all of the tabs in the selection.
2. Right-clicking on any tab in the selection should:
2a. Provide a context menu option of "Mute/Unmute selected tabs".
2b. The "Mute/Unmute tab" option will be hidden.
Blocks: 1458007
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 1468443
Comment on attachment 8986095 [details]
Bug 1458039 - Implement ability to mute/unmute a selection of tabs.

https://reviewboard.mozilla.org/r/251550/#review257816

::: browser/base/content/browser.js:7911
(Diff revision 1)
>    },
>    updateContextMenu: function updateContextMenu(aPopupMenu) {
>      this.contextTab = aPopupMenu.triggerNode.localName == "tab" ?
>                        aPopupMenu.triggerNode : gBrowser.selectedTab;
>      let disabled = gBrowser.tabs.length == 1;
> +    let multiselectionContext = this.contextTab.multiselected;

From this patch, we will consider we are on a mutliselect context (for context-tab menu) only when user right clicks on multiselected tab to match Amy's spec. Hence, I adapted for close menu.

::: browser/base/content/test/tabs/browser_multiselect_tabs_mute_unmute.js:4
(Diff revision 1)
> +const PREF_MULTISELECT_TABS = "browser.tabs.multiselect";
> +const PAGE = "https://example.com/browser/browser/base/content/test/tabs/file_mediaPlayback.html";
> +
> +async function wait_for_tab_playing_event(tab, expectPlaying) {

Many functions in this file are picked from https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_audioTabIcon.js with just slight change for a couple of them.
Comment on attachment 8986095 [details]
Bug 1458039 - Implement ability to mute/unmute a selection of tabs.

https://reviewboard.mozilla.org/r/251550/#review257816

I made just a couple of comments to make a bit easier your review ==> Jared!
Assignee: nobody → ablayelyfondou
Status: NEW → ASSIGNED
Comment on attachment 8986095 [details]
Bug 1458039 - Implement ability to mute/unmute a selection of tabs.

https://reviewboard.mozilla.org/r/251550/#review257946

Looks good!

::: browser/base/content/browser.js:7975
(Diff revision 1)
>      this.contextTab.toggleMuteMenuItem = toggleMute;
>      this._updateToggleMuteMenuItem(this.contextTab);

It looks like we'll need to do the same thing for toggleMultiSelectMute.

See https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#7900 for more details.

Also, I would move the if/else-if/else block below above these two lines to keep them together.

::: browser/base/content/browser.js:7981
(Diff revision 1)
>      this._updateToggleMuteMenuItem(this.contextTab);
>  
> +    // Adjust the state of the toggle mute menu item for multi-selected tabs.
> +    if (this.contextTab.hasAttribute("activemedia-blocked")) {
> +      toggleMultiSelectMute.label = gNavigatorBundle.getString("playTabs.label");
> +      toggleMultiSelectMute.accessKey = gNavigatorBundle.getString("playTab.accesskey");

We shouldn't share accesskeys between the singular and plural forms, as some languages might not share the same spelling.

::: browser/base/content/tabbrowser.js:3714
(Diff revision 1)
> +    if (aTab.activeMediaBlocked) {
> +      tabsToToggle = selectedTabs.filter(tab =>
> +        tab.activeMediaBlocked || tab.linkedBrowser.audioMuted
> +      );
> +    } else {
> +      let aTabMuted = aTab.linkedBrowser.audioMuted;

The 'a' prefix is usually only used for function arguments, so this can just be `let tabMuted = ...`

::: browser/base/content/tabbrowser.js:3720
(Diff revision 1)
> +      tabsToToggle = selectedTabs.filter(tab =>
> +        tab.linkedBrowser.audioMuted == aTabMuted && !tab.activeMediaBlocked
> +        // When a user is looking to mute selected tabs, then media-blocked tabs
> +        // should not be toggled. Otherwise those media-blocked tabs are going into a
> +        // playing and unmuted state.
> +        || tab.activeMediaBlocked && aTabMuted

Can you put the comment above line 3716? I didn't notice this part of the expression at first, and I think others may miss it too.

Also, can you please put the `||` at the end of the first line? That matches other code in this file.

::: browser/base/content/tabbrowser.xml:2033
(Diff revision 1)
>              gBrowser.clearMultiSelectedTabs(updatePositionalAttr);
>            }
>          }
>  
>          if (this._overPlayingIcon) {
> +          if (this.multiselected)

Can you please wrap these branches in curly brackets to match the coding style of the surrounding code?

::: browser/base/content/test/tabs/browser.ini:51
(Diff revision 1)
>  [browser_multiselect_tabs_using_Shift.js]
>  [browser_multiselect_tabs_close.js]
>  [browser_multiselect_tabs_positional_attrs.js]
> +[browser_multiselect_tabs_mute_unmute.js]
> +support-files =
> +  audio.ogg

You can use ../general/audio.ogg so that you don't need to copy the file.

This is already done by the pageinfo tests,
https://searchfox.org/mozilla-central/source/browser/base/content/test/pageinfo/browser.ini#9

::: browser/base/content/test/tabs/browser_multiselect_tabs_close.js:81
(Diff revision 1)
>      ok(tab2.multiselected, "Tab2 is multiselected");
>      ok(!tab3.multiselected, "Tab3 is not multiselected");
>      ok(!tab4.multiselected, "Tab4 is not multiselected");
>      is(gBrowser.multiSelectedTabsCount, 2, "Two multiselected tabs");
>  
> -    // Check the context menu with two multiselected tabs
> +    // Check the context menu with a no-multiselected tab

// Check the context menu with a non-multiselected tab
Attachment #8986095 - Flags: review?(jaws) → review-
Comment on attachment 8986095 [details]
Bug 1458039 - Implement ability to mute/unmute a selection of tabs.

https://reviewboard.mozilla.org/r/251550/#review258210

> Many functions in this file are picked from https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_audioTabIcon.js with just slight change for a couple of them.

Can you file a follow-up bug to undo some of this duplication?

::: browser/base/content/tabbrowser.js:3719
(Diff revision 2)
> +        tab.linkedBrowser.audioMuted == tabMuted && !tab.activeMediaBlocked
> +        || tab.activeMediaBlocked && tabMuted

```js
tab.linkedBrowser.audioMuted == tabMuted && !tab.activeMediaBlocked ||
tab.activeMediaBlocked && tabMuted
```
Attachment #8986095 - Flags: review?(jaws) → review+
Comment on attachment 8986095 [details]
Bug 1458039 - Implement ability to mute/unmute a selection of tabs.

https://reviewboard.mozilla.org/r/251550/#review258210

I will do once this patch is landed so that we have a link of both test files. 

should these duplicated functions be moved to BrowserTestUtils?
Comment on attachment 8986095 [details]
Bug 1458039 - Implement ability to mute/unmute a selection of tabs.

https://reviewboard.mozilla.org/r/251550/#review258210

I've pushed your patch to tryserver.

We should move browser_audioTabIcon.js to the /browser/base/content/test/tabs directory (and it's associated support files). Then we can move these functions to the tabs head.js file. We might need to move other tests that use the same shared files (hopefully they are all tab-related).
See Also: → 1469935
Blocks: 1469935
Comment on attachment 8986095 [details]
Bug 1458039 - Implement ability to mute/unmute a selection of tabs.

https://reviewboard.mozilla.org/r/251550/#review258210

You're right. This is a better idea.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f409b86e6e6288f02ae23701a57772c2ad16a945 -d 7eab423d6991: rebasing 468704:f409b86e6e62 "Bug 1458039 - Implement ability to mute/unmute a selection of tabs. r=jaws" (tip)
merging browser/base/content/browser.js
merging browser/base/content/browser.xul
merging browser/base/content/tabbrowser.js
merging browser/base/content/tabbrowser.xml
merging browser/base/content/test/tabs/browser.ini
merging browser/locales/en-US/chrome/browser/browser.properties
warning: conflicts while merging browser/base/content/test/tabs/browser.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff8e661f8b07
Implement ability to mute/unmute a selection of tabs. r=jaws
More details on how this feature works:

1. When user mutes a previously unmuted tab in the multi-selection:
 a. Unmuted tabs in the multi-selection will be muted except the active media-blocked tabs
 b. Active media-blocked and previously muted tabs will have their muted state not modified.

2. When user unmutes/plays a proviously muted/media-blocked tab in the multi-selection:
 a. Muted tabs in the multi-selection will become unmuted.
 b. Active media-blocked tabs will have sound playing (thus media-unblocked).
https://hg.mozilla.org/mozilla-central/rev/ff8e661f8b07
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
See Also: → 1470677
Verified fixed that this implementation is present on the latest Nightly Nightly 65.0a1(2018-11-22)and the latest Beta 64.0b11 as well .
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.