[css-contain] Blink renders contain:size multicol elements too small (disregarding column-gap & column-count when resolving min/max-content sizes) |
||||
Issue descriptionChrome Version: 69.0.3486.0 (Official Build) dev (64-bit) OS: Ubuntu 18.04 What steps will reproduce the problem? (1) Visit these two URLs: https://jsfiddle.net/r27jobv3/ https://jsfiddle.net/r27jobv3/1/ What is the expected result? The two examples should render the same -- in both cases, four lime boxes should be visible. What happens instead? In the second URL (the one with "/1/" in URL, with "contain:size" in its CSS), the four boxes render as 0-width, without any lime. Please use labels and text to provide additional information. "contain:size" here is simply supposed to make the multicol element render as if it had no contents, per https://drafts.csswg.org/css-contain/#containment-size . In this testcase the multicol element *already* has no contents, so contain:size shouldn't have any sizing impact. However, it does have a sizing impact in Chrome.
,
Jul 15
Thanks for reporting! Do we have WPT tests for this?
,
Jul 15
Hello! I'm working on contain functionality at Mozilla and wrote tests for this; they should be uploaded into WPT later this week, just need to make some changes for universality.
,
Jul 17
Imported as multicol-002, -003 here: https://github.com/web-platform-tests/wpt/tree/master/css/vendor-imports/mozilla/mozilla-central-reftests/contain
,
Jul 30
Thank you!
,
Aug 10
,
Sep 5
About the tests, why are you putting them in https://github.com/web-platform-tests/wpt/tree/master/css/vendor-imports/mozilla/mozilla-central-reftests/contain instead of using directly https://github.com/web-platform-tests/wpt/tree/master/css/css-contain ? It'd be really nice if you could contribute the tests to the main test suite too.
,
Sep 5
mstensho@ I don't know multi-column good enough but I've been taking a quick look to this.
The problem is in LayoutBlock::ComputeIntrinsicLogicalWidths()
where we have the following condition:
// Size-contained elements don't consider their contents for preferred sizing.
if (ShouldApplySizeContainment())
return;
We could try to override ComputeIntrinsicLogicalWidths() in LayoutMultiColumnFlowThread
but it won't be called because of how the layout tree is:
LayoutBlockFlow 0x22eaee24270 DIV
LayoutMultiColumnFlowThread (anonymous) 0x22eaee34010
So I'm not sure what would be the best way to solve this,
in any case these are just some quick thoughts about this
just in case they can be useful.
,
Sep 5
> About the tests, why are you putting them in > [...]/mozilla/mozilla-central-reftests/contain We're actually putting them in this directory: https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted ...which is a special section of our general "reftests" directory that gets synchronized with that WPT "mozilla-central-reftests" folder, via dbaron running some scripts periodically. We prefer to put reftests in this directory so that we can run them locally in mozilla's own reftest harness, which is (currently) stricter than the WPT reftest harness. (It catches nonfatal assertion failures, and allows for finer-grained failure annotations like "1 pixel sometimes differs on this platform due to known driver bugs".) Once https://bugzilla.mozilla.org/show_bug.cgi?id=1263568 is addressed that may be less of a motivation. > It'd be really nice if you could contribute the tests to the main test suite too. We're contributing them to WPT, but I can imagine it being annoying to have to figure out which subset of WPT to run locally in order to get test coverage for a particular change when the tests are spread out like this. I'm not sure the best way to address this, though, without either losing the benefits of our stricter reftest harness (if we just put them directly in WPT), or having to implement some more subtle synchronization logic, or ending up with multiple copies of the test living in the WPT suite (which would clearly be bad from a maintenance perspective).
,
Sep 5
Thanks for the feedback, if it's just one folder (e.g. css/vendor-imports/mozilla/mozilla-central-reftests/contain/) we can import that one too in Chromium and get your tests for this feature, I'll do it. In the future it'd be awesome if we could have just one place for the tests of the same spec. Some people have been doing work reviewing and improving the test suite under css/css-contain/ but I'm not sure if they were aware of the other tests too or not. BTW, are you running the tests under css/css-contain/ in Firefox too? That would help to improve interoperability between both implementations.
,
Sep 5
> if it's just one folder [...] Yeah, all of our css-contain tests should be in that one folder. (We've got intended-to-be-shared reftests for other css features in its sibling folders. All the other folders at https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted should end up being synchronized to that WPT "mozilla-central-reftests/" folder as well.) > In the future it'd be awesome if we could have just one place for the tests of the same spec. Agreed. > BTW, are you running the tests under css/css-contain/ in Firefox too? Yes. We run all the tests in the WPT css folder (at least on linux, I think - not on all platforms, due to a few quirks e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1425698 )
,
Sep 7
JFTR, Mozilla test are now imported too (see issue #881057 ).
,
Oct 11
JFTR, this works correctly with LayoutNGBlockFragmentation enabled (but not in the legacy engine or with the default LayoutNG feature set). |
||||
►
Sign in to add a comment |
||||
Comment 1 by dholb...@gmail.com
, Jul 13Summary: Blink renders contain:size multicol elements too small (disregarding column-gap & column-count when resolving min/max-content sizes) (was: Blink makes contain:size multicol elements too small (disregarding column-gap & column-count when resolving min/max-content sizes))