Refactor listview to always focus the row by LFDanLu · Pull Request #3000 · adobe/react-spectrum

LFDanLu · 2022-04-01T22:35:08Z

Closes #2946
Alternate approach from #2993, mainly to avoid adding more complexity to useGridCell

This approach essentially makes the outer ListView div behave as if it is the gridcell so we can easily navigate to the focusable children but still applies gridrow specific labeling to it. The inner gridcell div becomes decorative mainly

✅ 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:

Test the ListView stories in all browsers/AT combinations. Test that tab/keyboard navigation always focuses the outer row element and that focus is never set onto the inner grid cell. Contents of the row should NOT be announced all at once when focusing a row (may depend on screen reader), the row's text content should be announced specifically.
All other previous behavior should be the same (selection, navigation, etc)

🧢 Your Project:

RSP

Felt that the previous modifications to GridKeyboardDelegate were too specific to ListView so split out the changes. Also fixes pageUp/Down/Home/End when you are focused on the ListView row (we forgot to pass the layout to the delegate previously)

needed to override getItemRect for the ListView keyboard delegate so that it always uses the row key, namely due to the gridcell keys being unavailable in the listlayout. Changed getItem and getItemRect to protected funcs in GridKeyboardDelegate so I could do that

still need to fix pageUp/down

needed to override getKeyLeftOf/getKeyRightOf so they dont use getFirstKey or getLastKey or else focus would jump to the last row instead of wrapping inside the cell

LFDanLu · 2022-04-01T23:00:46Z

  });
  let layout = useListLayout(state, props.density || 'regular');
-  let keyboardDelegate = useMemo(() => new GridKeyboardDelegate({
+  let keyboardDelegate = useMemo(() => new ListGridKeyboardDelegate({


Note that the focusMode of the gridState and the keyboard delegate is still cell, I think I could change it to row but it was just easier to go ahead and delete onFocus from useGridRow rather than from useGridCell. The onFocus from useGridCell has some additional logic merged in from useSelectableItem

LFDanLu · 2022-04-04T17:15:20Z

+  "version": "3.0.0-alpha.1",
+  "private": true,


I forget, should I directly modify this or will the version/private be updated upon publish?

LFDanLu · 2022-04-04T17:16:30Z

+      ref={rowRef}
+      aria-label={item.textValue}>
      <div
+        // TODO: refactor the css here now that we are focusing the row?


Will be looking at refactoring the css after a couple of other ListView PRs w/ heavy css updates are merged

LFDanLu · 2022-04-04T17:18:00Z

+import {Key} from 'react';
+import {Rect} from '@react-stately/virtualizer';
+
+export class ListGridKeyboardDelegate<T> extends GridKeyboardDelegate<T, GridCollection<T>> {


Open to name suggestions

What are the differences between this and ListKeyboardDelegate now?

ListKeyboardDelegate will skip over disabled items whereas this one doesn't. Also handles the getKeyLeftOf/getKeyRightOf that useGridCell calls which ListKeyboardDelegate doesn't

hmm perhaps we could add getKeyLeftOf/getKeyRightOf to ListKeyboardDelegate? I don't really see why adding them would break anything. And we could possibly add an option to include disabled items too, that seems useful for other types of lists in some cases as well.

My main concern with this as currently implemented is that it's not really a grid, so we're having to override a lot of things. Easy to break later if we change the behavior of grid in some way, and then also need to override that here. Also simplifies the API to have just one ListKeyboardDelegate.

Sure I can go ahead and try modifying ListKeyboardDelegate instead. Only concern is if some grid keyboard delegate logic will spill over

I've done some poking around and have come up with f2ef4ef, which leverages ListLayout as the keyboard delegate instead of the new ListGridKeyboardDelegate. I feel a bit iffy about it and it still has some finicky behavior that I can't resolve without bringing in some grid specific logic into ListLayout.

Pain points:

An effort has been made to make the tracked focusedKey always be a row key. However, due to the nature of useGridCell, it is possible for the focusedKey to be a cell key (e.g. if a user clicks on a row button), resulting in some broken behavior since the ListLayout wasn't designed to handle such a distinction. If I get rid of the onFocus from useGridCell that would fix the problem of focusedKey being set to a cell key but would then mean focus wouldn't update properly if the user clicks on said row buttons. Focus would also get highjacked from a menu button after closing the menu and returned onto the row since useGridRow doesn't have it's own dedicated focus func that handles these kinds of situations.

the list collection only has access to the row keys, not the cell keys. This makes using ListKeyboardDelegate/ListLayout tricky since they don't know what to do with the cell keys unless I modify those functions so they always try to grab the row key from the provided grid collection

This mixture of using a grid collection with layouts/delegates that don't have a concept of row vs cell is pretty painful in general

ListKeyboarddDelegate makes an assumption that the component isn't virtualized and uses query selectors to find row height information

I found the old approach w/ the new keyboard delegate to be a bit easier to navigate since it was extending off the GridKeyboardDelegate and I was able to ensure the focusedKey would always be the cell key meaning there were less pitfalls

LFDanLu · 2022-04-04T19:21:40Z

@@ -0,0 +1,31 @@
+{
+  "name": "@react-aria/list",
+  "version": "3.0.0-alpha.1",


Removed the private since it was failing lint (published package uses this). Lemme know if I should bump this version to 3.0.0

adobe-bot · 2022-04-04T19:26:51Z

Build successful! 🎉

LFDanLu · 2022-04-04T19:27:04Z

+  let stately = `@react-stately/${basename}`;
+  let types = `@react-types/${basename}`;
+
+  if (scope === '@react-spectrum' && isDepUsed(aria, globSrc)) {


Updated this lint step since it was complaining that I wasn't importing react-stately/list in react-aria/list even though react-stately/list wasn't used in react-aria/list at all. Now it checks that @react-aria/${basename}/@react-stately/${basename}/@react-types/${basename} is actually being imported somewhere in the package's files before asserting that it exists in the package.json.

Open to suggestions on the changes here and/or I've misunderstood the purpose of these checks

how was this not a problem before? i know we have many aria packages that don't use a stately package

dunno, I'd have to go through the aria packages we have to check if this is the first instance of this problem or not. If it isn't then I'm mystified why it was caught here specifically

Ah right, so the reason why this wasn't a problem was because of this part of the check being passed to the softAssert: !pkgNames[stately] || json.dependencies[stately]. For a package like @react-aria/accordion, the first part will resolve to true since the @react-stately/accordion package doesn't exist (aka pkgNames doesn't contain @react-stately/accordion). However, for the newly introduced @react-aria/list package (ListView specific), @react-stately/list does exist so it expects it to be in the dependencies even though it isn't being used.

I could rename the listview package name to be listview instead of list which should side step this problem but it kinda breaks the previous naming conventions (e.g. TableView's package is table). Also I think that the update to this script still makes sense, doesn't feel like the previous logic was making a correct assumption.

yep, thanks for looking into that! i think when I wrote that I assumed naming parity
so maybe we should still reconsider the name of the package, however, the change you made also makes sense

snowystinger · 2022-04-04T21:16:15Z

+  let stately = `@react-stately/${basename}`;
+  let types = `@react-types/${basename}`;
+
+  if (scope === '@react-spectrum' && isDepUsed(aria, globSrc)) {


how was this not a problem before? i know we have many aria packages that don't use a stately package

adobe-bot · 2022-04-05T00:31:26Z

Build successful! 🎉

devongovett · 2022-04-27T15:56:14Z

One issue I see: In the dynamic items story, if you focus one of the buttons in the first row, tab out of the list view and shift tab back in, it focuses a different row rather than the first one. Didn't seem to happen previously. Seems like it's going to the last one that is visible.

LFDanLu · 2022-04-27T16:41:09Z

Strange, doesn't seem to be consistent either. Maybe something with useSelectableCollection and how focuses the last focusable element + GridCell behavior? I'll dig

…listview_row_focus_2

adobe-bot · 2022-04-27T20:21:15Z

Build successful! 🎉

LFDanLu · 2022-04-27T21:20:13Z

@devongovett think I've narrowed it down to it being specifically a story issue since tabbing out of the ListView moves you out of the iframe but I'll go into the details.

So the code that is causing the last visible row to be focused is

react-spectrum/packages/@react-aria/grid/src/useGridCell.ts

Lines 206 to 208 in a7642bf

    
           if (!isFocusVisible()) { 
        
             state.selectionManager.setFocusedKey(node.key); 
        
           }

. Due to standard browser behavior, shift tabbing back into the ListView will always focus the last focusable element (aka the last button in the last row) before attempting to move focus to the tracked focusedKey. Typically useGridCell's onFocus will ignore the focus event that happens if you are in keyboard modality, which for the most part works out for us when the user tabs/shift-tabs into the ListView since we have a document level keyboard listener that shift modality to "keyboard".

For this story however, tabbing out of the ListView will bring you out of the iframe. If you then move the pointer within the story iframe, modality will shift to "pointer". The subsequent Shift + Tab operation to bring focus back into the ListView happens outside of the iframe and thus the event listener responsible for shifting back to keyboard modality doesn't fire, resulting in useGridCell setting the focusedKey to the last rendered row since isFocusVisible resolves to false.

Here are some 100% reproduction steps (TableView story since it isn't just for ListView):

Navigate to https://reactspectrum.blob.core.windows.net/reactspectrum/2e88e5c9f2feb6aea265db1cfbdb3125cbff1699/storybook/index.html?path=/story/tableview--dynamic-with-selection
Tab into the table. Note what row is focused
Tab out of the table.
Move you mouse around in the story to shift modality to 'pointer'
Shift tab back into the table. Note that the row 7 checkbox is focused.

TLDR: This is pre-existing behavior and only happens if your focus leaves the document entirely + you shift into pointer modality before tabbing back into the ListView. It won't typically happen if you have focusable elements preceding and following the ListView/TableView.
I can add input fields before and after each ListView in the stories if you'd like: 3f7f3d8

helps avoid weird test cases like #3000 (comment)

…listview_row_focus_2

this makes it so the listview doesnt get squished on mobile

adobe-bot · 2022-04-29T21:57:59Z

Build successful! 🎉

adobe-bot · 2022-05-03T16:43:27Z

Build successful! 🎉

adobe-bot · 2022-05-04T18:45:27Z

Build successful! 🎉

adobe-bot · 2022-05-04T18:49:40Z

Build successful! 🎉

adobe-bot · 2022-05-04T21:36:42Z

Build successful! 🎉

* initial work to focus the row instead of the cell * cleanup * tentative approach to always skip focusing the cell * retaining focus on Listview focusable child when it is clicked * making ListView specific keyboard delegate Felt that the previous modifications to GridKeyboardDelegate were too specific to ListView so split out the changes. Also fixes pageUp/Down/Home/End when you are focused on the ListView row (we forgot to pass the layout to the delegate previously) * fixing pageUp/Down operations when within the ListView row needed to override getItemRect for the ListView keyboard delegate so that it always uses the row key, namely due to the gridcell keys being unavailable in the listlayout. Changed getItem and getItemRect to protected funcs in GridKeyboardDelegate so I could do that * adding tests * fixing tests after rebase * refactor to have outer div behave as both gridcell and grid row still need to fix pageUp/down * fixing Home/End needed to override getKeyLeftOf/getKeyRightOf so they dont use getFirstKey or getLastKey or else focus would jump to the last row instead of wrapping inside the cell * fixing test and getting rid of erroneous data id * cleanup * lint * removing todo * updating lint so we check that a dep is actually needed before throwing * removing private * pulling triggerPress outside the act * tentative approach to use ListLayout instead of making a ListGrid keyboard delegate * fixing build * removing useGridRow to ensure the focused key will always be a row also reverts changes to other files that arent necessary anymore. Adds tests for disabled keys * cleanup * adding listview story decorator for before and after focusable elements helps avoid weird test cases like #3000 (comment) * adding tests for refactor * creating list aria and types packages * adding useListSelectionCheckbox and exporting types * adding tests for rowindex and colindex * replacing grid hooks with list hooks also clean up and fixes from testing * replace useSelectableCollection call with useSelectableList * fixing docs build? * cleanup and fixing docs build again * making the story input fields only appear at certain screen widths this makes it so the listview doesnt get squished on mobile * addressing review comments * fixing dep version * fixing test failures * ignoring type for collection node for now * small story fix found when testing screen reader announcements

LFDanLu added 12 commits March 31, 2022 14:46

initial work to focus the row instead of the cell

3580959

cleanup

e51bd3f

tentative approach to always skip focusing the cell

928549a

retaining focus on Listview focusable child when it is clicked

2216e98

adding tests

2ebabe1

fixing tests after rebase

2a92a70

refactor to have outer div behave as both gridcell and grid row

312aaed

still need to fix pageUp/down

fixing Home/End

dd9dac5

needed to override getKeyLeftOf/getKeyRightOf so they dont use getFirstKey or getLastKey or else focus would jump to the last row instead of wrapping inside the cell

fixing test and getting rid of erroneous data id

17a11bd

cleanup

d9eb366

LFDanLu commented Apr 1, 2022

View reviewed changes

Comment thread packages/@react-spectrum/list/src/ListViewItem.tsx

lint

303304c

LFDanLu commented Apr 4, 2022

View reviewed changes

removing todo

0fe13e6

LFDanLu changed the title ~~(Approach 2) Refactor listview to always focus the row~~ Refactor listview to always focus the row Apr 4, 2022

LFDanLu mentioned this pull request Apr 4, 2022

(WIP) Refactor listview to always focus the row #2993

Closed

5 tasks

LFDanLu commented Apr 4, 2022

View reviewed changes

LFDanLu added 2 commits April 4, 2022 12:12

updating lint so we check that a dep is actually needed before throwing

b4a4f40

removing private

11302a3

LFDanLu commented Apr 4, 2022

View reviewed changes

snowystinger reviewed Apr 4, 2022

View reviewed changes

pulling triggerPress outside the act

c9ba923

Merge branch 'main' into refactor_listview_row_focus_2

db7fbe7

LFDanLu mentioned this pull request Apr 26, 2022

Refactor TagGroup to focus row instead #3070

Closed

Merge branch 'main' of github.com:adobe/react-spectrum into refactor_…

e659966

…listview_row_focus_2

LFDanLu added 3 commits April 27, 2022 17:14

adding listview story decorator for before and after focusable elements

3f7f3d8

helps avoid weird test cases like #3000 (comment)

Merge branch 'main' of github.com:adobe/react-spectrum into refactor_…

7cf09be

…listview_row_focus_2

making the story input fields only appear at certain screen widths

6c02faf

this makes it so the listview doesnt get squished on mobile

LFDanLu mentioned this pull request Apr 29, 2022

ListView hook refactor #3086

Merged

5 tasks

Merge branch 'main' into refactor_listview_row_focus_2

9eda1f9

snowystinger reviewed May 3, 2022

View reviewed changes

Comment thread packages/@react-stately/layout/src/ListLayout.ts Outdated

Comment thread packages/@react-spectrum/list/test/ListView.test.js

Comment thread packages/@react-spectrum/list/test/ListView.test.js Outdated

Comment thread packages/@react-spectrum/list/src/ListViewItem.tsx Outdated

devongovett previously approved these changes May 4, 2022

View reviewed changes

addressing review comments

e4086bf

LFDanLu dismissed devongovett’s stale review via e4086bf May 4, 2022 18:39

Merge branch 'main' into refactor_listview_row_focus_2

5ed6df6

devongovett approved these changes May 4, 2022

View reviewed changes

snowystinger approved these changes May 4, 2022

View reviewed changes

Merge branch 'main' into refactor_listview_row_focus_2

71809a1

LFDanLu merged commit 5917acf into main May 4, 2022

LFDanLu deleted the refactor_listview_row_focus_2 branch May 4, 2022 22:17

LFDanLu added the needs testing label May 4, 2022

devongovett removed the needs testing label May 10, 2022

devongovett mentioned this pull request Aug 12, 2022

ComboBox: left/right arrow keys do not work after inputing a value while dropdown menu is down #3400

Closed

Conversation

LFDanLu commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devongovett Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LFDanLu Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Apr 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Apr 5, 2022

Uh oh!

devongovett commented Apr 27, 2022

Uh oh!

LFDanLu commented Apr 27, 2022

Uh oh!

adobe-bot commented Apr 27, 2022

Uh oh!

LFDanLu commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adobe-bot commented Apr 29, 2022

Uh oh!

adobe-bot commented May 3, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adobe-bot commented May 4, 2022

Uh oh!

adobe-bot commented May 4, 2022

Uh oh!

adobe-bot commented May 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LFDanLu commented Apr 1, 2022 •

edited

Loading

devongovett Apr 21, 2022 •

edited

Loading

LFDanLu Apr 21, 2022 •

edited

Loading

LFDanLu commented Apr 27, 2022 •

edited

Loading