Implement the new UX visual mechanism for blocking autoplay media

RESOLVED FIXED in Firefox 54

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
7 months ago
2 months ago

People

(Reporter: alwu, Assigned: alwu)

Tracking

(Blocks: 1 bug)

Other Branch
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 disabled, firefox53 disabled, firefox54 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

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

7 months ago
Blocks: 1308154
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
See Also: bug 1305338bug 1308399
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Priority: -- → P3
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)
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

6 months 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

6 months 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

6 months 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

6 months 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

6 months 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

6 months 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

6 months 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

6 months 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)
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

6 months ago
Attachment #8805034 - Flags: review?(jaws)

Comment 33

6 months 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

6 months 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

6 months 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
Last Resolved: 6 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(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)
Depends on: 1318462
Depends on: 1319526
Depends on: 1319529

Updated

5 months ago
Depends on: 1319771

Updated

4 months ago
Depends on: 1327045

Updated

4 months ago
Depends on: 1327205

Updated

4 months ago
Depends on: 1328058

Comment 41

2 months 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-firefox52: fixed → disabled
status-firefox53: --- → disabled
status-firefox54: --- → verified
Depends on: 1320005
No longer depends on: 1318462
You need to log in before you can comment on or make changes to this bug.