Skip to content

Conversation

@snowystinger
Copy link
Member

Closes #1463

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Go to new story path=/story/textfield--in-a-scrolling-container and place the cursor in the start/middle/end. Press Home/End key once and twice.
If cursor is already at the start, Home will cause container scroll.
If cursor is already at the end, End will cause container scroll.
If cursor is anywhere else, cursor will move to the start or end when home or end is pressed, respectively. After that, a second tap will cause the container scroll.

🧢 Your Project:

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

Tested the story, confirmed that the fn + left/right (home/end) only scrolled the container if the cursor was already at the start/end, LGTM

Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

The code change looks good and the behavior works as described.
I tried to figure out what other components might inherit this and that ones that did, numberfield and colorfield have their own custom behaviors.
TextArea does not have this behavior, should it? Should TextArea have it's own custom page up or page down behavior? I'm thinking of someone using one in a texteditor, but in that case that should handle it themselves, right?

@devongovett
Copy link
Member

Hmm, not sure if we should be doing this on all textfields... it goes against default browser behavior for inputs. Should this be for combobox specifically?

@devongovett devongovett added the waiting Waiting on Issue Author label Mar 18, 2021
@snowystinger
Copy link
Member Author

snowystinger commented Mar 24, 2021

@ktabors i see the same behavior in both https://jsfiddle.net/snowystinger/4cfehau3/9/
@devongovett agreed, it would be different, but why would we do it to combobox if we don't do it to any others? If we're only doing it in combobox, i'd say let them all be the same
except, we do support onKeyDown in TextField and TextArea, but we don't in ComboBox, so anyone trying to do this would be unable to

I see it as we either need to enable onKeyDown in ComboBox and NumberField or we need to put this preventDefault logic in all of them

here's how our components currently behave same as native https://codesandbox.io/s/admiring-silence-wiq3z?file=/src/App.js

@snowystinger snowystinger removed the waiting Waiting on Issue Author label Apr 5, 2021
@ktabors
Copy link
Member

ktabors commented Apr 10, 2021

@snowystinger I think with my comment about TextArea I was trying to figure out how our component should behave and if it would your change would change how TextArea works.

Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

LGTM not that it isn't waiting. :)

@LFDanLu LFDanLu mentioned this pull request Sep 29, 2021
5 tasks
@snowystinger
Copy link
Member Author

Closing as there are no issues reported about this and it goes against native.
We could consider again for Grid edit mode or specifically for ComboBox

@snowystinger snowystinger deleted the fix-home-key-in-textfields branch April 7, 2022 21:53
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.

[combobox] behavior of Home and End keys causes scrolling

6 participants