[css-grid] Fix positioned children in RTL direction

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

 /*
  * Copyright (C) 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 1504 matching lines @@
 
     bool startIsAuto = startPosition.isAuto()
         || (startPosition.isNamedGridArea() && !GridPositionsResolver::isValidNamedLineOrArea(startPosition.namedGridLine(), styleRef(), GridPositionsResolver::initialPositionSide(direction)))
         || (startLine < firstExplicitLine)
         || (startLine > lastExplicitLine);
     bool endIsAuto = endPosition.isAuto()
         || (endPosition.isNamedGridArea() && !GridPositionsResolver::isValidNamedLineOrArea(endPosition.namedGridLine(), styleRef(), GridPositionsResolver::finalPositionSide(direction)))
         || (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];
+    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();
+    }
+
+    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 (startIsAuto)
-        breadth -= isForColumns ? borderStart() : borderBefore();
-    else
-        start -= isForColumns ? borderStart() : borderBefore();
-
-    if (endIsAuto) {
-        breadth -= isForColumns ? borderEnd() : borderAfter();
-        breadth -= scrollbarLogicalWidth();
+    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;
+        }
     }
 
-    offset = start;
-
     if (child.parent() == this && !startIsAuto) {
         // If column/row start is "auto" the static position has been already set in prepareChildForPositionedLayout().
         PaintLayer* childLayer = child.layer();
         if (isForColumns)
             childLayer->setStaticInlinePosition(borderStart() + offset);
         else
             childLayer->setStaticBlockPosition(borderBefore() + offset);
     }
 }
 
@@ skipping 485 matching lines @@
 
     return LayoutPoint(rowAxisOffset, columnAxisOffsetForChild(child, sizingData));
 }
 
 void LayoutGrid::paintChildren(const PaintInfo& paintInfo, const LayoutPoint& paintOffset) const
 {
     GridPainter(*this).paintChildren(paintInfo, paintOffset);
 }
 
 } // namespace blink