First pass at implementing the Flex Layout Algorithm

[SimonSapin]

CC #26639

[highfive]

Heads up! This PR modifies the following files:

• @emilio: components/style/values/computed/length.rs

[SimonSapin]

r? @nox

[SimonSapin]

@bors-servo try=wpt-2020

[bors-servo]

⌛ Trying commit 25469bd with merge 40e38cb...

[bors-servo]

💔 Test failed - status-taskcluster

[SimonSapin]

2 unexpected results that are NOT known-intermittents:
  ▶ OK [expected CRASH] /css/css-flexbox/layout-with-inline-svg-001.html
  ▶ FAIL [expected CRASH] /css/css-flexbox/dynamic-change-simplified-layout.html
  │   → /css/css-flexbox/dynamic-change-simplified-layout.html ['d5b47219b5f8817a35f1abe46e9490c6e792a39b']
  └   → /css/reference/ref-filled-green-100px-square.xht ['25f7c572f42824d5608beb0ca129c47eec6c3272']

The "expected" crashes are panics in layout::positioned::HoistedAbsolutelyPositionedBox::layout with message already mutably borrowed. In wpt/css/css-flexbox/layout-with-inline-svg-001.html for example it’s fairly reliable to reproduce on my machine, but disappears with STYLO_THREADS=1.

I suspect that any test with both flexbox and abspos is potentially impacted, so marking a couple file names as knows intermittent is not gonna help much. I’m looking into this to hopefully fix it before landing.

More generally though, @nox do you have thoughts on:

• How can we reason about the duration of these atomic refcell borrows and have some confidence that we expect them not to overlap regardless of thread scheduling?
• Or, do we need to switch to RwLock to block instead of panicking, if overlaps are unavoidable?

[SimonSapin]

Figured it out. This issue is when implementing this bit of spec:

If the flex item has align-self: stretch, redo layout for its contents, treating this used size as its definite cross size so that percentage-sized children can be resolved.

“Do layout” has the side effect of collecting absolutely-positioned descendants into PositioningContext, so when redoing we ended up with two entries for the same abspos box. Later we lay out that box, twice in parallel leading to overlapping atomic refcell borrows. The fix is to make a new PositioningContext when laying out a flex item and having it part of some return value. When "re-doing", we only use one of the results.

@bors-servo try=wpt-2020

[bors-servo]

⌛ Trying commit 22b60d8 with merge 7d84866...

[bors-servo]

☀️ Test successful - status-taskcluster
State: approved= try=True

[nox]

I'm a bit confused why some iterators need to be collected when they are used a single time any way, but otherwise, LGTM.

[SimonSapin]

For example the module-level layout function returns an impl Iterator which lives longer than layout’s call stack frame, so all intermediate values need to be owned rather than borrowed. But this has all been refactored a couple times already so it’s possible that I still missed some Vecs that don’t need to be collected.

[SimonSapin]

@bors-servo r=nox

[bors-servo]

📌 Commit 22b60d8 has been approved by nox

[bors-servo]

⌛ Testing commit 22b60d8 with merge 743ca89...

[bors-servo]

💔 Test failed - status-taskcluster

[jdm]

@bors-servo retry

• #27062 #23290

[bors-servo]

⌛ Testing commit 22b60d8 with merge 966f314...

[bors-servo]

💔 Test failed - status-taskcluster

[jdm]

@bors-servo retry

[bors-servo]

⌛ Testing commit 22b60d8 with merge 32cb62a...

[bors-servo]

☀️ Test successful - status-taskcluster
Approved by: nox
Pushing 32cb62a to master...

[Manishearth]

Can this loop be merged with the previous one?

[Manishearth]

I guess we're typically iterating over a small number of items so it won't matter much

[SimonSapin]

This PR was getting big, so at some point I stopped refactoring and focused on getting it landed. I’m sure it can be improved still.

There’s a limitation though where the returned impl Iterator needs to take ownership of every closure captures, it cannot borrow stuff on the stack. Some .collect() calls help with that.

[Manishearth]

should percentages be resolved relative to something here?

[SimonSapin]

content_box_size here originally comes from the ComputedValuesExt method of the same name, which already takes care of resolving percentages (but not auto)

[Manishearth]

Why does this need to be done? What does it do?

[SimonSapin]

The BoxFragment::content_rect::start_corner position of a fragment is a point in a coordinate space whose origin is the start corner of the parent in the in the fragment tree. Using relative coordinates like this allows translating an entire sub-tree by modifying the coordinates of its root fragment.

When hoisting an absolutely-positioned box "up" the tree, we’re effectively moving to another coordinate space. HoistedAbsolutelyPositionedBox::box_offsets may contain coordinates that will contribute to the position of the eventual fragment. adjust_static_positions effectively converts them to another coordinate space.

[Manishearth]

Where do we do the "treat auto as fit-content"?

[SimonSapin]

This code path is for replaced elements. I think auto and fit-content resolve to the same thing for those, though I might be wrong for some cases and it’s possible that we don’t implement everything in https://drafts.csswg.org/css-sizing/#intrinsic accurately.

[Manishearth]

why was this necessary?

[Manishearth]

as in, how does the positioning context help with side effects?

[SimonSapin]

IndependentFormattingContext::layout takes a &mut PositioningContext parameter, and has the side effect of (possibly) appending to it. To isolate this side-effect, we construct a new PositioningContext for each flex item, and keep it with the "result of layout". If we end up doing layout for a flex item again because of align-self: stretch, we throw away that PositioningContext and make a new one. This was added in 22b60d8. At some point though we want a single PositioningContext for the entire flex container, so we append the item-specific ones together.

I am working on a branch that removes &mut PositioningContext parameters everywhere and keeps hoisted boxes in respective "result of layout" types instead.