Closed
Bug 1458039
Opened 7 years ago
Closed 7 years ago
Implement ability to mute/unmute a selection of tabs
Categories
(Firefox :: Tabbed Browser, enhancement, P3)
Firefox
Tabbed Browser
Tracking
()
VERIFIED
FIXED
Firefox 62
People
(Reporter: ablayelyfondou, Assigned: ablayelyfondou, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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.
Assignee | ||
Updated•7 years ago
|
Blocks: tabs-multiselect
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review-reply |
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!
Updated•7 years ago
|
Assignee: nobody → ablayelyfondou
Status: NEW → ASSIGNED
Comment 4•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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 9•7 years ago
|
||
mozreview-review-reply |
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).
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
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.
Comment 11•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff8e661f8b07
Implement ability to mute/unmute a selection of tabs. r=jaws
Assignee | ||
Comment 14•7 years ago
|
||
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).
Comment 15•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Updated•7 years ago
|
Comment 16•6 years ago
|
||
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 .
You need to log in
before you can comment on or make changes to this bug.
Description
•