[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) {
+        start = LayoutUnit();
+    } else {
+        start = isForColumns ? m_columnPositions[startLine] : m_rowPositions[startLine];
+        start -= isForColumns ? m_columnPositions[0] : m_rowPositions[0];
+        start += isForColumns ? paddingStart() : paddingBefore();

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

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();
}

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

Done.

+    }
+
+    LayoutUnit end;
+    if (endIsAuto) {
+        end = isForColumns ? clientLogicalWidth() : clientLogicalHeight();
+    } else {
+        end = isForColumns ? m_columnPositions[endLine] : m_rowPositions[endLine];
+        end -= isForColumns ? m_columnPositions[0] : m_rowPositions[0];
+        end += isForColumns ? paddingStart() : paddingBefore();
+    }

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

Same comment than for the start block

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

Done.

 
     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).
+        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;
+        }

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

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?

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

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.

     }
 
-    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);
     }
+

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

Extra line.

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

Acknowledged.

 }
 
 GridArea LayoutGrid::cachedGridArea(const LayoutBox& gridItem) const
 {
     ASSERT(m_gridItemArea.contains(&gridItem));
     return m_gridItemArea.get(&gridItem);
 }
 
 GridSpan LayoutGrid::cachedGridSpan(const LayoutBox& gridItem, GridTrackSizingDirection direction) const
 {
@@ skipping 477 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