[css-grid] Fix positioned children in RTL direction

[css-grid] Fix positioned children in RTL direction

We were not honouring RTL for positioned children, we didn't have
any specific code for that so a lot of cases were not working properly.

Refactored the code in offsetAndBreadthForPositionedChild() in order to
calculate offset and breadth in a more clean way that works for both
LTR and RTL.

Added RTL cases for most of the positioned tests, which are now passing
with this patch.
Converted some of the tests to testharness tests to avoid the need of
updating the expected results.

BUG=568882

[Manuel Rego, 2016-03-23 09:57:16]

Description was changed from

==========
[css-grid] Fix positioned children in RTL direction

We were not honouring RTL for positioned children, we didn't have
any specific code for that so a lot of cases were not working properly.

Refactored the code in offsetAndBreadthForPositionedChild() in order to
calculate offset and breadth in a more clean way that works for both
LTR and RTL.

Added RTL cases for most of the positioned tests, which are now passing
with this patch.

BUG=568882
==========

to

==========
[css-grid] Fix positioned children in RTL direction

We were not honouring RTL for positioned children, we didn't have
any specific code for that so a lot of cases were not working properly.

Refactored the code in offsetAndBreadthForPositionedChild() in order to
calculate offset and breadth in a more clean way that works for both
LTR and RTL.

Added RTL cases for most of the positioned tests, which are now passing
with this patch.
Converted some of the tests to testharness tests to avoid the need of
updating the expected results.

BUG=568882
==========

[Manuel Rego, 2016-03-23 09:57:55]

rego@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jfernandez@igalia.com, svillar@igalia.com

[svillar, 2016-03-23 10:43:55]

Good start.

https://codereview.chromium.org/1824323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1824323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1531: start += isForColumns
? paddingStart() : paddingBefore();
Better than the nested ternary but still difficult to follow the flow (also you
don't need the startIsAuto branch). I suggest

LayoutUnit start;
if (!startIsAuto) {
    if (isForColumns)
        start = m_columnPositions[startLine]  - m_columnPositions[0] +
paddingStart();
    else
        start = m_rowPositions[startLine] - m_rowPositions[0] + paddingBefore();
}

https://codereview.chromium.org/1824323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1541: }
Same comment than for the start block

https://codereview.chromium.org/1824323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1554: }
I don't get this. This CL is about fixing the behavior of positioning children
in RTL and you add a block to handle non-static items?

https://codereview.chromium.org/1824323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1565: 
Extra line.

[Manuel Rego, 2016-03-23 11:20:52]

Thanks for the quick review.

New version with the suggested changes.

https://codereview.chromium.org/1824323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1824323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1531: start += isForColumns
? paddingStart() : paddingBefore();
On 2016/03/23 10:43:55, svillar wrote:
> Better than the nested ternary but still difficult to follow the flow (also
you
> don't need the startIsAuto branch). I suggest
> 
> LayoutUnit start;
> if (!startIsAuto) {
>     if (isForColumns)
>         start = m_columnPositions[startLine]  - m_columnPositions[0] +
> paddingStart();
>     else
>         start = m_rowPositions[startLine] - m_rowPositions[0] +
paddingBefore();
> }

Done.

https://codereview.chromium.org/1824323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1541: }
On 2016/03/23 10:43:55, svillar wrote:
> Same comment than for the start block

Done.

https://codereview.chromium.org/1824323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1554: }
I think you're mixing concepts here.

Of course this CL only has to do with positioned children.
This code is only executed for those elements.

The thing that is probably confusing you is the term "static position"
(https://www.w3.org/TR/CSS21/visudet.html#static-position)
and the "static" value in "position: static" vs "postiion: absolute".

Let's use an example of a regular block:
<div style="position: relative; border: 5px solid; padding: 10px;">
  <div style="position: absolute; background: lime;">test</div>
</div>

By default "left: auto;", the test element will use the static position,
it'll be placed in the padding: 15px (5px + 10px).
In this case hasStaticInlinePosition() returns TRUE.

However if you set "left: 0px" then it'd be placed in the border:
5px (5px + 0px). Here hasStaticInlinePosition() returns FALSE.

https://codereview.chromium.org/1824323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1565: 
On 2016/03/23 10:43:55, svillar wrote:
> Extra line.

Acknowledged.

[cbiesinger, 2016-03-23 19:50:49]

https://codereview.chromium.org/1824323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1824323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1545: // If the child
doesn't have a static inline position, we need to calculate the offset from the
left (even if we're in RTL).
Sorry, why do we need to calculate anything when the child doesn't use a static
position? It would just use the specified one anyway, right? What am I missing?

[Manuel Rego, 2016-03-28 08:57:29]

https://codereview.chromium.org/1824323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1824323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1545: // If the child
doesn't have a static inline position, we need to calculate the offset from the
left (even if we're in RTL).
On 2016/03/23 19:50:48, cbiesinger wrote:
> Sorry, why do we need to calculate anything when the child doesn't use a
static
> position? It would just use the specified one anyway, right? What am I
missing?

Let's use an example.
Imagine that we've a grid with 3 columns: 200px 100px 50px.
And that we're placing a positioned item in the 2nd column.

The positioned item might have something like: "left: 0".

If we're in LTR, the item will be placed in position 200px.
To achieve this behavior, we've a special condition in
LayoutBox::computePositionedLogicalWidth():
https://chromium.googlesource.com/chromium/blink/+/8d2d7506bebf376fcb885f8344...

However, if we're in RTL, it should be in position 50px.
So we need to calculate this new offset
(from the left even if we're in RTL),
in order to get the expected behavior.

[svillar, 2016-03-29 10:17:58]

https://codereview.chromium.org/1824323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1824323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1526: if (!startIsAuto) {
This deserves a comment about how positions are stored in RTL, and in order to
understand why you "normalize" the positions to the first column position.

https://codereview.chromium.org/1824323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1533: LayoutUnit end =
isForColumns ? clientLogicalWidth() : clientLogicalHeight();
The previous code was using logicalXXX. Is the client size really what we want?

https://codereview.chromium.org/1824323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1545: // If the child
doesn't have a static inline position, we need to calculate the offset from the
left (even if we're in RTL).
Perhaps the comment could be improved. Saying "if left: and right: are both
auto" is much clearer than "have static inline position" IMO.

This if is rather obscure (seems to ad-hoc for a symmetrical layout like grid).
Perhaps we could split this patch in 2, one with the refactoring and another
with the fix.

[Manuel Rego, 2016-03-29 10:34:51]

Thanks for the review, I'm closing this patch as I'll be splitting it in 2.

https://codereview.chromium.org/1824323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1824323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1533: LayoutUnit end =
isForColumns ? clientLogicalWidth() : clientLogicalHeight();
On 2016/03/29 10:17:58, svillar wrote:
> The previous code was using logicalXXX. Is the client size really what we
want?

Previous code was subtracting the border and we can avoid that now
thanks to using the client size.

https://codereview.chromium.org/1824323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1545: // If the child
doesn't have a static inline position, we need to calculate the offset from the
left (even if we're in RTL).
On 2016/03/29 10:17:58, svillar wrote:
> Perhaps the comment could be improved. Saying "if left: and right: are both
> auto" is much clearer than "have static inline position" IMO.

Not sure if I agree as the term "static position" can be found in the spec.
But I can try to improve it of course.

> This if is rather obscure (seems to ad-hoc for a symmetrical layout like
grid).
> Perhaps we could split this patch in 2, one with the refactoring and another
> with the fix.

Yeah, I'll split this in two patches, one for the refactoring,
and another for adding this "special" if.