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

Improve responsiveness of the floating text format toolbar buttons #4298

Merged

Conversation

ChronicLynx
Copy link
Contributor

Introduced a movement threshold so button clicks with slight motion to them aren't filtered out. The current behavior will ignore all clicks to the toolbar with even the slightest of mouse movement, causing the toolbar to feel unresponsive.

This was broken with the fix introduced in #3959. My fix should help to improve the feel of the buttons while still mitigating the toolbar flashes on drag to highlight.

Current Behavior:

Screen.Recording.2023-04-10.at.3.14.07.PM.mov

Fixed Behavior:

Screen.Recording.2023-04-10.at.3.13.07.PM.mov

@vercel
Copy link

vercel bot commented Apr 10, 2023

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

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 10, 2023 9:22pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 10, 2023 9:22pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 10, 2023
@zurfyx
Copy link
Member

zurfyx commented Apr 11, 2023

Thank you, mind expanding on the reasoning behind a px threshold vs checking whether mouse down + up happen within the same element?

@ChronicLynx
Copy link
Contributor Author

Thank you, mind expanding on the reasoning behind a px threshold vs checking whether mouse down + up happen within the same element?

Nothing in particular, I'm sure both ways would work just fine.

@acywatson
Copy link
Contributor

checking whether mouse down + up happen within the same element?

Could you hypothetically have different mousedown and mouseup targets though? Like an icon nested in a button? Haven't tested, just wondered if that would work as reliably. @zurfyx

@zurfyx
Copy link
Member

zurfyx commented Apr 22, 2023

checking whether mouse down + up happen within the same element?

Could you hypothetically have different mousedown and mouseup targets though? Like an icon nested in a button? Haven't tested, just wondered if that would work as reliably. @zurfyx

My hunch is that the target should be the most realiable approach, but we would check for the a common parent in this case.

That said, I think that what @ChronicLynx is proposing is valid and it fixes an existing problem so I'll proceed to merge this. Thank you!

@zurfyx zurfyx merged commit 1be6422 into facebook:main Apr 22, 2023
42 of 43 checks passed
This was referenced May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants