Support text selection in Tables without row selection #1787
Conversation
…ser is clicking on table body
…ar in the Table there is an area just above the scroll bar where the click target is the Table collection where we need to stop focus from moving to the collection so scroll jumping does not happen
LFDanLu
changed the title
LFDanLu
requested review from
dannify,
devongovett and
snowystinger
as code owners
|
Build successful! 🎉 |
| // e.target.parentElement check is for Table since the scrollbar click occurs on the Table body | ||
| if (e.target.parentElement === ref.current || e.target === ref.current) { | ||
| e.preventDefault(); | ||
| } |
There was a problem hiding this comment.
Bit iffy about this since it prevents text selection on a the collection's immediate child element (e.g. imagine that the Table body was just a div with text)
There was a problem hiding this comment.
We should move the table specific case into table so it doesn't apply to other components.
|
Build successful! 🎉 |
| @@ -27,7 +27,8 @@ export interface GridRowProps<T> { | |||
| } | |||
|
|
|||
| export interface GridRowAria { | |||
| rowProps: HTMLAttributes<HTMLElement> | |||
| rowProps: HTMLAttributes<HTMLElement>, | |||
| isPressed: boolean | |||
There was a problem hiding this comment.
Would prefer not to expose this
| @@ -524,10 +525,11 @@ function TableRow({item, children, ...otherProps}) { | |||
| styles, | |||
| 'spectrum-Table-row', | |||
| { | |||
| 'is-active': isPressed && allowsSelection, | |||
There was a problem hiding this comment.
IMO it would be better to add a separate usePress within the TableRow component rather than returning isPressed through all of these hooks.
| // e.target.parentElement check is for Table since the scrollbar click occurs on the Table body | ||
| if (e.target.parentElement === ref.current || e.target === ref.current) { | ||
| e.preventDefault(); | ||
| } |
There was a problem hiding this comment.
We should move the table specific case into table so it doesn't apply to other components.
|
|
||
| // Get rid of click/drag/pointer/mousedown handlers if selection isn't allowed so that text selection can happen | ||
| if (!allowsSelection) { | ||
| pressProps = { |
There was a problem hiding this comment.
This is relying on some implementation details of usePress. Maybe it would be better to pass through everything from itemProps except for onPress?
There was a problem hiding this comment.
Just to make sure I'm on the same page: this is suggesting that I modify itemProps if !allowSelection such that it doesn't have onPress/onPressStart/onPressUp when it gets passed to usePress correct?
There was a problem hiding this comment.
If so, usePress still returns onPointerDown/onClick/etc and those handlers do things like preventDefault and disableTextSelection which stop text selection from happening
There was a problem hiding this comment.
Would setting isDisabled on usePress when allowsSelection is false work?
There was a problem hiding this comment.
Unfortunately that doesn't work without modifications to usePress. If I change the following lines in usePress so that they early return if isDisabled then this appraoch will work, but I'm unsure what ramifications that would have on overall usePress behavior
https://github.com/adobe/react-spectrum/blob/main/packages/@react-aria/interactions/src/usePress.ts#L283
https://github.com/adobe/react-spectrum/blob/main/packages/@react-aria/interactions/src/usePress.ts#L318
There was a problem hiding this comment.
Yeah that's what I thought. That's why I originally suggested doing something like this:
if (!allowsSelection) {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
let {onPressStart, onPressEnd, onPressUp, onPress, ...otherProps} = itemProps;
pressProps = otherProps;
}We basically don't want to use the pressProps at all in that case, but only the other item props. I feel like this is less reliant on implementation details from usePress and useSelectableItem than specifically copying over certain DOM props. If either of those hooks changed in the future to add additional props the current solution would break.
There was a problem hiding this comment.
Ah I see, I misunderstood this earlier. Yeah this should work, I'll update
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| @@ -203,6 +204,16 @@ export function useGridCell<T, C extends GridCollection<T>>(props: GridCellProps | |||
| }); | |||
| }; | |||
|
|
|||
| // Get rid of click/drag/pointer/mousedown handlers if selection isn't allowed so that text selection can happen | |||
| if (!allowsSelection) { | |||
| pressProps = { | |||
There was a problem hiding this comment.
Think you need to update this one as well?
There was a problem hiding this comment.
ah derp, thanks for catching
|
Build successful! 🎉 |
Closes #1706
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: