Feature/viewability configs #301

fortmarek · 2022-04-11T11:19:59Z

Description

This PR partly implements #268.

It's currently missing:

enabling viewability config at item level instead of list - I am still on the fence about whether we want to implement this now (if ever) as it is a feature FlatList does not support.

The API itself closely copies FlatList as there was no real reason for us to deviate and by default, we try to stick what FlatList has.

One thing touched on in the original issue was the following:

We need to take into account that developers will be processing data in these callbacks so we need to be very smart in deferring them. We can provide a timestamp with each event instead of raising them as soon as possible.

The current state of the PR does not defer callbacks in any way - rather, it invokes them synchronously. This copies the behavior FlatList has. The current implementation does mean viewability callbacks will potentially worsen performance if JS thread is busy, user is scrolling quickly, and the minimumViewTime is high. The performance drop also depends on the users' implementation.

The idea to defer viewability callbacks when JS thread is not busy (using requestIdleCallback) is still on the table but it has its drawbacks:

we will divert from FlatList
the API will be slightly more difficult to use, requiring users to use a new timestamp field that would be used to track when the viewability event actually occurred
we'd still need to invoke all the callbacks before unmounting a view - in case there would be a lot of pending callbacks, it could lead to dropped frames during the navigation transition away from the view. There might be ways to mitigate it but will further complicate the implementation

Additionally, having a sensible minimumViewTime already ensures that the number of viewability callback invocations will stay low - which is why I have added a default of 250 ms if user does not specify it. But even if user does specify a lower minimumViewTime, even 0, I have not observed any noticeable drop in performance when using and not using viewability callbacks.

One thing we could do is to add timestamp field to the ViewToken object (sent in the viewability callback) now and instruct users to use it. This will enable us making those callbacks deferred in the future without breaking users' implementations.

Update:
We have decided to add the timestamp and instruct people to use it. This enables us to defer the callbacks in the future without breaking users' implementation: 50cb39a

Reviewers’ hat-rack 🎩

Try out Twitter and TwitterFlatList and observe viewability callbacks which are currently logged to the console (I suggest commenting out the blank area log in App.tsx

Checklist

I have added a changelog entry following the Keep a Changelog guidelines.

fortmarek · 2022-04-11T15:55:48Z

As for the performance, I have not found any performance regression when using viewability callbacks using this PR - the average FPS was 45.5 +/- 1.

naqvitalha · 2022-04-11T16:34:14Z

documentation/docs/fundamentals/usage.md

+Called when the viewability of rows changes, as defined by the `viewabilityConfig` prop. Array of `changed` includes `ViewToken`s that both visible and non-visible items. You can use the `isViewable` flag to filter the items.
+
+:::note
+If you are tracking the time a view becomes (non-)visible, use the `timestamp` property.


A short explanation can be helpful here

naqvitalha · 2022-04-11T16:37:38Z

documentation/docs/fundamentals/usage.md

+
+#### itemVisiblePercentThreshold
+
+Similar to `viewAreaCoveragePercentThreshold`, but considers the percent of the item that is visible, rather than the fraction of the viewable area it covers.


What's the rule if I specify both viewAreaCoveragePercentThreshold and itemVisiblePercentThreshold?

good catch, we should throw an exception in that case, as FlatList does.

naqvitalha · 2022-04-11T16:40:27Z

documentation/docs/fundamentals/usage.md

+recordInteraction();
+```
+
+Tells the list an interaction has occurred, which should trigger viewability calculations, e.g. if `waitForInteractions` is true and the user has not scrolled. This is typically called by taps on items or by navigation actions.


Can we be more explicit in stating that devs have to actually call this method? After reading the last line I felt that list will detect this automatically which, I'm assuming, is not the case.

yeah, I will rephrase this - it's from the React Native documentation but it certainly can be a bit more clear.

naqvitalha · 2022-04-11T16:42:29Z

fixture/src/twitter/Twitter.tsx

@@ -56,6 +56,14 @@ const Twitter = () => {
        ItemSeparatorComponent={Divider}
        data={tweets}
        initialScrollIndex={debugContext.initialScrollIndex}
+        viewabilityConfig={{


FlashList is a pure component so ideally we should avoid sending in new objects. It may be relevant if someone tries to draw inspiration from here. Something good to have but not a necessity if we look at these as maintainer only samples.

naqvitalha · 2022-04-11T16:44:37Z

src/FlashList.tsx

@@ -120,6 +130,14 @@ export interface FlashListProps<T> extends FlatListProps<T> {
   * If you're using ListEmptyComponent, this event is raised as soon as ListEmptyComponent is rendered.
   */
  onLoad?: (info: { elapsedTimeInMs: number }) => void;
+
+  /**
+   * Called when the viewability of rows changes, as defined by the viewablePercentThreshold prop.


I think the word row is not relevant to FlashList. Even with numColumns > 1 we treat everything as individual items

naqvitalha · 2022-04-11T17:45:39Z

src/FlashList.tsx

+   * Creates a new `ViewabilityHelper` instance with `onViewableItemsChanged` callback and `ViewabilityConfig`
+   * @returns `ViewabilityHelper` instance
+   */
+  private createViewabilityHelper = (


There really seems to be a need for a manager class that can setup all the viewability helpers and can forward right events. It would be better if FlashList doesn't do that. It will also reduce the additions required in this file.

naqvitalha · 2022-04-11T17:49:28Z

src/FlashList.tsx

          windowCorrectionConfig={this.getUpdatedWindowCorrectionConfig()}
        />
      </StickyHeaderContainer>
    );
  }

+  private onVisibleIndicesChanged = (all: number[]) => {
+    this.viewabilityHelpers.forEach(
+      (viewabilityHelper) => (viewabilityHelper.possiblyViewableIndices = all)


updateViewableItems method in viewabilityHelper can also accept and new array of items. It will make this cleaner.

naqvitalha · 2022-04-11T17:54:41Z

src/FlashList.tsx

+    this.viewabilityHelpers.forEach((viewabilityHelper) => {
+      viewabilityHelper.hasInteracted = true;
+    });
+    this.updateViewableItems();


also call this.props.onScrollBeginDrag incase the developer has sent it

naqvitalha · 2022-04-11T18:00:13Z

src/FlashList.tsx

+    this.viewabilityHelpers.forEach((viewabilityHelper) => {
+      viewabilityHelper.updateViewableItems(
+        this.props.horizontal ?? false,
+        this.rlvRef?.getCurrentScrollOffset() ?? 0,


This may not be enough. Check your calculation with a header. You may need to subtract distanceFromWindow here.

naqvitalha · 2022-04-11T18:02:00Z

src/ViewabilityHelper.ts

+    this.timers.forEach(clearTimeout);
+  }
+
+  updateViewableItems(


Can you mark public methods as public. Makes it easier to read

naqvitalha

@fortmarek I've left some comments. The tests look awesome btw.

Overall, some changes in design can be helpful. Like FlashList and viewablity helper do too much together and FlashList is acting as a manager of multiple instances of viewability helper.
Can we create a manager that takes minimum input from FlashList (a ref also works) and abstracts all the management away? Even better if the manager can have a generic interface.

Also, let's see the impact of this on performance (if any)

fortmarek · 2022-04-12T09:11:11Z

@naqvitalha I have created a new component ViewabilityManager which minimizes changes necessary in FlashList. Other comments have also been addressed.

ElviraBurchik

Tophatted, a couple of nits and questions 👍

ElviraBurchik · 2022-04-12T10:38:02Z

documentation/docs/fundamentals/usage.md

+  isViewable: boolean;
+  item: string;
+  key: string;
+  timestamp: number;


FlatList's ViewToken also has section prop. We don't support it yet, but maybe add a note about it?

ElviraBurchik · 2022-04-12T10:39:44Z

documentation/docs/fundamentals/usage.md

+`viewabilityConfig` is a default configuration for determining whether items are viewable.
+
+:::warning
+Changing viewabilityConfig on the fly is not supported


What does on the fly mean specifically, what's not possible?

you cannot change it after the first render. It's how FlatList phrases it - do you think there's a better way to explain this?

ElviraBurchik · 2022-04-12T10:49:43Z

src/__tests__/helpers/mountFlashList.tsx

@@ -39,6 +45,12 @@ export const mountFlashList = (props?: {
  ) => void;
  estimatedItemSize?: number;
  ListEmptyComponent?: FlashListProps<string>["ListEmptyComponent"];
+  viewabilityConfig?: ViewabilityConfig | null;
+  onViewableItemsChanged?:


nit: since onViewableItemsChanged is optional, is it necessary to define it's type as .. | null | undefined?

Yes, it is - for example, in the test utils (mountFlashList) we have:

onViewableItemsChanged={props?.onViewableItemsChanged}

This expression would be invalid if undefined is not specified in the type. Defining a property as xxx?: string | null | undefined means you don't have to specify it (in which case it is undefined) but if you do, it has to be equal to the type after :.

Therefore, we should definitely keep undefined, null might not be that useful but I think it's better to keep it as there's not a really strong reason to constrain users in defaulting to undefined.

ElviraBurchik · 2022-04-12T10:53:56Z

src/ViewabilityHelper.ts

+  /**
+   * Viewable indices regardless of the viewability config
+   */
+  possiblyViewableIndices: number[] = [];


nit: possiblyViewableIndices is not well explanatory without the comment since, it's actually viewed, but unfiltered items

those items are visible but may not fit the criteria - do you have a suggestion for a better name? I'd argue unfilteredViewableIndices is not self-explanatory, too, and can't think of anything better.

ElviraBurchik · 2022-04-12T10:58:10Z

src/ViewabilityManager.ts

+  constructor(flashListRef: FlashList<T>) {
+    this.flashListRef = flashListRef;
+    if (
+      flashListRef.props.onViewableItemsChanged !== null &&


should onViewableItemsChanged be both null | undefined? It could be reduced to check for undefined, no? Or it there any specific reason?

As explained here, I'd rather keep it.

ElviraBurchik · 2022-04-12T11:05:14Z

src/__tests__/ViewabilityHelper.test.ts

+    updateViewableItems({ viewabilityHelper });
+    // Initial call
+    expect(viewableIndicesChanged).toHaveBeenCalledWith(
+      [0, 1, 2],


Why [0, 1, 2] is expected twice?

the first array is what's viewable now whereas the second array defines what is newly viewable. The third then is for what is newly not viewable.

ElviraBurchik · 2022-04-12T11:10:06Z

src/__tests__/ViewabilityHelper.test.ts

+    );
+
+    updateViewableItems({ viewabilityHelper, scrollOffset: 50 });
+    expect(viewableIndicesChanged).toHaveBeenCalledWith([0, 1, 2, 3], [3], []);


could you please add comments about expected results? 🙌
It's a bit hard to understand on the go

naqvitalha · 2022-04-12T14:19:02Z

src/FlashList.tsx

+  };
+
+  private onScroll = () => {
+    this.viewabilityManager.updateViewableItems();


Call this.props.onScroll too

naqvitalha · 2022-04-12T14:22:26Z

src/ViewabilityManager.ts

+  };
+
+  public recordInteraction = () => {
+    this.viewabilityHelpers.forEach((viewabilityHelper) => {


This will run everytime onScrollBeginDrag happens. After the first call we can ignore it.

I don't think we need to call updateViewableItems also after first interaction

naqvitalha · 2022-04-12T14:34:49Z

src/FlashList.tsx

@@ -414,12 +435,27 @@ class FlashList<T> extends React.PureComponent<
          }
          initialOffset={initialOffset}
          onItemLayout={this.onItemLayout}
+          onScroll={this.onScroll}
+          onVisibleIndicesChanged={
+            this.viewabilityManager.onVisibleIndicesChanged


Can we avoid listening to this if devs don't want callbacks?

naqvitalha · 2022-04-12T14:40:20Z

src/ViewabilityManager.ts

+  };
+
+  public updateViewableItems = (newViewableIndices?: number[]) => {
+    const listSize =


If there's no registered viewability helper we can skip all this code. Check for length in the beginning?

naqvitalha

LGTM, awesome work @fortmarek

fortmarek added 16 commits April 11, 2022 10:48

WIP: Viewability config

5f041d2

Viewability callback with percentage

159c7a9

Support for minimumViewTime

4082996

Populate changed array

10b76a3

Implement waitForInteraction

a5f03d1

Move ViewabilityHelper to a separate file

3fc8ccf

Add tests for ViewabilityHelper

e416742

Implement ViewabilityHelper for horizontal

66b757d

Dismount timers

8789cab

Add support for viewabilityConfigCallbackPairs

46e7fef

Add test for onViewableItemsChanged in FlashList

7292ba5

Add test for viewabilityConfigCallbackPairs

71bada6

Add docs for viewability

881bbad

Add test for recordInteraction

42521c1

Enable blank area logs

8aa613f

Make minor changes

067f2e4

fortmarek marked this pull request as draft April 11, 2022 11:20

fortmarek requested a review from naqvitalha April 11, 2022 13:17

fortmarek added 2 commits April 11, 2022 15:20

Move onVisibleIndicesChanged to a separate method

7c8d9ae

Add timestamp to ViewToken

50cb39a

fortmarek requested a review from ElviraBurchik April 11, 2022 16:02

Edit changelog

1a7623b

fortmarek marked this pull request as ready for review April 11, 2022 16:04

naqvitalha reviewed Apr 11, 2022

View reviewed changes

fortmarek added 5 commits April 12, 2022 09:53

Address PR comments

142e910

Create ViewabilityManager

b4b945e

Add newViewableIndices param to updateViewableItems

55a0e10

Take distanceFromWindow into account

82d398d

Mark ViewabilityHelper methods as public

ccf9831

ElviraBurchik approved these changes Apr 12, 2022

View reviewed changes

Add comments

aa3bcac

naqvitalha reviewed Apr 12, 2022

View reviewed changes

src/FlashList.tsx

};

private onScroll = () => {

this.viewabilityManager.updateViewableItems();

Copy link

Collaborator

naqvitalha Apr 12, 2022

Choose a reason for hiding this comment

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

Call this.props.onScroll too

naqvitalha reviewed Apr 12, 2022

View reviewed changes

Address PR comments

9641d75

fortmarek requested a review from naqvitalha April 12, 2022 14:45

naqvitalha approved these changes Apr 12, 2022

View reviewed changes

fortmarek merged commit f321460 into main Apr 12, 2022

fortmarek deleted the feature/viewability-configs branch April 12, 2022 15:00

naqvitalha mentioned this pull request Apr 14, 2022

Impression Tracking API #268

Closed


		#### itemVisiblePercentThreshold

		Similar to `viewAreaCoveragePercentThreshold`, but considers the percent of the item that is visible, rather than the fraction of the viewable area it covers.

Feature/viewability configs #301

Feature/viewability configs #301

Conversation

fortmarek commented Apr 11, 2022 • edited

Description

Reviewers’ hat-rack 🎩

Checklist

fortmarek commented Apr 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naqvitalha left a comment

Choose a reason for hiding this comment

fortmarek commented Apr 12, 2022

ElviraBurchik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortmarek Apr 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naqvitalha left a comment

Choose a reason for hiding this comment

fortmarek commented Apr 11, 2022 •

edited

fortmarek Apr 12, 2022 •

edited