Changeset 273073 in webkit


Ignore:
Timestamp:
Feb 18, 2021 4:02:03 AM (3 years ago)
Author:
commit-queue@webkit.org
Message:

LayoutTests/imported/w3c:
Root scroll snapping broken overflow:scroll is set on the <body>
​https://bugs.webkit.org/show_bug.cgi?id=221407

Patch by Martin Robinson <​mrobinson@igalia.com> on 2021-02-18
Reviewed by Simon Fraser.

  • web-platform-tests/css/css-scroll-snap/scrollTo-scrollBy-snaps-expected.txt:

Source/WebCore:
Root scroll snapping broken overflow:scroll is set on the <body>
​https://bugs.webkit.org/show_bug.cgi?id=221407

Patch by Martin Robinson <​mrobinson@igalia.com> on 2021-02-18
Reviewed by Simon Fraser.

No new tests. This change fixes the WPT test:
imported/w3c/web-platform-tests/css/css-scroll-snap/scrollTo-scrollBy-snaps.html

Instead of walking up the render tree to find the containing element, be sure to
walk up the containing block chain. This will prevent WebKit from using the wrong
scrolling container for positioned elements that snap.

  • rendering/RenderBox.cpp:

(WebCore::RenderBox::findEnclosingScrollableContainerForSnapping const): Deleted.

  • rendering/RenderBox.h: Remove method definition.
  • rendering/RenderObject.cpp:

(WebCore::RenderObject::enclosingScrollableContainerForSnapping const): Find the
scrolling container respecting the containing block chain. Never consider the
RenderView as a containing block. Although this element has a scroll area
and looks scrollable, it's a historical artifact and the real scroll container
is the FrameView.

LayoutTests:
Root scroll snapping broken when overflow:scroll is set on the <body>
​https://bugs.webkit.org/show_bug.cgi?id=221407

Patch by Martin Robinson <​mrobinson@igalia.com> on 2021-02-18
Reviewed by Simon Fraser.

  • platform/ios-wk2/imported/w3c/web-platform-tests/css/css-scroll-snap/scrollTo-scrollBy-snaps-expected.txt: Removed.

This baseline no longer differs between platforms.

Location:
trunk
Files:
1 deleted
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r273072 r273073  
     12021-02-18  Martin Robinson  <mrobinson@igalia.com>
     2
     3        Root scroll snapping broken when overflow:scroll is set on the <body>
     4        https://bugs.webkit.org/show_bug.cgi?id=221407
     5
     6        Reviewed by Simon Fraser.
     7
     8        * platform/ios-wk2/imported/w3c/web-platform-tests/css/css-scroll-snap/scrollTo-scrollBy-snaps-expected.txt: Removed.
     9        This baseline no longer differs between platforms.
     10
    1112021-02-15  Sergio Villar Senin  <svillar@igalia.com>
    212

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r273072 r273073  
     12021-02-18  Martin Robinson  <mrobinson@igalia.com>
     2
     3        Root scroll snapping broken overflow:scroll is set on the <body>
     4        https://bugs.webkit.org/show_bug.cgi?id=221407
     5
     6        Reviewed by Simon Fraser.
     7
     8        * web-platform-tests/css/css-scroll-snap/scrollTo-scrollBy-snaps-expected.txt:
     9
    1102021-02-15  Sergio Villar Senin  <svillar@igalia.com>
    211

  • trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/scrollTo-scrollBy-snaps-expected.txt

    r271439 r273073  
    11
    22PASS assign scrollLeft and scrollTop for {left: 800} on div lands on (1000, 0)
    3 FAIL assign scrollLeft and scrollTop for {left: 800} on viewport-defining element lands on (1000, 0) assert_equals: expected 1000 but got 800
     3PASS assign scrollLeft and scrollTop for {left: 800} on viewport-defining element lands on (1000, 0)
    44PASS scrollTo({left: 800}) on div lands on (1000, 0)
    55PASS scrollBy({left: 800}) on div lands on (1000, 0)
    6 FAIL scrollTo({left: 800}) on viewport-defining element lands on (1000, 0) assert_equals: expected 1000 but got 800
    7 FAIL scrollBy({left: 800}) on viewport-defining element lands on (1000, 0) assert_equals: expected 1000 but got 800
    8 FAIL scrollTo({left: 800}) on window lands on (1000, 0) assert_equals: expected 1000 but got 800
    9 FAIL scrollBy({left: 800}) on window lands on (1000, 0) assert_equals: expected 1000 but got 800
     6PASS scrollTo({left: 800}) on viewport-defining element lands on (1000, 0)
     7PASS scrollBy({left: 800}) on viewport-defining element lands on (1000, 0)
     8PASS scrollTo({left: 800}) on window lands on (1000, 0)
     9PASS scrollBy({left: 800}) on window lands on (1000, 0)
    1010PASS assign scrollLeft and scrollTop for {top: 900} on div lands on (0, 1000)
    11 FAIL assign scrollLeft and scrollTop for {top: 900} on viewport-defining element lands on (0, 1000) assert_equals: expected 1000 but got 900
     11PASS assign scrollLeft and scrollTop for {top: 900} on viewport-defining element lands on (0, 1000)
    1212PASS scrollTo({top: 900}) on div lands on (0, 1000)
    1313PASS scrollBy({top: 900}) on div lands on (0, 1000)
    14 FAIL scrollTo({top: 900}) on viewport-defining element lands on (0, 1000) assert_equals: expected 1000 but got 900
    15 FAIL scrollBy({top: 900}) on viewport-defining element lands on (0, 1000) assert_equals: expected 1000 but got 900
    16 FAIL scrollTo({top: 900}) on window lands on (0, 1000) assert_equals: expected 1000 but got 900
    17 FAIL scrollBy({top: 900}) on window lands on (0, 1000) assert_equals: expected 1000 but got 900
     14PASS scrollTo({top: 900}) on viewport-defining element lands on (0, 1000)
     15PASS scrollBy({top: 900}) on viewport-defining element lands on (0, 1000)
     16PASS scrollTo({top: 900}) on window lands on (0, 1000)
     17PASS scrollBy({top: 900}) on window lands on (0, 1000)
    1818PASS assign scrollLeft and scrollTop for {left: 900, top: 800} on div lands on (1000, 1000)
    19 FAIL assign scrollLeft and scrollTop for {left: 900, top: 800} on viewport-defining element lands on (1000, 1000) assert_equals: expected 1000 but got 900
     19PASS assign scrollLeft and scrollTop for {left: 900, top: 800} on viewport-defining element lands on (1000, 1000)
    2020PASS scrollTo({left: 900, top: 800}) on div lands on (1000, 1000)
    2121PASS scrollBy({left: 900, top: 800}) on div lands on (1000, 1000)
    22 FAIL scrollTo({left: 900, top: 800}) on viewport-defining element lands on (1000, 1000) assert_equals: expected 1000 but got 900
    23 FAIL scrollBy({left: 900, top: 800}) on viewport-defining element lands on (1000, 1000) assert_equals: expected 1000 but got 900
    24 FAIL scrollTo({left: 900, top: 800}) on window lands on (1000, 1000) assert_equals: expected 1000 but got 900
    25 FAIL scrollBy({left: 900, top: 800}) on window lands on (1000, 1000) assert_equals: expected 1000 but got 900
     22PASS scrollTo({left: 900, top: 800}) on viewport-defining element lands on (1000, 1000)
     23PASS scrollBy({left: 900, top: 800}) on viewport-defining element lands on (1000, 1000)
     24PASS scrollTo({left: 900, top: 800}) on window lands on (1000, 1000)
     25PASS scrollBy({left: 900, top: 800}) on window lands on (1000, 1000)
    2626PASS assign scrollLeft and scrollTop for {left: 800, top: -100} on div lands on (1000, 0)
    27 FAIL assign scrollLeft and scrollTop for {left: 800, top: -100} on viewport-defining element lands on (1000, 0) assert_equals: expected 1000 but got 800
     27PASS assign scrollLeft and scrollTop for {left: 800, top: -100} on viewport-defining element lands on (1000, 0)
    2828PASS scrollTo({left: 800, top: -100}) on div lands on (1000, 0)
    2929PASS scrollBy({left: 800, top: -100}) on div lands on (1000, 0)
    30 FAIL scrollTo({left: 800, top: -100}) on viewport-defining element lands on (1000, 0) assert_equals: expected 1000 but got 800
    31 FAIL scrollBy({left: 800, top: -100}) on viewport-defining element lands on (1000, 0) assert_equals: expected 1000 but got 800
    32 FAIL scrollTo({left: 800, top: -100}) on window lands on (1000, 0) assert_equals: expected 1000 but got 800
    33 FAIL scrollBy({left: 800, top: -100}) on window lands on (1000, 0) assert_equals: expected 1000 but got 800
     30PASS scrollTo({left: 800, top: -100}) on viewport-defining element lands on (1000, 0)
     31PASS scrollBy({left: 800, top: -100}) on viewport-defining element lands on (1000, 0)
     32PASS scrollTo({left: 800, top: -100}) on window lands on (1000, 0)
     33PASS scrollBy({left: 800, top: -100}) on window lands on (1000, 0)
    3434PASS assign scrollLeft and scrollTop for {left: 10000, top: -100} on div lands on (1000, 0)
    35 FAIL assign scrollLeft and scrollTop for {left: 10000, top: -100} on viewport-defining element lands on (1000, 0) assert_equals: expected 1000 but got 3215
     35PASS assign scrollLeft and scrollTop for {left: 10000, top: -100} on viewport-defining element lands on (1000, 0)
    3636PASS scrollTo({left: 10000, top: -100}) on div lands on (1000, 0)
    3737PASS scrollBy({left: 10000, top: -100}) on div lands on (1000, 0)
    38 FAIL scrollTo({left: 10000, top: -100}) on viewport-defining element lands on (1000, 0) assert_equals: expected 1000 but got 3215
    39 FAIL scrollBy({left: 10000, top: -100}) on viewport-defining element lands on (1000, 0) assert_equals: expected 1000 but got 3215
    40 FAIL scrollTo({left: 10000, top: -100}) on window lands on (1000, 0) assert_equals: expected 1000 but got 3215
    41 FAIL scrollBy({left: 10000, top: -100}) on window lands on (1000, 0) assert_equals: expected 1000 but got 3215
     38PASS scrollTo({left: 10000, top: -100}) on viewport-defining element lands on (1000, 0)
     39PASS scrollBy({left: 10000, top: -100}) on viewport-defining element lands on (1000, 0)
     40PASS scrollTo({left: 10000, top: -100}) on window lands on (1000, 0)
     41PASS scrollBy({left: 10000, top: -100}) on window lands on (1000, 0)
    4242

  • trunk/Source/WebCore/ChangeLog

    r273072 r273073  
     12021-02-18  Martin Robinson  <mrobinson@igalia.com>
     2
     3        Root scroll snapping broken overflow:scroll is set on the <body>
     4        https://bugs.webkit.org/show_bug.cgi?id=221407
     5
     6        Reviewed by Simon Fraser.
     7
     8        No new tests. This change fixes the WPT test:
     9        imported/w3c/web-platform-tests/css/css-scroll-snap/scrollTo-scrollBy-snaps.html
     10
     11        Instead of walking up the render tree to find the containing element, be sure to
     12        walk up the containing block chain. This will prevent WebKit from using the wrong
     13        scrolling container for positioned elements that snap.
     14
     15        * rendering/RenderBox.cpp:
     16        (WebCore::RenderBox::findEnclosingScrollableContainerForSnapping const): Deleted.
     17        * rendering/RenderBox.h: Remove method definition.
     18        * rendering/RenderObject.cpp:
     19        (WebCore::RenderObject::enclosingScrollableContainerForSnapping const): Find the
     20        scrolling container respecting the containing block chain. Never consider the
     21        RenderView as a containing block. Although this element has a scroll area
     22        and looks scrollable, it's a historical artifact and the real scroll container
     23        is the FrameView.
     24
    1252021-02-15  Sergio Villar Senin  <svillar@igalia.com>
    226

  • trunk/Source/WebCore/rendering/RenderBox.cpp

    r272992 r273073  
    51795179}
    51805180
    5181 const RenderBox* RenderBox::findEnclosingScrollableContainerForSnapping() const
    5182 {
    5183     for (auto& candidate : lineageOfType<RenderBox>(*this)) {
    5184         if (candidate.hasOverflowClip())
    5185             return &candidate;
    5186     }
    5187 
    5188     // If all parent elements are not overflow scrollable and the frame is, then return
    5189     // the root element.
    5190     if (document().documentElement() && frame().view() && frame().view()->isScrollable())
    5191         return document().documentElement()->renderBox();
    5192 
    5193     return nullptr;
    5194 }
    5195 
    51965181LayoutRect RenderBox::absoluteAnchorRectWithScrollMargin(bool* insideFixed) const
    51975182{

  • trunk/Source/WebCore/rendering/RenderBox.h

    r272193 r273073  
    643643    virtual bool needsLayoutAfterFragmentRangeChange() const { return false; }
    644644
    645     const RenderBox* findEnclosingScrollableContainerForSnapping() const;
    646    
    647645    bool isGridItem() const { return parent() && parent()->isRenderGrid() && !isExcludedFromNormalLayout(); }
    648646    bool isFlexItem() const { return parent() && parent()->isFlexibleBox() && !isExcludedFromNormalLayout(); }

  • trunk/Source/WebCore/rendering/RenderObject.cpp

    r271906 r273073  
    456456const RenderBox* RenderObject::enclosingScrollableContainerForSnapping() const
    457457{
    458     auto& renderBox = enclosingBox();
    459     if (auto* scrollableContainer = renderBox.findEnclosingScrollableContainerForSnapping()) {
    460         // The scrollable container for snapping cannot be the node itself.
    461         if (scrollableContainer != this)
    462             return scrollableContainer;
    463         if (renderBox.parentBox())
    464             return renderBox.parentBox()->findEnclosingScrollableContainerForSnapping();
    465     }
    466     return nullptr;
     458    // Walk up the container chain to find the scrollable container that contains
     459    // this RenderObject. The important thing here is that `container()` respects
     460    // the containing block chain for positioned elements. This is important because
     461    // scrollable overflow does not establish a new containing block for children.
     462    auto* candidate = container();
     463    while (candidate) {
     464        // Currently the RenderView can look like it has scrollable overflow, but we never
     465        // want to return this as our container. Instead we should use the root element.
     466        if (candidate->isRenderView())
     467            break;
     468        if (candidate->hasOverflowClip())
     469            return static_cast<RenderBox*>(candidate);
     470        candidate = candidate->container();
     471    }
     472
     473    // If we reach the root, then the root element is the scrolling container.
     474    return document().documentElement() ? document().documentElement()->renderBox() : nullptr;
    467475}
    468476
Note: See TracChangeset for help on using the changeset viewer.