Attachment 357875 Details for Bug 190492 – Apply suggested changes

[patch] Apply suggested changes

bug-190492-20181221000058.patch (text/plain), 16.35 KB, created by Manuel Rego Casasnovas on 2018-12-20 15:00:59 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Manuel Rego Casasnovas

Created: 2018-12-20 15:00:59 PST

Size: 16.35 KB

patch

obsolete

>Subversion Revision: 239465
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 6c10bac620ed624c6f2f1c07551afd5f72f42b6b..e42ce59a4df41e36219a5247f9e8398dddf7ab61 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,32 @@
>+2018-10-11  Manuel Rego Casasnovas  <rego@igalia.com>
>+
>+        [css-grid] Fix percentages in relative offsets for grid items
>+        https://bugs.webkit.org/show_bug.cgi?id=190492
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The method RenderBoxModelObject::relativePositionOffset() was not considering the case of grid items,
>+        where the containing block is the grid area.
>+        The patch modifies the method so the new code uses overrideContainingBlockContentWidth|Height when required.
>+
>+        Test: imported/w3c/web-platform-tests/css/css-grid/grid-items/grid-items-relative-offsets-002.html
>+
>+        * rendering/RenderBox.cpp: Implement the physical versions of the already existent methods.
>+        (WebCore::RenderBox::overrideContainingBlockContentWidth const):
>+        (WebCore::RenderBox::overrideContainingBlockContentHeight const):
>+        (WebCore::RenderBox::hasOverrideContainingBlockContentWidth const):
>+        (WebCore::RenderBox::hasOverrideContainingBlockContentHeight const):
>+        * rendering/RenderBox.h:
>+        * rendering/RenderBoxModelObject.cpp:
>+        (WebCore::RenderBoxModelObject::relativePositionOffset const): Modified method
>+        to take into account overrideContainingBlockContentWidth|Height for grid items.
>+        * rendering/RenderBoxModelObject.h: Added new headers for physical virtual methods
>+        that will be overridden in RenderBox.
>+        (WebCore::RenderBoxModelObject::overrideContainingBlockContentWidth const):
>+        (WebCore::RenderBoxModelObject::overrideContainingBlockContentHeight const):
>+        (WebCore::RenderBoxModelObject::hasOverrideContainingBlockContentWidth const):
>+        (WebCore::RenderBoxModelObject::hasOverrideContainingBlockContentHeight const):
>+
> 2018-12-20  Chris Dumez  <cdumez@apple.com>
> 
>         Use Optional::valueOr() instead of Optional::value_or()
>diff --git a/Source/WebCore/rendering/RenderBox.cpp b/Source/WebCore/rendering/RenderBox.cpp
>index a671111659e521562fef74db45743ef9bbfddce9..06aebaf65d5df19b7b02df2aef6124e6a64f442c 100644
>--- a/Source/WebCore/rendering/RenderBox.cpp
>+++ b/Source/WebCore/rendering/RenderBox.cpp
>@@ -98,6 +98,7 @@ static OverrideSizeMap* gOverrideContentLogicalHeightMap = nullptr;
> static OverrideSizeMap* gOverrideContentLogicalWidthMap = nullptr;
> 
> // Used by grid elements to properly size their grid items.
>+// FIXME: We should store these based on physical direction.
> typedef WTF::HashMap<const RenderBox*, Optional<LayoutUnit>> OverrideOptionalSizeMap;
> static OverrideOptionalSizeMap* gOverrideContainingBlockContentLogicalHeightMap = nullptr;
> static OverrideOptionalSizeMap* gOverrideContainingBlockContentLogicalWidthMap = nullptr;
>@@ -1086,6 +1087,44 @@ LayoutUnit RenderBox::overrideContentLogicalHeight() const
>     return gOverrideContentLogicalHeightMap->get(this);
> }
> 
>+Optional<LayoutUnit> RenderBox::overrideContainingBlockContentWidth() const
>+{
>+    ASSERT(hasOverrideContainingBlockContentWidth());
>+    return containingBlock()->style().isHorizontalWritingMode()
>+        ? gOverrideContainingBlockContentLogicalWidthMap->get(this)
>+        : gOverrideContainingBlockContentLogicalHeightMap->get(this);
>+}
>+
>+Optional<LayoutUnit> RenderBox::overrideContainingBlockContentHeight() const
>+{
>+    ASSERT(hasOverrideContainingBlockContentHeight());
>+    return containingBlock()->style().isHorizontalWritingMode()
>+        ? gOverrideContainingBlockContentLogicalHeightMap->get(this)
>+        : gOverrideContainingBlockContentLogicalWidthMap->get(this);
>+}
>+
>+bool RenderBox::hasOverrideContainingBlockContentWidth() const
>+{
>+    RenderBlock* cb = containingBlock();
>+    if (!cb)
>+        return false;
>+
>+    return cb->style().isHorizontalWritingMode()
>+        ? gOverrideContainingBlockContentLogicalWidthMap && gOverrideContainingBlockContentLogicalWidthMap->contains(this)
>+        : gOverrideContainingBlockContentLogicalHeightMap && gOverrideContainingBlockContentLogicalHeightMap->contains(this);
>+}
>+
>+bool RenderBox::hasOverrideContainingBlockContentHeight() const
>+{
>+    RenderBlock* cb = containingBlock();
>+    if (!cb)
>+        return false;
>+
>+    return cb->style().isHorizontalWritingMode()
>+        ? gOverrideContainingBlockContentLogicalHeightMap && gOverrideContainingBlockContentLogicalHeightMap->contains(this)
>+        : gOverrideContainingBlockContentLogicalHeightMap && gOverrideContainingBlockContentLogicalHeightMap->contains(this);
>+}
>+
> Optional<LayoutUnit> RenderBox::overrideContainingBlockContentLogicalWidth() const
> {
>     ASSERT(hasOverrideContainingBlockContentLogicalWidth());
>diff --git a/Source/WebCore/rendering/RenderBox.h b/Source/WebCore/rendering/RenderBox.h
>index 88d91714161eac1546cd235c9637c76d427838ce..5510685294294abf8f5d6eb2a0cac2106f6a6c12 100644
>--- a/Source/WebCore/rendering/RenderBox.h
>+++ b/Source/WebCore/rendering/RenderBox.h
>@@ -314,6 +314,10 @@ public:
>     void clearOverrideContentLogicalHeight();
>     void clearOverrideContentLogicalWidth();
> 
>+    Optional<LayoutUnit> overrideContainingBlockContentWidth() const override;
>+    Optional<LayoutUnit> overrideContainingBlockContentHeight() const override;
>+    bool hasOverrideContainingBlockContentWidth() const override;
>+    bool hasOverrideContainingBlockContentHeight() const override;
>     Optional<LayoutUnit> overrideContainingBlockContentLogicalWidth() const;
>     Optional<LayoutUnit> overrideContainingBlockContentLogicalHeight() const;
>     bool hasOverrideContainingBlockContentLogicalWidth() const;
>diff --git a/Source/WebCore/rendering/RenderBoxModelObject.cpp b/Source/WebCore/rendering/RenderBoxModelObject.cpp
>index 96052ea1b3aaf790db3c9d3d3d49b0479b789ce0..fdc511c13cb415b356a472788d06e88556398925 100644
>--- a/Source/WebCore/rendering/RenderBoxModelObject.cpp
>+++ b/Source/WebCore/rendering/RenderBoxModelObject.cpp
>@@ -401,13 +401,18 @@ LayoutSize RenderBoxModelObject::relativePositionOffset() const
>     // in the case of relative positioning using percentages, we can't do this.  The offset should always be resolved using the
>     // available width of the containing block.  Therefore we don't use containingBlockLogicalWidthForContent() here, but instead explicitly
>     // call availableWidth on our containing block.
>-    if (!style().left().isAuto()) {
>-        if (!style().right().isAuto() && !containingBlock()->style().isLeftToRightDirection())
>-            offset.setWidth(-valueForLength(style().right(), !style().right().isFixed() ? containingBlock()->availableWidth() : 0_lu));
>-        else
>-            offset.expand(valueForLength(style().left(), !style().left().isFixed() ? containingBlock()->availableWidth() : 0_lu), 0_lu);
>-    } else if (!style().right().isAuto()) {
>-        offset.expand(-valueForLength(style().right(), !style().right().isFixed() ? containingBlock()->availableWidth() : 0_lu), 0_lu);
>+    // However for grid items the containing block is the grid area, so offsets should be resolved against that:
>+    // https://drafts.csswg.org/css-grid/#grid-item-sizing
>+    if (!style().left().isAuto() || !style().right().isAuto()) {
>+        LayoutUnit availableWidth = hasOverrideContainingBlockContentWidth()
>+            ? overrideContainingBlockContentWidth().valueOr(LayoutUnit()) : containingBlock()->availableWidth();
>+        if (!style().left().isAuto()) {
>+            if (!style().right().isAuto() && !containingBlock()->style().isLeftToRightDirection())
>+                offset.setWidth(-valueForLength(style().right(), !style().right().isFixed() ? availableWidth : 0_lu));
>+            else
>+                offset.expand(valueForLength(style().left(), !style().left().isFixed() ? availableWidth : 0_lu), 0_lu);
>+        } else if (!style().right().isAuto())
>+            offset.expand(-valueForLength(style().right(), !style().right().isFixed() ? availableWidth : 0_lu), 0_lu);
>     }
> 
>     // If the containing block of a relatively positioned element does not
>@@ -416,17 +421,29 @@ LayoutSize RenderBoxModelObject::relativePositionOffset() const
>     // where <html> and <body> assume the size of the viewport. In this case,
>     // calculate the percent offset based on this height.
>     // See <https://bugs.webkit.org/show_bug.cgi?id=26396>.
>+    // Another exception is a grid item, as the containing block is the grid area:
>+    // https://drafts.csswg.org/css-grid/#grid-item-sizing
>     if (!style().top().isAuto()
>         && (!style().top().isPercentOrCalculated()
>             || !containingBlock()->hasAutoHeightOrContainingBlockWithAutoHeight()
>-            || containingBlock()->stretchesToViewport()))
>-        offset.expand(0_lu, valueForLength(style().top(), !style().top().isFixed() ? containingBlock()->availableHeight() : 0_lu));
>-
>-    else if (!style().bottom().isAuto()
>+            || containingBlock()->stretchesToViewport()
>+            || hasOverrideContainingBlockContentHeight())) {
>+        // FIXME: The computation of the available height is repeated later for "bottom".
>+        // We could refactor this and move it to some common code for both ifs, however moving it outside of the ifs
>+        // is not possible as it'd cause performance regressions.
>+        offset.expand(0_lu, valueForLength(style().top(), !style().top().isFixed()
>+            ? (hasOverrideContainingBlockContentHeight() ? overrideContainingBlockContentHeight().valueOr(0_lu) : containingBlock()->availableHeight())
>+            : LayoutUnit()));
>+    } else if (!style().bottom().isAuto()
>         && (!style().bottom().isPercentOrCalculated()
>             || !containingBlock()->hasAutoHeightOrContainingBlockWithAutoHeight()
>-            || containingBlock()->stretchesToViewport()))
>-        offset.expand(0_lu, -valueForLength(style().bottom(), !style().bottom().isFixed() ? containingBlock()->availableHeight() : 0_lu));
>+            || containingBlock()->stretchesToViewport()
>+            || hasOverrideContainingBlockContentHeight())) {
>+        // FIXME: Check comment above for "top", it applies here too.
>+        offset.expand(0_lu, -valueForLength(style().bottom(), !style().bottom().isFixed()
>+            ? (hasOverrideContainingBlockContentHeight() ? overrideContainingBlockContentHeight().valueOr(0_lu) : containingBlock()->availableHeight())
>+            : LayoutUnit()));
>+    }
> 
>     return offset;
> }
>diff --git a/Source/WebCore/rendering/RenderBoxModelObject.h b/Source/WebCore/rendering/RenderBoxModelObject.h
>index 94b69a1c5af42be4f06d13a80338a91e27757cb2..86f4b8003b1e85b08311277dd4381ee9d7711a7f 100644
>--- a/Source/WebCore/rendering/RenderBoxModelObject.h
>+++ b/Source/WebCore/rendering/RenderBoxModelObject.h
>@@ -243,7 +243,12 @@ public:
>     virtual LayoutRect paintRectToClipOutFromBorder(const LayoutRect&) { return LayoutRect(); };
> 
>     bool hasRunningAcceleratedAnimations() const;
>-    
>+
>+    virtual Optional<LayoutUnit> overrideContainingBlockContentWidth() const { ASSERT_NOT_REACHED(); return -1_lu; }
>+    virtual Optional<LayoutUnit> overrideContainingBlockContentHeight() const { ASSERT_NOT_REACHED(); return -1_lu; }
>+    virtual bool hasOverrideContainingBlockContentWidth() const { return false; }
>+    virtual bool hasOverrideContainingBlockContentHeight() const { return false; }
>+
> protected:
>     RenderBoxModelObject(Element&, RenderStyle&&, BaseTypeFlags);
>     RenderBoxModelObject(Document&, RenderStyle&&, BaseTypeFlags);
>diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
>index 995fecbfa2791d02f4c9a7e4f212466abb6a0241..1e6f90224d7fd862ba8b81755bbdff27796173ef 100644
>--- a/LayoutTests/imported/w3c/ChangeLog
>+++ b/LayoutTests/imported/w3c/ChangeLog
>@@ -1,3 +1,13 @@
>+2018-10-11  Manuel Rego Casasnovas  <rego@igalia.com>
>+
>+        [css-grid] Fix percentages in relative offsets for grid items
>+        https://bugs.webkit.org/show_bug.cgi?id=190492
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * web-platform-tests/css/css-grid/grid-items/grid-items-relative-offsets-002-expected.txt:
>+          Update expected file as we're now passing this test.
>+
> 2018-12-19  Youenn Fablet  <youenn@apple.com>
> 
>         Remove RTCRtpTransceiver.setDirection
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-items/grid-items-relative-offsets-002-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-items/grid-items-relative-offsets-002-expected.txt
>index d4825b55b29e236e7af3ab9f5e8fe626a29866e1..368e0b76b85e84e67e56ea73cf50c87abc4fdc89 100644
>--- a/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-items/grid-items-relative-offsets-002-expected.txt
>+++ b/LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-items/grid-items-relative-offsets-002-expected.txt
>@@ -1,46 +1,10 @@
> 
>-FAIL .grid 1 assert_equals: 
>-<div class="grid verticalRL directionRTL">
>-  <div class="firstRowFirstColumn" style="left: 10%; top: 5%;" data-offset-x="219" data-offest-y="10" data-expected-width="90" data-expected-height="200"></div>
>-  <div class="secondRowSecondColumn" style="left: -20%; top: -10%;" data-offset-x="138" data-offest-y="75" data-expected-width="60" data-expected-height="150"></div>
>-  <div class="thirdRowThirdColumn" style="right: 70%; bottom: 30%;" data-offset-x="99" data-offest-y="120" data-expected-width="30" data-expected-height="100"></div>
>-</div>
>-offsetLeft expected 219 but got 240
>-FAIL .grid 2 assert_equals: 
>-<div class="grid verticalRL">
>-  <div class="firstRowFirstColumn" style="left: 10%; top: 5%;" data-offset-x="219" data-offest-y="10" data-expected-width="90" data-expected-height="200"></div>
>-  <div class="secondRowSecondColumn" style="left: -20%; top: -10%;" data-offset-x="138" data-offest-y="75" data-expected-width="60" data-expected-height="150"></div>
>-  <div class="thirdRowThirdColumn" style="right: 70%; bottom: 30%;" data-offset-x="99" data-offest-y="120" data-expected-width="30" data-expected-height="100"></div>
>-</div>
>-offsetLeft expected 219 but got 240
>-FAIL .grid 3 assert_equals: 
>-<div class="grid verticalLR directionRTL">
>-  <div class="firstRowFirstColumn" style="left: 10%; top: 5%;" data-offset-x="9" data-offest-y="10" data-expected-width="90" data-expected-height="200"></div>
>-  <div class="secondRowSecondColumn" style="left: -20%; top: -10%;" data-offset-x="78" data-offest-y="75" data-expected-width="60" data-expected-height="150"></div>
>-  <div class="thirdRowThirdColumn" style="right: 70%; bottom: 30%;" data-offset-x="129" data-offest-y="120" data-expected-width="30" data-expected-height="100"></div>
>-</div>
>-offsetLeft expected 9 but got 30
>-FAIL .grid 4 assert_equals: 
>-<div class="grid verticalLR">
>-  <div class="firstRowFirstColumn" style="left: 10%; top: 5%;" data-offset-x="9" data-offest-y="10" data-expected-width="90" data-expected-height="200"></div>
>-  <div class="secondRowSecondColumn" style="left: -20%; top: -10%;" data-offset-x="78" data-offest-y="75" data-expected-width="60" data-expected-height="150"></div>
>-  <div class="thirdRowThirdColumn" style="right: 70%; bottom: 30%;" data-offset-x="129" data-offest-y="120" data-expected-width="30" data-expected-height="100"></div>
>-</div>
>-offsetLeft expected 9 but got 30
>-FAIL .grid 5 assert_equals: 
>-<div class="grid directionRTL">
>-  <div class="firstRowFirstColumn" style="left: 5%; top: 10%;" data-offset-x="410" data-offest-y="9" data-expected-width="200" data-expected-height="90"></div>
>-  <div class="secondRowSecondColumn" style="left: -10%; top: -20%;" data-offset-x="235" data-offest-y="78" data-expected-width="150" data-expected-height="60"></div>
>-  <div class="thirdRowThirdColumn" style="right: 30%; bottom: 70%;" data-offset-x="120" data-offest-y="129" data-expected-width="100" data-expected-height="30"></div>
>-</div>
>-offsetLeft expected 410 but got 430
>-FAIL .grid 6 assert_equals: 
>-<div class="grid">
>-  <div class="firstRowFirstColumn" style="left: 5%; top: 10%;" data-offset-x="10" data-offest-y="9" data-expected-width="200" data-expected-height="90"></div>
>-  <div class="secondRowSecondColumn" style="left: -10%; top: -20%;" data-offset-x="185" data-offest-y="78" data-expected-width="150" data-expected-height="60"></div>
>-  <div class="thirdRowThirdColumn" style="right: 30%; bottom: 70%;" data-offset-x="320" data-offest-y="129" data-expected-width="100" data-expected-height="30"></div>
>-</div>
>-offsetLeft expected 10 but got 30
>+PASS .grid 1 
>+PASS .grid 2 
>+PASS .grid 3 
>+PASS .grid 4 
>+PASS .grid 5 
>+PASS .grid 6 
> Direction LTR
> 
> Direction RTL

Actions: View | Formatted Diff | Diff

Attachments on bug 190492: 352088 | 357871 | 357874 | 357875