1471485 - Hide autoplay permission doorhanger if user plays video

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, enhancement, P2)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

In Bug 1463919 we're adding a doorhanger permission prompt to request permission to autoplay media. If the prompt shows, and the user is otherwise able to play the media, for example by clicking a play button, we should hide the doorhanger.

If the playing HTMLMediaElement is the one for which we have play promises pending the resolution of the door hanger permission request, we should probably resolve those promises, otherwise we should probably reject the promises as something else is playing.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1471485 - Hide autoplay permission doorhanger if user plays video.

If we're showing a permission UI prompt for "autoplay-media", the user can
still actually play media without interacting with the doorhanger; if they
click a "play" button in the document, they'll "gesture activate" the document
and unblock autoplay and be able to start playback.

It doesn't make sense to keep showing the permission doorhanger to approve
autoplay when the page is already playing, as playback has already started, and
if they clicked on "block" then the site would receive a promise reject on the
promise returned on the first call to HTMLMediaElement.play() for which we were
showing the permission prompt for, even though the media is actually playing.
This will likely confuse JS video players.

So we should hide the permission prompt when playback in the page starts.

Review commit: https://reviewboard.mozilla.org/r/254368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254368/

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8989298 [details]
Bug 1471485 - Hide autoplay permission doorhanger if user plays video.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254368/diff/1-2/

Comment 3

[Johann Hofmann [:johannh]]

Comment on attachment 8989298 [details]
Bug 1471485 - Hide autoplay permission doorhanger if user plays video.

https://reviewboard.mozilla.org/r/254368/#review261702

I'm a bit unsure whether we can get by without removing the event listener here (see below).

::: browser/modules/PermissionUI.jsm:821
(Diff revision 2)
>          action: Ci.nsIPermissionManager.DENY_ACTION,
>      }];
>    },
>  
>    onBeforeShow() {
> +    // Hide the prompt if the tab stats playing media.

typo: starts

::: browser/modules/PermissionUI.jsm:822
(Diff revision 2)
>      }];
>    },
>  
>    onBeforeShow() {
> +    // Hide the prompt if the tab stats playing media.
> +    this.browser.addEventListener("DOMAudioPlaybackStarted", (e) => {

Correct me if I'm wrong, but if we don't hit this case this listener keeps on listening as long as that browser lives, right?

So, I'm not sure about the lifetime of that request object, but it seems a little unintentional to handle it based on playback on different sites.

To solve this we might need another helper method in the spirit of onBeforeShow (onAfterRemove?) and use it to unregister the callback when the notification is removed. Since that always happens on navigation or user interaction it seems right to me.

The place to call such a method would be in this eventCallback:

https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/browser/modules/PermissionUI.jsm#364

if the topic is "removed" (see https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/toolkit/modules/PopupNotifications.jsm#379)

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:234
(Diff revision 2)
> +      async () => {
> +        let video = content.document.getElementById("v1");
> +        ok(video.paused && !video.didPlay, "Video should not be playing");
> +      });
> +
> +    let pophidden = BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popuphidden");

nit: did you mean to call this variable "popuphidden"?

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1471485 - Ensure autoplay permission promises disconnected if media starts playing.

We can start playing while we're awaiting a response to an autoplay-media
permission prompt, for example if the user clicks on a play button. In such
cases, it doesn't make sense to keep the autoplay permission request promise
connected in HTMLMediaElement, as since we're playing we'll be resolving the
play() promises and thus we won't be taking action on the autoplay request
promise's result. So we should just disconnect the autoplay permission request
promise if it's connected when we start playing.

Review commit: https://reviewboard.mozilla.org/r/256504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256504/

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8989298 [details]
Bug 1471485 - Hide autoplay permission doorhanger if user plays video.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254368/diff/2-3/

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

I've rebased the fix here on top of Bug 1472580, which fixes a bunch of issues with the doorhanger prompt not always giving a response to the requester. I had some manual request.cancel() calls in my previous iteration of the patch, but they're not needed once the patches here are based on top of Bug 1472580.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8991573 [details]
Bug 1471485 - Ensure autoplay permission promises disconnected if media starts playing.

https://reviewboard.mozilla.org/r/256504/#review263368

::: dom/html/HTMLMediaElement.cpp:7869
(Diff revision 1)
>  }
>  
>  void
>  HTMLMediaElement::AsyncResolvePendingPlayPromises()
>  {
> +  // Disconnect requests for permission to play. We're playing either way,

How is this information passed on to the doorhanger , that is: the media is no longer playing and the doorhanger should disappear?

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8989298 [details]
Bug 1471485 - Hide autoplay permission doorhanger if user plays video.

https://reviewboard.mozilla.org/r/254368/#review263958

This looks great, thank you!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/529c718d618c
Hide autoplay permission doorhanger if user plays video. r=johannh
https://hg.mozilla.org/integration/autoland/rev/7764e30046d3
Ensure autoplay permission promises disconnected if media starts playing. r=jya

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/529c718d618c
https://hg.mozilla.org/mozilla-central/rev/7764e30046d3

Comment 12

[Gyula Palko[:gpalko]]

Verified on Nightly 63.0a1(20180823100106), that the autoplay doorhanger is hidden if the user plays media.