Skip to content

Conversation

@daun
Copy link
Member

@daun daun commented Jun 6, 2025

Description

  • Continuation of Replace scrl with a native scroll implementation #88
  • Scroll elements into view recursively (target + all scrollable parents)
  • Use compute-scroll-into-view package to ensure compliance with spec and browsers
  • Update naming around scroll targets vs. scroll containers

Playground example

Screen Recording 2025-06-08 at 11 14 03

Checks

  • The PR is submitted to the main branch
  • The code was linted before pushing (npm run lint)
  • All tests are passing (npm run test)
  • New or updated tests are included
  • The documentation was updated as required

Base automatically changed from feat/native-scrolling to next June 6, 2025 20:21
@daun
Copy link
Member Author

daun commented Jun 7, 2025

@hirasso Regarding naming:

TLDR

Let's go with scrollElement for the html/root element, scrollContainer for scrollable/overflowing elements, and scrollTarget for anchors being scrolled to.

Long rant

I tried a few different namings/spellings in this PR. Hadn't bothered to check back with you because it's still in draft. Would have written up a few notes to start a discussion. But let's discuss now :)

I found the element in scrollingElement confusing once it no longer relates to the documentElement, i.e. the absolute root element containing stuff. There is a distinction between the one mandatory element that scrolls the document (technically called the scrollingElement) and any number of possible elements with overflowing content (commonly called scroll container, if my intuition holds and if the MDN glossary is to be believed).

My confusion specifically stems from thinking that element could refer to any element participating in the scrolling, so it might (to somebody who didn't write the code) be either the containing element or the target element. Calling it a container gets rid of that ambiguity. You're right that in the swup context it opens another ambiguity, but I think tacking scroll in front of container very elegantly solves this.

Other alternatives:

  • Scrollable containers
  • Overflow containers
  • Scrollable areas

WDYT?

@hirasso
Copy link
Member

hirasso commented Jun 7, 2025

Sounds very reasonable. The scroll prefix definitely helps. Sorry for bringing this up so early, before you even asked for my review 😅

Signed-off-by: Philipp Daun <post@philippdaun.net>

# Conflicts:
#	package-lock.json
#	src/index.ts
@github-actions
Copy link

github-actions bot commented Jun 8, 2025

Playwright test results

passed  3 passed

Details

stats  3 tests across 1 suite
duration  
commit  bb62b87

daun added 3 commits June 8, 2025 10:52
Signed-off-by: Philipp Daun <post@philippdaun.net>
Signed-off-by: Philipp Daun <post@philippdaun.net>
Signed-off-by: Philipp Daun <post@philippdaun.net>
@daun daun changed the title Native scrolling details Add support for nested scroll containers Jun 8, 2025
@daun daun marked this pull request as ready for review June 8, 2025 09:21
@daun
Copy link
Member Author

daun commented Jun 8, 2025

Got nested containers working nicely now. Ready for review :)

@daun
Copy link
Member Author

daun commented Jun 8, 2025

Computing all the offsets got so frustrating and I never got the same results as the native browser implementation. Then I stumbled upon compute-scroll-into-view which is spec-compliant and does all the heavy lifting. It e.g. supports scroll-margin set on each element 😍

The package also enables easy horizontal scrolling, but I decided to add that in a separate PR in case we need to tweak any public APIs (very probable).

@daun
Copy link
Member Author

daun commented Jun 8, 2025

Updated the naming around element, container and target. And added a screen recording in the PR description.

@hirasso
Copy link
Member

hirasso commented Jun 8, 2025

Looks great! And damn - I discovered that package as well recently, but forgot to mention it here. Glad you found it yourself! I'm AFK at the moment, but will test this as soon as I get the chance.

@hirasso
Copy link
Member

hirasso commented Jun 8, 2025

Actually - this looks great and I don't want to cancel momentum. Approved! Since we are doing more work before merging this all into main, chances are we'll catch any issues before that when tackling the remaining features.

This is fun 🤩

@daun
Copy link
Member Author

daun commented Jun 8, 2025

Nice. And yeah, life on the next branch is exciting :)

@daun daun merged commit 77b187f into next Jun 8, 2025
2 checks passed
@daun daun deleted the feat/native-scrolling-details branch June 8, 2025 20:28
@daun daun mentioned this pull request Jun 8, 2025
Merged
13 tasks
@daun daun changed the title Add support for nested scroll containers Nested scroll containers Jun 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants