Closed
Bug 1308153
Opened 8 years ago
Closed 8 years ago
Implement the new UX visual mechanism for blocking autoplay media
Categories
(Core :: Audio/Video: Playback, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: alwu, Assigned: alwu)
References
Details
Attachments
(4 files)
We would provide the visual indicator for the blocked tab to notify user there is something happened, and user can press that icon to resume the media.
Assignee | ||
Updated•8 years ago
|
Blocks: delay-autoplay
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 6•8 years ago
|
||
The bug1308399 is about the UX spec and all UX related materials would be uploaded there.
I'll continue to work on this bug after getting the new SVG file.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•8 years ago
|
||
Maybe you'll want to know about this new feature for tab audio icon because you did the mute/unmuted icons.
Flags: needinfo?(ehsan)
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8799638 [details]
Bug 1308153 - part2 : Add media-unblocking icon.
https://reviewboard.mozilla.org/r/84770/#review88586
::: browser/themes/shared/tabbrowser/tab-audio.svg:10
(Diff revision 3)
> <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
> <style>
> path:not(:target) {
> display: none;
> }
> + .st0 {
Please use a more descriptive name than st0 and st1. Also, why do we want the fill here to be #333333? It seems it would be better to use #00 with opacity:.5;
Comment 13•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8799638 [details]
Bug 1308153 - part2 : Add media-unblocking icon.
https://reviewboard.mozilla.org/r/84770/#review88586
> Please use a more descriptive name than st0 and st1. Also, why do we want the fill here to be #333333? It seems it would be better to use #00 with opacity:.5;
I meant it would be better to use #000 with an opacity property, not necessarily 0.5.
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8799638 [details]
Bug 1308153 - part2 : Add media-unblocking icon.
https://reviewboard.mozilla.org/r/84770/#review89038
Please re-request review after addressing the comments I left in my previous review pass.
Attachment #8799638 -
Flags: review?(jaws) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8798714 [details]
Bug 1308153 - part1 : notify tabbrowser when the tab was blocked.
https://reviewboard.mozilla.org/r/84144/#review89188
Ask some browser peer to review this code too. I just reviewed the AudioChannlService part.
::: browser/base/content/tabbrowser.xml:5114
(Diff revision 5)
> + !event.isTrusted)
> + return;
> +
> + var browser = event.originalTarget;
> + var tab = this.getTabForBrowser(browser);
> + if (!tab)
{}
Attachment #8798714 -
Flags: review?(amarchesini) → review+
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8805033 [details]
Bug 1308153 - part3 : implement the logic about showing the unblocking icon.
https://reviewboard.mozilla.org/r/88858/#review89190
I cannot review browser code.
Attachment #8805033 -
Flags: review?(amarchesini)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•8 years ago
|
||
mozreview-review |
Comment on attachment 8798714 [details]
Bug 1308153 - part1 : notify tabbrowser when the tab was blocked.
https://reviewboard.mozilla.org/r/84144/#review89782
::: browser/base/content/tabbrowser.xml:5108
(Diff revision 6)
> + if (!Services.prefs.getBoolPref("browser.tabs.showAudioPlayingIcon") ||
> + !event.isTrusted) {
> + return;
> + }
> +
> + var browser = event.originalTarget;
> + var tab = this.getTabForBrowser(browser);
> + if (!tab) {
> + return;
> + }
These lines are duplicated here and in DOMAudioPlaybackBlockedEnd. Please pull these out to a separate function that can be called from both.
::: toolkit/content/browser-content.js:986
(Diff revision 6)
> + if (data === "block") {
> + name += "Block";
> + } else {
> - name += (data === "active") ? "Start" : "Stop";
> + name += (data === "active") ? "Start" : "Stop";
> + }
> sendAsyncMessage(name);
The reference above is using \*BlockedStart and \*BlockedEnd. Here it uses "Block" with no -ed, and also "Stop" instead of "End". Can you fix this inconsistency?
Attachment #8798714 -
Flags: review?(jaws) → review+
Comment 26•8 years ago
|
||
mozreview-review |
Comment on attachment 8799638 [details]
Bug 1308153 - part2 : Add media-unblocking icon.
https://reviewboard.mozilla.org/r/84770/#review89786
Attachment #8799638 -
Flags: review?(jaws) → review+
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8805033 [details]
Bug 1308153 - part3 : implement the logic about showing the unblocking icon.
https://reviewboard.mozilla.org/r/88858/#review89788
::: browser/base/content/tabbrowser.xml:6787
(Diff revision 3)
> + if (browser.audioBlocked) {
> + this.removeAttribute("blocked");
> + tabContainer.tabbrowser._tabAttrModified(this, ["blocked"]);
> +
> + // We don't want sound icon flickering between "blocked", "none" and
> + // "sound-playing", here adding the "soundplaying" is to keep the
> + // transition smoothly.
> + if (!this.hasAttribute("soundplaying")) {
> + this.setAttribute("soundplaying", true);
> + tabContainer.tabbrowser._tabAttrModified(this, ["soundplaying"]);
Please change this code to build up an array of which tab attribute are getting modified and only call \_tabAttrModified once with the full array of attributes that were modified.
Attachment #8805033 -
Flags: review?(jaws) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 32•8 years ago
|
||
Now we're waiting for UX's final decision about the icon text, which would be showed when cursor hovers on the icon.
Assignee | ||
Updated•8 years ago
|
Attachment #8805034 -
Flags: review?(jaws)
Comment 33•8 years ago
|
||
mozreview-review |
Comment on attachment 8805034 [details]
Bug 1308153 - part4 : add tooltip for unblock icon.
https://reviewboard.mozilla.org/r/88864/#review92132
::: browser/locales/en-US/chrome/browser/tabbrowser.properties:46
(Diff revision 4)
> # %S is the keyboard shortcut for "Unmute tab"
> tabs.unmuteAudio.tooltip=Unmute tab (%S)
> tabs.muteAudio.background.tooltip=Mute tab
> tabs.unmuteAudio.background.tooltip=Unmute tab
>
> +# LOCALIZATION NOTE (tabs.unblockedAudio.tooltip):
You can remove this comment since there is no accompanying localization note (and it's using the wrong string id anyways).
Attachment #8805034 -
Flags: review?(jaws) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 38•8 years ago
|
||
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/daaf3d17df32
part1 : notify tabbrowser when the tab was blocked. r=baku,jaws
https://hg.mozilla.org/integration/autoland/rev/0dcd1140565d
part2 : Add media-unblocking icon. r=jaws
https://hg.mozilla.org/integration/autoland/rev/a1dcb0e936b0
part3 : implement the logic about showing the unblocking icon. r=jaws
https://hg.mozilla.org/integration/autoland/rev/422c83e9e3be
part4 : add tooltip for unblock icon. r=jaws
Comment 39•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/daaf3d17df32
https://hg.mozilla.org/mozilla-central/rev/0dcd1140565d
https://hg.mozilla.org/mozilla-central/rev/a1dcb0e936b0
https://hg.mozilla.org/mozilla-central/rev/422c83e9e3be
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 40•8 years ago
|
||
(In reply to Alastor Wu [:alwu] from comment #11)
> Maybe you'll want to know about this new feature for tab audio icon because
> you did the mute/unmuted icons.
Thanks! Neat feature. :-)
Flags: needinfo?(ehsan)
Comment 41•8 years ago
|
||
Verified as fixed using the latest Nightly 54.0a1 (Build ID: 20170301030202) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.
status-firefox53:
--- → disabled
status-firefox54:
--- → verified
You need to log in
before you can comment on or make changes to this bug.
Description
•