Closed Bug 1477926 Opened 3 years ago Closed 3 years ago

Block Autoplay is not working as expected on "nytimes.com"

Categories

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

63 Branch
defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox63 --- affected

People

(Reporter: gpalko, Assigned: cpearce)

References

Details

Attachments

(3 files)

[Environment:]
Windows 10, Mac OSX 10.13
Nightly 63.0a1 BuildId 20180723220051

[Steps:]
1. Open https://www.nytimes.com/video
2. Click on a video from the page

[Actual Result:]
 The block autoplay doorhanger is displayed but the video starts playing before any action is made by the user. 

[Expected Result:]
 The video should not start before any user action on the page.
The site is figuring out that it can't play audibly, and it's falling back to playing inaudibly, and we're not hiding the doorhanger. We hide the doorhanger when playback becomes audible, but not if inaudible playback starts. We probably should...
Flags: needinfo?(gyula.palko)
In my opinion we should hide the doorhanger in this case... it could be confusing for the users, because the video starts to play even if they select "Don't Allow". 
Also, we could consider this as an edge case, but we don't know how many sites are having this behavior...
Flags: needinfo?(gyula.palko)
Interestingly it never starts playing if the user has chosen to block auto play before.
Rank: 15
Priority: -- → P2
(In reply to Chris Pearce (:cpearce) from comment #1)
> The site is figuring out that it can't play audibly, and it's falling back
> to playing inaudibly, and we're not hiding the doorhanger. We hide the
> doorhanger when playback becomes audible, but not if inaudible playback
> starts. We probably should...

I think some Reddit media are too affected by the same issue.

Steps to reproduce:
1. Visit https://www.reddit.com/r/funny/comments/91vs4k/im_trying_to_sleep_here/
  ** Make sure Reddit is serving the new interface. (You'll know because old interface does not trigger the doorhanger.)
2. Right click on the video -> Show Controls
3. Play the video. 

Actual Result:
The doorhanger stays despite any mouse interactions in the page.
=======================


Extension Imagus is probably affected too when previewing v.redd.it videos. Imagus plays a video simply by hovering on a link. It does not support playing the audio for v.redd.it videos however.

Steps to reproduce:
1. Install Imagus at https://addons.mozilla.org/firefox/addon/imagus/.
2. Visit https://old.reddit.com/r/funny/comments/91zow0/the_free_antivirus_software_that_comes_with_your/
 *** Do not click/interact anything in the page.
3. Hover on the v.redd.it link titled "The free antivirus software that comes with your computer".

The doorhanger should appear now. The pop-up video shows but does not play.

4. Click somewhere in the page. And hover v.redd.it link again.

Actual result:
The pop-up video now plays but the doorhanger stays.
Assignee: nobody → cpearce
The code looks good to me, but I am not sure that to keep the doorhanger until all tokes are cancel is good idea or not. 

Assume that the page have two audible media element (A & B), when it found that the media can't start successfully, it would retry to call video.play() for media element A until the media element A starts playing, and do nothing for B.

So, in this situation, if user has interacted with the page (eg. click), the media element A would starts playing.

However, B won't be started unless user clicks B's play button (if it has), the token of media element B would be still pending. It causes that the doorhanger doesn't disappear even when the page has started audible autoplay.

---

It makes me thinking about whether we should disable enable the feature which would activate the page by user gesture when the doorhanger is showing. Or, simplily to dismiss doorhanger when media starts playing, not only audible media. 

In addition, another problem (not directly related with this bug) is that the doorhanger seems having some conflict with the design of user gesture activation. Because now doorhanger would only be showed before the page hasn't been activated yet. 

That mean the doorhanger couldn't make sure to 100% prompt user when audible autoplay starts. If the page has been activated, the doorhanger won't be showed and user might feel confused why the autoplay starts but no doorhanger is showing.
Flags: needinfo?(cpearce)
(In reply to Alastor Wu [:alwu] from comment #8)
> The code looks good to me, but I am not sure that to keep the doorhanger
> until all tokes are cancel is good idea or not. 
> 
> Assume that the page have two audible media element (A & B), when it found
> that the media can't start successfully, it would retry to call video.play()
> for media element A until the media element A starts playing, and do nothing
> for B.
> 
> So, in this situation, if user has interacted with the page (eg. click), the
> media element A would starts playing.
> 
> However, B won't be started unless user clicks B's play button (if it has),
> the token of media element B would be still pending. It causes that the
> doorhanger doesn't disappear even when the page has started audible autoplay.


So basically, with the patches here, we'll leave the doorhanger visible as long as there are unfulfilled play() promises.

Once an HTMLMediaElement plays, any other unfulfilled play() promises for that HTMLMediaElement are resolved, since the media is playing. So in your example, if the user clicks on B to start it playing and A is already playing, the pending play() promise would resolve, and the doorhanger would also disappear.


> It makes me thinking about whether we should disable enable the feature
> which would activate the page by user gesture when the doorhanger is
> showing. Or, simplily to dismiss doorhanger when media starts playing, not
> only audible media. 

So if we dismissed the doorhanger when one HTMLMediaElement starts playing, what do we do with the play promises pending on the other HTMLMediaElements that *weren't* played? Do we reject their promises? Or never resolve them? Leaving them unfulfilled seems bad to me.

 
> In addition, another problem (not directly related with this bug) is that
> the doorhanger seems having some conflict with the design of user gesture
> activation. Because now doorhanger would only be showed before the page
> hasn't been activated yet. 
> 
> That mean the doorhanger couldn't make sure to 100% prompt user when audible
> autoplay starts.

We don't want the doorhanger to show on 100% of audible plays. When the user clicks on a play button to play a video, we don't want the doorhanger to show, that's why we have gesture activation, to prevent the doorhanger being shown.

> If the page has been activated, the doorhanger won't be
> showed and user might feel confused why the autoplay starts but no
> doorhanger is showing.


I think your underlying concern is that some gestures do not demonstrate the user's intent to play. To really solve that we'd need to only allow play() calls that happened in user generated event handlers. I tried this, and unfortunately lots of web sites break, so I don't think that's feasible.
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #9) 
> So basically, with the patches here, we'll leave the doorhanger visible as
> long as there are unfulfilled play() promises.
> 
> Once an HTMLMediaElement plays, any other unfulfilled play() promises for
> that HTMLMediaElement are resolved, since the media is playing. So in your
> example, if the user clicks on B to start it playing and A is already
> playing, the pending play() promise would resolve, and the doorhanger would
> also disappear.
> 

I know the doorhanger would disappear after B's promise is resolved, but it's not my point.

My concern is that, in this case, user would see the website start autoplay an audible media element, but the doorhanger still shows. User might not realize that "oh because there is another play promise is not pending, so the doorhanger is still there". They are just seeing the website starts playing without their permission.

In addition, if people is complaining about too much doorhangers, is it really good to make doorhanger even harder to disappear? 

> So if we dismissed the doorhanger when one HTMLMediaElement starts playing,
> what do we do with the play promises pending on the other HTMLMediaElements
> that *weren't* played? Do we reject their promises? Or never resolve them?
> Leaving them unfulfilled seems bad to me.

I think to dismiss doorhanger immediately after the non-audible media element starts playing is also not good. If the website always play the non-audible media before audible media, the doorhanger would lose its effect.

How about adding a timer for the doorhanger? If there is any media element starting playing, the doorhanger would automatically dismiss after passing specific time period. It should be same as the result when user press 'ESC', so we reject all the other pending promises.

Plus that, to disable the user activation when doorhanger is showing, it forces that the doorhanger is the only way to allow the autoplay.

> We don't want the doorhanger to show on 100% of audible plays. When the user
> clicks on a play button to play a video, we don't want the doorhanger to
> show, that's why we have gesture activation, to prevent the doorhanger being
> shown.

See below.

> I think your underlying concern is that some gestures do not demonstrate the
> user's intent to play. To really solve that we'd need to only allow play()
> calls that happened in user generated event handlers. I tried this, and
> unfortunately lots of web sites break, so I don't think that's feasible.

What I want to mention is, if the page has been activated before starting any autoplay media, we won't show the doorhanger and people won't know why.
(In reply to Alastor Wu [:alwu] from comment #10)
> Plus that, to disable the user activation when doorhanger is showing, it
> forces that the doorhanger is the only way to allow the autoplay.

Ah no, it couldn't work. Because we need the user activation to start the media...
(In reply to Alastor Wu [:alwu] from comment #10)
> (In reply to Chris Pearce (:cpearce) from comment #9) 
> > So basically, with the patches here, we'll leave the doorhanger visible as
> > long as there are unfulfilled play() promises.
> > 
> > Once an HTMLMediaElement plays, any other unfulfilled play() promises for
> > that HTMLMediaElement are resolved, since the media is playing. So in your
> > example, if the user clicks on B to start it playing and A is already
> > playing, the pending play() promise would resolve, and the doorhanger would
> > also disappear.
> > 
> 
> I know the doorhanger would disappear after B's promise is resolved, but
> it's not my point.
> 
> My concern is that, in this case, user would see the website start autoplay
> an audible media element, but the doorhanger still shows. User might not
> realize that "oh because there is another play promise is not pending, so
> the doorhanger is still there". They are just seeing the website starts
> playing without their permission.


I think this is a valid point; users won't necessarily realize there are multiple videos trying to play.


> > So if we dismissed the doorhanger when one HTMLMediaElement starts playing,
> > what do we do with the play promises pending on the other HTMLMediaElements
> > that *weren't* played? Do we reject their promises? Or never resolve them?
> > Leaving them unfulfilled seems bad to me.
> 
> I think to dismiss doorhanger immediately after the non-audible media
> element starts playing is also not good. If the website always play the
> non-audible media before audible media, the doorhanger would lose its effect.

I don't understand your point here sorry. If a website plays non-audible media it makes sense that we not show the doorhanger there; the doorhanger is requesting permission to play audible media.

We hide the doorhanger if an audible media that has tried to autoplay switches to playing in audibly, because we're no longer 
waiting on the doorhanger.


> How about adding a timer for the doorhanger? If there is any media element
> starting playing, the doorhanger would automatically dismiss after passing
> specific time period. It should be same as the result when user press 'ESC',
> so we reject all the other pending promises.


So I don't like the idea of a timeout... But I'm leaning towards just rejecting the other promises outright, as if the user plays one of the video elements they've shown intent to play one but not the others.

It also occurred to me that this is probably a rare edge case that isn't worth worrying to much about (how many pages on the internet have multiple audible video that play at the same time?), so if the strategy of just rejecting is wrong, we can come back later and fix it then.

So how about we just reject the other promises when one of the videos plays?
Flags: needinfo?(alwu)
Comment on attachment 8996598 [details]
Bug 1477926 - Test that we hide the autoplay-media permission doorhanger and cancel permission requests when a media element starts playing.

https://reviewboard.mozilla.org/r/260700/#review268184


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python/etc)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:361
(Diff revision 2)
> +    ok(promptShowing(), "Should now be showing permission prompt");
> +
> +    // Check that the video didn't start playing.
> +    await checkAllVideosNotPlayed(browser);
> +
>      let popuphidden = BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popuphidden");

Error: 'popuphidden' is assigned a value but never used. [eslint: no-unused-vars]
Comment on attachment 8996597 [details]
Bug 1477926 - Hide autoplay-media prompt on DOMAudioPlaybackAbortPermissionRequest.

https://reviewboard.mozilla.org/r/260698/#review268216

Makes sense, thanks!

::: commit-message-0e1ea:7
(Diff revision 2)
> +
> +Currently we're hiding the autoplay-media permission doorhanger when we detect
> +playback in the tab via a DOMAudioPlaybackStarted event. However this only
> +fires for audible playback in a tab; if a media element tries to play audibly,
> +detects that it's blocked, and then switches to playing inaudibly, we'll be
> +left still be showing the doorhanger even though it's not connected to

typo: left still be showing
Attachment #8996597 - Flags: review?(jhofmann) → review+
Comment on attachment 8996596 [details]
Bug 1477926 - Cancel autoplay-media permission requests when any media element waiting on a permission request plays.

https://reviewboard.mozilla.org/r/260696/#review268284

r+ with handling following issues. Thank you!

::: dom/html/AutoplayPermissionManager.cpp:95
(Diff revision 2)
> +AutoplayPermissionManager::CancelAllRequests()
> +{
> +  DenyPlayRequest();
> +
> +  // Send a message to chrome that it can hide the doorhanger.
> +

redundant line.

::: dom/html/HTMLMediaElement.cpp:6430
(Diff revision 2)
>      return;
>    }
>  
>    nsCOMPtr<nsIRunnable> event;
>  
>    if (aName.EqualsLiteral("playing")) {

Here is for disscusion.

I'm not sure canceling request after "play" event or canceling after "playing" event, which one is better.

(1) canceling after "play"
It make that the timing dismiss doorhanger matchs the timing user clicks interface. (if has)

(2) canceling after "playing"
It make that the timing dismiss doorhanger matchs the timing media starts playing.

::: dom/html/HTMLMediaElement.cpp:6431
(Diff revision 2)
>    }
>  
>    nsCOMPtr<nsIRunnable> event;
>  
>    if (aName.EqualsLiteral("playing")) {
> +    if (mAutoplayPermissionRequest.Exists()) {

You can wrap those parts into a function, and also need to call it in CheckAutoplayDataReady().
Attachment #8996596 - Flags: review?(alwu) → review+
Comment on attachment 8996597 [details]
Bug 1477926 - Hide autoplay-media prompt on DOMAudioPlaybackAbortPermissionRequest.

https://reviewboard.mozilla.org/r/260698/#review268294

::: browser/modules/PermissionUI.jsm:837
(Diff revision 2)
> -      "DOMAudioPlaybackStarted", this.handlePlaybackStart);
> +      "DOMAudioPlaybackAbortPermissionRequest", this.handlePlaybackStart);
>    },
>  
>    onBeforeShow() {
>      // Hide the prompt if the tab starts playing media.
>      this.handlePlaybackStart = () => {

maybe change this function to the name like "handleAbortingPermission"?
Comment on attachment 8996598 [details]
Bug 1477926 - Test that we hide the autoplay-media permission doorhanger and cancel permission requests when a media element starts playing.

https://reviewboard.mozilla.org/r/260700/#review268302

LGTM, thank you!

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:233
(Diff revision 2)
> +      } else {
> +        ok(false, "Invalid unblock mode.");
> +      }
> +      // Trying to play now that we're unblocked should work...
> +      let played = await video.play().then(() => true, () => false);
> +      ok(played, "Should have played as now muted");

as now muted or activated window.
Attachment #8996598 - Flags: review?(alwu) → review+
Flags: needinfo?(alwu)
Comment on attachment 8996596 [details]
Bug 1477926 - Cancel autoplay-media permission requests when any media element waiting on a permission request plays.

https://reviewboard.mozilla.org/r/260696/#review268284

> Here is for disscusion.
> 
> I'm not sure canceling request after "play" event or canceling after "playing" event, which one is better.
> 
> (1) canceling after "play"
> It make that the timing dismiss doorhanger matchs the timing user clicks interface. (if has)
> 
> (2) canceling after "playing"
> It make that the timing dismiss doorhanger matchs the timing media starts playing.

I think this is a good point. Firing on "play" makes sense.

> You can wrap those parts into a function, and also need to call it in CheckAutoplayDataReady().

Note that CheckAutoplayDataReady() fires the "play" (and "playing") events, so since we don't need to explicitly check in CheckAutoplayDataReady(), as we're already doing this.

https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/dom/html/HTMLMediaElement.cpp#6285
We talked about this at the block autoplay work week and decided that we'd rather leave the doorhanger there, rather than hide it. Often sites try to play audibly, decide they're blocked, and then play inaudibly. Sometimes they do this for multiple videos (abcnews for example), so that would cause the doorhanger to popup and hide and popup and hide multiple times. We'd rather just leave the doorhanger there.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.