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.

Sign up for GitHub

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

Closed
wants to merge 19 commits into from
Closed

(Fixes #1214) Preventing type select in Table from highjacking textfield editing #1261

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
ec94d58
adding isFocusWithinCell state to selection manager
LFDanLu Nov 11, 2020
ea9e5ff
moving blur to cell instead of cell child
LFDanLu Nov 11, 2020
04f2fcd
adding test
LFDanLu Nov 12, 2020
afd899f
expand typeselect test a bit
LFDanLu Nov 12, 2020
4fe03b9
updating code review
LFDanLu Nov 12, 2020
5fca959
Merge branch 'main' into issue_1214
LFDanLu Nov 12, 2020
fc49d12
Merge branch 'main' into issue_1214
LFDanLu Nov 16, 2020
4ebf38a
Merge branch 'main' into issue_1214
LFDanLu Nov 25, 2020
469d292
Merge branch 'main' into issue_1214
LFDanLu Dec 8, 2020
605267a
Merge branch 'main' into issue_1214
LFDanLu Jan 4, 2021
dc66d44
Merge branch 'main' of https://github.com/adobe/react-spectrum into i…
LFDanLu Jan 19, 2021
f428894
Merge branch 'main' of https://github.com/adobe/react-spectrum into i…
LFDanLu Jan 26, 2021
6928ac1
Merge branch 'main' into issue_1214
LFDanLu Jan 27, 2021
f284a49
Merge branch 'main' into issue_1214
LFDanLu Feb 10, 2021
f73fc04
Merge branch 'main' into issue_1214
LFDanLu Feb 24, 2021
ae16f65
Merge branch 'main' of github.com:adobe/react-spectrum into issue_1214
LFDanLu Mar 4, 2021
5f88db9
fixing merge
LFDanLu Mar 4, 2021
4db4730
stopping propagation on keydown/up for textfields
LFDanLu Mar 15, 2021
1ea4996
adding test for keydown/up stop propagation in useTextField
LFDanLu Mar 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@react-aria/selection/src/useTypeSelect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

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.

Copy link
Member Author

@LFDanLu LFDanLu Mar 15, 2021 •
edited
Loading

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

Copy link
Member Author

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?

Copy link
Member

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?

Copy link
Member Author

@LFDanLu LFDanLu Mar 18, 2021 •
edited
Loading

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".

Copy link
Member Author

Choose a reason for hiding this comment

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

bump

return;
}

Expand Down
8 changes: 7 additions & 1 deletion packages/@react-aria/table/src/useTableCell.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export function useTableCell<T>(props: GridCellProps, state: TableState<T>): Gri
let focusable = treeWalker.firstChild() as HTMLElement;
if (focusable) {
focusSafely(focusable);
state.selectionManager.setFocusWithinItem(true);
} else {
focusSafely(ref.current);
}
Expand Down Expand Up @@ -85,9 +86,14 @@ export function useTableCell<T>(props: GridCellProps, state: TableState<T>): Gri
});
};

let onBlur = () => {
state.selectionManager.setFocusWithinItem(false);
};

let gridCellProps: HTMLAttributes<HTMLElement> = mergeProps(pressProps, {
role: 'gridcell',
onFocus
onFocus,
onBlur
});

if (isVirtualized) {
Expand Down
40 changes: 40 additions & 0 deletions packages/@react-spectrum/table/stories/Table.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {Link} from '@react-spectrum/link';
import React from 'react';
import {storiesOf} from '@storybook/react';
import {Switch} from '@react-spectrum/switch';
import {TextField} from '@react-spectrum/textfield';
import {useAsyncList} from '@react-stately/data';

let columns = [
Expand Down Expand Up @@ -731,6 +732,45 @@ storiesOf('Table', module)
</TableBody>
</Table>
)
)
.add('table with textfields and buttons',
() => (
<Table aria-label="Table" selectionMode="none">
<TableHeader>
<Column isRowHeader minWidth={200}>First Name</Column>
<Column isRowHeader>Last Name</Column>
<Column>Birthday</Column>
</TableHeader>
<TableBody>
<Row>
<Cell>Sam</Cell>
<Cell>Smith</Cell>
<Cell>May 3</Cell>
</Row>
<Row>
<Cell>Julia</Cell>
<Cell>Jones</Cell>
<Cell>February 10</Cell>
</Row>
<Row>
<Cell>John</Cell>
<Cell>Doe</Cell>
<Cell>December 12</Cell>
</Row>
<Row>
<Cell textValue="Textfield">
<TextField width={150} aria-label="table textfield" />
</Cell>
<Cell textValue="Action Button">
<ActionButton isQuiet>
<Delete />
</ActionButton>
</Cell>
<Cell>December 12</Cell>
</Row>
</TableBody>
</Table>
)
);

function AsyncLoadingExample() {
Expand Down
32 changes: 32 additions & 0 deletions packages/@react-spectrum/table/test/Table.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1130,6 +1130,13 @@ describe('Table', function () {
<Cell>Doe</Cell>
<Cell>December 12</Cell>
</Row>
<Row>
<Cell>
<input data-testid="input" />
</Cell>
<Cell>King</Cell>
<Cell>December 12</Cell>
</Row>
</TableBody>
</Table>
);
Expand Down Expand Up @@ -1233,6 +1240,31 @@ describe('Table', function () {
moveFocus('S');
expect(document.activeElement).toBe(getCell(tree, 'Sam'));
});

it('doesn\'t trigger typeselect if focus is within a element in a cell', function () {
let tree = renderTypeSelect();
let textfield = tree.getByTestId('input');
let textfieldCell = textfield;

// Find the cell wrapping the input so we can focus it
while (textfieldCell && !/gridcell|rowheader|columnheader/.test(textfieldCell.getAttribute('role'))) {
textfieldCell = textfieldCell.parentElement;
}
act(() => textfieldCell.focus());
// Focus moves to the inner child when cell is focused
expect(document.activeElement).toBe(textfield);
typeText(document.activeElement, 'John ');

// Focus shouldn't have moved to the John cell and all the text should be registered
expect(textfield.value).toBe('John ');
expect(document.activeElement).toBe(textfield);

// Moving out to a focusable cell should restore typeselect
moveFocus('ArrowUp');
expect(document.activeElement).toBe(getCell(tree, 'John'));
moveFocus('S');
expect(document.activeElement).toBe(getCell(tree, 'Sam'));
});
});

describe('focus marshalling', function () {
Expand Down
14 changes: 14 additions & 0 deletions packages/@react-stately/selection/src/SelectionManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,20 @@ export class SelectionManager implements MultipleSelectionManager {
this.state.setFocused(isFocused);
}

/**
* Whether focus is currently within a cell in the collection.
*/
get isFocusWithinItem(): boolean {
return this.state.isFocusWithinItem;
}

/**
* Sets whether focus is currently within a cell in the collection.
*/
setFocusWithinItem(isFocused: boolean) {
this.state.setFocusWithinItem(isFocused);
}

/**
* The current focused key in the collection.
*/
Expand Down
4 changes: 4 additions & 0 deletions packages/@react-stately/selection/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ export interface FocusState {
readonly isFocused: boolean,
/** Sets whether the collection is focused. */
setFocused(isFocused: boolean): void,
/** Whether focus is currently within a cell in the collection. */
readonly isFocusWithinItem: boolean,
/** Sets whether focus is currently within a cell in the collection. */
setFocusWithinItem(isFocused: boolean): void,
/** The current focused key in the collection. */
readonly focusedKey: Key,
/** Sets the focused key. */
Expand Down
7 changes: 7 additions & 0 deletions packages/@react-stately/selection/src/useMultipleSelectionState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ 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 focusedKeyRef = useRef(null);
let [, setFocusedKey] = useState(null);
let selectedKeysProp = useMemo(() => convertSelection(props.selectedKeys), [props.selectedKeys]);
Expand All @@ -52,6 +53,12 @@ export function useMultipleSelectionState(props: MultipleSelection): MultipleSel
isFocusedRef.current = f;
setFocused(f);
},
get isFocusWithinItem() {
return isFocusWithinCellRef.current;
},
setFocusWithinItem(f) {
isFocusWithinCellRef.current = f;
},
get focusedKey() {
return focusedKeyRef.current;
},
Expand Down