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

API Proposal: User Event scroll function #1450

Closed
mdjastrzebski opened this issue Aug 14, 2023 · 20 comments
Closed

API Proposal: User Event scroll function #1450

mdjastrzebski opened this issue Aug 14, 2023 · 20 comments
Assignees

Comments

@mdjastrzebski
Copy link
Member

mdjastrzebski commented Aug 14, 2023

@siepra is working User Event scroll API design, the code is quite advanced, so it's high time that we have a solid API with nice DX for that feature. There is no RTL's User Event API for scrolling, so we need to come up with our own. As a base for discussion, I have prepare an initial proposal.

User Event scroll API

Base case

// vertical scroll
user.scrollTo(element, { y: targetPos })

// horizontal scroll:
user.scrollTo(element, { x: targetPos })

Note: the function is named scrollTo instead of scroll to remove ambiguity whether this is target value or relative value. Coincidently this is also the name of imperative handle for scrolling in RN.

This will emit a sequence of onScrollBeginDrag, onScroll (x N), onScrollEndDrag.

By default, the first scroll will start from (0, 0) and will emit default of 5 onScroll intermediate steps between initial position and target position. Subsequent scrolls will start from where the last scroll (using User Event) ended.

Intermediate steps

Explicit steps

Alternatively, you can pass exact steps that will be called as an array. First element is the initial position, and all other elements indicate subsequent steps. The number of steps will be equal to the length of the array.

user.scrollTo(element, { y: [0, 10, 20, 30] })

Number of steps (alternative approach)

You can customize the intermediate steps by passing number of steps and/or ininitial position:

user.scrollTo(element, { y: targetPos, steps: 10 })
user.scrollTo(element, { y: targetPos, fromY: initialPos })
user.scrollTo(element, { y: targetPos, fromY: initialPos, steps: 10 })

We probably should pick only one of the above. Based on my surveys with CK devs, the first one seems to be preferred.

Momentum scroll

Momentum scroll happens after initial sequence of onScrollBeginDrag, onScroll (x N), onScrollEndDrag when scroll view has some velocity (momentum) after user lifts the finger up. This will result in calling onMomentumScrollBegin, onScroll (x N), onMomentumScrollEnd.

By default scrollBy will only invoke drag scroll without momentum part. User can activate momentum scroll passing momentumY or momentumX variables:

// vertical
user.scrollTo({ momentumY: targetPos })

// horizontal
user.scrollTo({ momentumX: targetPos })

This can be linked with previous options:

// Drag scroll from initial position to targetPos1, then momentum scroll to targetPos2
user.scrollTo({ y: targetPos1, momentumY: targetPos2 })

// Pass explicit steps and/or momentum steps
user.scrollTo({ y: [0, 10, 20], momentumY: [30, 40] })

// Control the number of drag steps using `steps`, and momentum steps using `momentumSteps` (alternative intermediate steps approach)
user.scrollTo({ y: targetPos1, momentumY: targetPos2, steps: 5, momentumSteps: 4 })

If the user did not pass drag scroll position, then distance from last position, or default (0, 0), to target position will be divided equally between drag scroll and momentum scroll.

Please share your comments on all things involved, naming of function and options, ways of conveying ideas like intermediate steps, etc, etc. Feel free to challenge this design, propose alternatives, etc. The API is very flexible at this stage, changing it later will probably require breaking changes, etc.

Links

Specific questions:

  1. Should the API be named scrollTo or scroll?
  2. What should be default number of scroll steps, if not specified by user. Similar for momentum steps?
  3. Should we support indicating number of steps using steps param, y: [0, 10, 20] array or both? Or other perhaps?
  4. How should we expose momentum scroll concept: momentumY params, momentum: boolean param, user.momentumScrollTo() function, or other perhaps?
  5. Should RNTL remember the last scroll position and start next scroll from there? Or should we always start from explicit position or (0,0) if not provided?

CC: @pierrezimmermannbam @AugustinLF @MattAgn @thymikee @siepra

@mdjastrzebski mdjastrzebski added question Further information is requested discussion and removed question Further information is requested labels Aug 14, 2023
@flexbox
Copy link

flexbox commented Aug 17, 2023

  1. scrollTo is a better name, crystal clear on the intention.
  2. whatever works 😅
  3. array or both? I would say only arrays. If you take another example from react-query for the v3 to v4 migration they introduced a breaking change to support only arrays Query Keys (and Mutation Keys) need to be an Array. I have the feeling supporting both is prone to errors.
  4. without digging into the API, I can understand the concept behind momentumY
  5. that's tricky but I would say following what happens on the device is the way. I would choose "remember the last scroll position" because that's what happens when users doom scroll, change the app, back into the previous app.

@mdjastrzebski
Copy link
Member Author

@flexbox re 5: that would be limited to single test case. i.e. scroll position would be remembered between two subsequent user.scrollTo(element) calls. In a separate test case, element will have a different object identity, and hence will start again from (0, 0).

In real RN app, scroll position is most probably a native state (yoga might be involved as well?), you do not manage that through props like you would with a managed TextInput content. Since there is no "native" part in RNTL we would keep track of last scroll position for each scrollable element.

@pierrezimmermannbam
Copy link
Collaborator

  1. Yes I agree scrollTo is better

  2. I find the notion of step a bit confusing here. It looks like it serves two purposes: emitting an onScroll event and also a separate scroll because you can provide momentum for each step (user would stop scrolling between each step). Those two things should be decoupled and I believe we should see steps as independent scrolls, i.e. the two following syntaxes would be strictly equivalent:

user.scrollTo({ y: [0, 10], momentumY: [30, 40] })

user.scrollTo({ y: [0], momentumY: [30] })
user.scrollTo({ y: [10], momentumY: [40] })

That leaves up the question of how often should onScroll be called. From RN docs, onScroll Fires at most once per frame during scrolling. The frequency of the events can be controlled using the scrollEventThrottle prop. This isn't very precise so we can't be precise either but I think we should emit events every x ms (not sure about the value, it could change if the scrollEventThrottle prop is defined), making an assumption on the scroll speed to determine how long the scroll would last (scroll speed could be configurable eventually)

Now if the steps are equivalent to multiple scrollTo calls I'm not sure it's worth supporting them. I don't think it will be used that much and it does not make things much simpler imo. So maybe we should stick with a simpler API for now?

  1. If we do steps I would also say only array syntax

  2. I like the proposal, there is just something that's unclear to me, if I do

user.scrollTo({ y: [30], momentumY: [30] })

Does it scroll to 30 with 30 momentum or does it scroll to 60 (30+30 momentum)? If it's the first should we throw if momentum is greater than y? Maybe this behavior could be described through ts-doc

  1. It would be better to remember current scroll position but since elements can't be measured I don't know if it makes a huge difference. Also if scrollview is scrolled programmatically (through a ref) current position will be false because scrollview methods are mocked and don't do anything

@siepra
Copy link
Contributor

siepra commented Aug 21, 2023

  1. I agree with all the others. scrollTo is surely better.

  2. I think usually there're less momentum events than scroll events. Also I think it's important to emit multiple scroll events so one can test against multiple method calls, but maybe it's not that important how many of them will occur. I'd say e.g. 5 regular and 3 momentum scroll events is rational.

re: It looks like it serves two purposes: emitting an onScroll event and also a separate scroll because you can provide momentum for each step (user would stop scrolling between each step). : Momentum will always follow the current direction (it prolongs the gesture) so my original intention was to give the possibility to specify it's value as an addition to the x or y value.

re: Now if the steps are equivalent to multiple scrollTo calls I'm not sure it's worth supporting them : Not exactly. scrollTo emits a combination of events while those intermediate steps are scroll events only.

  1. I like the array approach, but it gives the possibility to do "strange" combinations like changing direction mid-gesture (it may not be that important though).

  2. See the "re:" above.

  3. I agree with @flexbox

@mdjastrzebski
Copy link
Member Author

@pierrezimmermannbam thanks for comments.
re 2: I think I've might written the momentum part in slightly confusing way. So to clarify my intent, momentum scroll will always happen after drag scroll:

user.scrollTo({ x: [0, 5, 10], momentumX: [20, 25, 30] });

Will emit:

scrollBeingDrag 0
scroll 5
scrollEndDrag 10 (experimantally there is on `scroll` event equal to `scrollEndDrag`)
momentumScrollBegin 20
scroll 25
scroll 30
momentumScrollEnd 30 (experimentally last `scroll` event of momentum part is equalt to `momentumScrollEnd`

So that would be equivalent to:

user.scrollTo({ x: [0, 5, 10] });
user.scrollTo({ momentumX: [20, 25, 30] });

@pierrezimmermannbam
Copy link
Collaborator

Oh I see, I didn't understand that, that makes sense. In your example if the drag ends at 10 then shouldn't the momentum begin at 10?

I'm still a bit afraid that this API is too complex and not easily understandable. I'm not convinced that steps bring value and imo they make the API way more complicated and also allow for incoherent values for the momentum like [0, 10, 0]. I can't find use cases where I'd use steps but maybe I haven't encountered some before, what cases can you think of?

For now I think I'd prefer a more simpler API that would look like this:

user.scrollTo({ x: 5, momentumX: 5});

That would emit

scrollBeingDrag 0
scroll 5
scrollEndDrag 5 (experimantally there is on `scroll` event equal to `scrollEndDrag`)
momentumScrollBegin 5
scroll 10
momentumScrollEnd 10

You'd need to have the previous position of the scrollview to know in which direction the momentum is though. It's not great either though because it's still a bit unclear wether you scroll to 5 having included the momentum scroll or to 5 + 5.

@mdjastrzebski
Copy link
Member Author

@pierrezimmermannbam re momentum scroll:

  • All of the values both x and momentumX represent the final scroll value, they are not relative/additive. This is the reason they are named scrollTo not scrollBy. I think this approach makes reasoning about tests much easier. E.g. user.scrollTo({ x: 5, momentumX: 10 }); will do drag scroll till 5 and then momentum scroll till 10 (not 15).
  • When supplying as single number, e.g. scrollTo({ x: 100 }) only specify that final scroll value, there will be intermediate scroll events but you ask UE to calculate these for you. By default there will be e.g. 5 of these intermediate steps.
  • When supplying the array of numbers, e.g. scrollTo({ x: [0, 10, 20] }) you have complete control about intermediate scroll events, these will be 0 (scrollBeginDrag), 10 (scroll), 20 (scrollEndDrag). These are for the case when you want to have control about intermediate values because e.g. you might want to do some assertions, etc.

@mdjastrzebski
Copy link
Member Author

@pierrezimmermannbam, @siepra, @flexbox

I wanted to run two ideas with you:

  1. back-and-forth scroll step patterns, e.g. scrollTo({ y: [0, 100, 50] }): these patterns makes sense for drag scroll as you can drag the scroll view up and down without lifting your finger. For momentum scroll they do not make sense, as it is inertial move in the last drag direction.

  2. momentum-only scroll, e.g. scrollTo({ momentumY: [100, 120, 130] }), should this be allowed and what is the sequence of events generated in this case.

For both of these points we have couple of options:

  • allow it but document that this does not exist in real RN apps.
  • allow it but print warning that this does not exist in real RN apps.
  • throw error and force users to: 1) supply increasing/decreasing only sequence of steps, 2) supply drag scroll when using momentum scroll

@pierrezimmermannbam, @siepra, @flexbox I am curious which of these options you would see as the best, or perhaps some other approach?

@pierrezimmermannbam
Copy link
Collaborator

I think we should not allow the case where there is just momentum because it doesn't male sense and it's easily doable through typescript. Also you should not be able to have momentumX with y scroll. Maybe we don't even need momentumX or momentumY? We could have a type that would look like this:

type NonEmptyArray<T> = [T, ...T[]];

type ScrollParams = ({
  x: NonEmptyArray<number>;
 } | {
    y: NonEmptyArray<number>;
 }) & {
    momentum?: NonEmptyArray<number>;
}

It's harder to prevent inputing values for momentum that make sense using typescript so in this case I think throwing would be my preferred solution. Either that or do nothing but I'm not a huge fan of warnings because they tend to be ignored

@mdjastrzebski
Copy link
Member Author

@pierrezimmermannbam

re cross-axis scroll, e.g. { x: 10, momentumY 20 }:
I've done checks with RN ref.scrollTo({ y, x }) method and it simply ignores the incorrect scroll direction. BTW we are inspecting horizontal prop of ScrollView to only pick the correct scroll direction, so user.scrollTo({ y }) will not emit events when horizontal={true} is set on ScrollView. Currently this check is silently ignoring incorrect scroll axis, although we could make it throwing/warning to match other error.

re: error/warning/ignore approach:
I think we would get better DX when we inform the developer that he is performing invalid action rather than silently ignoring it, as without such feedback user might be confused why UE is not working as intended and waste their time looking for a reason.

@pierrezimmermannbam
Copy link
Collaborator

Even though it doesn't change much if user provides x and y, I think it's still a better DX to only allow one of them. This makes it clearer what you can and cannot do with the API

@siepra
Copy link
Contributor

siepra commented Sep 4, 2023

In a real app, momentum will always prolong the current scroll direction, so it probably doesn't make sense to allow using it as a standalone value without providing X or Y first. Although, I think back-and-forth scroll step pattern sounds fine.

This is just another idea: Momentum value depends on a velocity so it's hard for the user to accurately estimate it. If we wanted to simplify the API, maybe it's worth to just stick to a boolean, and calculate the momentum basing on length of a drag (let's say 1/4 of it), eg:

scrollTo({ x: [0, 100], momentum: true })

would emit

scrollBeingDrag 0
scroll 50
scrollEndDrag 100
momentumScrollBegin 100
scroll 112,5
momentumScrollEnd 125

We can even extend the number of emitted scroll events basing on this "simulated" velocity. This way we kinda won't let the user decide, but irl one doesn't really control it either...

@mdjastrzebski
Copy link
Member Author

@siepra
re momentum scroll:

  1. I agree we should make the drag part (y or x) mandatory, and momentum part optional, as it matches real world interaction.
  2. I think we should require user to specify final scroll position (for drag, and for momentum as well if used). Describing scroll as x: 100, momentum: true resulting in drag scroll happening to x=100 and then on top of that unspecified amount momentum scroll will confuse the users about final scroll position. I think it's clearer to reason about if the same concept is passed as x: 100, momentumX: 125 which would be much more readable.

I imagine that majority of scroll tests will focus on scroll final position. Additionally, some test will want to check intermediate scroll position (onScroll) for which we will have the number[] parameter variant.

re details of intermediate steps:

  1. If the user specifies only the target scroll position then we would pick some intermediate scroll steps BUT the user should not rely on the actual values being stable between RNTL version, we would only consider the final value being API contract. 2. If they want to have control of the intermediate steps they should use the number[] version, and then they can be sure of intermediate steps that will be called.

@siepra
Copy link
Contributor

siepra commented Sep 4, 2023

How about instead of momentum increasing the target value, it stops at X but modifies the emitted events? I'd would work similar but maybe less confusing:

scrollTo({ x: [0, 100], momentum: true })
scrollBeingDrag 0
scroll 50
scrollEndDrag 75
momentumScrollBegin 75
scroll 87,5
momentumScrollEnd 100

Remember I only mention it for sake of API simplification. I think the main question here is what's more important: consistency in terms of direction and final position or control over the number of intermediate events

@mdjastrzebski
Copy link
Member Author

@siepra There is still the issue with in case user want's to specify exact scroll steps for drag/momentum scroll. If you have a single { x: [0, 10, 20, 30], momentum: true }, which are drag and which are momentum? With { x: [0, 10, 20], momentumX: [25, 30] } this is evident.

@siepra
Copy link
Contributor

siepra commented Sep 4, 2023

Of course, I agree. But in this case, someone may do weird combinations like { x: [0, 10, 20, 30], momentumX: [10, 15, 0] }. I'm referring to what's @pierrezimmermannbam pointed out: API is too complex and not easily understandable. I'm not convinced that steps bring value and imo they make the API way more complicated and also allow for incoherent values for the momentum. So my point of view is momentum should not be responsible for actually changing the target position, but rather enhancing the flow of events

@mdjastrzebski
Copy link
Member Author

I think we can all agree that the base API (single number) is reasonably simple and understandable:

// Drag scroll to 100 px (User Event to pick intermediate steps)
scrollTo({ y: 100 }) 

// Drag scroll to 100 px, then momentum scroll to 120 px (UserEvent to pick intermediate steps).
// Note: `y` is required parameter, user cannot pass only `momentumY` without `y`.
scrollTo({ y: 100, momentumY: 120 })

This API should cover all users who want to test the final position of the scroll, without relying on details of intermediate scroll events. We could reduce the scope of the initial release and remove the explicit scroll steps (number[]). Based on user feedback we can decide on adding it later.

@pierrezimmermannbam, @siepra wdyt?

@siepra
Copy link
Contributor

siepra commented Sep 4, 2023

Sure, the smaller releases the better. This API seems fine.

@pierrezimmermannbam
Copy link
Collaborator

I agree, let's keep it simple for now, it should cover the vast majority of use cases and fireEvent is still available for the rest of them

@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
Projects
None yet
Development

No branches or pull requests

6 participants