Do not virtualize items around the last focused item #32646
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
Base commit: b66db7a |
Base commit: b66db7a |
NickGerleman
force-pushed
the
realize-last-focused
branch
from
a4d2c3a to
94c7285
Compare
NickGerleman
force-pushed
the
realize-last-focused
branch
from
2a4dd9f to
21a1ae7
Compare
NickGerleman
changed the title
NickGerleman
force-pushed
the
realize-last-focused
branch
from
21a1ae7 to
3ab1de1
Compare
Builds upon the `CellRenderMask` data structure added with facebook#31420, and VirtualizedList coverage added with facebook#31401. VirtualizedList currently keeps a [first, last] range as state, tracking the region of cells to render. The render functions uses this as an input, along with a few special cases to render more (sticky headers, initial render region.) This change moves to instead keep state which describes discontiguous render regions. This mask is continually updated as the viewport changes, batch renders expand the region, etc. Special cases are baked into the render mask, with a relatively simple tranformation from the mask to render function. This representation makes it much easier to support keyboarding scenarios, which require keeping distinct regions (e.g. for last focused) realized while out of viewport. MS/FB folks have a video discussion about VirtualizedList here: https://msit.microsoftstream.com/video/fe01a1ff-0400-94b1-d4f1-f1eb924b1809 facebook#31401 added quite a few snapshot tests, centering around the logic this change is touching. I manually validated RNTester FlatList examples (and their should be some upstream UI testing for them).
This change also includes the contents of facebook#32638 This change makes VirtualizedList track the last focused cell, through the capture phase of `onFocus`. It will keep the last focus cell, and its neighbors rendered. This allows for some basic keyboard interactions, like tab/up/down when on an item out of viewport. We keep the last focused rendered even if blurred for the scenario of tabbing in and and out of the VirtualizedList. Validated via UT.
NickGerleman
force-pushed
the
realize-last-focused
branch
from
1bd3342 to
8437121
Compare
facebook-github-bot
pushed a commit
that referenced
this pull request
Jul 27, 2022
Summary: # This Change react-native-community/discussions-and-proposals#335 discussed a set of problems with VirtualizedList and focus. These were seen as severe externally for a11y on desktop. The issues center on users of keyboard and accessibility tools, where users expect to be able to move focus in an uninterrupted loop. The design and implementation evolved to be a bit more general, and without any API-surface behavior changes. It was implemented and rolled out externally as a series of changes. The remaining changes that were not upstreamed into RN are rolled into #32646 This diff brings this change into the repo, as a separate copy of VirtualizedList, to measure its impact to guardrail metrics, without yet making it the default implementation. The intention is for this to be temporary, until there is confidence the implementation is correct. ## List Implementation (more on GitHub) This change makes it possible to synchronously move native focus to arbitrary items in a VirtualizedList. This is implemented by switching component state to a sparse bitset. This was previously implemented and upstreamed as `CellRenderMask`. A usage of this is added, to keep the last focused item rendered. This allows the focus loop to remain unbroken, when scrolling away, or tab loops which leave/re-enter the list. VirtualizedList tracks the last focused cell through the capture phase of `onFocus`. It will keep the cell, and a viewport above and below the last focused cell rendered, to allow movement to it without blanking (without using too much memory). ## Experimentation Implementation A mechanism is added to gate the change via VirtualizedListInjection, mirroring the approach taken for Switch with D27381306 (683b825). It allows VirtualizedList to delegate to a global override. It has a slight penalty to needing to import both modules, but means code which imports VirtualizedList directly is affected the changes. Changelog: [Internal][Added] - Add VirtualizedList_EXPERIMENTAL (CellRenderMask Usage) Reviewed By: lunaleaps Differential Revision: D38020408 fbshipit-source-id: ad0aaa6791f3f4455e3068502a2841f3ffb40b41
roryabraham
pushed a commit
to Expensify/react-native
that referenced
this pull request
Aug 17, 2022
Summary: # This Change react-native-community/discussions-and-proposals#335 discussed a set of problems with VirtualizedList and focus. These were seen as severe externally for a11y on desktop. The issues center on users of keyboard and accessibility tools, where users expect to be able to move focus in an uninterrupted loop. The design and implementation evolved to be a bit more general, and without any API-surface behavior changes. It was implemented and rolled out externally as a series of changes. The remaining changes that were not upstreamed into RN are rolled into facebook#32646 This diff brings this change into the repo, as a separate copy of VirtualizedList, to measure its impact to guardrail metrics, without yet making it the default implementation. The intention is for this to be temporary, until there is confidence the implementation is correct. ## List Implementation (more on GitHub) This change makes it possible to synchronously move native focus to arbitrary items in a VirtualizedList. This is implemented by switching component state to a sparse bitset. This was previously implemented and upstreamed as `CellRenderMask`. A usage of this is added, to keep the last focused item rendered. This allows the focus loop to remain unbroken, when scrolling away, or tab loops which leave/re-enter the list. VirtualizedList tracks the last focused cell through the capture phase of `onFocus`. It will keep the cell, and a viewport above and below the last focused cell rendered, to allow movement to it without blanking (without using too much memory). ## Experimentation Implementation A mechanism is added to gate the change via VirtualizedListInjection, mirroring the approach taken for Switch with D27381306 (facebook@683b825). It allows VirtualizedList to delegate to a global override. It has a slight penalty to needing to import both modules, but means code which imports VirtualizedList directly is affected the changes. Changelog: [Internal][Added] - Add VirtualizedList_EXPERIMENTAL (CellRenderMask Usage) Reviewed By: lunaleaps Differential Revision: D38020408 fbshipit-source-id: ad0aaa6791f3f4455e3068502a2841f3ffb40b41
roryabraham
pushed a commit
to Expensify/react-native
that referenced
this pull request
Aug 17, 2022
Summary: # This Change react-native-community/discussions-and-proposals#335 discussed a set of problems with VirtualizedList and focus. These were seen as severe externally for a11y on desktop. The issues center on users of keyboard and accessibility tools, where users expect to be able to move focus in an uninterrupted loop. The design and implementation evolved to be a bit more general, and without any API-surface behavior changes. It was implemented and rolled out externally as a series of changes. The remaining changes that were not upstreamed into RN are rolled into facebook#32646 This diff brings this change into the repo, as a separate copy of VirtualizedList, to measure its impact to guardrail metrics, without yet making it the default implementation. The intention is for this to be temporary, until there is confidence the implementation is correct. ## List Implementation (more on GitHub) This change makes it possible to synchronously move native focus to arbitrary items in a VirtualizedList. This is implemented by switching component state to a sparse bitset. This was previously implemented and upstreamed as `CellRenderMask`. A usage of this is added, to keep the last focused item rendered. This allows the focus loop to remain unbroken, when scrolling away, or tab loops which leave/re-enter the list. VirtualizedList tracks the last focused cell through the capture phase of `onFocus`. It will keep the cell, and a viewport above and below the last focused cell rendered, to allow movement to it without blanking (without using too much memory). ## Experimentation Implementation A mechanism is added to gate the change via VirtualizedListInjection, mirroring the approach taken for Switch with D27381306 (facebook@683b825). It allows VirtualizedList to delegate to a global override. It has a slight penalty to needing to import both modules, but means code which imports VirtualizedList directly is affected the changes. Changelog: [Internal][Added] - Add VirtualizedList_EXPERIMENTAL (CellRenderMask Usage) Reviewed By: lunaleaps Differential Revision: D38020408 fbshipit-source-id: ad0aaa6791f3f4455e3068502a2841f3ffb40b41
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This change also includes the contents of the active PR #32638 which adds usage of the
CellRenderMaskstructure. That PR builds upon theCellRenderMaskdata structure added with #31420, and VirtualizedList coverage added with #31401 The first commit can be skipped to review this change independently.This change makes VirtualizedList track the last focused cell, through the capture phase of
onFocus. It will keep the last focus cell, and its neighbors rendered. This allows for some basic keyboard interactions, like tab/up/down when on an item out of viewport. We keep the last focused rendered even if blurred for the scenario of tabbing in and and out of the VirtualizedList.Changelog
[General] [Fixed] - Do not virtualize items adjacent to the last focused item
Test Plan
Validated via UT (#31401), and in a complex product scenario. Here's a concrete video (of code you can try in Office Beta), where we retain keyboard focus after pagedown'ing hundreds of items away, and can resume navigation where the user had selected.
Commenting.Example.mp4