Bug 229996

Summary: [css-flexbox] Add support for self-start & self-end css-align-3 positional alignment properties
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, clopez, esprehn+autocc, ews-watchlist, glenn, jfernandez, koivisto, kondapallykalyan, pdr, rego, simon.fraser, svillar, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch rego: review+

Description Sergio Villar Senin 2021-09-07 04:38:43 PDT
[css-flexbox] Add support for self-start & self-end css-align-3 positional alignment properties
Comment 1 Sergio Villar Senin 2021-09-07 05:01:59 PDT
Created attachment 437489 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2021-09-07 06:48:42 PDT
Comment on attachment 437489 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Added support for Left and Right positional alignment properties from

Left and Right? Isn't now SelfStart and SelfEnd?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1727
> +        bool isOrthogonal = style().isHorizontalWritingMode() != child.style().isHorizontalWritingMode();

Nit: We should move that isOrthogonal() method to RenderObject one of these days. O:-)

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1728
> +        if (isOrthogonal && (style().isFlippedBlocksWritingMode() == child.style().isLeftToRightDirection()))

I don't understand this comparison "(style().isFlippedBlocksWritingMode() == child.style().isLeftToRightDirection())".

> LayoutTests/TestExpectations:4168
>  webkit.org/b/221468 imported/w3c/web-platform-tests/css/css-flexbox/flexbox-align-self-horiz-002.xhtml [ ImageOnlyFailure ]

This test is pretty similar to the next one, why is this not passing?
Comment 3 Sergio Villar Senin 2021-09-10 05:03:47 PDT
Comment on attachment 437489 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Added support for Left and Right positional alignment properties from
> 
> Left and Right? Isn't now SelfStart and SelfEnd?

Ooops copy-paste is hard :)

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1727
>> +        bool isOrthogonal = style().isHorizontalWritingMode() != child.style().isHorizontalWritingMode();
> 
> Nit: We should move that isOrthogonal() method to RenderObject one of these days. O:-)

sure :), let me land this sequence of patches and I'll do it.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1728
>> +        if (isOrthogonal && (style().isFlippedBlocksWritingMode() == child.style().isLeftToRightDirection()))
> 
> I don't understand this comparison "(style().isFlippedBlocksWritingMode() == child.style().isLeftToRightDirection())".

A bit convoluted maybe. The general idea is that for orthogonal modes SelfStart corresponds to FlexStart with a couple of exceptions, which is when the container has flipped blocks writing mode (vrl or hbt) and the child is LTR, or when it isn't flipped (vlr or htb) but the child is RTL. A couple of examples could illustrate it better:
1) a HTB ltr child inside a VRL container. In this case the physical left edge is flex item's self-start but flexbox line's flex-end
2) a HTB RTL child inside a VLR container. In this case the physical left edge is flex item's self-end but flexbox line's flex-start

>> LayoutTests/TestExpectations:4168
>>  webkit.org/b/221468 imported/w3c/web-platform-tests/css/css-flexbox/flexbox-align-self-horiz-002.xhtml [ ImageOnlyFailure ]
> 
> This test is pretty similar to the next one, why is this not passing?

Interesting and good question, checking the test case, there is just one block that is not positioned correctly. I'll try to figure out why.
Comment 4 Sergio Villar Senin 2021-09-10 05:13:21 PDT
(In reply to Sergio Villar Senin from comment #3)
> Comment on attachment 437489 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437489&action=review

[...]

> >> LayoutTests/TestExpectations:4168
> >>  webkit.org/b/221468 imported/w3c/web-platform-tests/css/css-flexbox/flexbox-align-self-horiz-002.xhtml [ ImageOnlyFailure ]
> > 
> > This test is pretty similar to the next one, why is this not passing?
> 
> Interesting and good question, checking the test case, there is just one
> block that is not positioned correctly. I'll try to figure out why.

It turns out that the test result is correct, it's the expectation the one that is not properly rendered. I'll research why but IMHO it shouldn't block this patch as it is a different bug (the expectations are not using the alignment properties at all).
Comment 5 Manuel Rego Casasnovas 2021-09-10 07:02:08 PDT
Comment on attachment 437489 [details]
Patch

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

r=me

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1728
>>> +        if (isOrthogonal && (style().isFlippedBlocksWritingMode() == child.style().isLeftToRightDirection()))
>> 
>> I don't understand this comparison "(style().isFlippedBlocksWritingMode() == child.style().isLeftToRightDirection())".
> 
> A bit convoluted maybe. The general idea is that for orthogonal modes SelfStart corresponds to FlexStart with a couple of exceptions, which is when the container has flipped blocks writing mode (vrl or hbt) and the child is LTR, or when it isn't flipped (vlr or htb) but the child is RTL. A couple of examples could illustrate it better:
> 1) a HTB ltr child inside a VRL container. In this case the physical left edge is flex item's self-start but flexbox line's flex-end
> 2) a HTB RTL child inside a VLR container. In this case the physical left edge is flex item's self-end but flexbox line's flex-start

Thanks for the explanation. Maybe add a comment with this.

>>>> LayoutTests/TestExpectations:4168
>>>>  webkit.org/b/221468 imported/w3c/web-platform-tests/css/css-flexbox/flexbox-align-self-horiz-002.xhtml [ ImageOnlyFailure ]
>>> 
>>> This test is pretty similar to the next one, why is this not passing?
>> 
>> Interesting and good question, checking the test case, there is just one block that is not positioned correctly. I'll try to figure out why.
> 
> It turns out that the test result is correct, it's the expectation the one that is not properly rendered. I'll research why but IMHO it shouldn't block this patch as it is a different bug (the expectations are not using the alignment properties at all).

Yeah I don't think that should block anything. But it might be good to report that separated bug and link it from TestExpectations.
Comment 6 Sergio Villar Senin 2021-09-10 07:27:06 PDT
Committed r282267 (241545@main): <https://commits.webkit.org/241545@main>
Comment 7 Radar WebKit Bug Importer 2021-09-10 09:14:21 PDT
<rdar://problem/82975122>