[css-scroll-snap] Add tests for scroll-into-view operations.

[fantasai]

No description provided.

[wpt-pr-bot]

There are no owners for this pull request. Please reach out on W3C's irc server (irc.w3.org, port 6665) on channel #testing (web client) to get help with this. Thank you!

[ghost]

Build PASSED

Started: 2017-12-07 01:46:30
Finished: 2017-12-07 01:55:50

View more information about this build on:

• WPT PR Status
• TravisCI

[frivoal]

Good tests, but with some bugs and typos to be fixed before merging.
Also included some (non mandatory) suggestions about improving them.

[frivoal]

I forgot to say in the review, but I think you should also have one test in the scroll-target-padding series that tests if the scroll padding correctly affects the scroll position when the caret moves, as it too is one of the things that must stay in the optimal viewing region (upgraded from a should, with the rest of that section).

[frivoal]

@fantasai ping: feedback provided. Please either update the PR or tell me why I'm wrong.

[frivoal]

(closed accidentally. Reopening)

[wpt-pr-bot]

Peer review is not required for CSS editors.

[hiikezoe]

Hey, @fantasai , those are actually pretty neat test cases, but I think some of test cases which specify both of scroll-margin and scroll-padding should only specify scroll-margin only. Because the scroll-padding doesn't affect the target position if the target element is positioned far from the scrollport edge. So I believe

.container { scroll-padding: .5em 0 0; } #target { scroll-margin: .5em 0 0; }

should be

#target { scroll-margin: 1em 0 0; }
in some tests (I did confirm on chrome too). Or both Chrome and I are misunderstanding the spec.

Also using overflow:hidden instead of overflow:auto would be nice, the scrollbar is not necessary for those test cases I believe. Showing scrollbars might cause some rendering fuzziness on Gecko, see https://bugzilla.mozilla.org/show_bug.cgi?id=1524031 for example. If Chrome supports scrollbar-width, we could of course use it.

[hiikezoe]

Hey, @fantasai , those are actually pretty neat test cases, but I think some of test cases which specify both of scroll-margin and scroll-padding should only specify scroll-margin only. Because the scroll-padding doesn't affect the target position if the target element is positioned far from the scrollport edge. So I believe

.container { scroll-padding: .5em 0 0; } #target { scroll-margin: .5em 0 0; }

should be

#target { scroll-margin: 1em 0 0; }
in some tests (I did confirm on chrome too). Or both Chrome and I are misunderstanding the spec.

Oh, no. This was totally my misunderstanding. Why Chrome doesn't work as expected? I have no idea yet.

[hiikezoe]

Nvm about my previous comments. I was just looking at scroll-target-align-00x.html test cases, scroll-target-padding-002.html works as expected.

As for scroll-padding-003.html might not work as expected, since it uses Element.focus() and the relevant spec says (block flow direction position set to a UA-defined value)[https://html.spec.whatwg.org/multipage/interaction.html#focus-management-apis:scroll-an-element-into-view].

[hiikezoe]

Ok, now I understand the reason why XX-001.html doesn't work as expected. The content height in the iframe is large enough not to scroll. I don't know there is a spec definition for this behavior. So we need overflow: auto (overflow: hidden) in iframe.

[fantasai]

Also using overflow:hidden instead of overflow:auto would be nice, the scrollbar is not necessary for those test cases I believe. Showing scrollbars might cause some rendering fuzziness on Gecko, see https://bugzilla.mozilla.org/show_bug.cgi?id=1524031 for example. If Chrome supports scrollbar-width, we could of course use it.

Added scrollbar-width: none. If Chrome doesn't support it, that's OK, unless it's causing fuzziness also on Chrome, right?

As for scroll-padding-003.html might not work as expected, since it uses Element.focus() and the relevant spec says block flow direction position set to a UA-defined value.

And the scroll-snap spec says scrolling an element into view should put its scroll snap area within the snapport. That test constrains the size of the snapport to exactly match the scroll snap area, so the UA has no discretion: there is only one position that fits the constraints.

Ok, now I understand the reason why XX-001.html doesn't work as expected. The content height in the iframe is large enough not to scroll. I don't know there is a spec definition for this behavior. So we need overflow: auto (overflow: hidden) in iframe.

Not sure what you mean. The iframe is 80px (4em) tall, and the content height is definitely taller than that.

[frivoal]

Looks good. Thanks for the fixes and for your patience.