[webkit-reviews] review granted: [Bug 229996] [css-flexbox] Add support for self-start & self-end css-align-3 positional alignment properties : [Attachment 437489] Patch

[bugzilla-daemon at webkit.org]

Manuel Rego Casasnovas <rego at igalia.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 229996: [css-flexbox] Add support for self-start & self-end css-align-3
positional alignment properties
https://bugs.webkit.org/show_bug.cgi?id=229996

Attachment 437489: Patch

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




--- Comment #5 from Manuel Rego Casasnovas <rego at igalia.com> ---
Comment on attachment 437489
  --> https://bugs.webkit.org/attachment.cgi?id=437489
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.xh
tml [ 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.