Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

All video in Brave should default with autoplay=false, w/site level opt-in exceptions #7663

Closed
lukemulks opened this issue Mar 12, 2017 · 5 comments
Closed

All video in Brave should default with autoplay=false, w/site level opt-in exceptions #7663

lukemulks opened this issue Mar 12, 2017 · 5 comments
Assignees
@bridiver @BrendanEich @jonathansampson @lukemulks
Labels
feature/autoplay QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Milestone
0.15.300 (Frozen,...

Comments

@lukemulks
Copy link
Collaborator

lukemulks commented Mar 12, 2017 •
edited by luixxiul
Loading

Test plan

#8609 (comment)

  • Did you search for similar issues before submitting this one?

Yes - Any features that appear similar also appear to be optional in nature. This specific issue is filed to make the default behavior for all video to not autoplay (requested from @BrendanEich ).

Autoplay should be opt-in v opt-out within Brave. Minimal negative impact, as many/most mobile browsers require a tap to initiate playback (autoplay = disabled by default on mobile). User should be able to enable globally via prefs to opt-in if desired.

The fast fix for this appears to be at the DOM level, via video-dom-autoplay.htm
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/media/video-dom-autoplay.html?dr=C

<!DOCTYPE html>
<title>Test media "autoplay" attribute set via DOM.</title>
<script src="../resources/testharness.js"></script>
<script src="../resources/testharnessreport.js"></script>
<script src="media-file.js"></script>
<video></video>
<script>
async_test(function(t) {
    var video = document.querySelector("video");
    assert_false(video.autoplay);
    video.autoplay = true;
    assert_true(video.autoplay);
    assert_not_equals(video.getAttribute("autoplay"), null);

    video.onplay = t.step_func_done(function() {
        assert_false(video.paused);
    });

    video.src = findMediaFile("video", "content/test");
});
</script>

This appears to be the test for ensuring the above setting functions as expected:

https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/media/video-no-autoplay.html?dr=C

<!DOCTYPE html>
<title>Test that play event does not fire when "src" set with no autoplay attribute.</title>
<script src="../resources/testharness.js"></script>
<script src="../resources/testharnessreport.js"></script>
<script src="media-file.js"></script>
<video></video>
<script>
async_test(function(t) {
    var video = document.querySelector("video");
    assert_true(video.paused);

    video.onplay = t.unreached_func();

    video.oncanplaythrough = t.step_func(function() {
        setTimeout(t.step_func_done(function() {
            assert_true(video.paused);
        }), 500);
    });

    video.src = findMediaFile("video", "content/test");
});
</script>
  • Platform (Win7, 8, 10? macOS? Linux distro?):
    All

@bridiver @BrendanEich The above = what I've been able to gather from searching the chromium source, so I'm hoping that if it doesn't provide the full solve, that it at least moves the needle to getting default video playback behavior to disable autoplay across the browser.

IIRC mobile is a non-issue w/autoplay.

#4045 #2227
@lukemulks
Copy link
Collaborator Author

I ran a quick check in muon and I think this is where we could update to disable autoplay:
https://github.com/brave/muon/blob/3571a34c91ca0d1e1c25837cd6e550b63a1eeb9a/chromium_src/chrome/renderer/content_settings_observer.cc#L511

Could be some other locations too, but that appears most likely. There are 5 instances of autoplay mentioned on that page, and a couple additional results came up when queried in the muon repo, but the above looks most likely in my non-C++ savvy mind.

@lukemulks lukemulks changed the title Default Behavior: All video in Brave should default with autoplay=false All video in Brave should default with autoplay=false, w/site level opt-in exceptions Mar 28, 2017
@lukemulks
Copy link
Collaborator Author

Update:
@bridiver pointed out that browser-laptop has the contentSettings.js file where we can make the global behavior change for autoplay to false. This is the location for the global update:

browser-laptop/js/state/contentSettings.js

Line 280 in 6f9a4f2

})

In addition to global behavior, we are going to want to have site-level exceptions, most likely in the site-level Shields menu. Discussing approach with the team on this portion now.

We may set default behavior first, while developing the exceptions in the UI/config. TBD, in process of discussing this now.

@lukemulks
Copy link
Collaborator Author

@jonathansampson from the product discussion via @BrendanEich , the latest with location for the update. vc @bradleyrichter

@cndouglas cndouglas mentioned this issue Apr 22, 2017
Closed
@jonathansampson jonathansampson mentioned this issue May 1, 2017
Merged
4 tasks
@darkdh darkdh added this to the 0.15.3 milestone May 5, 2017
@alexwykoff alexwykoff mentioned this issue May 5, 2017
Closed
21 tasks
@srirambv srirambv mentioned this issue May 6, 2017
Closed
This was referenced May 23, 2017
Closed
Closed
@alexwykoff alexwykoff mentioned this issue May 25, 2017
Closed
@tomm174
Copy link

tomm174 commented Jun 8, 2018

Autoplay is definitely still happening in my browser (freshly downloaded 08/06/18 )
Version Info : Brave | 0.22.727, V8 | 6.6.346.32, rev | 2c94bba, Muon | 6.0.12

Examples :
https://www.w3schools.com/TAgs/tryit.asp?filename=tryhtml5_video_autoplay
http://www.wpmarketertools.com/autotube-2/

Could it be that I haven't yet found the relevant preference ?

@cndouglas
Copy link

@tomm174 Check your autoplay setting (Settings > Security > Autoplay Media):

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bridiver bridiver

@BrendanEich BrendanEich

@jonathansampson jonathansampson

@lukemulks lukemulks

Labels
feature/autoplay QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Projects
None yet
Milestone
0.15.300 (Frozen, only critical adds ...
Development

No branches or pull requests
10 participants
@bridiver @BrendanEich @jonathansampson @alexwykoff @tomm174 @luixxiul @lukemulks @darkdh @srirambv @cndouglas