Changeset 269506 in webkit


Ignore:
Timestamp:
Nov 6, 2020 1:44:54 AM (21 months ago)
Author:
commit-queue@webkit.org
Message:

Scroll snap specified on :root doesn't work
​https://bugs.webkit.org/show_bug.cgi?id=210469
<rdar://problem/61746676>

Source/WebCore:

Properly handle scroll-snap-type on the root element.

This change is originally based on a patch by Simon Fraser.

Patch by Martin Robinson <​mrobinson@igalia.com> on 2020-11-06
Reviewed by Simon Fraser.

Tests: tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal-legacy.html

tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical-legacy.html
tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe-legacy.html

  • page/FrameView.cpp:

(WebCore::FrameView::updateSnapOffsets): Send the body or root element style depending
on where the scroll-snap properties are set, but always use the FrameView viewport
when calculating geometry for scroll snap.

  • page/scrolling/AxisScrollSnapOffsets.cpp:

(WebCore::updateSnapOffsetsForScrollableArea): Accept a viewport rectangle from the
caller and also eliminate redundant argument that was causing a bit of confusion.

  • page/scrolling/AxisScrollSnapOffsets.h: Update the function declaration.
  • platform/ScrollableArea.cpp:

(WebCore::ScrollableArea::clearSnapOffsets): Added a helper to clear both vertical and horizontal snap info.

  • platform/ScrollableArea.h:
  • rendering/RenderBox.cpp:

(WebCore::RenderBox::findEnclosingScrollableContainerForSnapping const): Renamed findEnclosingScrollableContainer
to this because now it is only useful for scroll snapping. The only preexisting caller was
the scroll snap code.
(WebCore::RenderBox::findEnclosingScrollableContainer const): Deleted.

  • rendering/RenderBox.h: Updated method name.
  • rendering/RenderLayer.cpp:

(WebCore::RenderLayer::updateSnapOffsets): Update to reflect changes to updateSnapOffsetsForScrollableArea.

  • rendering/RenderObject.cpp:

(WebCore::RenderObject::enclosingScrollableContainerForSnapping const): Always return the
document root as the container instead of the body. The body never captures scroll
snap points.

LayoutTests:

Patch by Martin Robinson <​mrobinson@igalia.com> on 2020-11-06
Reviewed by Simon Fraser.

Modified main frame scroll snap tests to reflect the specification and add a
few more tests to test the legacy behavior which this change maintains for
backwards compatibility.

  • css3/scroll-snap/scroll-snap-positions-mainframe.html:
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal-legacy-expected.txt: Added.
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal-legacy.html: Copied from LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal.html.
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal.html:
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-horizontal.html:
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-vertical.html:
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical-legacy-expected.txt: Added.
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical-legacy.html: Copied from LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical.html.
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical-then-horizontal.html:
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical.html:
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe-legacy-expected.txt: Added.
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe-legacy.html: Copied from LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe.html.
  • tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe.html:
Location:
trunk
Files:
3 added
18 edited
3 copied

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r269499 r269506  
     12020-11-06  Martin Robinson  <mrobinson@igalia.com>
     2
     3        Scroll snap specified on :root doesn't work
     4        https://bugs.webkit.org/show_bug.cgi?id=210469
     5        <rdar://problem/61746676>
     6
     7        Reviewed by Simon Fraser.
     8
     9        Modified main frame scroll snap tests to reflect the specification and add a
     10        few more tests to test the legacy behavior which this change maintains for
     11        backwards compatibility.
     12
     13        * css3/scroll-snap/scroll-snap-positions-mainframe.html:
     14        * tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal-legacy-expected.txt: Added.
     15        * tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal-legacy.html: Copied from LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal.html.
     16        * tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal.html:
     17        * tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-horizontal.html:
     18        * tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-vertical.html:
     19        * tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical-legacy-expected.txt: Added.
     20        * tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical-legacy.html: Copied from LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical.html.
     21        * tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical-then-horizontal.html:
     22        * tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical.html:
     23        * tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe-legacy-expected.txt: Added.
     24        * tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe-legacy.html: Copied from LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe.html.
     25        * tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe.html:
     26
    1272020-11-05  Alex Christensen  <achristensen@webkit.org>
    228

  • trunk/LayoutTests/css3/scroll-snap/scroll-snap-positions-mainframe.html

    r217150 r269506  
    22<head>
    33    <style>
    4         body {
     4        html {
    55            margin: 0;
    66            scroll-snap-type: y mandatory;
    77        }
    8 
     8        body {
     9            margin: 0;
     10        }
    911        .vertical {
    1012            width: 100%;

  • trunk/LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal.html

    r258701 r269506  
    33    <head>
    44        <style>
     5            html {
     6                scroll-snap-type: x mandatory;
     7            }
    58            .horizontalGallery {
    69                width: 600vw;
    … …  
    811                margin: 0;
    912                padding: 0;
    10                 scroll-snap-type: x mandatory;
    1113            }
    1214            .colorBox {

  • trunk/LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-horizontal.html

    r258701 r269506  
    33    <head>
    44        <style>
     5            html {
     6                scroll-snap-type: x mandatory;
     7            }
    58            .horizontalGallery {
    69                width: 600vw;
    … …  
    811                margin: 0;
    912                padding: 0;
    10                 scroll-snap-type: x mandatory;
    1113            }
    1214            .colorBox {

  • trunk/LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-vertical.html

    r258701 r269506  
    33    <head>
    44        <style>
     5            html {
     6                scroll-snap-type: y mandatory;
     7            }
    58            .verticalGallery {
    69                width: 100vw;
    … …  
    811                margin: 0;
    912                padding: 0;
    10                 scroll-snap-type: y mandatory;
    1113            }
    1214            .colorBox {

  • trunk/LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical-then-horizontal.html

    r258701 r269506  
    33    <head>
    44        <style>
     5            html {
     6                scroll-snap-type: y mandatory;
     7            }
    58            .verticalGallery {
    69                width: 100vw;
    … …  
    811                margin: 0;
    912                padding: 0;
    10                 scroll-snap-type: y mandatory;
    1113            }
    1214            .colorBox {

  • trunk/LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical.html

    r258701 r269506  
    33    <head>
    44        <style>
     5            html {
     6                scroll-snap-type: y mandatory;
     7            }
    58            .verticalGallery {
    69                width: 100vw;
    … …  
    811                margin: 0;
    912                padding: 0;
    10                 scroll-snap-type: y mandatory;
    1113            }
    1214            .colorBox {

  • trunk/LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe-legacy.html

    r269505 r269506  
    1111                position: absolute;
    1212                scroll-snap-type: y proximity;
    13                 -webkit-scroll-snap-type: proximity;
    14                 scroll-snap-type: proximity;
    1513            }
    1614
    … …  
    2119                opacity: 0.5;
    2220                scroll-snap-align: start;
    23                 -webkit-scroll-snap-coordinate: 0 0;
    24                 scroll-snap-coordinate: 0 0;
    2521            }
    2622

  • trunk/LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe.html

    r235893 r269506  
    33    <head>
    44        <style>
     5            html {
     6                scroll-snap-type: y proximity;
     7            }
    58            body {
    69                margin: 0;
    … …  
    1013                overflow-y: scroll;
    1114                position: absolute;
    12                 scroll-snap-type: y proximity;
    13                 -webkit-scroll-snap-type: proximity;
    14                 scroll-snap-type: proximity;
    1515            }
    1616
    … …  
    2121                opacity: 0.5;
    2222                scroll-snap-align: start;
    23                 -webkit-scroll-snap-coordinate: 0 0;
    24                 scroll-snap-coordinate: 0 0;
    2523            }
    2624

  • trunk/Source/WebCore/ChangeLog

    r269503 r269506  
     12020-11-06  Martin Robinson  <mrobinson@igalia.com>
     2
     3        Scroll snap specified on :root doesn't work
     4        https://bugs.webkit.org/show_bug.cgi?id=210469
     5        <rdar://problem/61746676>
     6
     7        Properly handle scroll-snap-type on the root element.
     8
     9        This change is originally based on a patch by Simon Fraser.
     10
     11        Reviewed by Simon Fraser.
     12
     13        Tests: tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal-legacy.html
     14               tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical-legacy.html
     15               tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-mainframe-legacy.html
     16
     17        * page/FrameView.cpp:
     18        (WebCore::FrameView::updateSnapOffsets): Send the body or root element style depending
     19        on where the scroll-snap properties are set, but always use the FrameView viewport
     20        when calculating geometry for scroll snap.
     21        * page/scrolling/AxisScrollSnapOffsets.cpp:
     22        (WebCore::updateSnapOffsetsForScrollableArea): Accept a viewport rectangle from the
     23        caller and also eliminate redundant argument that was causing a bit of confusion.
     24        * page/scrolling/AxisScrollSnapOffsets.h: Update the function declaration.
     25        * platform/ScrollableArea.cpp:
     26        (WebCore::ScrollableArea::clearSnapOffsets):  Added a helper to clear both vertical and horizontal snap info.
     27        * platform/ScrollableArea.h:
     28        * rendering/RenderBox.cpp:
     29        (WebCore::RenderBox::findEnclosingScrollableContainerForSnapping const): Renamed findEnclosingScrollableContainer
     30        to this because now it is only useful for scroll snapping. The only preexisting caller was
     31        the scroll snap code.
     32        (WebCore::RenderBox::findEnclosingScrollableContainer const): Deleted.
     33        * rendering/RenderBox.h: Updated method name.
     34        * rendering/RenderLayer.cpp:
     35        (WebCore::RenderLayer::updateSnapOffsets): Update to reflect changes to updateSnapOffsetsForScrollableArea.
     36        * rendering/RenderObject.cpp:
     37        (WebCore::RenderObject::enclosingScrollableContainerForSnapping const): Always return the
     38        document root as the container instead of the body. The body never captures scroll
     39        snap points.
     40
    1412020-11-05  Said Abou-Hallawa  <said@apple.com>
    242

  • trunk/Source/WebCore/page/FrameView.cpp

    r269437 r269506  
    920920        return;
    921921
    922     // FIXME: Should we allow specifying snap points through <html> tags too?
    923     HTMLElement* body = frame().document()->bodyOrFrameset();
    924     if (!renderView() || !body || !body->renderer())
    925         return;
    926    
    927     updateSnapOffsetsForScrollableArea(*this, *body, *renderView(), body->renderer()->style());
     922    auto& document = *frame().document();
     923    auto* documentElement = document.documentElement();
     924    RenderBox* bodyRenderer = document.bodyOrFrameset() ? document.bodyOrFrameset()->renderBox() : nullptr;
     925    RenderBox* rootRenderer = documentElement ? documentElement->renderBox() : nullptr;
     926    auto rendererSyleHasScrollSnap = [](const RenderObject* renderer) {
     927        return renderer && renderer->style().scrollSnapType().strictness != ScrollSnapStrictness::None;
     928    };
     929
     930    const RenderStyle* styleToUse = nullptr;
     931    if (rendererSyleHasScrollSnap(bodyRenderer)) {
     932        //  The specification doesn't allow setting scroll-snap-type on the body, but
     933        //  we do this to ensure backwards compatibility with an earlier version of the
     934        //  specification: See webkit.org/b/200643.
     935        styleToUse = &bodyRenderer->style();
     936    } else if (rendererSyleHasScrollSnap(rootRenderer))
     937        styleToUse = &rootRenderer->style();
     938
     939    if (!styleToUse || !documentElement) {
     940        clearSnapOffsets();
     941        return;
     942    }
     943
     944    LayoutRect viewport = LayoutRect(IntPoint(), baseLayoutViewportSize());
     945    updateSnapOffsetsForScrollableArea(*this, *rootRenderer, *styleToUse, viewport);
    928946}
    929947

  • trunk/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp

    r268889 r269506  
    161161}
    162162
    163 void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, HTMLElement& scrollingElement, const RenderBox& scrollingElementBox, const RenderStyle& scrollingElementStyle)
    164 {
    165     auto* scrollContainer = scrollingElement.renderer();
     163void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, const RenderBox& scrollingElementBox, const RenderStyle& scrollingElementStyle, LayoutRect viewportRectInBorderBoxCoordinates)
     164{
    166165    auto scrollSnapType = scrollingElementStyle.scrollSnapType();
    167     if (!scrollContainer || scrollSnapType.strictness == ScrollSnapStrictness::None || scrollContainer->view().boxesWithScrollSnapPositions().isEmpty()) {
    168         scrollableArea.clearHorizontalSnapOffsets();
    169         scrollableArea.clearVerticalSnapOffsets();
     166    const auto& boxesWithScrollSnapPositions = scrollingElementBox.view().boxesWithScrollSnapPositions();
     167    if (scrollSnapType.strictness == ScrollSnapStrictness::None || boxesWithScrollSnapPositions.isEmpty()) {
     168        scrollableArea.clearSnapOffsets();
    170169        return;
    171170    }
    … …  
    185184
    186185    // The bounds of the scrolling container's snap port, where the top left of the scrolling container's border box is the origin.
    187     auto scrollSnapPort = computeScrollSnapPortOrAreaRect(scrollingElementBox.paddingBoxRect(), scrollingElementStyle.scrollPadding(), InsetOrOutset::Inset);
     186    auto scrollSnapPort = computeScrollSnapPortOrAreaRect(viewportRectInBorderBoxCoordinates, scrollingElementStyle.scrollPadding(), InsetOrOutset::Inset);
    188187    LOG_WITH_STREAM(ScrollSnap, stream << "Computing scroll snap offsets for " << scrollableArea << " in snap port " << scrollSnapPort);
    189     for (auto* child : scrollContainer->view().boxesWithScrollSnapPositions()) {
    190         if (child->enclosingScrollableContainerForSnapping() != scrollContainer)
     188    for (auto* child : boxesWithScrollSnapPositions) {
     189        if (child->enclosingScrollableContainerForSnapping() != &scrollingElementBox)
    191190            continue;
    192191
    … …  
    194193        // The snap area is the bounding box of the child element's border box, after applying transformations.
    195194        // FIXME: For now, just consider whether the scroller is RTL. The behavior of LTR boxes inside a RTL scroller is poorly defined: https://github.com/w3c/csswg-drafts/issues/5361.
    196         auto scrollSnapArea = LayoutRect(child->localToContainerQuad(FloatQuad(child->borderBoundingBox()), scrollingElement.renderBox()).boundingBox());
     195        auto scrollSnapArea = LayoutRect(child->localToContainerQuad(FloatQuad(child->borderBoundingBox()), &scrollingElementBox).boundingBox());
    197196
    198197        // localToContainerQuad will transform the scroll snap area by the scroll position, except in the case that this position is

  • trunk/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h

    r222113 r269506  
    2828#if ENABLE(CSS_SCROLL_SNAP)
    2929
     30#include "LayoutRect.h"
    3031#include "LayoutUnit.h"
    3132#include "ScrollSnapOffsetsInfo.h"
    … …  
    4041class ScrollableArea;
    4142
    42 void updateSnapOffsetsForScrollableArea(ScrollableArea&, HTMLElement& scrollingElement, const RenderBox& scrollingElementBox, const RenderStyle& scrollingElementStyle);
     43// Update the snap offsets for this scrollable area, given the RenderBox of the scroll container, the RenderStyle
     44// which defines the scroll-snap properties, and the viewport rectangle with the origin at the top left of
     45// the scrolling container's border box.
     46void updateSnapOffsetsForScrollableArea(ScrollableArea&, const RenderBox& scrollingElementBox, const RenderStyle& scrollingElementStyle, LayoutRect viewportRectInBorderBoxCoordinates);
    4347
    4448const unsigned invalidSnapOffsetIndex = UINT_MAX;

  • trunk/Source/WebCore/platform/ScrollableArea.cpp

    r269437 r269506  
    531531}
    532532
     533void ScrollableArea::clearSnapOffsets()
     534{
     535    clearHorizontalSnapOffsets();
     536    clearVerticalSnapOffsets();
     537}
     538
    533539void ScrollableArea::clearHorizontalSnapOffsets()
    534540{

  • trunk/Source/WebCore/platform/ScrollableArea.h

    r269437 r269506  
    9797    void setHorizontalSnapOffsetRanges(const Vector<ScrollOffsetRange<LayoutUnit>>&);
    9898    void setVerticalSnapOffsetRanges(const Vector<ScrollOffsetRange<LayoutUnit>>&);
     99    void clearSnapOffsets();
    99100    void clearHorizontalSnapOffsets();
    100101    void clearVerticalSnapOffsets();

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

    r269450 r269506  
    50345034}
    50355035
    5036 const RenderBox* RenderBox::findEnclosingScrollableContainer() const
     5036const RenderBox* RenderBox::findEnclosingScrollableContainerForSnapping() const
    50375037{
    50385038    for (auto& candidate : lineageOfType<RenderBox>(*this)) {
    … …  
    50405040            return &candidate;
    50415041    }
    5042     // If all parent elements are not overflow scrollable, check the body.
    5043     // FIXME: We should not treat the body as the scrollable element (see webkit.org/b/210469).
    5044     if (document().body() && frame().view() && frame().view()->isScrollable())
    5045         return document().body()->renderBox();
    5046    
     5042
     5043    // If all parent elements are not overflow scrollable and the frame is, then return
     5044    // the root element.
     5045    if (document().documentElement() && frame().view() && frame().view()->isScrollable())
     5046        return document().documentElement()->renderBox();
     5047
    50475048    return nullptr;
    50485049}

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

    r269144 r269506  
    638638    virtual bool needsLayoutAfterFragmentRangeChange() const { return false; }
    639639
    640     const RenderBox* findEnclosingScrollableContainer() const;
     640    const RenderBox* findEnclosingScrollableContainerForSnapping() const;
    641641   
    642642    bool isGridItem() const { return parent() && parent()->isRenderGrid() && !isExcludedFromNormalLayout(); }

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

    r269070 r269506  
    35933593
    35943594    RenderBox* box = enclosingElement()->renderBox();
    3595     updateSnapOffsetsForScrollableArea(*this, *downcast<HTMLElement>(enclosingElement()), *box, box->style());
     3595    updateSnapOffsetsForScrollableArea(*this, *box, box->style(), box->paddingBoxRect());
    35963596}
    35973597

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

    r269442 r269506  
    456456{
    457457    auto& renderBox = enclosingBox();
    458     if (auto* scrollableContainer = renderBox.findEnclosingScrollableContainer()) {
     458    if (auto* scrollableContainer = renderBox.findEnclosingScrollableContainerForSnapping()) {
    459459        // The scrollable container for snapping cannot be the node itself.
    460460        if (scrollableContainer != this)
    461461            return scrollableContainer;
    462462        if (renderBox.parentBox())
    463             return renderBox.parentBox()->findEnclosingScrollableContainer();
     463            return renderBox.parentBox()->findEnclosingScrollableContainerForSnapping();
    464464    }
    465465    return nullptr;
Note: See TracChangeset for help on using the changeset viewer.