Skip to content

fix(ScrollControls): resolve infinite scrolling issues across browsers #2508

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kaylaa0
Copy link

@kaylaa0 kaylaa0 commented Aug 6, 2025

Why

There were a couple of issues with the current infinite scrolling behavior mentioned in #2507:

What

--- a/web/ScrollControls.js
+++ b/web/ScrollControls.js
@@ -119,9 +119,9 @@ function ScrollControls({
     if (events.connected === el) {
       const containerLength = size[horizontal ? 'width' : 'height'];
       const scrollLength = el[horizontal ? 'scrollWidth' : 'scrollHeight'];
-      const scrollThreshold = scrollLength - containerLength;
+      const scrollThreshold = Math.floor(scrollLength - containerLength);
       let current = 0;
-      let disableScroll = true;
+      let disableScroll = false;
       let firstRun = true;
       const onScroll = () => {
         // Prevent first scroll because it is indirectly caused by the one pixel offset
@@ -131,14 +131,14 @@ function ScrollControls({
         scroll.current = current / scrollThreshold;
         if (infinite) {
           if (!disableScroll) {
-            if (current >= scrollThreshold) {
+            if (current >= scrollThreshold - 1) {
               const damp = 1 - state.offset;
               el[horizontal ? 'scrollLeft' : 'scrollTop'] = 1;
               scroll.current = state.offset = -damp;
               disableScroll = true;
             } else if (current <= 0) {
               const damp = 1 + state.offset;
-              el[horizontal ? 'scrollLeft' : 'scrollTop'] = scrollLength;
+              el[horizontal ? 'scrollLeft' : 'scrollTop'] = scrollThreshold - 1
               scroll.current = state.offset = damp;
               disableScroll = true;
             }
@@ -150,13 +150,30 @@ function ScrollControls({
         passive: true
       });
       requestAnimationFrame(() => firstRun = false);
-      const onWheel = e => el.scrollLeft += e.deltaY / 2;
-      if (horizontal) el.addEventListener('wheel', onWheel, {
+      const onWheel = e => {
+        if(horizontal) {
+          const previous  = el.scrollLeft;
+          el.scrollLeft = el.scrollLeft += e.deltaY / 2;
+          if(infinite && previous  === el.scrollLeft ) {
+            onScroll();
+          }
+        } else if(infinite) {
+          if (el.scrollTop <= 1 && e.deltaY < 0) {
+            onScroll();
+          } else if (
+            el.scrollTop >= el.scrollHeight - el.clientHeight - 1 &&
+            e.deltaY > 0
+          ) {
+            onScroll();
+          }
+        }
+      };
+      if (horizontal || infinite) el.addEventListener('wheel', onWheel, {
         passive: true
       });
       return () => {
         el.removeEventListener('scroll', onScroll);
-        if (horizontal) el.removeEventListener('wheel', onWheel);
+        if (horizontal || infinite) el.removeEventListener('wheel', onWheel);
       };
     }
   }, [el, events, size, infinite, state, invalidate, horizontal, enabled]);

These are the fixes applied for each of the issues above:

  • This happened because .scrollLeft is often rounded to an integer or 0.5 precision. To ensure consistent behavior across browsers, we now explicitly floor the value so it reflects the actual maximum scrollLeft can reach. This has been tested on Chrome, Firefox, and Edge
  • disableScroll was incorrectly set to true on load. It should be false initially, since we already handle the first run using a separate mechanism. This flag is only needed later to prevent event bubbling
  • The issue was caused by setting scrollLeft to scrollLength, which is incorrect. It should instead be set to scrollThreshold - 1. The minus one acts as padding because just as wrapping backward, we don't set the scroll to 0, but to 1, so the browser triggers onScroll on the next wheel event
  • Horizontal scrolling with the mouse wheel is handled by listening to onWheel and updating scrollLeft, which normally triggers onScroll. However, if we are already at the edge, this doesn’t happen. We now manually call onScroll when at the scroll boundary to ensure wrapping
  • On Chrome, the scroll position never reaches the true maximum and always stops just short of it. Combined with inconsistent scroll handling across browsers, this caused issues in our custom infinite scroll implementation. We now listen to onWheel not only for horizontal scrolling but also during scroll wrapping. This way, when the browser fails to trigger onScroll near the edge, we can manually call it to ensure wrapping occurs

Checklist

  • Documentation updated (example)
  • Storybook entry added (example)
  • Ready to be merged

- Adjusted `.scrollLeft` to `Math.floor(scrollLength - containerLength)` to ensure consistent maximum scroll position across browsers.
- Set `disableScroll` to `false` initially to prevent unintended scroll blocking.
- Corrected scroll wrapping logic by setting `scrollLeft` to `scrollThreshold - 1` instead of `scrollLength`.
- Implemented manual `onScroll` calls at scroll edges to handle horizontal and vertical wrapping correctly.
- Addressed Chrome-specific issue where `scrollTop` was consistently one pixel short of the maximum value.

closes issue pmndrs#2507
Copy link

vercel bot commented Aug 6, 2025

@kaylaa0 is attempting to deploy a commit to the Poimandres Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

codesandbox-ci bot commented Aug 6, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

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.

1 participant