Don't skip past auto-height flexboxes in quirks mode. #15325
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
approved these changes
Feb 11, 2019
|
@mstensho can you file a bug on the quirks spec, given it looks like it requires this behaviour (that apparently only Blink (and WebKit?) has). |
|
@gsnedders https://chromium-review.googlesource.com/c/chromium/src/+/1459643 will make Blink behave like EdgeHTML and Gecko, and no longer like WebKit. But yeah... good point, time to agree on something here. :) |
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-1459643
branch
from
f2e1949
to
c14445e
Compare
This is essentially a reland of https://chromium-review.googlesource.com/c/chromium/src/+/1052768/ 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. Need to remove external/wpt/css/css-flexbox/percentage-heights-quirks-node.html now, since that test expects special behavior in quirks mode, which is exactly what this CL is removing. This makes us behave like Edge and Firefox. 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 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: If7d62ff37d3176b8ee51cdf9b7d2e02d75c3dc02 Reviewed-on: https://chromium-review.googlesource.com/c/1459643 Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Reviewed-by: Christian Biesinger <cbiesinger@chromium.org> Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Cr-Commit-Position: refs/heads/master@{#631184}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-1459643
branch
from
c14445e
to
94702b4
Compare
|
Flaky tests on Firefox: /css/css-flexbox/quirks-auto-block-size-with-percentage-item.html
Unstable results
|
|
I wonder if waiting for the image to load would make this stable in Firefox? It shouldn't, but… |
zcorpan
added a commit
to whatwg/quirks
that referenced
this pull request
Mar 29, 2019
Also walk containing block chain (drive-by bug fix). Fixes #39. Tests: web-platform-tests/wpt#15380 web-platform-tests/wpt#15325
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is essentially a reland of
https://chromium-review.googlesource.com/c/chromium/src/+/1052768/
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.
Need to remove
external/wpt/css/css-flexbox/percentage-heights-quirks-node.html now,
since that test expects special behavior in quirks mode, which is
exactly what this CL is removing. This makes us behave like Edge and
Firefox.
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
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: If7d62ff37d3176b8ee51cdf9b7d2e02d75c3dc02
Reviewed-on: https://chromium-review.googlesource.com/c/1459643
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#631184}