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

Normalize delta values on platforms where event.deltaX is 0 even as event.shiftKey is true #48

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

trondpet
Copy link
Contributor

Hi,

The current rsuite-table version has an issue where horizontal scroll with mousewheel + shitKey does not work as expected.

The issue can be reproduced & observed by trying to horizontally scroll tables with the mousewheel (wheel + shift key). The common behavior is to scroll horizontally when the shift key is pressed. For rsuite-table this works on some platforms (notably Mac) but not others (Win).

There's a repro case for rsuite-table (using dom-lib) here:
https://codesandbox.io/s/runtime-star-utr4bh?file=/src/App.js

And a stand-alone repro case for dom-lib showcasing the issue at:
https://codesandbox.io/s/practical-mestorf-6qpp44?file=/src/App.js

Please also ref. the screen recordings below in order to see difference in normalized delta values on Mac vs Windows. Note how the value for deltaX stays 0 on Windows, incl. when shiftKey === true -- this happens across different browsers (tested onChrome, Edge, Firefox):

dom-lib deltas on mac
dom-lib deltas on windows

Since dom-lib appears to be normalizing the deltas I'm proposing doing so here instead of in rsuite-table.
I do realize there's more risk involved in doing it here, though, and would be happy to submit a PR for rsuite-table where the values from dom-lib are used if you don't want to pull the change in here.

Thanks,
Trond

@vercel
Copy link

vercel bot commented Sep 23, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
dom-lib ❌ Failed (Inspect) Sep 27, 2022 at 3:52PM (UTC)

@@ -42,6 +43,12 @@ function normalizeWheel(event: any) {
pX = event.deltaX;
}

// on some platforms (e.g. Win10), browsers do not automatically swap deltas for horizontal scroll
if (!isSideScrolling && !event.ctrlKey && event.shiftKey && pX === 0) {
Copy link
Contributor Author

@trondpet trondpet Sep 23, 2022

Choose a reason for hiding this comment

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

Adding !isSideScrolling to try and limit any potential of backward breaking change

@@ -4001,14 +4001,20 @@
}
},
"node_modules/caniuse-lite": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@simonguo simonguo left a comment

Choose a reason for hiding this comment

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

I think it's more reasonable to handle it in WheelHandler.js, what do you think?

const normalizedEvent = normalizeWheel(event);

let normalizedEvent = normalizeWheel(event);

if(navigator.platform !== 'MacIntel' && event.shiftKey){ 
    normalizedEvent = swapWheelAxis(normalizedEvent);
 }
const swapWheelAxis = (normalizedEvent)=>{
  return {
      spinX: normalizedEvent.spinY,
      spinY: normalizedEvent.spinX,
      pixelX: normalizedEvent.pixelY,
      pixelY: normalizedEvent.pixelX,
    };
}

@trondpet
Copy link
Contributor Author

trondpet commented Sep 27, 2022

@simonguo

I think it's more reasonable to handle it in WheelHandler.js, what do you think?

Sure - that does sound reasonable and I can def. move it
(the reason I originally put it in the normalizeWheel util is that there was similar code there for "side scrolling on FF with DOMMouseScroll", but I don't mind moving it out of course).

Re: the navigator.platform !== 'MacIntel' check; I don't mind putting that in either, but had stayed away from such checks since I don't know if it might affect other platforms & had seen it done this way in the past. If you prefer to limit the impact by adding the platform check I'm fine w/that as well. That would work for us. I've updated the PR accordingly.

Thanks for your help :)

…rizontal scroll in normalizeWheel (certain platforms)

chore: npx browserslist@latest --update-db
@simonguo simonguo merged commit ddbc120 into rsuite:master Sep 28, 2022
@github-actions
Copy link

🎉 This PR is included in version 3.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants