Bug 205386 - [css-flex][css-grid] Out-of-flow boxes should not be children of anonymous items
Summary: [css-flex][css-grid] Out-of-flow boxes should not be children of anonymous items
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oriol Brufau
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-18 03:23 PST by Manuel Rego Casasnovas
Modified: 2020-05-22 09:46 PDT (History)
13 users (show)

See Also:


Attachments
Patch (7.59 KB, patch)
2020-05-20 15:24 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (7.53 KB, patch)
2020-05-21 17:53 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (5.02 KB, patch)
2020-05-22 07:27 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2019-12-18 03:23:06 PST
This is causing that WebKit fails the following WPT tests
(they pass in Chromium and Firefox):
* http://w3c-test.org/css/css-flexbox/anonymous-flex-item-004.html
* http://w3c-test.org/css/css-grid/grid-items/anonymous-grid-item-001.html

This was already fixed in Chromium:
https://chromium-review.googlesource.com/c/chromium/src/+/895844/
But the code in WebKit regarding tree construction is different now,
so it cannot just be ported directly into WebKit.

There's also a related test that only passes in Firefox right now:
http://w3c-test.org/css/css-flexbox/anonymous-flex-item-005.html
Comment 1 Oriol Brufau 2020-05-20 15:24:16 PDT
Created attachment 399898 [details]
Patch
Comment 2 Oriol Brufau 2020-05-20 15:38:19 PDT
https://crrev.com/533825 has another change in LayoutBlock::AddChildBeforeDescendant which amends https://codereview.chromium.org/120603002

I'm not including that since WebKit doesn't have the earlier change (maybe it should, but that's not relevant to this bug).
Comment 3 Manuel Rego Casasnovas 2020-05-20 15:56:20 PDT
Comment on attachment 399898 [details]
Patch

r=me
Comment 4 zalan 2020-05-20 15:58:05 PDT
Do you mind holding off of cq+? I wanted to take a look at it.
Comment 5 zalan 2020-05-20 15:58:36 PDT
(In reply to zalan from comment #4)
> Do you mind holding off of cq+? I wanted to take a look at it.
I'll cq+ it for you.
Comment 6 Oriol Brufau 2020-05-20 16:11:20 PDT
Sure, there's no hurry
Comment 7 zalan 2020-05-20 18:31:36 PDT
(In reply to Oriol Brufau from comment #6)
> Sure, there's no hurry
Cool. Thanks!
Comment 8 zalan 2020-05-21 16:42:39 PDT
Oh I see. A flex container's (which establishes the flex formatting context) in flow children are all flex items and since they establish BFCs they all need to be blockified one way or another.
Thanks to the setChildrenInline(false) hack in the flex container's c'tor, the first inline level box gets forced into an anonymous block level container and we start using this anon block as parent for the sibling inline level boxes, while the subsequent block level boxes are added as siblings. Now since an out-of-flow box does not trigger tree normalization, meaning that we can have an out-of-flow block box next to an inline level box, it gets pushed inside the anon container and the subsequent inline level boxes are added as siblings. At the end we get a tree like this:
flex container
  anonymous block
    anonymous inline level box ([first])
    out-of-flow block level box
    anonymous inline level box ([text])
and since the anonymous block (as a flex item) establishes a BFC, the content is laid out like this "[fist][second]".

But instead what we want is
flex container
  anonymous block 
    anonymous inline level box ([first])
  out-of-flow block level box
  anonymous block 
    anonymous inline level box ([text])
to properly layout the content like this:
[first]
[second]

I think the patch is fine for now, but in a long run (probably in next-gen timeframe) the flex container should have its own builder to be able to properly set up/blockify its direct children. This is a very simple problem and we should not need to implement hacks like this (or that setChildrenInline(false)).
Comment 9 EWS 2020-05-21 17:13:28 PDT
Tools/Scripts/svn-apply failed to apply attachment 399898 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 10 Oriol Brufau 2020-05-21 17:53:36 PDT
Created attachment 400005 [details]
Patch
Comment 11 Oriol Brufau 2020-05-22 07:27:33 PDT
Created attachment 400044 [details]
Patch
Comment 12 EWS 2020-05-22 09:45:41 PDT
Committed r262061: <https://trac.webkit.org/changeset/262061>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400044 [details].
Comment 13 Radar WebKit Bug Importer 2020-05-22 09:46:14 PDT
<rdar://problem/63543311>