-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix FormTokenField suggestions broken scrollbar when __experimentalExpandOnFocus
is defined
#56426
Conversation
I have this obtuse typescript error that I'll need some help resolving!
|
Just wanted to let you know that with this PR, #50514 can also be closed. |
This tested well for me. I pushed a fix for the type issue, but probably needs someone with more experience with the components typing to take a look at it. It seems like the onFocus method is expecting the native |
At a quick glance, the focus and key down/up handlers are applied to the wrapping |
__experimentalExpandOnFocus
is defined__experimentalExpandOnFocus
is defined
f9a413c
to
bbfd447
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for finding this bug and working on a solution! It's interesting that the only way to reproduce seems to be to click and drag the scrollbar — using the keyboard or the mousewheel seems to work fine.
Anyway, the changes are testing well for me (tested in Chrome/Safari/Firefox on MacOS).
I have this obtuse typescript error that I'll need some help resolving!
This definitely bit me in the past — FocusEvent
and React.FocusEvent
!
It seems like the onFocus method is expecting the native FocusEvent but onBlur the react event version - but not sure why this is.
I took a look and I believe that both the onFocus
prop and the onFocusHandler
also should be typed using ReactFocusEvent
(instead of the native one). I logged the event
object and it definitely looks like a react synthetic event.
bbfd447
to
8281f8d
Compare
Thanks for the feedback Marco, good catch! I've updated that now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested well for me, and scrolling categories works with scroll wheel and when dragging scrollbar.
Looks good to go once changelog conflict resolved.
…lExpandOnFocus` is defined
7d8f583
to
910a69f
Compare
Fixes #56419
What?
Fixes an issue with
FormTokenField
whereby when the__experimentalExpandOnFocus
prop is defined, the suggestions list can't be scrolled by dragging it with the mouse.How?
The issue seemed to be that the
isExpanded
prop was being repeatedly switched betweentrue
andfalse
. This remounting of the suggestions was causing the scroll position to be fixed in place.Notably the
onFocusHandler
handler checks both input focus and container focus, while theonBlur
handler didn't, so the fix was to replicate that inonBlur
to preventisExpanded
being set tofalse
.(note: I'm not sure if this is a good enough explanation, as I don't know why it works ok normally 😄 ).
Testing Instructions
__experimentalExpandOnFocus
Testing Instructions for Keyboard
I don't think this is reproducible using the keyboard
Screenshots or screencast
Before
Kapture.2023-11-22.at.18.18.29.mp4
After
Kapture.2023-11-22.at.18.14.58.mp4