Closed Bug 1477415 Opened 2 years ago Closed 2 years ago

Autoplay is blocked even on standalone top level audio/video document

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox63 --- verified

People

(Reporter: evilpie, Assigned: alwu)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

I think people expect top level audio/video to play automatically. For example: https://www.sample-videos.com/audio/mp3/crowd-cheering.mp3
Agree, Chrome allows video document to autoplay as well.
Assignee: nobody → alwu
Priority: -- → P2
Comment on attachment 8994330 [details]
Bug 1477415 - part1 : allow video document autoplay.

https://reviewboard.mozilla.org/r/258856/#review265908

r+ with HTMLMediaElement::mIsCreatedByVideoDocument removed.

::: dom/media/AutoplayPolicy.cpp:123
(Diff revision 1)
>    if (aElement.Volume() == 0.0 || aElement.Muted()) {
>      return nsIAutoplay::ALLOWED;
>    }
>  
> +  // Allow autoplay when user directly views the video/audio url
> +  if (aElement.IsCreatedByVideoDocument()) {

I think you can just check whether aElement.OwnerDoc()->MediaDocumentKind() == MediaDocumentKind::Video here instead of having to set a flag to store whether the HTMLMediaElement is in a MediaDocument.
Attachment #8994330 - Flags: review?(cpearce) → review+
Comment on attachment 8994331 [details]
Bug 1477415 - part2 : add test.

https://reviewboard.mozilla.org/r/258858/#review265910

::: toolkit/content/tests/browser/browser_autoplay_videoDocument.js:8
(Diff revision 1)
> +const PAGE = "https://example.com/browser/toolkit/content/tests/browser/audio.ogg";
> +
> +function setup_test_preference() {
> +  return SpecialPowers.pushPrefEnv({"set": [
> +    ["media.autoplay.default", SpecialPowers.Ci.nsIAutoplay.PROMPT],
> +    ["media.autoplay.enabled", false],

The "media.autoplay.enabled" pref has been removed, so you can remove it here too.

::: toolkit/content/tests/browser/browser_autoplay_videoDocument.js:14
(Diff revision 1)
> +    ["media.autoplay.enabled.user-gestures-needed", true],
> +    ["media.autoplay.ask-permission", true],
> +  ]});
> +}
> +
> +function checkIsVideoDocumentAutoplay(browser) {

There's a few things wrong here...

When you have an async function, the interpreter wraps the function with a state machine that yields control when you "await", returning a promise, and resuming execution when the promise the async function is awaiting on fulfils and the event loop is free.

If your async function runs to completion, the automatically generated promise resolves with the return value (undefined if your async function didn't return anything). If anything inside the async function threw an exception, the promise is rejected with the thrown exception.

Whereas here, you're returning a promise that contains an async function. The extra promise is unnecessary, as you have one being automatically generated here by the interpreter because your function has the "async" keyword.

Also it looks like you're manually creating a new promise here so that you can reject() on failure, but you're not actually checking whether the promise returned by checkIsVideoDocumentAutoplay() is resolved or rejected, so the extra promise is unnececssary.

Also, you're handling the case of the video having played to completion or potentially having nto started playback yet. This means sometimes you're testing that cideo documents play automatically, and other times you're testing that calling play() in a MediaDocument works.

You also can make the test cleaner by just testing whether calling play() in a MediaDocument works. I think that's sufficient.

So you can should be able to make this:

    return ContentTask.spawn(browser, null, async () => {
      let video = content.document.getElementsByTagName("video")[0];
      let played = video && await video.play().then(() => true, () => false);
      ok(played, "Should be able to play in video document");
    });

::: toolkit/content/tests/browser/browser_autoplay_videoDocument.js:44
(Diff revision 1)
> +add_task(async function testAutoplayVideoDocument() {
> +  info("- setup test preference -");
> +  await setup_test_preference();
> +
> +  info("- open new foreground tab -");
> +  let tab = await BrowserTestUtils.openNewForegroundTab(window.gBrowser,

You can use BrowserTestUtils.withNewTab() to automatically load the URI and remove the tab afterwards. See browser_autoplay_policy_request_permission.js as an example.
Attachment #8994331 - Flags: review?(cpearce) → review+
Thank for your suggestion!
I'll modify my patches and upload them later.
I think you need to make sure it is a top-level document, otherwise <iframe src="http://xxx/xxx.mp3"> would be allowed to play automatically.
(In reply to Tom Schuster [:evilpie] from comment #12)
> I think you need to make sure it is a top-level document, otherwise <iframe
> src="http://xxx/xxx.mp3"> would be allowed to play automatically.

We make the video document always can autoplay that is a document which only contains one media element. It happens when user visit video or audio by directly visiting its url.
Keywords: checkin-needed
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 33761c9a58efb2c7268f978d38a89c092061b1ce -d 5389bb094bc4: rebasing 474474:33761c9a58ef "Bug 1477415 - part1 : allow video document autoplay. r=cpearce"
merging dom/media/AutoplayPolicy.cpp
warning: conflicts while merging dom/media/AutoplayPolicy.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/784d6695694c
part1 : allow video document autoplay. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/8890b8f7e9cc
part2 : add test. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/784d6695694c
https://hg.mozilla.org/mozilla-central/rev/8890b8f7e9cc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Duplicate of this bug: 1478599
Depends on: 1480738
Comment on attachment 8994330 [details]
Bug 1477415 - part1 : allow video document autoplay.

https://reviewboard.mozilla.org/r/258856/#review268700

::: dom/media/AutoplayPolicy.cpp:107
(Diff revision 3)
>  
>  static bool
>  IsMediaElementAllowedToPlay(const HTMLMediaElement& aElement)
>  {
>    return ((aElement.Volume() == 0.0 || aElement.Muted()) &&
> -          Preferences::GetBool("media.autoplay.allow-muted", true)) ||
> +           Preferences::GetBool("media.autoplay.allow-muted", true)) ||

Indentation is wrong here...
(In reply to Jean-Yves Avenard [:jya] from comment #21)
> ::: dom/media/AutoplayPolicy.cpp:107
> (Diff revision 3)
> >  
> >  static bool
> >  IsMediaElementAllowedToPlay(const HTMLMediaElement& aElement)
> >  {
> >    return ((aElement.Volume() == 0.0 || aElement.Muted()) &&
> > -          Preferences::GetBool("media.autoplay.allow-muted", true)) ||
> > +           Preferences::GetBool("media.autoplay.allow-muted", true)) ||
> 
> Indentation is wrong here...

This has been fixed :)
Verified on Nightly 63.0a1(20180819221915), that autoplay is not blocked on top level audio document.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.