Bug 229996 – [css-flexbox] Add support for self-start & self-end css-align-3 positional alignment properties

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[css-flexbox] Add support for self-start & self-end css-align-3 positional alignment properties

[Sergio Villar Senin]

Created attachment 437489 [details]
Patch

[Manuel Rego Casasnovas]

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?

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

(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).

[Manuel Rego Casasnovas]

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.

[Sergio Villar Senin]

Committed r282267 (241545@main): <https://commits.webkit.org/241545@main>

[Radar WebKit Bug Importer]

<rdar://problem/82975122>