1534070 - scroll-padding and scroll-margin should be applied even if the scroller element's scroll-snap-type is none

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, defect, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

fantasai noticed me the fact. This should be done before shipping the new scroll snap type.

Comment 1

[fantasai]

Filed https://github.com/w3c/csswg-drafts/issues/3721 to make the spec a bit more emphatic. :)

Relevant spec prose...

scroll-padding

This property specifies <ins>(for all scroll containers, not just scroll snap containers)</ins> offsets that define the optimal viewing region of a scrollport: the region used as the target region for placing things in view of the user. This allows the author to exclude regions of the scrollport that are obscured by other content (such as fixed-positioned toolbars or sidebars) or simply to put more breathing room between a targetted element and the edges of the scrollport.

The scroll-padding property is a shorthand property that sets all of the scroll-padding-* longhands in one declaration, assigning values to the longhands representing each side exactly as the padding property does for its longhands. Values have the following meanings...

These offsets reduce the region of the viewport that is considered “viewable” for scrolling operations: they have no effect on layout, on the scroll origin or initial position, or on whether or not an element is considered actually visible, but should affect whether an element or the caret is considered scrolled into view, and reduce the amount of scrolling for paging operations (such as using the PgUp and PgDn keys or triggering equivalent operations from the scrollbar) so that within the optimal viewing region of the scrollport the user sees a continuous stream of content.

For a scroll snap container this region also defines the scroll snapport...

scroll-margin

If a page is navigated to a fragment that defines a target element (one that would be matched by :target, or the target of scrollIntoView()), the UA should use the element’s scroll snap area, rather than just its border box, to determine which area of the scrollable overflow region to bring into view, <ins>even when snapping is off or not applied on this element</ins>.”

scroll-snap-align (MUST when snapping / MAY with snapping off)

If a page is navigated to a fragment that defines a target element (one that would be matched by :target, or the target of scrollIntoView()), and that element defines some snap positions, the user agent must snap to one of that element’s snap positions if its nearest scroll container is a scroll snap container. The user agent may also do this even when the scroll container has scroll-snap-type: none.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Thanks fantasai for the summary. It's awesome (for me).

(In reply to fantasai from comment #1)

scroll-snap-align (MUST when snapping / MAY with snapping off)

If a page is navigated to a fragment that defines a target element (one that would be matched by :target, or the target of scrollIntoView()), and that element defines some snap positions, the user agent must snap to one of that element’s snap positions if its nearest scroll container is a scroll snap container. The user agent may also do this even when the scroll container has scroll-snap-type: none.

I did open a new bug, bug 1534520 for this part. It's going to introduce more complexity without removing the old scroll snap implementation.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7bdc886b9042debe65bcad28f2dfe14bfe46f90

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1534070 - Factor scroll-padding into the position calculation where nsIPresShell::ScrollContentIntoView() is going to scroll if necessary. r?botond!

In the case where scroll-snap-type is specified for the scroll container, the
scroll-padding is also factored into in ScrollFrameHelper::ComputeScrollSnapInfo
which is called via ScrollFrameHelper::ScrollToWithOrigin. It doesn't double
the scroll-padding value, but it's actually redundant, we should avoid it.
We could separate the functionality of ScrollToWithOrigin, one is to scroll
to a given element, the other is to scroll to a given position. The former will
be used for Element.scrollIntoElement and relevant stuff, the latter will be
used for Element.scrollTo and relevant stuff. That's being said, as of now, we
have still the old scroll snap implementation, so the separation will introduce
complexity, the separation should be done once after the old implementation
removed.

cssom-view/scrollIntoView-scrollPadding.html which has some tests that is
actually testing scroll-padding with scrollIntoView passes with this change.

The reftest in this change is a test case that the browser navigates to an
element with specifying the anchor to the element.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1534070 - Factor scroll-margin into the position calculation where nsIPresShell::ScrollFrameRectIntoView() is going to scroll. r?botond!

Depends on D23084

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:hiro, could you have a look please?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b723c3a2cae2
Factor scroll-padding into the position calculation where nsIPresShell::ScrollContentIntoView() is going to scroll if necessary. r=botond
https://hg.mozilla.org/integration/autoland/rev/fb721c0cbc98
Factor scroll-margin into the position calculation where nsIPresShell::ScrollFrameRectIntoView() is going to scroll. r=botond

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b723c3a2cae2
https://hg.mozilla.org/mozilla-central/rev/fb721c0cbc98