Closed
Bug 1477415
Opened 6 years ago
Closed 6 years ago
Autoplay is blocked even on standalone top level audio/video document
Categories
(Core :: Audio/Video: Playback, defect, P2)
Core
Audio/Video: Playback
Tracking
()
VERIFIED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | verified |
People
(Reporter: evilpie, Assigned: alwu)
References
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
Assignee | ||
Comment 1•6 years ago
|
||
Agree, Chrome allows video document to autoplay as well.
Assignee: nobody → alwu
Updated•6 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
Comment 5•6 years ago
|
||
mozreview-review |
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 6•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 7•6 years ago
|
||
Thank for your suggestion!
I'll modify my patches and upload them later.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
Comment hidden (mozreview-request) |
Reporter | ||
Comment 12•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•6 years ago
|
||
(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.
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 15•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•6 years ago
|
||
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
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/784d6695694c
https://hg.mozilla.org/mozilla-central/rev/8890b8f7e9cc
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 21•6 years ago
|
||
mozreview-review |
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...
Assignee | ||
Comment 22•6 years ago
|
||
(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 :)
Comment 23•6 years ago
|
||
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.
Description
•