Attachment 357871 Details for Bug 190492 – Rebased patch

[patch] Rebased patch

bug-190492-20181220232424.patch (text/plain), 16.41 KB, created by Manuel Rego Casasnovas on 2018-12-20 14:24:25 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Manuel Rego Casasnovas

Created: 2018-12-20 14:24:25 PST

Size: 16.41 KB

patch

obsolete

>Subversion Revision: 238464
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 70faf9aae5920c9aa9dac999c6c6a53aa65150ed..e82fb881cbd0abc0f7797453f5b897fd5bae56a7 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-11-22  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         Updating href on textPath doesn't update its rendering
>diff --git a/Source/WebCore/rendering/RenderBox.cpp b/Source/WebCore/rendering/RenderBox.cpp
>index bf88bd50e9a17e3d7a56dbe838d2b5043df08230..09ac6d9ee9c60d8ad820349ac251b80f81796d61 100644
>--- a/Source/WebCore/rendering/RenderBox.cpp
>+++ b/Source/WebCore/rendering/RenderBox.cpp
>@@ -99,6 +99,7 @@ static OverrideSizeMap* gOverrideContentLogicalWidthMap = nullptr;
> 
> // Used by grid elements to properly size their grid items.
> typedef WTF::HashMap<const RenderBox*, std::optional<LayoutUnit>> OverrideOptionalSizeMap;
>+// FIXME: We should store these based on physical direction.
> static OverrideOptionalSizeMap* gOverrideContainingBlockContentLogicalHeightMap = nullptr;
> static OverrideOptionalSizeMap* gOverrideContainingBlockContentLogicalWidthMap = nullptr;
> 
>@@ -1086,6 +1087,44 @@ LayoutUnit RenderBox::overrideContentLogicalHeight() const
>     return gOverrideContentLogicalHeightMap->get(this);
> }
> 
>+std::optional<LayoutUnit> RenderBox::overrideContainingBlockContentWidth() const
>+{
>+    ASSERT(hasOverrideContainingBlockContentWidth());
>+    return containingBlock()->style().isHorizontalWritingMode()
>+        ? gOverrideContainingBlockContentLogicalWidthMap->get(this)
>+        : gOverrideContainingBlockContentLogicalHeightMap->get(this);
>+}
>+
>+std::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);
>+}
>+
> std::optional<LayoutUnit> RenderBox::overrideContainingBlockContentLogicalWidth() const
> {
>     ASSERT(hasOverrideContainingBlockContentLogicalWidth());
>diff --git a/Source/WebCore/rendering/RenderBox.h b/Source/WebCore/rendering/RenderBox.h
>index db1094d69e57333f5bab39490ad2ab0da861bcc8..e2e45dd82d58c6f7699d306ad36276a3440e62e8 100644
>--- a/Source/WebCore/rendering/RenderBox.h
>+++ b/Source/WebCore/rendering/RenderBox.h
>@@ -314,6 +314,10 @@ public:
>     void clearOverrideContentLogicalHeight();
>     void clearOverrideContentLogicalWidth();
> 
>+    std::optional<LayoutUnit> overrideContainingBlockContentWidth() const override;
>+    std::optional<LayoutUnit> overrideContainingBlockContentHeight() const override;
>+    bool hasOverrideContainingBlockContentWidth() const override;
>+    bool hasOverrideContainingBlockContentHeight() const override;
>     std::optional<LayoutUnit> overrideContainingBlockContentLogicalWidth() const;
>     std::optional<LayoutUnit> overrideContainingBlockContentLogicalHeight() const;
>     bool hasOverrideContainingBlockContentLogicalWidth() const;
>diff --git a/Source/WebCore/rendering/RenderBoxModelObject.cpp b/Source/WebCore/rendering/RenderBoxModelObject.cpp
>index 96052ea1b3aaf790db3c9d3d3d49b0479b789ce0..f5dc39e7506d3ea9dff4e1d8a97cebf90bce1e0a 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().value_or(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,30 @@ 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
>+    bool hasOverrideContainingBlockContent = hasOverrideContainingBlockContentHeight();
>     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()
>+            || hasOverrideContainingBlockContent)) {
>+        // 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()
>+            ? (hasOverrideContainingBlockContent ? overrideContainingBlockContentHeight().value_or(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()
>+            || hasOverrideContainingBlockContent)) {
>+        // FIXME: Check comment above for "top", it applies here too.
>+        offset.expand(0_lu, -valueForLength(style().bottom(), !style().bottom().isFixed()
>+            ? (hasOverrideContainingBlockContent ? overrideContainingBlockContentHeight().value_or(0_lu) : containingBlock()->availableHeight())
>+            : LayoutUnit()));
>+    }
> 
>     return offset;
> }
>diff --git a/Source/WebCore/rendering/RenderBoxModelObject.h b/Source/WebCore/rendering/RenderBoxModelObject.h
>index 94b69a1c5af42be4f06d13a80338a91e27757cb2..903f40b00d0d8f33455e01233f3e426dade07d39 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 std::optional<LayoutUnit> overrideContainingBlockContentWidth() const { ASSERT_NOT_REACHED(); return -1_lu; }
>+    virtual std::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 7705daf691b429897a8c9c6f420426e9530099d2..650193d3b4d07d1d206bc6d013c9d23714deb388 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-11-23  Javier Fernandez  <jfernandez@igalia.com>
> 
>         [css-grid] Implement Baseline Alignment for grid items
>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