-
Notifications
You must be signed in to change notification settings - Fork 1.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
(Fixes #1214) Preventing type select in Table from highjacking textfield editing #1261
Conversation
added this state so that table could track when focus is within a cell and therefore disable typeahead until focus leaves the cell item
Build successful! 🎉 |
@@ -29,6 +29,8 @@ export function useMultipleSelectionState(props: MultipleSelection): MultipleSel | |||
// But we also need to trigger a react re-render. So, we have both a ref (sync) and state (async). | |||
let isFocusedRef = useRef(false); | |||
let [, setFocused] = useState(false); | |||
let isFocusWithinCellRef = useRef(false); | |||
let [, setFocusWithinCell] = useState(false); |
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.
Can we get away with only the ref in this case? Do we need to trigger a re-render when this property updates?
@@ -52,6 +54,13 @@ export function useMultipleSelectionState(props: MultipleSelection): MultipleSel | |||
isFocusedRef.current = f; | |||
setFocused(f); | |||
}, | |||
get isFocusWithinCell() { |
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.
Should we make the name more general than cell as it might apply to more than just tables?
Build successful! 🎉 |
Build successful! 🎉 |
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.
Assuming issues found around clicking will be addressed in a different PR, this appears to fix the space bar issue
Build successful! 🎉 |
Build successful! 🎉 |
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.
LGTM
Build successful! 🎉 |
Build successful! 🎉 |
@devongovett lemme know if I've addressed your comments sufficiently here |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
@@ -48,7 +48,7 @@ export function useTypeSelect(options: TypeSelectOptions): TypeSelectAria { | |||
|
|||
let onKeyDown = (e: KeyboardEvent) => { | |||
let character = getStringForKey(e.key); | |||
if (!character || e.ctrlKey || e.metaKey) { | |||
if (!character || e.ctrlKey || e.metaKey || selectionManager.isFocusWithinItem) { |
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.
I think this is going to be too restrictive. There are many things that could be placed inside a table cell and receive focus, e.g. a switch, a button, etc. I don't think we'd want to restrict type select in these cases. We need to make this specific to textfields. There's been some talk of an edit mode for tables which may be something we should consider.
Did you try simply stopping propagation on keyboard events inside useTextField
? Actually that's already the case via useKeyboard
, but only if you pass in an onKeyDown
/onKeyUp
handler. Makes sense in general that we only do that if there's a handler, but for text fields, we should do it always I think since the input itself has default keyboard behavior (not implemented in JS). This should also fix cursor navigation with the arrow keys while editing a textfield inside a table cell as well, just just typeahead.
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.
If I remember correctly, the problem here is that useTypeSelect
has a capturing keydown listener, but lemme try again
EDIT: yeah, the useTypeSelect
capturing keydown listener is a problem because it picks up the keydown event first (as intended). I'll go ahead and stop propagation on keydown/up in useTextField anyways though for the arrow key functionality though, good catch
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.
Only way I can think of making the current solution less restrictive is by checking a combination of tagName
and type
(and changing what gets set on selectionManager
instead of setFocusWithinItem
) when focus
fires in useGridCell
which feels kinda gross. Implementing a edit mode for the table feels a lot cleaner. I can try to implement that in this PR instead or close this and open a new one, thoughts?
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.
hmm yeah. I don't really remember why we made it a capturing listener, but seems like it has caused a bunch of issues (e.g. that useSelect issue you fixed). What breaks if we change it to not be a capturing listener?
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.
The reason useTypeSelect
uses capturing listeners is to capture the space
keydowns that happen in Listbox/Table. If a user types "Item 1" we don't want the space
keydown to select "Item 1".
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.
bump
this is so that pressing arrow keys in a textfield inside a table moves the cursor within the field rather than changing cell focus
Build successful! 🎉 |
I think we might need to wait for an edit mode in tables to do this. Mainly because right now you get stuck in the textfield and can't move to the next cell. We'll need to explicitly enter edit mode by pressing Enter, and disable all grid navigation until the user presses Enter or Escape again. See https://www.w3.org/TR/wai-aria-practices-1.2/#gridNav_inside. |
Closing in favor of a edit mode in table |
Closes #1214
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: