Implement last baseline alignment for Flexbox containers. #4067

sammygill · 2022-09-06T20:13:36Z

`a7e014a`

Implement last baseline alignment for Flexbox containers.
https://bugs.webkit.org/show_bug.cgi?id=244734
rdar://99506835

Reviewed by Brent Fulgham.

In order to implement last baseline alignment for flex containers, we
need to hold a little bit more information. Currently,
RenderFlexibleBox::LineContext holds a maxAscent value, which holds the
value of the furthest baseline in the cross axis. This is then used to
shift the other flex items to this baseline. A new field called
lastBaselineMaxAscent was added to achieve this same functionality for
the last baseline elements. This needs to be a separate field in
scenarios where you may have certain items being aligned by their
baselines and others by their last baselines.

This patch also adds the “margin before” to the first baseline of table
elements since it was not being added before. This is needed in order to
properly align other elements alongside a table with margins. Without
this addition, we were rendering the expectation for both
css-flexbox/flexbox-align-self-baseline-horiz-001a and
css-flexbox/flexbox-align-self-baseline-horiz-001b incorrectly.

* LayoutTests/TestExpectations:
* Source/WebCore/rendering/RenderFlexibleBox.cpp:
(WebCore::RenderFlexibleBox::LineContext::LineContext):
(WebCore::RenderFlexibleBox::marginBoxAscentForChild):
Since marginBoxAscentForChild is being called during alignment purposes,
we can determine what type of alignment is being done here and return
the appropriate ascent (first baseline vs last baseline).

(WebCore::alignmentOffset):
(WebCore::RenderFlexibleBox::layoutAndPlaceChildren):
Depending on whether the item is being first baseline aligned or last
baseline aligned, we need to either update maxAscent or
lastBaselineMaxAscent. Later on when the alignment is being done, we
can check the type of alignment one more time and use the correct
value.

(WebCore::RenderFlexibleBox::alignChildren):
Since items need to be pushed to the end of the cross axis when
wrap-reverse is specified, we can use the same logic to push the items
that are being last baseline aligned. There was also a little bit of
cleanup that I was able to do here. Instead of waiting until all of the
lines have had their items aligned to make any adjustments (e.g. due
to wrap-reverse), we can do it after laying out each line. We can
determine whether or not we need to loop over the line again by checking
the value of minMarginAfterBaseline. If this value was set then that
means that not only is there room to move the items in the flex line,
but also that we need to because the flex container has wrap-reverse
set or some items are being last baseline aligned.

(WebCore::RenderFlexibleBox::childIsParticipatingInBaselineAlignment const):
* Source/WebCore/rendering/RenderFlexibleBox.h:
* Source/WebCore/rendering/RenderTableSection.cpp:
(WebCore::RenderTable::baselinePosition const):
This is not directly related to the last baseline implementation but to
a test case for it. There were 2 expectation files we were rendering
incorrectly
(css-flexbox/flexbox-align-self-baseline-horiz-001a-expected.xhtml and
flexbox-align-self-baseline-horiz-001b-expected.xhtml).

These were using the first baseline for table elements and we were not
computing the correct first baseline. This caused us to fail the tests
even though we were rendering the actual test file correctly.

Canonical link: https://commits.webkit.org/255383@main

c1e411a

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 🧪 win~~
	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
✅ 🛠 🧪 merge	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-09-06T20:13:50Z

EWS run on previous version of this PR (hash 4296caa)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

Source/WebCore/rendering/RenderBlockFlow.cpp

Source/WebCore/rendering/RenderBlock.cpp

webkit-early-warning-system · 2022-09-15T19:12:31Z

Starting EWS tests for 4f2c4cd. Live statuses available at the PR page, #4067

webkit-early-warning-system · 2022-09-15T19:12:31Z

EWS run on previous version of this PR (hash 4f2c4cd)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk1
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2
	✅ 🛠 watch	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-09-22T22:32:18Z

EWS run on previous version of this PR (hash ebec868)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk1
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2
	✅ 🛠 watch	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

sammygill · 2022-09-22T22:33:16Z

Updated RenderBlockFlow::lastLineBaseline so that the call to modern layout lastLinePhysicalBaseline was guarded by an if instead of an assert. Now we should fall back to legacy line layout the scenarios where modernLineLayout returns a nullptr

webkit-early-warning-system · 2022-09-27T05:16:43Z

EWS run on previous version of this PR (hash 49f2d52)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-09-27T17:37:07Z

EWS run on previous version of this PR (hash 63e7fc6)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-09-28T19:02:50Z

EWS run on previous version of this PR (hash 2ba17d5)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

Loirooriol

It sounds reasonable, but not an expert in baselines nor css-align in general.

Source/WebCore/rendering/RenderBlock.cpp

Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.h

Loirooriol · 2022-10-04T22:00:18Z

Source/WebCore/rendering/RenderTableSection.cpp

    LayoutUnit firstLineBaseline = m_grid[0].baseline;
    if (firstLineBaseline)
-        return firstLineBaseline + m_rowPos[0];
+        return firstLineBaseline + m_rowPos[0] + tableMarginBefore;


Not sure how this is related to the last baseline?

It is not directly related to the last baseline implementation but to a test case for it. There were 2 expectation files we were rendering incorrectly (css-flexbox/flexbox-align-self-baseline-horiz-001a-expected.xhtml and flexbox-align-self-baseline-horiz-001b-expected.xhtml)

These were using the first baseline for table elements and we were not computing the correct first baseline. This caused us to fail the tests even though we were rendering the actual test file correctly. This was the incorrect way to do it and I moved the logic to RenderTable::baselinePosition. Now it is similar to other objects (e.g. RenderBlock::baselinePosition).

Source/WebCore/rendering/RenderFlexibleBox.cpp

webkit-early-warning-system · 2022-10-05T21:41:16Z

EWS run on previous version of this PR (hash 8710e23)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	⏳ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🛠 mac-AS-debug	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	⏳ 🧪 api-mac	~~🧪 api-gtk~~
	✅ 🛠 tv	⏳ 🧪 mac-wk1
	✅ 🛠 tv-sim	⏳ 🧪 mac-wk2
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

Loirooriol · 2022-10-05T22:33:59Z

Source/WebCore/rendering/RenderFlexibleBox.cpp

+bool RenderFlexibleBox::childIsParticipatingInBaselineAlignment(const RenderBox& child) const
+{
+    auto alignment = alignmentForChild(child);
+    return (alignment == ItemPosition::Baseline || alignment == ItemPosition::LastBaseline) && !hasAutoMarginsInCrossAxis(child);
+}


This method only has a single caller and the logic is short, so adding it seems unnecessary to me.
By removing it you can reuse the alignmentForChild(child).

Agreed. It was useful before, but now after the cleanup it is much less useful. Changed.

It's no longer used, remove it.

Loirooriol · 2022-10-05T22:43:05Z

Source/WebCore/rendering/RenderFlexibleBox.cpp

-
+    bool containerHasWrapReverse = style().flexWrap() == FlexWrap::Reverse;
+    auto shouldAdjustItemTowardsCrossAxis = [this, containerHasWrapReverse](const RenderBox& box, ItemPosition alignment) { 
+        return (containerHasWrapReverse && alignment == ItemPosition::Baseline && !hasAutoMarginsInCrossAxis(box)) || (alignment == ItemPosition::LastBaseline && !containerHasWrapReverse); 


Why check !hasAutoMarginsInCrossAxis(box) for the 1st baseline but not for the last one?

If it should be checked in both cases, then the logic could be shared with another conditional:

auto something = [containerHasWrapReverse](ItemPosition alignment) { return (containerHasWrapReverse && alignment == ItemPosition::Baseline) || (alignment == ItemPosition::LastBaseline && !containerHasWrapReverse); }; // ... if (something(position)) minMarginAfterBaseline = std::min(minMarginAfterBaseline, availableAlignmentSpaceForChild(lineCrossAxisExtent, flexItem.box) - offset); // ... if (!hasAutoMarginsInCrossAxis(flexItem.box) && something(alignmentForChild(flexItem.box))) adjustAlignmentForChild(flexItem.box, minMarginAfterBaseline);

Yes it should be for both. I made the adjustment, but let me know if you see any other issues with this

Remove !hasAutoMarginsInCrossAxis(box) from shouldAdjustItemTowardsCrossAxis since the caller that needs it is already checking it?

Removed now

webkit-early-warning-system · 2022-10-05T23:45:27Z

EWS run on previous version of this PR (hash 9336c2b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🛠 mac-AS-debug	⏳ 🧪 gtk-wk2
	~~🧪 api-ios~~	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
	✅ 🛠 tv	⏳ 🧪 mac-wk1
	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-10-06T00:06:01Z

EWS run on previous version of this PR (hash 14fc529)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	~~🛠 ios~~	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	⏳ 🛠 ios-sim	⏳ 🛠 mac-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	⏳ 🛠 mac-AS-debug	⏳ 🧪 gtk-wk2
	⏳ 🧪 api-ios	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk1	⏳ 🛠 jsc-armv7
	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2	⏳ 🧪 jsc-armv7-tests
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-10-06T00:11:02Z

EWS run on previous version of this PR (hash 2cffcf6)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

brentfulgham

LGTM with the changes Loirooriol (and I) suggested.

Source/WebCore/rendering/RenderFlexibleBox.cpp

Source/WebCore/rendering/RenderTable.cpp

Loirooriol

Informal LGTM with the suggested changes

Loirooriol · 2022-10-07T18:31:35Z

Source/WebCore/rendering/RenderFlexibleBox.cpp

+bool RenderFlexibleBox::childIsParticipatingInBaselineAlignment(const RenderBox& child) const
+{
+    auto alignment = alignmentForChild(child);
+    return (alignment == ItemPosition::Baseline || alignment == ItemPosition::LastBaseline) && !hasAutoMarginsInCrossAxis(child);
+}


It's no longer used, remove it.

Loirooriol · 2022-10-07T18:33:38Z

Source/WebCore/rendering/RenderFlexibleBox.cpp

-
+    bool containerHasWrapReverse = style().flexWrap() == FlexWrap::Reverse;
+    auto shouldAdjustItemTowardsCrossAxis = [this, containerHasWrapReverse](const RenderBox& box, ItemPosition alignment) { 
+        return (containerHasWrapReverse && alignment == ItemPosition::Baseline && !hasAutoMarginsInCrossAxis(box)) || (alignment == ItemPosition::LastBaseline && !containerHasWrapReverse); 


Remove !hasAutoMarginsInCrossAxis(box) from shouldAdjustItemTowardsCrossAxis since the caller that needs it is already checking it?

Loirooriol · 2022-10-07T19:05:27Z

Source/WebCore/rendering/RenderTable.cpp

-    });
+    auto baselinePos = firstLineBaseline();
+    if (baselinePos)
+        return (direction == HorizontalLine ? marginTop() : marginRight()) + firstLineBaseline().value();


I get that you need this fix for some tests, but it's actually fixing the reference, not the flexbox tests, so it seems strange to do it here.
I would prefer to do this change in its own PR, and have a specific test not involving flexbox, like comparing

<div style="display: inline-block; background: lime;">line 1</div> <div style="display: inline-table; margin-top: 20px; background: aqua;">line 1<br/>line 2</div>

with this reference:

<div style="display: inline-block; margin-top: 20px; background: lime; vertical-align: top">line 1</div> <div style="display: inline-block; margin-top: 20px; background: aqua;">line 1<br/>line 2</div>

That makes sense. I refactored it out and created its own PR

#5153

webkit-early-warning-system · 2022-10-07T19:42:10Z

EWS run on previous version of this PR (hash 132311a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

sammygill · 2022-10-07T21:37:57Z

Once Add margin when comptuing baseline position for tables. #5153 lands need to remove the code that was refactored out. 2 of the TestExpectations rely on that patch in order to pass

webkit-early-warning-system · 2022-10-10T19:22:13Z

EWS run on previous version of this PR (hash 6857a96)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

sammygill · 2022-10-10T19:23:05Z

Rebaselined test expectations with the new tests imported from WPT

webkit-early-warning-system · 2022-10-11T01:11:18Z

EWS run on current version of this PR (hash c1e411a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 🧪 win~~
	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
✅ 🛠 🧪 merge	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

https://bugs.webkit.org/show_bug.cgi?id=244734 rdar://99506835 Reviewed by Brent Fulgham. In order to implement last baseline alignment for flex containers, we need to hold a little bit more information. Currently, RenderFlexibleBox::LineContext holds a maxAscent value, which holds the value of the furthest baseline in the cross axis. This is then used to shift the other flex items to this baseline. A new field called lastBaselineMaxAscent was added to achieve this same functionality for the last baseline elements. This needs to be a separate field in scenarios where you may have certain items being aligned by their baselines and others by their last baselines. This patch also adds the “margin before” to the first baseline of table elements since it was not being added before. This is needed in order to properly align other elements alongside a table with margins. Without this addition, we were rendering the expectation for both css-flexbox/flexbox-align-self-baseline-horiz-001a and css-flexbox/flexbox-align-self-baseline-horiz-001b incorrectly. * LayoutTests/TestExpectations: * Source/WebCore/rendering/RenderFlexibleBox.cpp: (WebCore::RenderFlexibleBox::LineContext::LineContext): (WebCore::RenderFlexibleBox::marginBoxAscentForChild): Since marginBoxAscentForChild is being called during alignment purposes, we can determine what type of alignment is being done here and return the appropriate ascent (first baseline vs last baseline). (WebCore::alignmentOffset): (WebCore::RenderFlexibleBox::layoutAndPlaceChildren): Depending on whether the item is being first baseline aligned or last baseline aligned, we need to either update maxAscent or lastBaselineMaxAscent. Later on when the alignment is being done, we can check the type of alignment one more time and use the correct value. (WebCore::RenderFlexibleBox::alignChildren): Since items need to be pushed to the end of the cross axis when wrap-reverse is specified, we can use the same logic to push the items that are being last baseline aligned. There was also a little bit of cleanup that I was able to do here. Instead of waiting until all of the lines have had their items aligned to make any adjustments (e.g. due to wrap-reverse), we can do it after laying out each line. We can determine whether or not we need to loop over the line again by checking the value of minMarginAfterBaseline. If this value was set then that means that not only is there room to move the items in the flex line, but also that we need to because the flex container has wrap-reverse set or some items are being last baseline aligned. (WebCore::RenderFlexibleBox::childIsParticipatingInBaselineAlignment const): * Source/WebCore/rendering/RenderFlexibleBox.h: * Source/WebCore/rendering/RenderTableSection.cpp: (WebCore::RenderTable::baselinePosition const): This is not directly related to the last baseline implementation but to a test case for it. There were 2 expectation files we were rendering incorrectly (css-flexbox/flexbox-align-self-baseline-horiz-001a-expected.xhtml and flexbox-align-self-baseline-horiz-001b-expected.xhtml). These were using the first baseline for table elements and we were not computing the correct first baseline. This caused us to fail the tests even though we were rendering the actual test file correctly. Canonical link: https://commits.webkit.org/255383@main

webkit-commit-queue · 2022-10-11T15:35:16Z

Committed 255383@main (a7e014a): https://commits.webkit.org/255383@main

Reviewed commits have been landed. Closing PR #4067 and removing active labels.

sammygill self-assigned this Sep 6, 2022

sammygill added CSS Cascading Style Sheets implementation WebKit Nightly Build labels Sep 6, 2022

sammygill requested a review from rwlbuis September 8, 2022 17:39

rwlbuis reviewed Sep 8, 2022

View reviewed changes

Source/WebCore/rendering/RenderBlockFlow.cpp Outdated Show resolved Hide resolved

Source/WebCore/rendering/RenderBlock.cpp Outdated Show resolved Hide resolved

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from 4296caa to 4f2c4cd Compare September 15, 2022 19:12

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 15, 2022

sammygill removed the merging-blocked Applied to prevent a change from being merged label Sep 22, 2022

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from 4f2c4cd to ebec868 Compare September 22, 2022 22:31

sammygill requested a review from alanbaradlay September 22, 2022 22:32

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 23, 2022

sammygill removed the merging-blocked Applied to prevent a change from being merged label Sep 27, 2022

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from ebec868 to 49f2d52 Compare September 27, 2022 05:16

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from 49f2d52 to 63e7fc6 Compare September 27, 2022 17:36

sammygill changed the title ~~Implement last baseline for block elements in a flex container.~~ Implement last baseline for block flow elements and last baseline alignment for Flexbox containers Sep 27, 2022

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from 63e7fc6 to 2ba17d5 Compare September 28, 2022 19:02

sammygill changed the title ~~Implement last baseline for block flow elements and last baseline alignment for Flexbox containers~~ Implement last baseline alignment for Flexbox containers. Sep 28, 2022

sammygill marked this pull request as draft October 4, 2022 16:43

Loirooriol reviewed Oct 4, 2022

View reviewed changes

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from 2ba17d5 to 8710e23 Compare October 5, 2022 21:41

sammygill marked this pull request as ready for review October 5, 2022 21:42

Loirooriol reviewed Oct 5, 2022

View reviewed changes

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from c97879e to 9336c2b Compare October 5, 2022 23:45

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from 9336c2b to 14fc529 Compare October 6, 2022 00:05

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from 14fc529 to 2cffcf6 Compare October 6, 2022 00:10

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 6, 2022

brentfulgham approved these changes Oct 7, 2022

View reviewed changes

Source/WebCore/rendering/RenderFlexibleBox.cpp Show resolved Hide resolved

Source/WebCore/rendering/RenderFlexibleBox.cpp Outdated Show resolved Hide resolved

Source/WebCore/rendering/RenderTable.cpp Outdated Show resolved Hide resolved

Loirooriol approved these changes Oct 7, 2022

View reviewed changes

sammygill removed the merging-blocked Applied to prevent a change from being merged label Oct 7, 2022

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from 2cffcf6 to 132311a Compare October 7, 2022 19:41

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 8, 2022

sammygill removed the merging-blocked Applied to prevent a change from being merged label Oct 10, 2022

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from 132311a to 6857a96 Compare October 10, 2022 19:21

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 11, 2022

sammygill removed the merging-blocked Applied to prevent a change from being merged label Oct 11, 2022

sammygill force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from 6857a96 to c1e411a Compare October 11, 2022 01:11

rreno added the merge-queue Applied to send a pull request to merge-queue label Oct 11, 2022

webkit-early-warning-system force-pushed the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch from c1e411a to a7e014a Compare October 11, 2022 15:35

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 11, 2022

webkit-early-warning-system merged commit a7e014a into WebKit:main Oct 11, 2022

sammygill deleted the eng/Add-Flexbox-last-baseline-alignment-for-block-flow-items branch November 30, 2022 19:17

Implement last baseline alignment for Flexbox containers. #4067

Implement last baseline alignment for Flexbox containers. #4067

Conversation

sammygill commented Sep 6, 2022 • edited by webkit-early-warning-system

webkit-early-warning-system commented Sep 6, 2022 • edited

webkit-early-warning-system commented Sep 15, 2022

webkit-early-warning-system commented Sep 15, 2022 • edited

webkit-early-warning-system commented Sep 22, 2022 • edited

sammygill commented Sep 22, 2022

webkit-early-warning-system commented Sep 27, 2022 • edited

webkit-early-warning-system commented Sep 27, 2022 • edited

webkit-early-warning-system commented Sep 28, 2022 • edited

Loirooriol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Oct 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Oct 5, 2022 • edited

webkit-early-warning-system commented Oct 6, 2022 • edited

webkit-early-warning-system commented Oct 6, 2022 • edited

brentfulgham left a comment

Choose a reason for hiding this comment

Loirooriol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Oct 7, 2022 • edited

sammygill commented Oct 7, 2022 • edited

webkit-early-warning-system commented Oct 10, 2022 • edited

sammygill commented Oct 10, 2022

webkit-early-warning-system commented Oct 11, 2022 • edited

webkit-commit-queue commented Oct 11, 2022

sammygill commented Sep 6, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Sep 6, 2022 •

edited

webkit-early-warning-system commented Sep 15, 2022 •

edited

webkit-early-warning-system commented Sep 22, 2022 •

edited

webkit-early-warning-system commented Sep 27, 2022 •

edited

webkit-early-warning-system commented Sep 27, 2022 •

edited

webkit-early-warning-system commented Sep 28, 2022 •

edited

webkit-early-warning-system commented Oct 5, 2022 •

edited

webkit-early-warning-system commented Oct 5, 2022 •

edited

webkit-early-warning-system commented Oct 6, 2022 •

edited

webkit-early-warning-system commented Oct 6, 2022 •

edited

webkit-early-warning-system commented Oct 7, 2022 •

edited

sammygill commented Oct 7, 2022 •

edited

webkit-early-warning-system commented Oct 10, 2022 •

edited

webkit-early-warning-system commented Oct 11, 2022 •

edited