Regression: 'Send Feedback' overlay becomes unresponsive on 'play.google.com' after Fullscreen action.
Reported by
db...@etouch.net,
Jan 24 2018
|
|||||||||||
Issue descriptionChrome Version: 66.0.3330.0 Revision 44fe59c8dd90d446da8eceda0cb28a5117311be1-refs/heads/master@{#531403}(32/64 bit) OS: Windows (7,8,8.1,10), Mac(10.12.6,10.13.1,10.13.3), Linux (14.04 LTS). What steps will reproduce the problem? (1) Launch chrome, navigate to https://play.google.com/store/movies page and play any video. (2) Click on Fullscreen icon and then click on 'Send Feedback' icon, exit fullscreen. (3) Observe on Send Feedback overlay. Actual: 'Send Feedback' overlay becomes unresponsive after existing fullscreen. Expected: 'Send Feedback' overlay should be responsive after existing fullscreen. This is a regression issue, broken in 'M65'and providing the bisect using per-revision bisect. Good Build:65.0.3288.0(Revision: 522667) Bad Build: 65.0.3289.0(Revision: 522956) You are probably looking for a change made after 522770 (known good), but no later than 522771 (first known bad). CHANGELOG URL: The script might not always return single CL as suspect as some perf builds might get missing due to failure. https://chromium.googlesource.com/chromium/src/+log/3b0721d2e0a2f632436646b89648959f69a9dce9..17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2 Suspect: https://chromium.googlesource.com/chromium/src/+/17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2 @Philip Jägenstedt: Please help me to reassign this issue if your change is not cause for it. Note: Issue is also seen on M65 Dev(#65.0.3322.3)
,
Jan 24 2018
Adding release block stable for this issue.Please remove if not the case. Thank You!
,
Jan 24 2018
Correction : Added with Beta Blocker. Please remove or reduce priority if not the case.
,
Jan 29 2018
Gentle ping to get an update on this issue as it is marked as beta blocker. Thanks..!
,
Jan 29 2018
Reproduce the issue on Chrome65.0.3325.20/CrOS10323.5.0 - Candy
,
Jan 30 2018
Interesting, the repro steps cause a crash for me on Chrome Canary Mac 66.0.3334.0. I'll see what a debug build does.
,
Jan 30 2018
An important part of the repro is that you need to be logged in to a Google account. Without that, I didn't get the feedback button.
,
Jan 30 2018
OK, I think I understand the problem. After entering an exiting fullscreen, the elements in the iframe for the feedback form are all inert, verified by exposing Node::IsInert to scripts and just querying it. I'm pretty sure something like the SetIsInert call in InertSubtreesChanged for HTMLDialogElement.cpp is needed. That's the code that I copied and thought that part wasn't needed because fullscreen itself already propagates *up* since each ancestor document will get a fullscreen element, but there's nothing that propagates inertness *down*.
,
Jan 30 2018
M65 Beta promotion is coming soon and your bug is labelled as Beta ReleaseBlock, pls make sure to land the fix and request a merge to M65 branch 3325 ASAP (merge has to be done latest by this Friday, 02/02 @ 5:00 PM PT). Thank you.
,
Jan 31 2018
,
Jan 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/818e182ca6da2b0eee55994b374723eec00b908e commit 818e182ca6da2b0eee55994b374723eec00b908e Author: Philip Jägenstedt <foolip@chromium.org> Date: Wed Jan 31 20:45:40 2018 Propagate inert state into subframes on fullscreen changes Bug: 805392 Change-Id: Ie6cdb9ffd9c34ce9226553e61f855bd456749e7f Reviewed-on: https://chromium-review.googlesource.com/893718 Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Reviewed-by: Mounir Lamouri (slow) <mlamouri@chromium.org> Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/heads/master@{#533391} [modify] https://crrev.com/818e182ca6da2b0eee55994b374723eec00b908e/content/browser/accessibility/fullscreen_browsertest.cc [add] https://crrev.com/818e182ca6da2b0eee55994b374723eec00b908e/content/test/data/accessibility/fullscreen/iframe.html [modify] https://crrev.com/818e182ca6da2b0eee55994b374723eec00b908e/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp
,
Feb 1 2018
Per #9, requesting merge, even though this has not yet reached Canary.
,
Feb 1 2018
Nice, this reached Canary for Mac in 66.0.3336.0 and I've tested it, problem confirmed fixed.
,
Feb 1 2018
It has now also reached Canary for Win, although I haven't tested it.
,
Feb 2 2018
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact 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 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8b02a53effabee8ecfc196b0a854f33c9e84771 commit e8b02a53effabee8ecfc196b0a854f33c9e84771 Author: Philip Jägenstedt <foolip@chromium.org> Date: Fri Feb 02 08:23:55 2018 Propagate inert state into subframes on fullscreen changes Bug: 805392 Change-Id: Ie6cdb9ffd9c34ce9226553e61f855bd456749e7f Reviewed-on: https://chromium-review.googlesource.com/893718 Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Reviewed-by: Mounir Lamouri (slow) <mlamouri@chromium.org> Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#533391}(cherry picked from commit 818e182ca6da2b0eee55994b374723eec00b908e) Reviewed-on: https://chromium-review.googlesource.com/898882 Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#256} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/e8b02a53effabee8ecfc196b0a854f33c9e84771/content/browser/accessibility/fullscreen_browsertest.cc [add] https://crrev.com/e8b02a53effabee8ecfc196b0a854f33c9e84771/content/test/data/accessibility/fullscreen/iframe.html [modify] https://crrev.com/e8b02a53effabee8ecfc196b0a854f33c9e84771/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp
,
Feb 2 2018
,
Feb 2 2018
,
Feb 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/09f47138722c9827751f06007f1836f138204a5e commit 09f47138722c9827751f06007f1836f138204a5e Author: Philip Jägenstedt <foolip@chromium.org> Date: Fri Feb 02 17:14:38 2018 Revert "Propagate inert state into subframes on fullscreen changes" This reverts commit e8b02a53effabee8ecfc196b0a854f33c9e84771. Reason for revert: build failure, see crbug.com/808396 Original change's description: > Propagate inert state into subframes on fullscreen changes > > Bug: 805392 > Change-Id: Ie6cdb9ffd9c34ce9226553e61f855bd456749e7f > Reviewed-on: https://chromium-review.googlesource.com/893718 > Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> > Reviewed-by: Mounir Lamouri (slow) <mlamouri@chromium.org> > Commit-Queue: Philip Jägenstedt <foolip@chromium.org> > Cr-Original-Commit-Position: refs/heads/master@{#533391}(cherry picked from commit 818e182ca6da2b0eee55994b374723eec00b908e) > Reviewed-on: https://chromium-review.googlesource.com/898882 > Reviewed-by: Philip Jägenstedt <foolip@chromium.org> > Cr-Commit-Position: refs/branch-heads/3325@{#256} > Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} TBR=foolip@chromium.org Change-Id: Idc20791f70eae1b559484b4a9c1fc15b646a64e2 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 805392 , 808396 Reviewed-on: https://chromium-review.googlesource.com/899702 Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#260} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/09f47138722c9827751f06007f1836f138204a5e/content/browser/accessibility/fullscreen_browsertest.cc [delete] https://crrev.com/4be80ac6d260af6ed4b7e57a31639ad906508241/content/test/data/accessibility/fullscreen/iframe.html [modify] https://crrev.com/09f47138722c9827751f06007f1836f138204a5e/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp
,
Feb 2 2018
Reverted due to build failure in issue 808396 because I'm OOO and can't fix it until Tuesday. IMHO, this breakage isn't super serious and it would be fine to not block beta in this and call it RBS instead. Will be relanded on Tuesday if the build failure is as trivial as it appears.
,
Feb 3 2018
Thank you foolip@. We won't block initial beta release for this but pls try to get it fixed on Tuesday.
,
Feb 5 2018
If we are not blocking beta on this should we punt the releaseblock label to stable?
,
Feb 5 2018
We would like to get fix in for next week beta if possible, so leaving Beta blocker as it is for now.
,
Feb 5 2018
Ok we can just remove the OS=Chrome tag so that it does not confuse the test folks on the CrOS side.
,
Feb 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1363c3db6ef141ff2e82a24650763f094803b86f commit 1363c3db6ef141ff2e82a24650763f094803b86f Author: Philip Jägenstedt <foolip@chromium.org> Date: Tue Feb 06 13:20:41 2018 Reland "Propagate inert state into subframes on fullscreen changes" This reverts commit 09f47138722c9827751f06007f1836f138204a5e. Reason for revert: Build failure stemming from https://chromium-review.googlesource.com/877417 fixed. Original change's description: > Revert "Propagate inert state into subframes on fullscreen changes" > > This reverts commit e8b02a53effabee8ecfc196b0a854f33c9e84771. > > Reason for revert: build failure, see crbug.com/808396 > > Original change's description: > > Propagate inert state into subframes on fullscreen changes > > > > Bug: 805392 > > Change-Id: Ie6cdb9ffd9c34ce9226553e61f855bd456749e7f > > Reviewed-on: https://chromium-review.googlesource.com/893718 > > Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> > > Reviewed-by: Mounir Lamouri (slow) <mlamouri@chromium.org> > > Commit-Queue: Philip Jägenstedt <foolip@chromium.org> > > Cr-Original-Commit-Position: refs/heads/master@{#533391}(cherry picked from commit 818e182ca6da2b0eee55994b374723eec00b908e) > > Reviewed-on: https://chromium-review.googlesource.com/898882 > > Reviewed-by: Philip Jägenstedt <foolip@chromium.org> > > Cr-Commit-Position: refs/branch-heads/3325@{#256} > > Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} > > TBR=foolip@chromium.org > > Change-Id: Idc20791f70eae1b559484b4a9c1fc15b646a64e2 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 805392 , 808396 > Reviewed-on: https://chromium-review.googlesource.com/899702 > Reviewed-by: Philip Jägenstedt <foolip@chromium.org> > Cr-Commit-Position: refs/branch-heads/3325@{#260} > Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} TBR=foolip@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 805392 , 808396 Change-Id: Ibbf6a74cc0cc94838a294e06b5b0c261d1b17e0d Reviewed-on: https://chromium-review.googlesource.com/904282 Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#344} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/1363c3db6ef141ff2e82a24650763f094803b86f/content/browser/accessibility/fullscreen_browsertest.cc [add] https://crrev.com/1363c3db6ef141ff2e82a24650763f094803b86f/content/test/data/accessibility/fullscreen/iframe.html [modify] https://crrev.com/1363c3db6ef141ff2e82a24650763f094803b86f/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp
,
Feb 6 2018
I've relanded the fix on the M65 branch with the error in issue 808396 resolved, it was indeed trivial. |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by db...@etouch.net
, Jan 24 2018