Changeset 271480 in webkit


Ignore:
Timestamp:
Jan 14, 2021 3:02:43 AM (3 years ago)
Author:
commit-queue@webkit.org
Message:

[css-scroll-snap] scroll-snap-align parsing is incorrect/backwards
​https://bugs.webkit.org/show_bug.cgi?id=191865
<rdar://problem/46346516>

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

LayoutTests/imported/w3c:

  • web-platform-tests/css/css-scroll-snap/scroll-snap-type-change-expected.txt: Update test expectations.
  • web-platform-tests/css/css-scroll-snap/scroll-snap-type-on-root-element-expected.txt: Ditto.
  • web-platform-tests/css/css-scroll-snap/snap-after-relayout/changing-scroll-snap-type-expected.txt: Ditto.
  • web-platform-tests/css/css-scroll-snap/snap-after-relayout/snap-to-different-targets-expected.txt: Ditto.
  • web-platform-tests/css/css-scroll-snap/snap-inline-block-expected.txt: Ditto.

Source/WebCore:

An earlier version of the scroll snap specification specified that the arguments to
scroll-snap-align should be <inline> and then <block>, but a later version reversed
the order. This change aligns the WebKit implementation with the specification.

This has some web compatibility implications, but current measurements show that
the use of the two value variant is still low.

  • css/CSSComputedStyleDeclaration.cpp:

(WebCore::valueForScrollSnapAlignment): Reverse the order of scroll-snap-align serialization.

  • style/StyleBuilderConverter.h:

(WebCore::Style::BuilderConverter::convertScrollSnapAlign): Reverse the order of scroll-snap-align parsing.

LayoutTests:

  • TestExpectations: Update test expectations.
  • css3/scroll-snap/scroll-snap-mismatch.html: Flip scroll-snap-align arguments in this test.
  • platform/ios-wk2/imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-snap-type-on-root-element-expected.txt: Update results to reflect new pass.
Location:
trunk
Files:
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r271478 r271480  
     12021-01-14  Martin Robinson  <mrobinson@igalia.com>
     2
     3        [css-scroll-snap] scroll-snap-align parsing is incorrect/backwards
     4        https://bugs.webkit.org/show_bug.cgi?id=191865
     5        <rdar://problem/46346516>
     6
     7        Reviewed by Simon Fraser.
     8
     9        * TestExpectations: Update test expectations.
     10        * css3/scroll-snap/scroll-snap-mismatch.html: Flip scroll-snap-align arguments in this test.
     11        * platform/ios-wk2/imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-snap-type-on-root-element-expected.txt: Update results to reflect new pass.
     12
    1132021-01-13  Lauro Moura  <lmoura@igalia.com>
    214

  • trunk/LayoutTests/TestExpectations

    r271436 r271480  
    45214521imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-target-snap-003.html [ ImageOnlyFailure ]
    45224522imported/w3c/web-platform-tests/css/css-scroll-snap/snap-after-initial-layout/direction-rtl.html [ ImageOnlyFailure ]
    4523 imported/w3c/web-platform-tests/css/css-scroll-snap/snap-after-initial-layout/writing-mode-horizontal-tb.html [ ImageOnlyFailure ]
     4523imported/w3c/web-platform-tests/css/css-scroll-snap/snap-after-initial-layout/writing-mode-vertical-lr.html [ ImageOnlyFailure ]
    45244524
    45254525# Cocoa-only

  • trunk/LayoutTests/css3/scroll-snap/scroll-snap-mismatch.html

    r217150 r271480  
    1313
    1414            .good {
    15                 scroll-snap-align: start none;
     15                scroll-snap-align: none start;
    1616            }
    1717
    1818            .bad {
    19                 scroll-snap-align: none start;
     19                scroll-snap-align: start none;
    2020            }
    2121

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r271447 r271480  
     12021-01-14  Martin Robinson  <mrobinson@igalia.com>
     2
     3        [css-scroll-snap] scroll-snap-align parsing is incorrect/backwards
     4        https://bugs.webkit.org/show_bug.cgi?id=191865
     5        <rdar://problem/46346516>
     6
     7        Reviewed by Simon Fraser.
     8
     9        * web-platform-tests/css/css-scroll-snap/scroll-snap-type-change-expected.txt: Update test expectations.
     10        * web-platform-tests/css/css-scroll-snap/scroll-snap-type-on-root-element-expected.txt: Ditto.
     11        * web-platform-tests/css/css-scroll-snap/snap-after-relayout/changing-scroll-snap-type-expected.txt: Ditto.
     12        * web-platform-tests/css/css-scroll-snap/snap-after-relayout/snap-to-different-targets-expected.txt: Ditto.
     13        * web-platform-tests/css/css-scroll-snap/snap-inline-block-expected.txt: Ditto.
     14
    1152021-01-13  Ziran Sun  <zsun@igalia.com>
    216

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

    r268250 r271480  
    11
    2 FAIL scroll-snap-type on HTML should control snapping behavior and changing it takes effect assert_equals: scrolling should snap expected 200 but got 100
    3 FAIL scroll-snap-type on DIV should control snapping behavior and changing it takes effect assert_equals: scrolling should snap expected 200 but got 100
     2FAIL 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
     3PASS scroll-snap-type on DIV should control snapping behavior and changing it takes effect
    44

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

    r268250 r271480  
    22FAIL The scroll-snap-type on the root element is applied assert_equals: expected 515 but got 800
    33FAIL The writing-mode (vertical-lr) on the body is used assert_equals: inline should snap expected 515 but got 800
    4 FAIL The writing-mode (horizontal-tb) on the body is used  assert_equals: inline should snap expected 100 but got 200
     4PASS The writing-mode (horizontal-tb) on the body is used
    55

  • trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-after-relayout/changing-scroll-snap-type-expected.txt

    r268250 r271480  
    11
    2 FAIL Changing the scroller's snap type to y should make it resnap on the y-axis. assert_equals: expected 100 but got 0
    3 FAIL Changing the scroller's snap type to x should make it resnap on the x-axis. assert_equals: expected 100 but got 0
    4 FAIL Changing the scroller's snap type axis should make it resnap. assert_equals: expected 100 but got 0
     2PASS Changing the scroller's snap type to y should make it resnap on the y-axis.
     3PASS Changing the scroller's snap type to x should make it resnap on the x-axis.
     4PASS Changing the scroller's snap type axis should make it resnap.
    55

  • trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-after-relayout/snap-to-different-targets-expected.txt

    r268856 r271480  
    11
    2 FAIL Scroller should snap to at least one of the targets if unable to snap toboth after a layout change. assert_equals: expected 200 but got 400
     2FAIL Scroller should snap to at least one of the targets if unable to snap toboth after a layout change. assert_true: expected true got false
    33

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

    r271439 r271480  
    11
    2 FAIL Snaps correctly for horizontal-tb writing mode with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on x expected 300 but got 115
    3 PASS Snaps correctly for vertical-lr writing mode with 'scroll-snap-align: end start' alignment
    4 FAIL Snaps correctly for vertical-rl writing mode with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on x expected -315 but got -485
     2PASS Snaps correctly for horizontal-tb writing mode with 'scroll-snap-align: end start' alignment
     3FAIL 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
     4FAIL 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
    7 FAIL Snaps correctly for vertical-rl writing mode with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected -500 but got -300
    8 FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on x expected -500 but got -300
    9 FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected -315 but got -485
     7FAIL 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
     8FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on x expected -500 but got -485
     9FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected -315 but got -300
    1010

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

    r268250 r271480  
    22FAIL The scroll-snap-type on the root element is applied assert_equals: expected 500 but got 800
    33FAIL The writing-mode (vertical-lr) on the body is used assert_equals: inline should snap expected 500 but got 800
    4 FAIL The writing-mode (horizontal-tb) on the body is used  assert_equals: inline should snap expected 100 but got 200
     4PASS The writing-mode (horizontal-tb) on the body is used
    55

  • trunk/Source/WebCore/ChangeLog

    r271476 r271480  
     12021-01-14  Martin Robinson  <mrobinson@igalia.com>
     2
     3        [css-scroll-snap] scroll-snap-align parsing is incorrect/backwards
     4        https://bugs.webkit.org/show_bug.cgi?id=191865
     5        <rdar://problem/46346516>
     6
     7        Reviewed by Simon Fraser.
     8
     9        An earlier version of the scroll snap specification specified that the arguments to
     10        scroll-snap-align should be <inline> and then <block>, but a later version reversed
     11        the order. This change aligns the WebKit implementation with the specification.
     12
     13        This has some web compatibility implications, but current measurements show that
     14        the use of the two value variant is still low.
     15
     16        * css/CSSComputedStyleDeclaration.cpp:
     17        (WebCore::valueForScrollSnapAlignment): Reverse the order of scroll-snap-align serialization.
     18        * style/StyleBuilderConverter.h:
     19        (WebCore::Style::BuilderConverter::convertScrollSnapAlign): Reverse the order of scroll-snap-align parsing.
     20
    1212021-01-13  Andres Gonzalez  <andresg_22@apple.com>
    222

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

    r271447 r271480  
    11131113{
    11141114    auto value = CSSValueList::createSpaceSeparated();
     1115    value->append(CSSPrimitiveValue::create(alignment.y));
    11151116    value->append(CSSPrimitiveValue::create(alignment.x));
    1116     value->append(CSSPrimitiveValue::create(alignment.y));
    11171117    return value;
    11181118}

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

    r268665 r271480  
    923923    auto& values = downcast<CSSValueList>(value);
    924924    ScrollSnapAlign alignment;
    925     alignment.x = downcast<CSSPrimitiveValue>(*values.item(0));
     925    alignment.y = downcast<CSSPrimitiveValue>(*values.item(0));
    926926    if (values.length() == 1)
    927         alignment.y = alignment.x;
     927        alignment.x = alignment.y;
    928928    else
    929         alignment.y = downcast<CSSPrimitiveValue>(*values.item(1));
     929        alignment.x = downcast<CSSPrimitiveValue>(*values.item(1));
    930930    return alignment;
    931931}
Note: See TracChangeset for help on using the changeset viewer.