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.

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

Keyboard-friendly List Virtualization Behavior #335

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

NickGerleman
Copy link

@NickGerleman NickGerleman commented Feb 27, 2021

VirtualizedList virtualizes away items based on proximity to the lists viewport,
without consideration for other inputs such as focus or selection. This
strategy limits the ability to correctly implement some common keyboard
interactions. This especially impacts desktop platforms (react-native-web,
react-native-windows, react-native-macos), but also impacts the tablet with
keyboard form-factor on iPadOS/Android.

This proposal outlines these issues, proposing a set of behavior changes and new
APIs to address them. It does not address concrete implementation.

VirtualizedList virtualizes away items based on proximity to the lists viewport,
without consideration for other features such as focus or selection. This
strategy limits the ability to correctly implement some common keyboard
interactions. This especially impacts desktop platforms (react-native-web, react-native-windows, react-native-macos), but also impacts the tablet with
keyboard form-factor on iPadOS/Android.

This proposal outlines these issues, proposing a set of behavior changes and new
APIs to address them. It does not address concrete implementation.
@NickGerleman
Copy link
Author

This has some intersection with existing functionality, where users can mark stickyIndices for sticky headers which are kept realized. This solely keeps sticky headers realized though, without additional cells around them.

NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Mar 1, 2021
See react-native-community/discussions-and-proposals#335 for extra context.

A VirtualizedList may have sticky headers, forwarded on to ScrollView. These sticky headers are exempt from virtualization once realized for the first time. This change documents the behavior of keeping sticky header cells realized after scrolling away.

This scenario performs the same behavior as creating an internal "realization window" for sticky headers with a single cell window size. Generalizing the concept of realization windows should be shaped to support the existing sticky header scenario.
@NickGerleman
Copy link
Author

Pushed out a new commit with a few updates:

  • Focus tracking via capture phase
  • Smaller realization windows
  • Focused -> "most recent focused"
  • Talk about home intersection with initial render
  • Talk about batch rendering behavior

*
* Defaults to keeping a single screen at the beginning of the list realized
*/
home?: RealizationWindow;
Copy link

Choose a reason for hiding this comment

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

I think we should call this start instead. The "home" terminology doesn't really fit well with our other APIs, and "start" is clear.

*/
export type RealizationWindow =
| boolean
| [boolean, {windowSize?: number}];
Copy link

Choose a reason for hiding this comment

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

I feel like this might be too much unnecessary configuration being able to adjust each windowSize. Any reason we'd ever want this to not be 1?

Copy link
Author

Choose a reason for hiding this comment

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

The default windowSize for the viewport is fairly large, rendering a whole 10 screens above and below. This is a pretty aggressive amount of pre-rendering. It seems like existing setup optimizes for avoiding blanking on fast movement/interaction over memory consumption.

The realization areas have the potential for similar issues. A conceivable user scenario is to quickly tab between items taller than a viewport. Items would possibly not be realized in time for the keyboard interaction. Keeping a larger window fixes this, but a viewport's worth of native controls could be a non-trivial amount of memory, or cause jank due to excessive native renders.

The right amount of prerendering here seems to depend on scenario. I think we can pick something sane as a default, but am not confident a specific size would work well in all scenarios

I think there might be some alternate APIs to make configuration cleaner, and want to play around with that. I think configurability would be useful here in a general-sense though.


Endpoint detection is generally performed via `Platform` APIs, such as
`Platform.isTV()` or `Platform.isPad()`. This change would add a
`Platform.isDesktop()` endpoint query to allow desktop-specific behavior.
Copy link

Choose a reason for hiding this comment

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

Love this! I suggest this in facebook/react-native#29937 but that never was completed.

Copy link
Author

Choose a reason for hiding this comment

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

I think Android also has this, withPlatformConstants.constants.uiMode being 'desk' for desktop.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 11, 2021
Summary:
See react-native-community/discussions-and-proposals#335 for extra context.

A VirtualizedList may have sticky headers, forwarded on to ScrollView. These sticky headers are exempt from virtualization once realized for the first time. This change documents the behavior of keeping sticky header cells realized after scrolling away.

This scenario performs the same behavior as creating an internal "realization window" for sticky headers with a single cell window size. Generalizing the concept of realization windows should be shaped to support the existing sticky header scenario.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Internal] [Added] - Add tests describing current sticky header realization behavior

Pull Request resolved: #31075

Reviewed By: lunaleaps

Differential Revision: D26767582

Pulled By: appden

fbshipit-source-id: 0d151bd6046fcb5384c646205aafa1ca7edf6c77
@rozele
Copy link
Collaborator

rozele commented Mar 20, 2021

Perhaps somewhat related, I was considering if it might be possible to reduce the number of onLayout events emitted made by items in a FlatList, especially in inverted scenarios where content is refreshed from the top.

From what I could tell, there are at least two uses for onLayout events that aren't in the fixed start/end realization windows. One is for calculating the height of the spacers and the other is for implementing scrollToIndex.

If the native platform offered something to scroll a given component into view, e.g., BringIntoView on XAML or scrollIntoView on Web, we would only need to capture the size metrics of the virtualization candidates, which are significantly less likely to change (but we'd need to add some onSizeChanged or similar event to complement onLayout that could fire less frequently).

In order for something like native scrollToIndex to work, we'd need the ability to realize a window around that view prior to invoking a command, so I'm curious if what you have proposed here would be compatible with this theoretical optimization 😀

@NickGerleman
Copy link
Author

PRs for internal refactoring of VirtualizedList state representation are now out. This should let us start realizing discontiguous regions.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jul 26, 2022
Summary:
Add capture-phase focus events to the type system, for use in the refactored VirtualizedList https://github.com/facebook/react-native/pull/32646/files

Tracking the last focused child is done via focus events. Focus events are bubbling (vs direct events like onLayout), and are given both a "capture" phase, and "bubbling phase", like DOM events on the web. https://stackoverflow.com/questions/4616694/what-is-event-bubbling-and-capturing

The VirtualizedList change wants to know if a child will receive focus. This is not possible to reliably capture in the bubbling phase, since a child may stop propagation.

See react-native-community/discussions-and-proposals#335 (comment) for some discussion with Scott Kyle about this issue back in the day

This is done by convention in React by adding a "capture" variant of the `onXXX` method. For all platforms I've seen with focus events, these map the `topFocus` native event to `onFocus` for bubbling phase, and `onFocusCapture` for capture phase. See https://reactjs.org/docs/events.html#supported-events

Changelog:
[General][Added] - Add types for onFocusCapture/onBlurCapture

Reviewed By: javache

Differential Revision: D38013861

fbshipit-source-id: 7bda22e1a4d5e36ac5e34e804abf6fb318a41baf
facebook-github-bot pushed a commit to facebook/react-native 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants