fix getComputedStyle positioned element values

third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp

Index: third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
diff --git a/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp b/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
index c2262b9e55127ffd155c225031147f7ce0ffcf65..816a7abe9abc46d48ff1f8a92356f55cd1db911e 100644
--- a/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
+++ b/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
@@ -162,19 +162,23 @@ static PassRefPtrWillBeRawPtr<CSSValue> valueForFillSourceType(EMaskSourceType t
 
 static PassRefPtrWillBeRawPtr<CSSValue> valueForPositionOffset(const ComputedStyle& style, CSSPropertyID propertyID, const LayoutObject* layoutObject)
 {
-    Length offset;
+    Length offset, opposite;
     switch (propertyID) {
     case CSSPropertyLeft:
         offset = style.left();
+        opposite = style.right();
         break;
     case CSSPropertyRight:
         offset = style.right();
+        opposite = style.left();
         break;
     case CSSPropertyTop:
         offset = style.top();
+        opposite = style.bottom();
         break;
     case CSSPropertyBottom:
         offset = style.bottom();
+        opposite = style.top();
         break;
     default:
         return nullptr;
@@ -186,10 +190,65 @@ static PassRefPtrWillBeRawPtr<CSSValue> valueForPositionOffset(const ComputedSty
             toLayoutBox(layoutObject)->containingBlockLogicalHeightForGetComputedStyle();
         return zoomAdjustedPixelValue(valueForLength(offset, containingBlockSize), style);
     }
+
     if (offset.isAuto()) {
-        // FIXME: It's not enough to simply return "auto" values for one offset if the other side is defined.
-        // In other words if left is auto and right is not auto, then left's computed value is negative right().
-        // So we should get the opposite length unit and see if it is auto.
+        if (style.display() != NONE) {

[mstensho (USE GERRIT), 2016/03/29 13:13:26]

Can just check if you have a layoutObject instead of checking 'display'.

+            // If the property applies to a positioned element and the resolved value of the display
+            // property is not none, the resolved value is the used value.
+            if (layoutObject->isInFlowPositioned()) {
+                // If e.g. left is auto and right is not auto, then left's computed value is negative right().

[mstensho (USE GERRIT), 2016/03/29 13:13:26]

Should remove "()" from "right()".

+                // So we get the opposite length unit and see if it is auto.
+                if (opposite.isAuto()) {
+                    return cssValuePool().createValue(0, CSSPrimitiveValue::UnitType::Pixels);
+                }

[mstensho (USE GERRIT), 2016/03/29 13:13:26]

No need for curly braces here.

+                opposite *= -1.f;

[mstensho (USE GERRIT), 2016/03/29 13:13:26]

Why not "opposite = -opposite"?

Oh, right, Length doesn't provide operator-(). We could add it, but I wonder if
we can do without this negation, by just modifying the code below to eliminate
the need for it. Performing arithmetic on Length variables seems like a very
uncommon thing to do.

E.g. zoomAdjustedPixelValueForLength(opposite, style) could be written as
zoomAdjustedPixelValue(-opposite.pixels(), style)

+                if (opposite.hasPercent()) {
+                    LayoutUnit containingBlockSize =
+                        (propertyID == CSSPropertyLeft || propertyID == CSSPropertyRight) ?
+                        toLayoutBox(layoutObject)->containingBlockLogicalWidthForContent() :
+                        toLayoutBox(layoutObject)->containingBlockLogicalHeightForGetComputedStyle();
+                    return zoomAdjustedPixelValue(valueForLength(opposite, containingBlockSize), style);
+                }
+                return zoomAdjustedPixelValueForLength(opposite, style);

[mstensho (USE GERRIT), 2016/03/29 13:13:26]

Note: We don't take care of over-constrained situations, where both |left| and
|right| or both |top| and |bottom| are non-auto [1][2], but this has always been
missing in this function, so we can worry about that later, I suppose. Maybe
file a CL at some point that fixes this completely for the entire function.

[1] Relpos:
https://www.w3.org/TR/2011/REC-CSS2-20110607/visuren.html#relative-positioning
[2] Abspos:
https://www.w3.org/TR/2011/REC-CSS2-20110607/visudet.html#abs-non-replaced-width

+            }
+
+            if (layoutObject->isOutOfFlowPositioned()) {
+                // For fixed and absolute positioned elements, the top, left, bottom, and right
+                // are defined relative to the corresponding sides of the containing block.
+                LayoutBlock* container = layoutObject->containingBlock();
+
+                const LayoutBox* layoutBox = toLayoutBox(layoutObject);
+                // locationOffset is the distance from this object's border edge to
+                // the container's border edge (which is not always the parent).
+                // Thus it includes any logical top/left along with this box's
+                // margins.
+                const LayoutSize locationOffset = layoutBox->locationOffset();
+                LayoutUnit left = locationOffset.width() - container->borderLeft() - layoutBox->marginLeft();

[mstensho (USE GERRIT), 2016/03/29 13:13:26]

We subtract layoutBox->marginLeft() here, which is correct for case
CSSPropertyLeft, but if it's CSSPropertyRight, we have to compensate for that
again further below, which just seems convoluted.

I think the code would look cleaner if we didn't declare |left| and |top|, and
instead performed the necessary calculations for each of the 4 cases.

Might look nice if you defined locationOffset (or whatever you prefer to call it
instead) as layoutBox->locationOffset() - LayoutSize(container->clientLeft(),
container->clientTop()), maybe? (Yes, better use clientLeft() and clientTop()
instead of borderLeft() and borderTop() here, so that left-handed scrollbars get
handled correctly.)

+                LayoutUnit top = locationOffset.height() - container->borderTop() - layoutBox->marginTop();
+                LayoutUnit position;
+
+                switch (propertyID) {
+                case CSSPropertyLeft:
+                    position = left;
+                    break;
+                case CSSPropertyTop:
+                    position = top;
+                    break;
+                // clientWidth and clientHeight exclude the border width
+                case CSSPropertyRight:
+                    position = container->clientWidth() - layoutBox->offsetWidth() - left;
+                    position -= (layoutBox->marginRight() + layoutBox->marginLeft());
+                    break;
+                case CSSPropertyBottom:
+                    position = container->clientHeight() - layoutBox->offsetHeight() - top;
+                    position -= (layoutBox->marginBottom() + layoutBox->marginTop());
+                    break;
+                default:
+                    return nullptr;

[mstensho (USE GERRIT), 2016/03/29 13:13:26]

You have handled all the possible cases, so better just do an
ASSERT_NOT_REACHED() here.

+                }
+                return zoomAdjustedPixelValue(position, style);
+            }
+        }
         return cssValuePool().createIdentifierValue(CSSValueAuto);
     }