Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove a couple of scroll snap tests tied to an open spec issue from Interop 2023 #602

Closed
hiikezoe opened this issue Nov 8, 2023 · 12 comments
Labels
focus area: Scrolling test-change-proposal Proposal to add or remove tests for an interop area

Comments

@hiikezoe
Copy link

hiikezoe commented Nov 8, 2023

As skobes told in the carryover evaluation issue for Interop 2024, some of scroll snap related tests in Interop 2023 have failed due to an open spec issue. So it would make sense to remove them from Interop 2023.

The affected tests in the list of the skobes comment are;

css/css-scroll-snap/input/snap-area-overflow-boundary.html
css/css-scroll-snap/overflowing-snap-areas.html
css/css-scroll-snap/scroll-snap-stop-002.html
css/css-scroll-snap/snap-after-relayout/changing-scroll-snap-align.html

And I believe
css/css-scroll-snap/snap-after-initial-layout/scroll-snap-writing-mode-000.html

is also one of such tests since the test has started failing both on Firefox and Safari since Chrome changed the test itself as well as they changed their behavior to address the open spec issue.

CC @skobes-chromium and @nt1m

@gsnedders gsnedders added the test-change-proposal Proposal to add or remove tests for an interop area label Nov 8, 2023
@nt1m
Copy link
Member

nt1m commented Nov 9, 2023

I'm in favor of removing these tests from Interop 2023.

@nt1m
Copy link
Member

nt1m commented Nov 13, 2023

cc @flackr for Chromium input.

@nt1m
Copy link
Member

nt1m commented Nov 17, 2023

@foolip Since the tests are failing because of an open spec issue, I would suggest removing them from Interop.

@flackr
Copy link

flackr commented Nov 21, 2023

We have agreed on the linked issue that avoiding inner snap areas is expected and that the text does suggest this without prescribing exactly how it works. We haven't yet agreed on the exact behavior which is what we're attempting to do in w3c/csswg-drafts#9187. Would it make sense / can we have a high level test that inner areas are respected / avoided in a way that doesn't prescribe the exact behavior? @skobes-chromium

Also, which tests are actually depending on this issue? As far as I can tell the ones listed do not contain or prescribe behaviors for nested snap areas - only the ones with -nested- in the name do.

@flackr
Copy link

flackr commented Nov 22, 2023

The plan is to make sure that we're not including any tests related to avoiding nested snap areas in 2023, but we would like to resolve and test these in 2024. @skobes-chromium is going to make sure that we've not accidentally included these in suites in 2023 and we can make the nested tests .tentative for now.

@skobes-chromium
Copy link

skobes-chromium commented Dec 1, 2023

Nested snap cases have been marked as tentative (crbug.com/1505893).

@nt1m
Copy link
Member

nt1m commented Dec 1, 2023

@skobes-chromium Thanks for doing this!

@hiikezoe Can you verify that all the relevant tests have been excluded from Interop 2023?

@hiikezoe
Copy link
Author

hiikezoe commented Dec 1, 2023

It looks like the nested cases have been excluded, but some of tests which are not nested cases still remain. It's super unclear to me what the relation ship between them is.

For example, changing-scroll-snap-align.html was tweaked in https://chromium-review.googlesource.com/c/chromium/src/+/4842691 which is, I believe, for one of the changes for nested cases.

@skobes-chromium
Copy link

The new cases in changing-scroll-snap-align.html are

  • "Changing a large target's snap alignment shouldn't make the scroller resnap if the scroller is already in a valid snap position"
  • "Changing the current (non-covering) target's snap alignment should make the scroller snap according to the new alignment"

At a glance these both seemed reasonable to expect within the existing spec so I did not move them into .tentative.

@hiikezoe
Copy link
Author

hiikezoe commented Dec 3, 2023

If the test change was unrelated to the nest fix, that's quite unfortunate that it was done along with the fix. :/

Moreover as I commented, to me the test change isn't compliant with the existing spec text, even if I have misunderstand the spec, Chrome also had misunderstood before the nested fix, Safari has misunderstood even now. That's clearly a thing needs a spec issue to make the text clearer?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 4, 2023
These were added for nested snap work on crbug.com/1467300, but the
behavior is subject to an open spec issue as discussed at:

web-platform-tests/interop#602

Bug: 1505893
Change-Id: Iaf9783f7220acc5eeb19387e57f6ec3f88bc19e2
aarongable pushed a commit to chromium/chromium that referenced this issue Dec 5, 2023
These were added for nested snap work on crbug.com/1467300, but the
behavior is subject to an open spec issue as discussed at:

web-platform-tests/interop#602

Bug: 1505893
Change-Id: Iaf9783f7220acc5eeb19387e57f6ec3f88bc19e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5085089
Reviewed-by: David Awogbemila <[email protected]>
Commit-Queue: Steve Kobes <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1233355}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 5, 2023
These were added for nested snap work on crbug.com/1467300, but the
behavior is subject to an open spec issue as discussed at:

web-platform-tests/interop#602

Bug: 1505893
Change-Id: Iaf9783f7220acc5eeb19387e57f6ec3f88bc19e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5085089
Reviewed-by: David Awogbemila <[email protected]>
Commit-Queue: Steve Kobes <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1233355}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 5, 2023
These were added for nested snap work on crbug.com/1467300, but the
behavior is subject to an open spec issue as discussed at:

web-platform-tests/interop#602

Bug: 1505893
Change-Id: Iaf9783f7220acc5eeb19387e57f6ec3f88bc19e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5085089
Reviewed-by: David Awogbemila <[email protected]>
Commit-Queue: Steve Kobes <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1233355}
@foolip
Copy link
Member

foolip commented Dec 6, 2023

@hiikezoe
Copy link
Author

hiikezoe commented Dec 7, 2023

No. scroll-snap-writing-mode-000.html remains there, but I don't have any concrete evidences/argues that it should also be excluded. I just saw a comment which implies it's somewhat related to the nested change, so anyways that's fine for now.

Thank you guys, @skobes-chromium, @flackr and @foolip. I am going to close this issue.

@hiikezoe hiikezoe closed this as completed Dec 7, 2023
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 14, 2023
…-snap-align.html as tentative., a=testonly

Automatic update from web-platform-tests
Mark two test cases from changing-scroll-snap-align.html as tentative.

These were added for nested snap work on crbug.com/1467300, but the
behavior is subject to an open spec issue as discussed at:

web-platform-tests/interop#602

Bug: 1505893
Change-Id: Iaf9783f7220acc5eeb19387e57f6ec3f88bc19e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5085089
Reviewed-by: David Awogbemila <[email protected]>
Commit-Queue: Steve Kobes <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1233355}

--

wpt-commits: 7db499bf01c922eacee27edeb21db75356ee8178
wpt-pr: 43506
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Dec 16, 2023
…-snap-align.html as tentative., a=testonly

Automatic update from web-platform-tests
Mark two test cases from changing-scroll-snap-align.html as tentative.

These were added for nested snap work on crbug.com/1467300, but the
behavior is subject to an open spec issue as discussed at:

web-platform-tests/interop#602

Bug: 1505893
Change-Id: Iaf9783f7220acc5eeb19387e57f6ec3f88bc19e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5085089
Reviewed-by: David Awogbemila <awogbemilachromium.org>
Commit-Queue: Steve Kobes <skobeschromium.org>
Cr-Commit-Position: refs/heads/main{#1233355}

--

wpt-commits: 7db499bf01c922eacee27edeb21db75356ee8178
wpt-pr: 43506

UltraBlame original commit: 186934d842a247a5e2e6af07e06e4c36373d6842
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Dec 16, 2023
…-snap-align.html as tentative., a=testonly

Automatic update from web-platform-tests
Mark two test cases from changing-scroll-snap-align.html as tentative.

These were added for nested snap work on crbug.com/1467300, but the
behavior is subject to an open spec issue as discussed at:

web-platform-tests/interop#602

Bug: 1505893
Change-Id: Iaf9783f7220acc5eeb19387e57f6ec3f88bc19e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5085089
Reviewed-by: David Awogbemila <awogbemilachromium.org>
Commit-Queue: Steve Kobes <skobeschromium.org>
Cr-Commit-Position: refs/heads/main{#1233355}

--

wpt-commits: 7db499bf01c922eacee27edeb21db75356ee8178
wpt-pr: 43506

UltraBlame original commit: 186934d842a247a5e2e6af07e06e4c36373d6842
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Dec 16, 2023
…-snap-align.html as tentative., a=testonly

Automatic update from web-platform-tests
Mark two test cases from changing-scroll-snap-align.html as tentative.

These were added for nested snap work on crbug.com/1467300, but the
behavior is subject to an open spec issue as discussed at:

web-platform-tests/interop#602

Bug: 1505893
Change-Id: Iaf9783f7220acc5eeb19387e57f6ec3f88bc19e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5085089
Reviewed-by: David Awogbemila <awogbemilachromium.org>
Commit-Queue: Steve Kobes <skobeschromium.org>
Cr-Commit-Position: refs/heads/main{#1233355}

--

wpt-commits: 7db499bf01c922eacee27edeb21db75356ee8178
wpt-pr: 43506

UltraBlame original commit: 186934d842a247a5e2e6af07e06e4c36373d6842
aosmond pushed a commit to aosmond/gecko that referenced this issue Dec 16, 2023
…-snap-align.html as tentative., a=testonly

Automatic update from web-platform-tests
Mark two test cases from changing-scroll-snap-align.html as tentative.

These were added for nested snap work on crbug.com/1467300, but the
behavior is subject to an open spec issue as discussed at:

web-platform-tests/interop#602

Bug: 1505893
Change-Id: Iaf9783f7220acc5eeb19387e57f6ec3f88bc19e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5085089
Reviewed-by: David Awogbemila <[email protected]>
Commit-Queue: Steve Kobes <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1233355}

--

wpt-commits: 7db499bf01c922eacee27edeb21db75356ee8178
wpt-pr: 43506
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus area: Scrolling test-change-proposal Proposal to add or remove tests for an interop area
Projects
None yet
Development

No branches or pull requests

6 participants