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

Add initial scroll position hook to HTML spec #10759

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

DavMila
Copy link

@DavMila DavMila commented Nov 12, 2024

scroll-initial-target is a CSS property which affects an element's initial scroll position. A scroll container's initial scroll position determines what scroll offset that scroll container should be at before any "explicit" scrolling (e.g. user gesture, programmatic scroll API) occurs on it. This initial scroll position is affected by CSS properties such as scroll-initial-target and might change as a document is loaded.

This patch adds a step in the "Updating the document" section of the HTML spec to direct user agents on when to re-evaluate initial scroll positions and adjust accordingly.

(See WHATWG Working Mode: Changes for more details.)


/acknowledgements.html ( diff )
/browsing-the-web.html ( diff )
/infrastructure.html ( diff )

A scroll container's initial scroll position determines what scroll
offset that scroll container should be at before any "explicit"
scrolling (e.g. user gesture, programmatic scroll API) occurs on it.
This initial scroll position is affected by CSS properties and may
change as a document is loaded, fetches content/stylesheets, and runs
script.

This patch adds a step in the "Updating the  document" section of the
HTML spec to direct user agents on when to re-evaluate initial scroll
positions and adjust accordingly.
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbaron would you be willing to review this as well?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry about where this integrates into the page lifecycle. It's in "update document for history step application", and occurs in both documentIsNew = true and false cases.

This means:

  • It will run for bfcache traversals. This is probably not correct; bfcache traversals should probably just leave the document as it was, instead of performing extra scroll adjustments.

  • For new documents, it will run before most of the document content is loaded. This is probably not correct; documents will generally have no scroll containers at this early stage of the process.

Can I ask how you are implementing this in Chromium, and in particular, at what point in the document lifecycle? Do you do so immediately (like this spec), and just ignore the fact that there probably will be no scrollers at this early point? Do you wait for the load or DOMContentLoaded events? Do you do periodic polling and retrying, like the spec's current "try to scroll to the fragment"?

@DavMila
Copy link
Author

DavMila commented Nov 15, 2024

  • It will run for bfcache traversals. This is probably not correct; bfcache traversals should probably just leave the document as it was, instead of performing extra scroll adjustments.
  • For new documents, it will run before most of the document content is loaded. This is probably not correct; documents will generally have no scroll containers at this early stage of the process.

Can I ask how you are implementing this in Chromium, and in particular, at what point in the document lifecycle? Do you do so immediately (like this spec), and just ignore the fact that there probably will be no scrollers at this early point? Do you wait for the load or DOMContentLoaded events? Do you do periodic polling and retrying, like the spec's current "try to scroll to the fragment"?

I think you're right that the placement of the step is not correct. I ought to have placed the step within the "If documentIsNew is true" block, and I've done so now. The intent is for it to have similar timing to (but lower priority than) "try to scroll to the fragment" so that user agents treat late-arriving scroll containers and/or their scroll-start-target elements the same way as late-arriving fragments which I believe would still be scrolled to.

@domenic
Copy link
Member

domenic commented Nov 18, 2024

The issue is that "try to scroll to the fragment" does a periodic polling operation, which your spec does not. Your spec only tries once, at initial document load, at which point no scrollers will exist.

Again, can I ask how you implemented this in Chromium?

@DavMila
Copy link
Author

DavMila commented Nov 20, 2024

The issue is that "try to scroll to the fragment" does a periodic polling operation, which your spec does not. Your spec only tries once, at initial document load, at which point no scrollers will exist.

Ah thanks, that's clear to me now.

Again, can I ask how you implemented this in Chromium?

The implementation in Chromium does periodically poll like you asked. I guess the hook I'm adding should follow a similar pattern to "try to scroll to the fragment"?
Perhaps I should:

  • keep the placement of the new step ("update the initial scroll positions for scroll containers") in "If documentIsNew is true", and
  • modify the new step to periodically poll each scroll container until the user agent believes the user is no longer interested in the initial scroll position.

Let me know if this sounds reasonable or there's a totally different better approach, thanks!

@domenic
Copy link
Member

domenic commented Nov 21, 2024

That strategy does sound reasonable, although I wonder if you should merge the algorithm with "try to scroll to the fragment" so that they're not competing with each other. Again, I wonder what you do in Chromium; do you have two separate periodic polling algorithms running, one for fragment and one for scroll containers? Or just one?

@DavMila
Copy link
Author

DavMila commented Nov 25, 2024

That strategy does sound reasonable, although I wonder if you should merge the algorithm with "try to scroll to the fragment" so that they're not competing with each other. Again, I wonder what you do in Chromium; do you have two separate periodic polling algorithms running, one for fragment and one for scroll containers? Or just one?

In Chromium we poll separately for scrolling to the fragment and scroll containers. And we treat scrolling to the fragment as an "explicit scroll" (like a scroll gesture) as a way of giving it precedence over initial-scroll-position-affecting properties like scroll-start-target.
So a scroll to a fragment would be a reason why the user agent "believes the user is no longer interested in the initial scroll position" (like with a gesture scroll).

@DavMila DavMila requested a review from domenic December 2, 2024 15:48
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@DavMila DavMila requested a review from domenic December 6, 2024 19:16
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@DavMila DavMila requested a review from domenic December 9, 2024 14:34
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We can merge after all the checkboxes in the OP are filled out.

Can you be sure that the following are tested in the web platform tests?

  • That scrollers inside shadow trees are included
  • That the order is shadow-including tree order, not e.g. flat tree order

@domenic domenic added addition/proposal New features or enhancements topic: navigation integration Better coordination across standards needed labels Dec 10, 2024
@skobes-chromium
Copy link

@domenic is there a reason you think we should avoid using flat tree order?

@domenic
Copy link
Member

domenic commented Jan 11, 2025

I'm not sure what the best order is, but the order specified in this PR is shadow-including tree order, so that's what I would expect to be tested.

@DavMila
Copy link
Author

DavMila commented Jan 13, 2025

Learned a bit here :) ... I now realize that the difference between flat tree order and shadow-including order is not whether the shadow DOM is included. Rather, it relates to the order of slot-related elements.

I think what we want here is flat tree order since it matches the top-level DOM tree, includes the shadow DOM and traverses through slots in a way that more closely matches what is displayed/layout. I've updated the PR.

@DavMila DavMila requested a review from domenic January 13, 2025 19:06
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 15, 2025
Per request[1] from HTML spec reviewers, this adds a test that scroll
containers in shadow trees also honor their scroll-initial-targets.

[1] whatwg/html#10759 (review)

Bug: 40909052
Change-Id: I5ff8029dbe043ebdcf3cc18473e014140247c0e3
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 15, 2025
Per request[1] from HTML spec reviewers, this adds a test that scroll
containers in shadow trees also honor their scroll-initial-targets.

[1] whatwg/html#10759 (review)

Bug: 40909052
Change-Id: I5ff8029dbe043ebdcf3cc18473e014140247c0e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6165203
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: David Awogbemila <awogbemila@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1406768}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 15, 2025
Per request[1] from HTML spec reviewers, this adds a test that scroll
containers in shadow trees also honor their scroll-initial-targets.

[1] whatwg/html#10759 (review)

Bug: 40909052
Change-Id: I5ff8029dbe043ebdcf3cc18473e014140247c0e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6165203
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: David Awogbemila <awogbemila@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1406768}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 15, 2025
Per request[1] from HTML spec reviewers, this adds a test that scroll
containers in shadow trees also honor their scroll-initial-targets.

[1] whatwg/html#10759 (review)

Bug: 40909052
Change-Id: I5ff8029dbe043ebdcf3cc18473e014140247c0e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6165203
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: David Awogbemila <awogbemila@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1406768}
@domenic
Copy link
Member

domenic commented Jan 17, 2025

The spec is still using shadow-including tree-order though to iterate.

Unlike "shadow-including tree order", which has a clear definition we can link to, I don't see a similar thing for the flat tree. I did find one instance in https://html.spec.whatwg.org/#focus-processing-model which says

pre-order, depth-first traversal of the flat tree

so maybe we can say something similar like

For each scroller of scrollers, in the same order in which they appear during a pre-order, depth-first traversal of the flat tree.

Or we could change the step that assembles scrollers, and then just use the normal list order? Something like this:

Let scrollers be the list obtained by performing a pre-order, depth-first traversal of document's flat tree, and filtering to keep only elements that establish a scrolling box.

For each scroller of scrollers:

Of course, this is all assuming that pre-order, depth-first traversal is the order you want.


Also, it unfortunately looks like a merge conflict came into effect over the holidays. If you could try to resolve that, it would be appreciated; if it's giving you trouble though you can leave it to the editors.

David Awogbemila and others added 9 commits January 20, 2025 10:11
Step should happen within "If documentIsNew" block like
fragment-scroll.
There might be no scroll containers the first time the algorithm runs,
so we need to keep polling until the user agent believes the user is no
longer interested in the initial scroll position.
The search for scroll containers includes shadow trees, so the list
traversal order should account for shadow trees.

Adds a note to clarify the recursive nature of "update initial scroll
positions for scroll containers" as it differs from "try to scroll to
the fragment."
@DavMila
Copy link
Author

DavMila commented Jan 20, 2025

Or we could change the step that assembles scrollers, and then just use the normal list order? Something like this:

Let scrollers be the list obtained by performing a pre-order, depth-first traversal of document's flat tree, and filtering to keep only elements that establish a scrolling box.
For each scroller of scrollers:

Thanks for the suggestion. Yes, pre-order depth-first traversal, which is tree order, is what we want. I've adopted this suggestion.

Also, it unfortunately looks like a merge conflict came into effect over the holidays. If you could try to resolve that, it would be appreciated; if it's giving you trouble though you can leave it to the editors.

I believe I've been able to fix the conflicts.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 20, 2025
…for scroll-initial-target, a=testonly

Automatic update from web-platform-tests
[css-scroll-snap-2] Add shadow DOM test for scroll-initial-target

Per request[1] from HTML spec reviewers, this adds a test that scroll
containers in shadow trees also honor their scroll-initial-targets.

[1] whatwg/html#10759 (review)

Bug: 40909052
Change-Id: I5ff8029dbe043ebdcf3cc18473e014140247c0e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6165203
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: David Awogbemila <awogbemila@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1406768}

--

wpt-commits: d3a159497499ebf65e5915d536c0379264eb0669
wpt-pr: 50101
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, nice work, this looks good! Please ping me when all the boxes in the OP are checked, and we can get this merged.

i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jan 24, 2025
…for scroll-initial-target, a=testonly

Automatic update from web-platform-tests
[css-scroll-snap-2] Add shadow DOM test for scroll-initial-target

Per request[1] from HTML spec reviewers, this adds a test that scroll
containers in shadow trees also honor their scroll-initial-targets.

[1] whatwg/html#10759 (review)

Bug: 40909052
Change-Id: I5ff8029dbe043ebdcf3cc18473e014140247c0e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6165203
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: David Awogbemila <awogbemila@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1406768}

--

wpt-commits: d3a159497499ebf65e5915d536c0379264eb0669
wpt-pr: 50101
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements integration Better coordination across standards needed topic: navigation
Development

Successfully merging this pull request may close these issues.

4 participants