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

brush: fix brush jumping around after mouseup #1836

Merged
merged 5 commits into from
May 24, 2024
Merged

Conversation

tfineberg4
Copy link
Contributor

🐛 Bug Fix

How to induce?
resize the brush window by dragging and releasing one handle quickly, or quickly drag the brush window and release (harder to induce this way)

Impact?
Handle ends up quite far away from the mouse pointer's location. Sometimes the location of both handles will be updated, resulting in a drastic resize of the brush window.

Two relevant issue have been linked below- I believe this PR should resolve both of them!

Issue 1
Issue 2

Root cause
mouseup action is supposed to disable subsequent dragging of the handle upon mousemove. This is achieved by updating the brush's state. However, when moving quickly, mousemove actions can trigger handleWindowPointerMove before the state change is complete. This leads to handle movement after mouseup. Additionally, the stale state causes the occasionally erratic placement of the handle(s).

Before

Screen.Recording.2024-05-21.at.3.08.32.PM.mov

After

Can't induce the bug anymore, even with very quick use of the brush.

Screen.Recording.2024-05-21.at.3.12.11.PM.mov

@wassgha
Copy link

wassgha commented May 21, 2024

thanks 👍

@hshoff
Copy link
Member

hshoff commented May 21, 2024

Hi @tfineberg4, thanks for the PR! The before/after videos look good. Nice work.

The fix for the root problem looks right to me, but would you mind updating this PR without bringing in the @lodash/debounce dependency? Fine to just add a local debounce function in the file itself. We try to avoid dependencies if we can to keep package size down and reduce maintenance burden.

Thank you again, excited to close these long standing issues!

@tfineberg4 tfineberg4 marked this pull request as ready for review May 22, 2024 18:15
@tfineberg4 tfineberg4 marked this pull request as draft May 22, 2024 18:19
@tfineberg4 tfineberg4 marked this pull request as ready for review May 22, 2024 19:16
@tfineberg4
Copy link
Contributor Author

Hey @hshoff !

Oops, my bad! Totally thought lodash was already a dependency here. I slipped the debounce function into utils.ts. If you don't prefer that, of course I can move it into BaseBrush.tsx.

Thanks so much for an awesome set of packages, as well as your quick response here 👍

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

+1 thanks for looking into these longstanding issues.

(side note: love your visuals and the use of brush as a ✨ range slider! 😎 )

@@ -320,6 +320,8 @@ export default class BaseBrush extends React.Component<BaseBrushProps, BaseBrush
}
};

debouncedHandleWindowPointerMove = debounce(this.handleWindowPointerMove, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

could potentially add a comment for why we need this debounce since it was such a cryptic issue before!

Copy link
Member

@hshoff hshoff left a comment

Choose a reason for hiding this comment

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

I pulled your branch down and ran the demo brush component locally and compared it to https://airbnb.io/visx/brush. Can confirm bug fixed.

Interesting to discover the bug isn't present in Safari on https://airbnb.io/visx/brush, but present in Chrome.

Fix looks good to me, thanks again for contributing to visx!

@hshoff hshoff merged commit 07e814a into airbnb:master May 24, 2024
2 checks passed
Copy link

🎉 This PR is included in version v3.10.4 of the packages modified 🎉

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

Successfully merging this pull request may close these issues.

4 participants