[css-grid] Fix positioned children in RTL direction

third_party/WebKit/Source/core/layout/LayoutGrid.cpp

Index: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
index 9f538821e67418d11cc5f8fc6fef418f1c31d3ea..7f427529f36cae48ad6d23dbb6cf0d512d35ba40 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -1522,23 +1522,36 @@ void LayoutGrid::offsetAndBreadthForPositionedChild(const LayoutBox& child, Grid
         || (endLine < firstExplicitLine)
         || (endLine > lastExplicitLine);
 
-    LayoutUnit start = startIsAuto ? LayoutUnit() : isForColumns ?  m_columnPositions[startLine] : m_rowPositions[startLine];
-    LayoutUnit end = endIsAuto ? isForColumns ? logicalWidth() : logicalHeight() : isForColumns ?  m_columnPositions[endLine] : m_rowPositions[endLine];
-
-    breadth = end - start;
-
-    if (startIsAuto)
-        breadth -= isForColumns ? borderStart() : borderBefore();
-    else
-        start -= isForColumns ? borderStart() : borderBefore();
+    LayoutUnit start;
+    if (!startIsAuto) {

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

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.

+        if (isForColumns)
+            start = m_columnPositions[startLine] - m_columnPositions[0] + paddingStart();
+        else
+            start = m_rowPositions[startLine] - m_rowPositions[0] + paddingBefore();
+    }
 
-    if (endIsAuto) {
-        breadth -= isForColumns ? borderEnd() : borderAfter();
-        breadth -= scrollbarLogicalWidth();
+    LayoutUnit end = isForColumns ? clientLogicalWidth() : clientLogicalHeight();

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

The previous code was using logicalXXX. Is the client size really what we want?

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

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

+    if (!endIsAuto) {
+        if (isForColumns)
+            end = m_columnPositions[endLine] - m_columnPositions[0] + paddingStart();
+        else
+            end = m_rowPositions[endLine] - m_rowPositions[0] + paddingBefore();
     }
 
+    breadth = end - start;
     offset = start;
 
+    if (isForColumns && !styleRef().isLeftToRightDirection() && !child.styleRef().hasStaticInlinePosition(child.isHorizontalWritingMode())) {
+        // 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).

[cbiesinger, 2016/03/23 19:50:48]

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]

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]

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]

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.
Yeah, I'll split this in two patches, one for the refactoring,
and another for adding this "special" if.

+        if (endIsAuto) {
+            offset = LayoutUnit();
+        } else {
+            LayoutUnit alignmentOffset =  m_columnPositions[0] - borderAndPaddingStart();
+            LayoutUnit offsetFromLastLine = m_columnPositions[m_columnPositions.size() - 1] - m_columnPositions[endLine];
+            offset = paddingLeft() +  alignmentOffset + offsetFromLastLine;
+        }
+    }
+
     if (child.parent() == this && !startIsAuto) {
         // If column/row start is "auto" the static position has been already set in prepareChildForPositionedLayout().
         PaintLayer* childLayer = child.layer();