Changeset 278193 in webkit

Timestamp:

May 28, 2021 1:59:20 AM (14 months ago)

Author:

Martin Robinson

Message:

More readily layout when scroll-snap properties change
​https://bugs.webkit.org/show_bug.cgi?id=225950

Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

• web-platform-tests/css/css-scroll-snap/scroll-snap-type-change-expected.txt: Updated expectation

for newly passing test.

Source/WebCore:

Do a layout when scroll-snap properties change instead of trying
to update scroll positions without a layout.

Test: css3/scroll-snap/scroll-snap-style-change-crash.html

• rendering/RenderLayerModelObject.cpp:

(WebCore::RenderLayerModelObject::styleDidChange): Remove code dealing with scrollports
as this is now handled by the RenderStyle diff. Now trigger a layout for scrollports
when the scrolled children change their snap properties.

• rendering/RenderObject.cpp:

(WebCore::RenderObject::enclosingScrollableContainerForSnapping const): Made this return a non-const.

• rendering/RenderObject.h:
• rendering/style/RenderStyle.cpp:

(WebCore::RenderStyle::changeRequiresLayout const): Added code to handle scrollport specific
properties.

LayoutTests:

• css3/scroll-snap/scroll-snap-style-change-crash-expected.txt: Added.
• css3/scroll-snap/scroll-snap-style-change-crash.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/css3/scroll-snap/scroll-snap-style-change-crash-expected.txt (added)
• LayoutTests/css3/scroll-snap/scroll-snap-style-change-crash.html (added)
• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-snap-type-change-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderLayerModelObject.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderObject.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderObject.h (modified) (1 diff)
• Source/WebCore/rendering/style/RenderStyle.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-05-28  Martin Robinson  <mrobinson@webkit.org>
+
+        More readily layout when scroll-snap properties change
+        https://bugs.webkit.org/show_bug.cgi?id=225950
+
+        Reviewed by Simon Fraser.
+
+        * css3/scroll-snap/scroll-snap-style-change-crash-expected.txt: Added.
+        * css3/scroll-snap/scroll-snap-style-change-crash.html: Added.
+
 2021-05-27  Amir Mark Jr  <amir_mark@apple.com>
 

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2021-05-28  Martin Robinson  <mrobinson@webkit.org>
+
+        More readily layout when scroll-snap properties change
+        https://bugs.webkit.org/show_bug.cgi?id=225950
+
+        Reviewed by Simon Fraser.
+
+        * web-platform-tests/css/css-scroll-snap/scroll-snap-type-change-expected.txt: Updated expectation
+        for newly passing test.
+
 2021-05-27  Said Abou-Hallawa  <said@apple.com>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-snap-type-change-expected.txt

@@ -1 +1 @@
 
-FAIL scroll-snap-type on HTML should control snapping behavior and changing it takes effect assert_equals: scrolling should not snap expected 100 but got 200
+PASS scroll-snap-type on HTML should control snapping behavior and changing it takes effect
 PASS scroll-snap-type on DIV should control snapping behavior and changing it takes effect
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-05-28  Martin Robinson  <mrobinson@webkit.org>
+
+        More readily layout when scroll-snap properties change
+        https://bugs.webkit.org/show_bug.cgi?id=225950
+
+        Reviewed by Simon Fraser.
+
+        Do a layout when scroll-snap properties change instead of trying
+        to update scroll positions without a layout.
+
+        Test: css3/scroll-snap/scroll-snap-style-change-crash.html
+
+        * rendering/RenderLayerModelObject.cpp:
+        (WebCore::RenderLayerModelObject::styleDidChange): Remove code dealing with scrollports
+        as this is now handled by the RenderStyle diff. Now trigger a layout for scrollports
+        when the scrolled children change their snap properties.
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::enclosingScrollableContainerForSnapping const): Made this return a non-const.
+        * rendering/RenderObject.h:
+        * rendering/style/RenderStyle.cpp:
+        (WebCore::RenderStyle::changeRequiresLayout const): Added code to handle scrollport specific
+        properties.
+
 2021-05-27  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebCore/rendering/RenderLayerModelObject.cpp

@@ -108 +108 @@
 }
 
-#if ENABLE(CSS_SCROLL_SNAP)
-static bool scrollSnapContainerRequiresUpdateForStyleUpdate(const RenderStyle& oldStyle, const RenderStyle& newStyle)
-{
-    return oldStyle.scrollPadding() != newStyle.scrollPadding() || oldStyle.scrollSnapType() != newStyle.scrollSnapType();
-}
-#endif
-
 void RenderLayerModelObject::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
 {
@@ -172 +165 @@
 
 #if ENABLE(CSS_SCROLL_SNAP)
-    if (oldStyle && scrollSnapContainerRequiresUpdateForStyleUpdate(*oldStyle, newStyle)) {
-        if (RenderLayer* renderLayer = layer()) {
-            if (auto* scrollableArea = renderLayer->scrollableArea()) {
-                scrollableArea->updateSnapOffsets();
-                scrollableArea->updateScrollSnapState();
-            }
-        } else if (isBody() || isDocumentElementRenderer()) {
-            FrameView& frameView = view().frameView();
-            frameView.updateSnapOffsets();
-            frameView.updateScrollSnapState();
-            frameView.updateScrollingCoordinatorScrollSnapProperties();
-        }
-    }
-
     bool scrollMarginChanged = oldStyle && oldStyle->scrollMargin() != newStyle.scrollMargin();
     bool scrollAlignChanged = oldStyle && oldStyle->scrollSnapAlign() != newStyle.scrollSnapAlign();
     bool scrollSnapStopChanged = oldStyle && oldStyle->scrollSnapStop() != newStyle.scrollSnapStop();
     if (scrollMarginChanged || scrollAlignChanged || scrollSnapStopChanged) {
-        auto* scrollSnapBox = enclosingScrollableContainerForSnapping();
-        if (scrollSnapBox && scrollSnapBox->layer()) {
-            const RenderStyle& style = scrollSnapBox->style();
-            if (style.scrollSnapType().strictness != ScrollSnapStrictness::None) {
-                if (auto* scrollableArea = scrollSnapBox->layer()->scrollableArea()) {
-                    scrollableArea->updateSnapOffsets();
-                    scrollableArea->updateScrollSnapState();
-                }
-                if (scrollSnapBox->isBody() || scrollSnapBox->isDocumentElementRenderer())
-                    scrollSnapBox->view().frameView().updateSnapOffsets();
-                    scrollSnapBox->view().frameView().updateScrollSnapState();
-                    scrollSnapBox->view().frameView().updateScrollingCoordinatorScrollSnapProperties();
-            }
-        }
+        if (auto* scrollSnapBox = enclosingScrollableContainerForSnapping())
+            scrollSnapBox->setNeedsLayout();
     }
 #endif

trunk/Source/WebCore/rendering/RenderObject.cpp

@@ -470 +470 @@
 }
 
-const RenderBox* RenderObject::enclosingScrollableContainerForSnapping() const
+RenderBox* RenderObject::enclosingScrollableContainerForSnapping() const
 {
     // Walk up the container chain to find the scrollable container that contains

trunk/Source/WebCore/rendering/RenderObject.h

@@ -156 +156 @@
     WEBCORE_EXPORT RenderBox& enclosingBox() const;
     RenderBoxModelObject& enclosingBoxModelObject() const;
-    const RenderBox* enclosingScrollableContainerForSnapping() const;
+    RenderBox* enclosingScrollableContainerForSnapping() const;
 
     // Return our enclosing flow thread if we are contained inside one. Follows the containing block chain.

trunk/Source/WebCore/rendering/style/RenderStyle.cpp

@@ -952 +952 @@
             return true;
     }
+
+#if ENABLE(CSS_SCROLL_SNAP)
+    if (scrollPadding() != other.scrollPadding() || scrollSnapType() != other.scrollSnapType())
+        return true;
+#endif
 
     return false;