DCHECK failure !NeedsLayout() fails, flexbox inside VIDEO in quirks mode |
|||||
Issue description
VIDEO elements (LayoutVideo) are unsuitable as containing blocks, so we should never skip past them when walking upwards for something.
Layout subtree at VIDEO:
LayoutVideo (positioned) VIDEO style="position:fixed;"
LayoutFlexibleBox (relative positioned) DIV class="phase-pre-ready state-no-source"
LayoutBlockFlow DIV
LayoutFlexibleBox DIV
What happens in this case is that the inner flexbox calls LayoutBox::ComputePercentageLogicalHeight(). As part of registering itself in some ancestor as being a "percentage height descendant", it skips its parent flexbox, and eventually ends up at an ancestor of VIDEO, and adds itself there. This may be problematic in a next layout pass, because we might end up marking the inner flexbox as needing layout, although its actual containing block isn't going to be laid out.
The attached test fails the DCHECK in the second layout pass (there'll be two passes, because we always initially assume that auto document scrollbars will result in a scrollbar, but we won't need a scrollbar in this case, so we relayout).
,
May 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aa9c26bad48e9fd8e861f1e4494672a5aa162ce2 commit aa9c26bad48e9fd8e861f1e4494672a5aa162ce2 Author: Morten Stenshorne <mstensho@chromium.org> Date: Fri May 18 20:15:25 2018 Don't skip past auto-height flexboxes in quirks mode. Quirky flexbox behavior really shouldn't be necessary, since flexbox was invented over a decade after quirky browsers were on the street. Replace css3/flexbox/percentage-sizes-quirks.html with a copy of percentage-sizes.html, except that it's in quirks mode. The root problem, as far as the DCHECK failure in bug 841276 is concerned, though, is that LayoutVideo is a very unsuitable container for pretty much anything (because it's not a LayoutBlock, let alone LayoutBlockFlow). By default we end up with the following layout tree for a VIDEO element: LayoutVideo VIDEO LayoutFlexibleBox (relative positioned) DIV class="phase-pre-ready state-no-source" LayoutBlockFlow DIV LayoutFlexibleBox DIV See the testcase. The VIDEO there is fixed-positioned, and it also has another fixed-positioned ancestor (regular block otherwise). When attempting to locate the right ancestor (descendant of VIDEO) in the testcase to register a percentage height descendant, we don't see the LayoutVideo object at all on our way up the tree (because it's not LayoutBlock). So the percentage height descendant is incorrectly registered with the outer fixed positioned block. This confuses the layout machinery, so that when re-laying out the outer fixed positioned block (see the testcase), we mark the inner flexbox for layout, since it's a percentage height descendant. But we never get down there, since the containing VIDEO element isn't going to require layout. Thus we'll fail DCHECKs that require the entire tree to be clean after layout. Bug: 841276, 531783 Change-Id: I5385d1ad9bbe0f9d739418e9d1d7fc630aa3d3a3 Reviewed-on: https://chromium-review.googlesource.com/1052768 Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Reviewed-by: Christian Biesinger <cbiesinger@chromium.org> Cr-Commit-Position: refs/heads/master@{#560004} [add] https://crrev.com/aa9c26bad48e9fd8e861f1e4494672a5aa162ce2/third_party/WebKit/LayoutTests/css3/flexbox/fixedpos-video-in-abspos-quirk-crash.html [modify] https://crrev.com/aa9c26bad48e9fd8e861f1e4494672a5aa162ce2/third_party/WebKit/LayoutTests/css3/flexbox/percentage-sizes-quirks.html [modify] https://crrev.com/aa9c26bad48e9fd8e861f1e4494672a5aa162ce2/third_party/blink/renderer/core/layout/layout_box.cc
,
May 18 2018
,
May 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b6a921ec94dfae58a6f168b1f7b7178bb2216f34 commit b6a921ec94dfae58a6f168b1f7b7178bb2216f34 Author: Morten Stenshorne <mstensho@chromium.org> Date: Tue May 22 14:27:39 2018 Revert "Don't skip past auto-height flexboxes in quirks mode." This reverts commit aa9c26bad48e9fd8e861f1e4494672a5aa162ce2. Reason for revert: Broke youtube. crbug.com/845431 Original change's description: > Don't skip past auto-height flexboxes in quirks mode. > > Quirky flexbox behavior really shouldn't be necessary, since flexbox was > invented over a decade after quirky browsers were on the street. > > Replace css3/flexbox/percentage-sizes-quirks.html with a copy of > percentage-sizes.html, except that it's in quirks mode. > > The root problem, as far as the DCHECK failure in bug 841276 is > concerned, though, is that LayoutVideo is a very unsuitable container > for pretty much anything (because it's not a LayoutBlock, let alone > LayoutBlockFlow). By default we end up with the following layout tree > for a VIDEO element: > > LayoutVideo VIDEO > LayoutFlexibleBox (relative positioned) DIV class="phase-pre-ready state-no-source" > LayoutBlockFlow DIV > LayoutFlexibleBox DIV > > See the testcase. The VIDEO there is fixed-positioned, and it also has > another fixed-positioned ancestor (regular block otherwise). When > attempting to locate the right ancestor (descendant of VIDEO) in the > testcase to register a percentage height descendant, we don't see the > LayoutVideo object at all on our way up the tree (because it's not > LayoutBlock). So the percentage height descendant is incorrectly > registered with the outer fixed positioned block. This confuses the > layout machinery, so that when re-laying out the outer fixed positioned > block (see the testcase), we mark the inner flexbox for layout, since > it's a percentage height descendant. But we never get down there, since > the containing VIDEO element isn't going to require layout. Thus we'll > fail DCHECKs that require the entire tree to be clean after layout. > > Bug: 841276, 531783 > Change-Id: I5385d1ad9bbe0f9d739418e9d1d7fc630aa3d3a3 > Reviewed-on: https://chromium-review.googlesource.com/1052768 > Commit-Queue: Morten Stenshorne <mstensho@chromium.org> > Reviewed-by: Christian Biesinger <cbiesinger@chromium.org> > Cr-Commit-Position: refs/heads/master@{#560004} TBR=cbiesinger@chromium.org,mstensho@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 841276, 531783 Change-Id: I53ed4c6d0894267bbd867dcf633eabc580e5d75c Reviewed-on: https://chromium-review.googlesource.com/1068868 Reviewed-by: Morten Stenshorne <mstensho@chromium.org> Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Cr-Commit-Position: refs/heads/master@{#560566} [delete] https://crrev.com/46791aea1ab97f75521abb02942a43e6af16cd7f/third_party/WebKit/LayoutTests/css3/flexbox/fixedpos-video-in-abspos-quirk-crash.html [modify] https://crrev.com/b6a921ec94dfae58a6f168b1f7b7178bb2216f34/third_party/WebKit/LayoutTests/css3/flexbox/percentage-sizes-quirks.html [modify] https://crrev.com/b6a921ec94dfae58a6f168b1f7b7178bb2216f34/third_party/blink/renderer/core/layout/layout_box.cc
,
May 22 2018
Need to fix bug 845431 before landing that fix.
,
May 28 2018
,
May 28 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by mstensho@chromium.org
, May 9 2018