cloneNode of html5 muted video turns on the volume
Reported by
sentimen...@gmail.com,
Feb 13 2018
|
||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.140 Safari/537.36 Steps to reproduce the problem: 1. Add a video tag with autoplay and muted attrs 2. Clone that video (cloneNode(true)) 3. Audio starts playing! But we haven't even pasted the cloned elem to the page. It is rather weird Example: http://tabasca.github.io/index.html What is the expected behavior? Clone evt doesn't need to set audio on. What went wrong? After cloning, clone's audio starts playing Did this work before? Yes 63 Does this work in other browsers? No Can reproduce this bug in FF (found that it was broken from 41 version of FF!) Other browsers (IE11, Edge, Safari 11) work as expected Chrome version: 64.0.3282.140 Channel: stable OS Version: 10.0 Flash Version: The only workaround is to manually set muted attr via javascript to the inserted clone-elem.
,
Feb 13 2018
,
Feb 14 2018
sentimentaleq@ Thanks for the issue. Tested this issue on Mac OS 10.12.6, Windows 10 and Ubuntu 14.04 on the Stable 64.0.3282.167 and Canary 66.0.3347.0 and able to reproduce the issue by following the steps mentioned above. Bisect Information: =================== Good Build: 64.0.3254.0 (Revision - 512694) Bad Build : 64.0.3255.0 (Revision - 513029) On executing the per-revision bisect script, below is the Changelog URL: https://chromium.googlesource.com/chromium/src/+log/6803a25bec5591feb3863bfe5e7aa7bf4b10a507..343baf357156bea80f4251c3df1a2ef931086e57 From the above Changelog, suspecting the below change: Reviewed-on: https://chromium-review.googlesource.com/739262 srirama.m@ Please check and confirm if this issue is related to your change, else help us in assigning to the right owner. Adding ReleaseBlock-Stable as this is a recent regression. Please feel to remove the same if it is not applicable. Thanks.
,
Feb 14 2018
The bisect information is correct. Verified in firefox and the behaviour is same.
,
Feb 20 2018
Let's target this for M65.
,
Feb 21 2018
M65 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Merge has to happen latest by 4:00 PM PT Monday (02/26/18) in order to make it to last M65 beta release next week. Thank you.
,
Feb 22 2018
@Mounir, can we duplicate this to 813544 ?
,
Feb 22 2018
This doesn't seem like an obvious duplicate because it's using clone() and not setting the attribute. Did I miss something?
,
Feb 22 2018
Right, it is not exactly duplicate, though the change for the regression is same. Now in this case the element is not added to tree, so the function ParserDidSetAttributes() never gets called and the muted attribute never gets set. Interestingly it is same behaviour in mozilla. Setting muted_ flag in loadResource() though solves this problem but there is one failure in wpt test. WDYT we should do now?
,
Feb 26 2018
Gentle ping !! It is marked as stable blocker for M65. Please merge fix ASAP as M65 stable promotion is scheduled today night. Thanks..!
,
Feb 26 2018
I can confirm that this is working as intended on Safari and Edge and broken on Chrome and Firefox.
,
Feb 26 2018
+foolip@ FYI as muted is weird and he might want to know about the weird cases we are hitting for the spec. srirama, I would suggest do override one of the Clone* methods from Element. Maybe `CloneAttributesFrom()`? This would be a quick fix but maybe not the best. FWIW, the real issue here is that we use `ParserDidSetAttributes` to init the attributes but the clone code doesn't call this. Firefox is using something similar but I guess their cloning code re-uses the parser code. Safari is simply reading the muted state before playback like we used to.
,
Feb 26 2018
+foolip for realz
,
Feb 26 2018
Thanks @Mounir, i will try overriding the "CloseAttributesFrom()" method. Does this fix needs to land tonight ?
,
Feb 26 2018
Hmm, if the original element has a muted *content attribute* (not just the state bit set) then cloning it should work. In some part of the cloning, the attribute should be set on the new element, and that should set the internal state bit. Details of fix unclear, but in agreement that this should work per spec.
,
Feb 26 2018
Based on comment #10, the earlier the better. I'm happy to write the fix if it's too late in your timezone.
,
Feb 26 2018
yes, can you please do it, thanks in advance.
,
Feb 26 2018
,
Feb 26 2018
I've sent a CL for review to foolip@
,
Feb 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0d23cbfab31479d700140aa5d26d70d857f7be48 commit 0d23cbfab31479d700140aa5d26d70d857f7be48 Author: Mounir Lamouri <mlamouri@chromium.org> Date: Mon Feb 26 18:39:17 2018 HTMLMediaElement: set `muted_` when cloning element with muted attribute. This CL works around the fact that cloning does not call the parser callbacks and the HTML specifications require the attribute to only have an effect when the element was created. There is unfortunately no notification that the element creation from `cloneNode()` happens so we instead rely on a callback meant for something slightly different. Bug: 811743 Change-Id: Ib7ac55088b9eaad23f9bce0cfc8366fec6fc5b0d Reviewed-on: https://chromium-review.googlesource.com/937504 Commit-Queue: Mounir Lamouri <mlamouri@chromium.org> Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/heads/master@{#539207} [modify] https://crrev.com/0d23cbfab31479d700140aa5d26d70d857f7be48/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/media-elements/user-interface/muted.html [modify] https://crrev.com/0d23cbfab31479d700140aa5d26d70d857f7be48/third_party/WebKit/Source/core/html/media/HTMLMediaElement.cpp [modify] https://crrev.com/0d23cbfab31479d700140aa5d26d70d857f7be48/third_party/WebKit/Source/core/html/media/HTMLMediaElement.h
,
Feb 27 2018
Tested this issue on Windows 10 on the reported version 64.0.3282.140 and latest Canary 66.0.3356.0 by following the steps mentioned in the original comment. Able to reproduce this issue on the reported version 64.0.3282.140 and the issue is fixed on the latest Canary 66.0.3356.0. On loading the html page, the audio is not turned on after cloning. Attached is the screen cast for reference. Hence adding TE verified labels as the fix is working as intended. Thanks..
,
Feb 27 2018
Requesting merge as it is a RBS.
,
Feb 27 2018
This bug requires manual review: We are only 6 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 27 2018
// correction in comment #21. Tested the issue on Windows 10, Mac OS 10.12.6 and Ubuntu 14.04 and verifying the fix on all the 3 OS. Thanks..
,
Feb 27 2018
mlamouri@, How safe is the change to merge to M65? M65 is very close to stable promotion so we can only take critical and safe merge in at this point.
,
Feb 27 2018
I think the change is safe but I do not know if it is worth a last minute merge. Issues with compatibility bugs like this is that we might hit Stable and discover that we broke a large website. If we can have one last Beta roll, I would prefer over directly hitting Stable.
,
Feb 27 2018
Approving merge to M65 branch 3325 based on comments #21 and #26. Please merge ASAP so we can pick it up for last beta release tomorrow. Thank you.
,
Feb 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cc6571e0d02d8098824bacbff599bcb02ca04e80 commit cc6571e0d02d8098824bacbff599bcb02ca04e80 Author: Mounir Lamouri <mlamouri@chromium.org> Date: Tue Feb 27 16:13:15 2018 HTMLMediaElement: set `muted_` when cloning element with muted attribute. This CL works around the fact that cloning does not call the parser callbacks and the HTML specifications require the attribute to only have an effect when the element was created. There is unfortunately no notification that the element creation from `cloneNode()` happens so we instead rely on a callback meant for something slightly different. Bug: 811743 Change-Id: Ib7ac55088b9eaad23f9bce0cfc8366fec6fc5b0d Reviewed-on: https://chromium-review.googlesource.com/937504 Commit-Queue: Mounir Lamouri <mlamouri@chromium.org> Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#539207}(cherry picked from commit 0d23cbfab31479d700140aa5d26d70d857f7be48) Reviewed-on: https://chromium-review.googlesource.com/939561 Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#605} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/cc6571e0d02d8098824bacbff599bcb02ca04e80/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/media-elements/user-interface/muted.html [modify] https://crrev.com/cc6571e0d02d8098824bacbff599bcb02ca04e80/third_party/WebKit/Source/core/html/media/HTMLMediaElement.cpp [modify] https://crrev.com/cc6571e0d02d8098824bacbff599bcb02ca04e80/third_party/WebKit/Source/core/html/media/HTMLMediaElement.h
,
Feb 27 2018
|
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by sentimen...@gmail.com
, Feb 13 2018