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)

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- disabled
firefox53 --- disabled
firefox54 --- verified

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.
See Also: 13053381308399
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.
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 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 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 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 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 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 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 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 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+
Now we're waiting for UX's final decision about the icon text, which would be showed when cursor hovers on the icon.
Attachment #8805034 - Flags: review?(jaws)
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+
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
(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: 1319771
Depends on: 1327045
Depends on: 1327205
Depends on: 1328058
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: