fullscreen <html> element changes layout when toggling display:none |
|||||||||||||||||
Issue description
Canary: 57.0.2958.0
macOS: 10.11.5
Reproduction steps:
1) Run Chrome Canary with "--disable-web-security" e.g.
/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --user-data-dir=/tmp/ah928 --disable-web-security file:///Users/erikchen/projects/chromium/src/fullscreen_layout_bug.html
2) Click the "fullscreen button"
3) Make sure the content area is focused "by left clicking it"
4) Press "a"
Expected behavior:
No change in layout. Screen stays red.
Actual behavior:
Layout change. Screen becomes black with thin red column.
What's going on:
When "a" is pressed, the documentElement is reattached. This triggers code in "LayoutTreeBuilderForElement::createLayoutObject" that wraps the newly created layoutobject in a flex box anonymous layoutobject. This changes the width of the layoutobject for the HTML element.
Pre-relayout
#document 1905 1105
HTML 1905 1121
BODY style="background:#FF0000; width:100%; height:100%" 1905 1105
Post-relayout
#document 1920 1120
0x313a61c38198 1920 1120
HTML 0x313a61c1c700 16 1136
BODY style="background:#FF0000; width:100%; height:100%" 16 1120
What's wrong:
At least one of the following two points is broken [possibly both].
1) We should be triggering a reattachment of the documentElement() upon entering fullscreen. Pressing "a" should be an idempotent operation.
2) The anonymous layout object shouldn't be causing the HTML element to have width 16. [Not sufficiently familiar with layout to know exactly what's wrong. Should the anonymous object not be flex?]
,
Jan 2 2017
,
Jan 17 2017
Ping? This is blocking https://codereview.chromium.org/2564633002/
,
Jan 18 2017
Is this a recent regression? Added Needs-Bisect to find out, as there are other Fullscreen regressions that I'm looking into now. If this isn't a regression, then I will not prioritize it as layout issues are somewhat likely to be swept away by issue 240576 .
,
Jan 19 2017
,
Jan 20 2017
Thanks for verifying. I do mean to eventually look at all Fullscreen issues before declaring myself done, but I think a better use of time for me will be working towards the top layer rewrite, as that's likely to make many issues obsolete. I would expect to land that sometime after BlinkOn, in February. Marking as available if someone wants to investigate in the meantime.
,
Feb 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8f04daf8b30d368349191fc0522e05c1dc32a98 commit d8f04daf8b30d368349191fc0522e05c1dc32a98 Author: erikchen <erikchen@chromium.org> Date: Fri Feb 03 20:48:43 2017 Disable fullscreen/full-screen-iframe-zIndex.html test. The test currently passes, but fails when the feature "Stop creating layout objects for elements inside display: none iframes" is enabled. The root cause is a long-standing fullscreen issue, see minimized repro in crbug.com/676432 . The issue is likely going to go away with "migrate fullscreen to use top layer". BUG= 676432 , 650433 , 240576 Review-Url: https://codereview.chromium.org/2662453002 Cr-Commit-Position: refs/heads/master@{#448067} [modify] https://crrev.com/d8f04daf8b30d368349191fc0522e05c1dc32a98/third_party/WebKit/LayoutTests/TestExpectations
,
Feb 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c715c5116fb070cf376342de9bc67bc7dbfea23d commit c715c5116fb070cf376342de9bc67bc7dbfea23d Author: erikchen <erikchen@chromium.org> Date: Wed Feb 15 07:41:08 2017 Disable virtual/android/fullscreen/full-screen-iframe-zIndex.html The test currently passes, but fails when the feature "Stop creating layout objects for elements inside display: none iframes" is enabled. The root cause is a long-standing fullscreen issue, see minimized repro in crbug.com/676432 . The issue is likely going to go away with "migrate fullscreen to use top layer". BUG= 676432 , 650433 , 240576 Review-Url: https://codereview.chromium.org/2692363002 Cr-Commit-Position: refs/heads/master@{#450626} [modify] https://crrev.com/c715c5116fb070cf376342de9bc67bc7dbfea23d/third_party/WebKit/LayoutTests/TestExpectations
,
Apr 26 2017
Removing 'Needs-Bisect & needs triage' labels as fix already landed. Thanks..!!
,
Dec 14 2017
This bug also repros without any iframes: https://output.jsbin.com/duleqic Making an <html> element fullscreen doesn't normally create a LayoutFullScreen, due to this check in Fullscreen::FullscreenElementChanged: if (new_element != GetDocument()->documentElement()) { LayoutFullScreen::WrapLayoutObject(...) } But after toggling display: none, we acquire a LayoutFullScreen, causing shrink-wrapped layout (because LayoutFullScreen is a flexbox).
,
Dec 14 2017
,
Dec 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/229920bc36ac1e548ce1d0d9ab4877651005fba0 commit 229920bc36ac1e548ce1d0d9ab4877651005fba0 Author: Steve Kobes <skobes@chromium.org> Date: Thu Dec 14 22:46:32 2017 Don't wrap <html> with LayoutFullScreen. Making an element fullscreen wraps it with a LayoutFullScreen, unless it is the <html> element in which case Fullscreen::FullscreenElementChanged specifically avoids wrapping it. This patch updates the layout tree construction code to match the logic used when entering fullscreen mode. This ensures that toggling display: none on a fullscreen <html> element doesn't inadvertently create a LayoutFullScreen where we didn't have one before. Bug: 676432 Change-Id: I847b49fe8efa3af2ae0e02c4879fe6dc89a6a146 Reviewed-on: https://chromium-review.googlesource.com/826468 Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/heads/master@{#524215} [modify] https://crrev.com/229920bc36ac1e548ce1d0d9ab4877651005fba0/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/229920bc36ac1e548ce1d0d9ab4877651005fba0/third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp [modify] https://crrev.com/229920bc36ac1e548ce1d0d9ab4877651005fba0/third_party/WebKit/Source/core/layout/LayoutFullScreen.cpp
,
Dec 15 2017
|
|||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||
Comment 1 by erikc...@chromium.org
, Dec 21 2016Cc: esprehn@chromium.org foolip@chromium.org
Components: Blink>Layout
Labels: -Pri-3 Pri-1