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

feature: UserEvent scroll() #1441

Closed
mdjastrzebski opened this issue Aug 7, 2023 · 29 comments
Closed

feature: UserEvent scroll() #1441

mdjastrzebski opened this issue Aug 7, 2023 · 29 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@mdjastrzebski
Copy link
Member

Describe the Feature

Create a User Event interaction(s) for scrolling. This seems to be pretty frequent interaction on touch based devices, at the same time there are numerous events involved: onScroll, onMomentumScrollBegin/End, onScrollBegin/EndDrag, etc. This feature should work with ScrollViewas well asFlatList/SectionList` (hopefully these will be same event sequences).

Possible Implementations

As with other User Event interactions:

  1. Create ScrollView/FlatList experiment in the experiments app
  2. Conduct scroll interactions (dragging, vertical swiping) on both iOS & Android, record logged events in our Wiki
  3. Define unified event sequence roughly matching both iOS & Android, as well as events documentation
  4. Write implementation and relevant unit tests

Related Issues

#1405 - core User Event code
#1386 - press() implementation
#1396 - type() implementation

@mdjastrzebski mdjastrzebski added the help wanted Extra attention is needed label Aug 7, 2023
@mdjastrzebski
Copy link
Member Author

CC: @pierrezimmermannbam @MattAgn @vanGalilea perhaps any of you would be interested? :-)

@mdjastrzebski mdjastrzebski added the good first issue Good for newcomers label Aug 7, 2023
@siepra
Copy link
Contributor

siepra commented Aug 7, 2023

Hi, do you mind if I tried?

@mdjastrzebski
Copy link
Member Author

@siepra go ahead.

In order to achieve the high-quality simulation pls submit experiments app PR first, then fill in the Wiki, only then start to work on the actual code (as described in the issue desc).

@siepra
Copy link
Contributor

siepra commented Aug 7, 2023

Linking a PR (draft) for experiments #1442
Should I care about onContentSizeChange callback? I'm not sure if it's relevant

@mdjastrzebski
Copy link
Member Author

You should log it in experiment, but probably does not happen on actual scrolling

@mdjastrzebski
Copy link
Member Author

@siepra step 1 merged. Time for recording logged events on wiki.

@siepra
Copy link
Contributor

siepra commented Aug 8, 2023

Great, thank you!
I don't have access to edit wiki pages in this repo, am I right? I'm attaching a file containing my recorded logs along with notes on observations (I'm not sure whether the formatting is fully correct) so you can post it by yourself. Or if there's another way, please let me know!
wiki-draft.md

@mdjastrzebski
Copy link
Member Author

mdjastrzebski commented Aug 8, 2023

@siepra I've copied your research into the wiki. Good work!

Now we need to decide on the API for user interactions:

  • what interactions to support
  • what should be it's parameters
  • what events should they emit

Some initial thoughts about API:

  • user.scrollTo(element, { x, y })
    • with x and y being optional in order to allow both vertical or horizontal scroll.
    • We should assume the initial scroll position is (x, y) = (0, 0).
    • In order not to assume that each of the scroll operations is starting from (0, 0) and going to (x, y), we should save the current scroll values for the element. Note: this will be "virtual state" not affected by calling ref.scrollTo().
    • This helper could emit onScrollBeginDrag, a number of onScroll events (steps between initial and final position), and onScrollEndDrag
    • We could have options like momentumScroll: boolean to control whether there should be onMomentumScrollBegin+ onScroll x n, onMomentumScrollEnd happening after the initial drag scrolling
    • These should work for both ScrollView and FlatList.
    • Regarding FlatLists's onEndReached event we could treat it as an option to scrollTo
  • user.scrollToTop(element) as a specialized interaction

These are just rough ideas, feel free to challenge them, you can also start coding the implementations & tests to get a better feel for the API.

@siepra
Copy link
Contributor

siepra commented Aug 8, 2023

I think you're right with most of those ideas. Nevertheless setting x and y values may be frustrating if one doesn't know the exact content size of the scrollable element so I'm wondering if it might be better to let the user type in e.g. percentage value? I'm not really sure what the real life use-case here is. I think the extremes will be the most useful: scrollToTop, scrollToBottom.

Also I'm curious what the number of onScroll events depend on. The first thing that comes to my mind is velocity changes and if that's true it means we don't really have to worry about that and we can probably emit only one of those events.

@mdjastrzebski
Copy link
Member Author

RNTL will not be able to calculate the exact content size, so we should allow user to somehow specify it, e.g. by passing scrollTo({ y: 100, contentHeight: 1000 }). If the user does not care about content height then we might assume some artificial number like 400.

re. scrollToTop is a special action invoked in native iOS then you tap the status bar and it scrolls the scroll view all the way to the top. it has it's own event handler. This is different then user just scrolling all the way to the top. There is no scrollToBottom feature in iOS/Android as far as I'm aware

re number of scroll events could be fixed to say 10, but also configurable by the user, e.g. scrollTo({ y: 100, steps: 5 }). That should be reasonable API for testing. Normally users should not care about the exact scroll points but in some specific cases they might.

@siepra
Copy link
Contributor

siepra commented Aug 8, 2023

Bummer, I thought we're able to calculate content size and make it smarter.

I'm worried about validating host element type and writing tests for FlatList as it renders it's child components and I see ReactTestRenderer doesn't handle it well (or maybe I'm doing something wrong?):

import { render } from '../../..';

render(
  <FlatList
    data={[]}
    renderItem={jest.fn()}
  />
);
    TypeError: Cannot destructure property 'getItem' of 'props' as it is undefined.

      11 |   let renderer: ReactTestRenderer;
      12 |
    > 13 |   TestRenderer.act(() => {
         |                ^
      14 |     renderer = TestRenderer.create(component, options);
      15 |   });
      16 |

@mdjastrzebski
Copy link
Member Author

You should pass a function creating actual JSX elements to renderItem instead of jest.fn() which return undefined by default.

@siepra
Copy link
Contributor

siepra commented Aug 8, 2023

You mean like:

() => {
    return <View />
}

It doesn't work neither

@mdjastrzebski
Copy link
Member Author

@siepra You're right, I was able to replicate it easily. tbh I was not aware of that, so that's for catching it.

Anyway, let's work on ScrollView first, and I will do some research on FlatList, so we could enable it later.

@siepra
Copy link
Contributor

siepra commented Aug 9, 2023

So far I've come with
A scroll API endpoint, which accepts:

  • x?, y?
  • (optional) number of intermediate onScroll callbacks
  • (optional) momentum, consisting of it's value and number of intermediate onScroll callbacks

A scrollToTop API endpoint.

They both work with ScrollView and are directly accessible in userEvent.
There is no validation on host element type due to FlatList suspense.

There are some questions that popped into my head:

  • @mdjastrzebski you said we should save the current scroll values for the element. Could you drop some hints on how to achieve that? We'd have to globally keep the reference to an element instance, wouldn't we?
  • Do we want to decorate scrollToTop method somehow? Is a comment in the code enough?

Remaining things:

  • Add support for FlatList
  • Provide option to scroll until onEndReached (FlatList)
  • Perform validation on host element type
  • Emit intermediate onScroll events in scrollToTop

Is there something else I'm missing?

@mdjastrzebski
Copy link
Member Author

mdjastrzebski commented Aug 9, 2023

re saving values:

The way we could achieve this is to have some WeakMap object keyed by ScrollView element. Something like that:

interface ScrollState { x: number, y: number };

const scrollStateForElement = new WeakMap<ReactTestInstance, ScrollState>();

export function getElementScrollState(element: ReactTestInstance) {
  return scrollStateForElement[element] ?? { x: 0, y: 0 }; // default to (0, 0)
}

export function setElementScrollState(element: ReactTestInstance, scrollState: ScrollState) {
  scrollStateForElement[element] = scrollState
}

WeakMap has two important properties:

  1. It can be keyed by any object, not only strings/symbols as regular objects
  2. It holds only a weak reference to the element, hence it will automatically drop it when JS garbage collector decides that given element is no longer reachable, hence preventing memory leaks.

Additionally, iirc. ReactTestInstances are stable across re-renders, as they are cached in a similar way we want to handle storing scroll state: https://github.com/facebook/react/blob/a20eea25197df0da80104917df414747eeab1ac9/packages/react-test-renderer/src/ReactTestRenderer.js#L616

We need to confirm experimentally that it works, but it should work fine :-)

@pierrezimmermannbam
Copy link
Collaborator

If understand correctly the API the user wouldn't need to pass an element to the scroll function. I think it's a good idea but in that case how do we know on which element to apply the scroll events?

@mdjastrzebski
Copy link
Member Author

@pierrezimmermannbam they definitely need to pass element there. Otherwise how would we know which element to scroll?

@siepra
Copy link
Contributor

siepra commented Aug 10, 2023

@mdjastrzebski it works :) TypeScript doesn't like the idea of ReactTestInstance being an index though...
image
I assume @ts-expect-error isn't the desired solution?

Also, in tests the rendered <ScrollView /> element is always being treated as the same instance and I don't really know what to do about it. I've been trying to change it's props but it doesn't seem to make any difference. I've generated snapshots individually per each case for now.

@mdjastrzebski
Copy link
Member Author

@siepra you have to use set and get methods, as index operator ([]) will attach it as object prop, but since these can be only string or symbol, this means that the actual key will be like [Object object] and one entry will override the other.

@siepra
Copy link
Contributor

siepra commented Aug 10, 2023

Got it!

@siepra
Copy link
Contributor

siepra commented Aug 10, 2023

@mdjastrzebski I think the only remaining thing is FlatList now. Would you mind taking a look at #1445 and sharing thoughts on the code?

@siepra
Copy link
Contributor

siepra commented Aug 11, 2023

(FlatList) not sure if it's helpful but I see the problem is re-creating component instance at react-test-renderer.development.js:4460. React Native's FlatList receives actual props in it's constructor but then it only references this.props which is always undefined.

I haven't been lurking that much into rendering components yet, so I probably need to learn more about an overall context but maybe it'll save someone some logs printing

@mdjastrzebski
Copy link
Member Author

mdjastrzebski commented Aug 11, 2023

I've created a separate issue for FlatList in #1449, this might actually be an upstream (React Native) issue.

Edit: this seems to be issue occurring in our repo, as FlatList renders correctly in our's example/basic app.

@pierrezimmermannbam
Copy link
Collaborator

@pierrezimmermannbam they definitely need to pass element there. Otherwise how would we know which element to scroll?

Well usually there is a single element scrollable on a screen. There may be more but they are most of the time not in the same direction. For most use cases we could detect either a scrollview or a flat list and use that I think. Maybe the element could be optional so that in case user has nested scroll views he may be able to target one specifically? Because otherwise how do you target a scrollview if not with a test id?

@mdjastrzebski
Copy link
Member Author

I think we should require element for now, as it matches the convention of all other FireEvent/UserEvent API which also require element to trigger event/interaction on. Users are familiar with this pattern, and it makes the tests more robust, in cases user introduces another scroll view in the complex component.

There are valid UI patterns when you have 2+ scroll views, e.g. 1+ horizontal scroll views that can be scrolled independently inside overall vertical scroll view.

@pierrezimmermannbam
Copy link
Collaborator

In the case where there is an horizontal and a vertical scrollview we would know which one to scroll on based on the direction of the scroll but it wouldn't work if there are several horizontal scroll views though. Some other cases like scrollviews within tabs may also be problematic so maybe it's not a good idea after all. I thought about this because in e2e tests you don't provide the element but unfortunately we have limitations that prevent us from having the same API so I agree it's better to have the element being mandatory

@mdjastrzebski
Copy link
Member Author

I've created a separate issue for discussion just the scroll API, without dealing with implementation detals: #1450

@mdjastrzebski
Copy link
Member Author

Resolved in #1445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants