Bug 224538 – [css-grid] last-baseline shouldn't affect baseline alignment

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

Spec link:
https://github.com/w3c/csswg-drafts/issues/5293

A couple of subtests have failed in css/css-grid/alignment/grid-baseline-004.html after an update from 
https://github.com/web-platform-tests/wpt/commit/e3698c7bb3c309df69134e9bc0a375f00535e226

Created attachment 426028 [details]
Patch

[Javier Fernandez]

Comment on attachment 426028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426028&action=review

> Source/WebCore/rendering/RenderGrid.cpp:1246
> +    return isBaselinePosition(align) && align != ItemPosition::LastBaseline && !hasAutoMargins;

I don't think this is correct. The purpose of this function was to determine whether a grid item has or not a value for the CSS Box Alignment properties that may imply it participates in baseline alignment (this would depend on other factors too). Hence, last-baseline is a valid value for this purpose.

If I've understood the CSS WG resolution, what we want is to prevent last-baseline to affect how we compute the grid container's baseline, which the algorithm described in the spec states that we should consider grid items participating in baseline alignment "along the first row". That indeed implies excluding items with last-baseline value, but only for this specific purpose. 

I'd rather add the logic to exclude the last-baseline value as part of the algorithm implemented in  RenderGrid::firstLineBaseline.

Created attachment 426201 [details]
Patch

(In reply to Javier Fernandez from comment #2)
> Comment on attachment 426028 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426028&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:1246
> > +    return isBaselinePosition(align) && align != ItemPosition::LastBaseline && !hasAutoMargins;
> 
> I don't think this is correct. The purpose of this function was to determine
> whether a grid item has or not a value for the CSS Box Alignment properties
> that may imply it participates in baseline alignment (this would depend on
> other factors too). Hence, last-baseline is a valid value for this purpose.
> 
> If I've understood the CSS WG resolution, what we want is to prevent
> last-baseline to affect how we compute the grid container's baseline, which
> the algorithm described in the spec states that we should consider grid
> items participating in baseline alignment "along the first row". That indeed
> implies excluding items with last-baseline value, but only for this specific
> purpose. 
> 
> I'd rather add the logic to exclude the last-baseline value as part of the
> algorithm implemented in  RenderGrid::firstLineBaseline.

Code updated. Thank you!

[Darin Adler]

Comment on attachment 426201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426201&action=review

> Source/WebCore/rendering/RenderGrid.cpp:1272
> +            if (isBaselineAlignmentForChild(*child) && !(alignSelfForChild(*child).position() == ItemPosition::LastBaseline || justifySelfForChild(*child).position() == ItemPosition::LastBaseline)) {

Is this the most elegant way to write it? I have trouble understanding the code. Also, consider the Demorgan’s Law equivalent:

    if (isBaselineAlignmentForChild(*child) && alignSelfForChild(*child).position() != ItemPosition::LastBaseline && justifySelfForChild(*child).position() != ItemPosition::LastBaseline) {

Maybe another named helper function like isBaselineAlignmentForChild could make the code more readable, although I don’t know what I’d name it.

Created attachment 426399 [details]
Patch

[Javier Fernandez]

Comment on attachment 426399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426399&action=review

> Source/WebCore/rendering/RenderGrid.cpp:1277
> +            if (isBaselineAlignmentForChild(*child) && !iSelfAlignmentForChildAtLastBaseline(*child)) {

If you let me suggest something, I'd rather create a new function in GridBaselineAlignment.h for checking only first-line baseline (although such keyword doesn't exist formally in the spec). Then, we can pass that function as an argument for RenderGrid::isBaselineAlignmentForChild (perhaps use a template here ?). Something like this:

static inline bool isFirstBaselinePosition(ItemPosition position) { return position == ItemPosition::Baseline; }

template <typename F>
bool RenderGrid::isBaselineAlignmentForChild(const RenderBox& child, F isBaseline) const;

template <typename F>
bool RenderGrid::isBaselineAlignmentForChild(const RenderBox& child, GridAxis baselineAxis, F isBaseline) const;

Then, we can make the non-template versions of these 2 methods so that they use the "isBaselinePosition" function we already have. This way we don't need to apply any change on the many callers of isBaselineAlignmentForChild. 

For the case we are changing the behavior in this patch, we would use the new isFirstBaseline call we are defining now.

Created attachment 426539 [details]
Patch

[Javier Fernandez]

Comment on attachment 426539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426539&action=review

> Source/WebCore/rendering/RenderGrid.cpp:1236
> +bool RenderGrid::isBaselineAlignmentForChild(const RenderBox& child, F isBaseline) const

As we discussed privately, perhaps this approach is not the ideal one. We can achieve the same using an enumeration instead of the function argument, so that we can figure whether first baseline, last baseline or both are valid values. 
We can define the new argument to default to "both", which is the common case and it'd avoid to change any of the other callers. What do you think ?

Created attachment 426665 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/76948720>

[Javier Fernandez]

Comment on attachment 426665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426665&action=review

r=me

> Source/WebCore/rendering/RenderGrid.cpp:1246
> +    bool isBaseline = allowed == FirstLine? isFirstBaselinePosition(align) : isBaselinePosition(align);

nit: there must be a space between FirstLine and the "?" symbol.

Created attachment 426670 [details]
Patch

[EWS]

Committed r276356 (236834@main): <https://commits.webkit.org/236834@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426670 [details].