Changeset 278350 in webkit


Ignore:
Timestamp:
Jun 2, 2021 5:37:00 AM (14 months ago)
Author:
commit-queue@webkit.org
Message:

[css-scroll-snap] Scroll snap is broken with non-horizontal writing modes
​https://bugs.webkit.org/show_bug.cgi?id=226010

Patch by Martin Robinson <​mrobinson@igalia.com> on 2021-06-02
Reviewed by Frédéric Wang.

LayoutTests/imported/w3c:

  • web-platform-tests/css/css-scroll-snap/scroll-snap-type-on-root-element-expected.txt: Update expectations to mark tests as passing.
  • web-platform-tests/css/css-scroll-snap/snap-inline-block-expected.txt: Ditto.

Source/WebCore:

Fix issues related to vertical writing modes and scroll snap.

This change fixes three existing WPT tests.

imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-snap-type-on-root-element.html
imported/w3c/web-platform-tests/css/css-scroll-snap/snap-after-initial-layout/writing-mode-vertical-lr.html
imported/w3c/web-platform-tests/css/css-scroll-snap/snap-inline-block.html

  • css/CSSComputedStyleDeclaration.cpp:

(WebCore::valueForScrollSnapAlignment): Update to reflect new member names of ScrollSnapAlign.

  • page/FrameView.cpp:

(WebCore::FrameView::updateSnapOffsets): Pass in the text direction and writing mode of the
container, allowing the values specified on the body to override those specified on the root
element.

  • page/scrolling/ScrollSnapOffsetsInfo.cpp:

(WebCore::updateSnapOffsetsForScrollableArea): Properly handle the writing mode and the
text direction of the container.

  • page/scrolling/ScrollSnapOffsetsInfo.h: Update function signature.
  • rendering/RenderLayerModelObject.cpp:

(WebCore::scrollSnapContainerRequiresUpdateForStyleUpdate): Pass in the writing mode and
text direction of the scrolling container.

  • rendering/RenderLayerScrollableArea.cpp:

(WebCore::RenderLayerScrollableArea::updateSnapOffsets): Update to reflect new member names
of ScrollSnapAlign.

  • rendering/style/RenderStyle.cpp:

(WebCore::RenderStyle::hasSnapPosition const): Ditto.

  • rendering/style/StyleScrollSnapPoints.h: Change the name of the members of ScrollSnapAlign

to match what is described in the specification. The values provided are for block and
inline directions, but depending on the scroll container.
(WebCore::operator==): Ditto.

  • style/StyleBuilderConverter.h:

(WebCore::Style::BuilderConverter::convertScrollSnapAlign): Ditto.

LayoutTests:

  • TestExpectations: Mark one test as passing.
  • platform/ios-wk2/imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-snap-type-on-root-element-expected.txt: Removed.
  • platform/ios/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-inline-block-expected.txt:
Location:
trunk
Files:
1 deleted
15 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r278346 r278350  
     12021-06-02  Martin Robinson  <mrobinson@igalia.com>
     2
     3        [css-scroll-snap] Scroll snap is broken with non-horizontal writing modes
     4        https://bugs.webkit.org/show_bug.cgi?id=226010
     5
     6        Reviewed by Frédéric Wang.
     7
     8        * TestExpectations: Mark one test as passing.
     9        * platform/ios-wk2/imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-snap-type-on-root-element-expected.txt: Removed.
     10        * platform/ios/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-inline-block-expected.txt:
     11
    1122021-06-02  Antti Koivisto  <antti@apple.com>
    213

  • trunk/LayoutTests/TestExpectations

    r278276 r278350  
    45444544imported/w3c/web-platform-tests/css/css-scroll-snap/snap-after-initial-layout/scroll-snap-initial-layout-000.html [ ImageOnlyFailure ]
    45454545imported/w3c/web-platform-tests/css/css-scroll-snap/snap-after-initial-layout/scroll-snap-writing-mode-000.html [ ImageOnlyFailure ]
    4546 imported/w3c/web-platform-tests/css/css-scroll-snap/snap-after-initial-layout/writing-mode-vertical-lr.html [ ImageOnlyFailure ]
    45474546webkit.org/b/218325 imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-target-margin-001.html [ Pass ImageOnlyFailure ]
    45484547webkit.org/b/218325 imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-target-margin-003.html [ Pass ImageOnlyFailure ]

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r278336 r278350  
     12021-06-02  Martin Robinson  <mrobinson@igalia.com>
     2
     3        [css-scroll-snap] Scroll snap is broken with non-horizontal writing modes
     4        https://bugs.webkit.org/show_bug.cgi?id=226010
     5
     6        Reviewed by Frédéric Wang.
     7
     8        * web-platform-tests/css/css-scroll-snap/scroll-snap-type-on-root-element-expected.txt: Update expectations to mark tests as passing.
     9        * web-platform-tests/css/css-scroll-snap/snap-inline-block-expected.txt: Ditto.
     10
    1112021-06-01  Jean-Yves Avenard  <jya@apple.com>
    212

  • trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-snap-type-on-root-element-expected.txt

    r272610 r278350  
    11
    22PASS The scroll-snap-type on the root element is applied
    3 FAIL The writing-mode (vertical-lr) on the body is used assert_equals: inline should snap expected 515 but got 800
     3PASS The writing-mode (vertical-lr) on the body is used
    44PASS The writing-mode (horizontal-tb) on the body is used
    55

  • trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-inline-block-expected.txt

    r276182 r278350  
    11
    22PASS Snaps correctly for horizontal-tb writing mode with 'scroll-snap-align: end start' alignment
    3 FAIL Snaps correctly for vertical-lr writing mode with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on x expected 115 but got 300
    4 FAIL Snaps correctly for vertical-rl writing mode with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on y expected 300 but got 165
    5 FAIL Snaps correctly for horizontal-tb writing mode with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected 115 but got 0
    6 FAIL Snaps correctly for vertical-lr writing mode with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected 300 but got 0
    7 FAIL Snaps correctly for vertical-rl writing mode with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on y expected 165 but got 300
    8 FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on y expected 165 but got 0
    9 FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on y expected 300 but got 0
     3PASS Snaps correctly for vertical-lr writing mode with 'scroll-snap-align: end start' alignment
     4PASS Snaps correctly for vertical-rl writing mode with 'scroll-snap-align: end start' alignment
     5PASS Snaps correctly for horizontal-tb writing mode with 'scroll-snap-align: start end' alignment
     6PASS Snaps correctly for vertical-lr writing mode with 'scroll-snap-align: start end' alignment
     7PASS Snaps correctly for vertical-rl writing mode with 'scroll-snap-align: start end' alignment
     8PASS Snaps correctly for 'direction: rtl' with 'scroll-snap-align: end start' alignment
     9PASS Snaps correctly for 'direction: rtl' with 'scroll-snap-align: start end' alignment
    1010

  • trunk/LayoutTests/platform/ios/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-inline-block-expected.txt

    r276182 r278350  
    11
    22PASS Snaps correctly for horizontal-tb writing mode with 'scroll-snap-align: end start' alignment
    3 FAIL Snaps correctly for vertical-lr writing mode with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on x expected 115 but got 300
     3PASS Snaps correctly for vertical-lr writing mode with 'scroll-snap-align: end start' alignment
    44FAIL Snaps correctly for vertical-rl writing mode with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on x expected -315 but got -300
    55FAIL Snaps correctly for horizontal-tb writing mode with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected 115 but got 0
    66FAIL Snaps correctly for vertical-lr writing mode with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected 300 but got 0
    77FAIL Snaps correctly for vertical-rl writing mode with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected -500 but got -485
    8 FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on x expected -500 but got -485
    9 FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected -315 but got -300
     8FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on x expected -500 but got -300
     9FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected -315 but got -485
    1010

  • trunk/Source/WebCore/ChangeLog

    r278346 r278350  
     12021-06-02  Martin Robinson  <mrobinson@igalia.com>
     2
     3        [css-scroll-snap] Scroll snap is broken with non-horizontal writing modes
     4        https://bugs.webkit.org/show_bug.cgi?id=226010
     5
     6        Reviewed by Frédéric Wang.
     7
     8        Fix issues related to vertical writing modes and scroll snap.
     9
     10        This change fixes three existing WPT tests.
     11            imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-snap-type-on-root-element.html
     12            imported/w3c/web-platform-tests/css/css-scroll-snap/snap-after-initial-layout/writing-mode-vertical-lr.html
     13            imported/w3c/web-platform-tests/css/css-scroll-snap/snap-inline-block.html
     14
     15        * css/CSSComputedStyleDeclaration.cpp:
     16        (WebCore::valueForScrollSnapAlignment): Update to reflect new member names of ScrollSnapAlign.
     17        * page/FrameView.cpp:
     18        (WebCore::FrameView::updateSnapOffsets): Pass in the text direction and writing mode of the
     19        container, allowing the values specified on the body to override those specified on the root
     20        element.
     21        * page/scrolling/ScrollSnapOffsetsInfo.cpp:
     22        (WebCore::updateSnapOffsetsForScrollableArea): Properly handle the writing mode and the
     23        text direction of the container.
     24        * page/scrolling/ScrollSnapOffsetsInfo.h: Update function signature.
     25        * rendering/RenderLayerModelObject.cpp:
     26        (WebCore::scrollSnapContainerRequiresUpdateForStyleUpdate): Pass in the writing mode and
     27        text direction of the scrolling container.
     28        * rendering/RenderLayerScrollableArea.cpp:
     29        (WebCore::RenderLayerScrollableArea::updateSnapOffsets): Update to reflect new member names
     30        of ScrollSnapAlign.
     31        * rendering/style/RenderStyle.cpp:
     32        (WebCore::RenderStyle::hasSnapPosition const): Ditto.
     33        * rendering/style/StyleScrollSnapPoints.h: Change the name of the members of ScrollSnapAlign
     34        to match what is described in the specification. The values provided are for block and
     35        inline directions, but depending on the scroll container.
     36        (WebCore::operator==): Ditto.
     37        * style/StyleBuilderConverter.h:
     38        (WebCore::Style::BuilderConverter::convertScrollSnapAlign): Ditto.
     39
    1402021-06-02  Antti Koivisto  <antti@apple.com>
    241

  • trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp

    r278253 r278350  
    11011101{
    11021102    auto value = CSSValueList::createSpaceSeparated();
    1103     value->append(CSSPrimitiveValue::create(alignment.y));
    1104     if (alignment.x != alignment.y)
    1105         value->append(CSSPrimitiveValue::create(alignment.x));
     1103    value->append(CSSPrimitiveValue::create(alignment.blockAlign));
     1104    if (alignment.inlineAlign != alignment.blockAlign)
     1105        value->append(CSSPrimitiveValue::create(alignment.inlineAlign));
    11061106    return value;
    11071107}

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

    r278253 r278350  
    950950    viewport.move(-rootRenderer->marginLeft(), -rootRenderer->marginTop());
    951951
    952     updateSnapOffsetsForScrollableArea(*this, *rootRenderer, *styleToUse, viewport);
     952    updateSnapOffsetsForScrollableArea(*this, *rootRenderer, *styleToUse, viewport, rootRenderer->style().writingMode(), rootRenderer->style().direction());
    953953}
    954954

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

    r278253 r278350  
    198198}
    199199
    200 void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, const RenderBox& scrollingElementBox, const RenderStyle& scrollingElementStyle, LayoutRect viewportRectInBorderBoxCoordinates)
     200void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, const RenderBox& scrollingElementBox, const RenderStyle& scrollingElementStyle, LayoutRect viewportRectInBorderBoxCoordinates, WritingMode writingMode, TextDirection textDirection)
    201201{
    202202    auto scrollSnapType = scrollingElementStyle.scrollSnapType();
    … …  
    218218    HashMap<float, SnapOffset<LayoutUnit>> horizontalSnapOffsetsMap;
    219219    Vector<LayoutRect> snapAreas;
    220     bool hasHorizontalSnapOffsets = scrollSnapType.axis == ScrollSnapAxis::Both || scrollSnapType.axis == ScrollSnapAxis::XAxis || scrollSnapType.axis == ScrollSnapAxis::Inline;
    221     bool hasVerticalSnapOffsets = scrollSnapType.axis == ScrollSnapAxis::Both || scrollSnapType.axis == ScrollSnapAxis::YAxis || scrollSnapType.axis == ScrollSnapAxis::Block;
    222220
    223221    auto maxScrollOffset = scrollableArea.maximumScrollOffset();
    224222    auto scrollPosition = LayoutPoint { scrollableArea.scrollPosition() };
    225     bool scrollerIsRTL = !scrollingElementBox.style().isLeftToRightDirection();
     223
     224    // text-direction flips the inline axis and writing-mode can flip the block axis. Whether or
     225    // not the writing-mode is vertical determines the physical orientation of the block and inline axes.
     226    bool scrollerHasVerticalWritingMode = isVerticalWritingMode(writingMode);
     227    bool blockAxisFlipped = isFlippedWritingMode(writingMode);
     228    bool inlineAxisFlipped = textDirection == TextDirection::RTL;
     229    bool xAxisFlipped = scrollerHasVerticalWritingMode ? blockAxisFlipped : inlineAxisFlipped;
     230    bool yAxisFlipped = scrollerHasVerticalWritingMode ? inlineAxisFlipped : blockAxisFlipped;
     231
     232    bool hasHorizontalSnapOffsets = scrollSnapType.axis == ScrollSnapAxis::Both || scrollSnapType.axis == ScrollSnapAxis::XAxis;
     233    bool hasVerticalSnapOffsets = scrollSnapType.axis == ScrollSnapAxis::Both || scrollSnapType.axis == ScrollSnapAxis::YAxis;
     234    if (scrollSnapType.axis == ScrollSnapAxis::Block) {
     235        hasHorizontalSnapOffsets = scrollerHasVerticalWritingMode;
     236        hasVerticalSnapOffsets = !scrollerHasVerticalWritingMode;
     237    }
     238    if (scrollSnapType.axis == ScrollSnapAxis::Inline) {
     239        hasHorizontalSnapOffsets = !scrollerHasVerticalWritingMode;
     240        hasVerticalSnapOffsets = scrollerHasVerticalWritingMode;
     241    }
    226242
    227243    // The bounds of the scrolling container's snap port, where the top left of the scrolling container's border box is the origin.
    … …  
    234250        // The bounds of the child element's snap area, where the top left of the scrolling container's border box is the origin.
    235251        // The snap area is the bounding box of the child element's border box, after applying transformations.
    236         // 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.
    237252        auto scrollSnapArea = LayoutRect(child->localToContainerQuad(FloatQuad(child->borderBoundingBox()), &scrollingElementBox).boundingBox());
    238253
    … …  
    247262        auto stop = child->style().scrollSnapStop();
    248263
    249         bool snapsHorizontally = hasHorizontalSnapOffsets && alignment.x != ScrollSnapAxisAlignType::None;
    250         bool snapsVertically = hasVerticalSnapOffsets && alignment.y != ScrollSnapAxisAlignType::None;
     264        ScrollSnapAxisAlignType xAlign = scrollerHasVerticalWritingMode ? alignment.blockAlign : alignment.inlineAlign;
     265        ScrollSnapAxisAlignType yAlign = scrollerHasVerticalWritingMode ? alignment.inlineAlign : alignment.blockAlign;
     266        bool snapsHorizontally = hasHorizontalSnapOffsets && xAlign != ScrollSnapAxisAlignType::None;
     267        bool snapsVertically = hasVerticalSnapOffsets && yAlign != ScrollSnapAxisAlignType::None;
     268
    251269        if (!snapsHorizontally && !snapsVertically)
    252270            continue;
    … …  
    258276
    259277        if (snapsHorizontally) {
    260             auto absoluteScrollXPosition = computeScrollSnapAlignOffset(scrollSnapArea.x(), scrollSnapArea.maxX(), alignment.x, scrollerIsRTL) - computeScrollSnapAlignOffset(scrollSnapPort.x(), scrollSnapPort.maxX(), alignment.x, scrollerIsRTL);
     278            auto absoluteScrollXPosition = computeScrollSnapAlignOffset(scrollSnapArea.x(), scrollSnapArea.maxX(), xAlign, xAxisFlipped) - computeScrollSnapAlignOffset(scrollSnapPort.x(), scrollSnapPort.maxX(), xAlign, xAxisFlipped);
    261279            auto absoluteScrollOffset = clampTo<int>(scrollableArea.scrollOffsetFromPosition({ roundToInt(absoluteScrollXPosition), 0 }).x(), 0, maxScrollOffset.x());
    262280            addOrUpdateStopForSnapOffset(horizontalSnapOffsetsMap, { absoluteScrollOffset, stop, snapAreas.size() - 1 });
    263281        }
    264282        if (snapsVertically) {
    265             auto absoluteScrollYPosition = computeScrollSnapAlignOffset(scrollSnapArea.y(), scrollSnapArea.maxY(), alignment.y, false) - computeScrollSnapAlignOffset(scrollSnapPort.y(), scrollSnapPort.maxY(), alignment.y, false);
     283            auto absoluteScrollYPosition = computeScrollSnapAlignOffset(scrollSnapArea.y(), scrollSnapArea.maxY(), yAlign, yAxisFlipped) - computeScrollSnapAlignOffset(scrollSnapPort.y(), scrollSnapPort.maxY(), yAlign, yAxisFlipped);
    266284            auto absoluteScrollOffset = clampTo<int>(scrollableArea.scrollOffsetFromPosition({ 0, roundToInt(absoluteScrollYPosition) }).y(), 0, maxScrollOffset.y());
    267285            addOrUpdateStopForSnapOffset(verticalSnapOffsetsMap, { absoluteScrollOffset, stop, snapAreas.size() - 1 });

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

    r278253 r278350  
    9797// which defines the scroll-snap properties, and the viewport rectangle with the origin at the top left of
    9898// the scrolling container's border box.
    99 void updateSnapOffsetsForScrollableArea(ScrollableArea&, const RenderBox& scrollingElementBox, const RenderStyle& scrollingElementStyle, LayoutRect viewportRectInBorderBoxCoordinates);
     99void updateSnapOffsetsForScrollableArea(ScrollableArea&, const RenderBox& scrollingElementBox, const RenderStyle& scrollingElementStyle, LayoutRect viewportRectInBorderBoxCoordinates, WritingMode, TextDirection);
    100100
    101101template <typename T> WTF::TextStream& operator<<(WTF::TextStream& ts, SnapOffset<T> offset)

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

    r278253 r278350  
    14701470
    14711471    RenderBox* box = m_layer.enclosingElement()->renderBox();
    1472     updateSnapOffsetsForScrollableArea(*this, *box, box->style(), box->paddingBoxRect());
     1472    updateSnapOffsetsForScrollableArea(*this, *box, box->style(), box->paddingBoxRect(), box->style().writingMode(), box->style().direction());
    14731473}
    14741474

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

    r278253 r278350  
    25792579{
    25802580    const ScrollSnapAlign& alignment = this->scrollSnapAlign();
    2581     return alignment.x != ScrollSnapAxisAlignType::None || alignment.y != ScrollSnapAxisAlignType::None;
     2581    return alignment.blockAlign != ScrollSnapAxisAlignType::None || alignment.inlineAlign != ScrollSnapAxisAlignType::None;
    25822582}
    25832583#endif

  • trunk/Source/WebCore/rendering/style/StyleScrollSnapPoints.h

    r270023 r278350  
    4949
    5050struct ScrollSnapAlign {
    51     ScrollSnapAxisAlignType x { ScrollSnapAxisAlignType::None };
    52     ScrollSnapAxisAlignType y { ScrollSnapAxisAlignType::None };
     51    ScrollSnapAxisAlignType blockAlign { ScrollSnapAxisAlignType::None };
     52    ScrollSnapAxisAlignType inlineAlign { ScrollSnapAxisAlignType::None };
    5353};
    5454
    5555inline bool operator==(const ScrollSnapAlign& a, const ScrollSnapAlign& b)
    5656{
    57     return a.x == b.x && a.y == b.y;
     57    return a.blockAlign == b.blockAlign && a.inlineAlign == b.inlineAlign;
    5858}
    5959

  • trunk/Source/WebCore/style/StyleBuilderConverter.h

    r278340 r278350  
    922922    auto& values = downcast<CSSValueList>(value);
    923923    ScrollSnapAlign alignment;
    924     alignment.y = downcast<CSSPrimitiveValue>(*values.item(0));
     924    alignment.blockAlign = downcast<CSSPrimitiveValue>(*values.item(0));
    925925    if (values.length() == 1)
    926         alignment.x = alignment.y;
     926        alignment.inlineAlign = alignment.blockAlign;
    927927    else
    928         alignment.x = downcast<CSSPrimitiveValue>(*values.item(1));
     928        alignment.inlineAlign = downcast<CSSPrimitiveValue>(*values.item(1));
    929929    return alignment;
    930930}
Note: See TracChangeset for help on using the changeset viewer.