cypress icon indicating copy to clipboard operation
cypress copied to clipboard

Auto scrolling is not working

Open mohanbabu-zumen opened this issue 2 years ago • 19 comments

Current behavior

Initially, auto-scrolling is enabled. When we run the scripts, it's automatically disabled in the run time.

Desired behavior

It's shouldn't be automatically disabled

Test code to reproduce

https://user-images.githubusercontent.com/102032328/206662654-bde7397f-7741-4113-8d07-d40f0be7d48f.mp4

Cypress Version

11.2.0

Node version

16.17.0

Operating System

Windows 11

Debug Logs

No response

Other

No response

mohanbabu-zumen avatar Dec 09 '22 08:12 mohanbabu-zumen

Yes, this is highly annoying for me also.. It wasn't too bad when "auto scroll" was a checkbox on the toolbar, but now that it's hidden in a sub menu with NOTHING ELSE.. It's a massive hinderance.

WORMSS avatar Dec 10 '22 19:12 WORMSS

Yeah 10.11 same issue so annoying

romankhomitskyi avatar Dec 16 '22 23:12 romankhomitskyi

yes, It's very annoying. Even If it is a radio button like the older version, It does not affect us much.

mohanbabu-zumen avatar Dec 17 '22 14:12 mohanbabu-zumen

I've tried to reproduce this and have not been able to. I believe it is happening for you - I feel like I have noticed this behavior though...It'll be hard for us to track down the root issue and be confident in a fix if we aren't able reproduce this.

@mohanbabu-zumen @romankhomitskyi @WORMSS is this happen in a specific browser or all browsers? also what browser versions are you using?

@romankhomitskyi @WORMSS can you share your OS?

emilyrohrbough avatar Dec 29 '22 14:12 emilyrohrbough

I've tried to reproduce this and have not been able to. I believe it is happening for you - I feel like I have noticed this behavior though...It'll be hard for us to track down the root issue and be confident in a fix if we aren't able reproduce this.

@mohanbabu-zumen @romankhomitskyi @WORMSS is this happen in a specific browser or all browsers? also what browser versions are you using?

@romankhomitskyi @WORMSS can you share your OS?

OS version: Big Sur 11.6.5 Browser: Chrome, it has been reproducing since Cypress 10, during this time browser version have been constantly changing from 102(not sure, could be 101) to 108 (currently) Cypress version: 10.11

Not sure about firefox because I dont run tests in it as much

romankhomitskyi avatar Dec 29 '22 14:12 romankhomitskyi

I mean to reproduce it you might need to have a test with commands logs more than 300 lines

Then do some hover over commands, clicking, messing arround, than move mouse over the Application under test, than retry test, leave mouse on the command log, something like that

romankhomitskyi avatar Dec 29 '22 14:12 romankhomitskyi

It might have smth to do with this issue Issue

romankhomitskyi avatar Dec 29 '22 14:12 romankhomitskyi

@romankhomitskyi If you are clicking on the command logs while the test is executing that will disable the auto-scroll when tests are running. That behavior is expected. Is the issue you are seeing that the next run isn't restoring the autoscroll behavior?

emilyrohrbough avatar Dec 29 '22 17:12 emilyrohrbough

I've tried to reproduce this and have not been able to. I believe it is happening for you - I feel like I have noticed this behavior though...It'll be hard for us to track down the root issue and be confident in a fix if we aren't able reproduce this.

@mohanbabu-zumen @romankhomitskyi @WORMSS is this happen in a specific browser or all browsers? also what browser versions are you using?

@romankhomitskyi @WORMSS can you share your OS?

Hi @emilyrohrbough It's happened in all browsers frequently. My chrome version is 108.0.5359.125

mohanbabu-zumen avatar Dec 29 '22 17:12 mohanbabu-zumen

@romankhomitskyi If you are clicking on the command logs while the test is executing that will disable the auto-scroll when tests are running. That behavior is expected. Is the issue you are seeing that the next run isn't restoring the autoscroll behavior?

@emilyrohrbough Yes. If we click command logs or anything else, then the disable behavior is expected. In my case, I haven't clicked anything. But, It's disabled.

mohanbabu-zumen avatar Dec 29 '22 17:12 mohanbabu-zumen

@romankhomitskyi If you are clicking on the command logs while the test is executing that will disable the auto-scroll when tests are running. That behavior is expected. Is the issue you are seeing that the next run isn't restoring the autoscroll behavior?

Hmmm interesting, I guess I have expirienced both cases

romankhomitskyi avatar Dec 29 '22 18:12 romankhomitskyi

@romankhomitskyi any browser or all? what is your OS?

emilyrohrbough avatar Dec 30 '22 15:12 emilyrohrbough

@romankhomitskyi any browser or all? what is your OS?

I gave a details a couple of comments above

romankhomitskyi avatar Dec 30 '22 15:12 romankhomitskyi

@romankhomitskyi whoops. Missed those details. thanks.

emilyrohrbough avatar Jan 03 '23 15:01 emilyrohrbough

I reached out to @marktnoonan to see if he had any insights to this issue. He said:

I’ve never been able to reproduce it but I think I do understand the issue, which is that lots of stuff filling up the command log quickly can be cause this scroll listener we have on there to fire and disable the auto scroll, even though it’s not actually user scrolling. When this issue was first reported in run mode, where it’s impossible to be the user’s action, I made this PR to avoid reacting to the scroll event in run mode and that fixed it.

Sounds like we understand the basics of the issue at least which should be enough to route this. and Mark has a few ideas on how we might be able to improve this.

emilyrohrbough avatar Jan 03 '23 15:01 emilyrohrbough

Linking up to the previous issue that mentioned this: https://github.com/cypress-io/cypress/issues/24171

Also noticed this morning that there’s already a rubric, which looks arbitrary, that we could change based on trying to avoid "false positives" in scroll listeners: https://github.com/cypress-io/cypress/blob/develop/packages/reporter/src/lib/scroller.ts#L48-L49

That would be a good place to do another checks we think are needed, or we could just beef up the threshold for how many scroll events are needed to prove it's the user and not the browser reacting to elements populating the log.

marktnoonan avatar Jan 03 '23 15:01 marktnoonan

For what it's worth, I'm still experiencing this issue.

I added some logpoints and confirmed that, without any user scrolling at all (hands completely away from all pointing devices), I do see a number of scroll events within a 50ms window which is enough to nullify the "false positive" guard and disable autoscrolling.

In my case, anecdotally it seems to occur at a particular test that has a slow-ish set of commands (enough to exceed the 1s threshold and automatically expand the command log), and after the test passes and the command log is automatically collapsed, the appState.autoScrollEnabled flag that was previously true is now false and all scrolling ceases.

It would seem that the assumption of "3 or more scroll events within a 50ms timeframe === user is scrolling" is an unreliable one, and that some browsers (in my case, Chrome) may trigger scroll events without any intentional user activity.

Perhaps instead of counting scroll events, another heuristic might be if the scroll position indicated in the scroll event is a certain distance away from the scroll position calculated by the scroller? (i.e. user must actually scroll a minimum threshold before autoscrolling is disabled?)

scottohara avatar Mar 18 '24 05:03 scottohara

I have a hypothesis on what may be causing this.

(Apologies, I don't yet have anything that I'm able to share that will reproduce the issue, but I can reliably reproduce it in our internal test suite. I hope to follow up this comment with a link to an example at some point, if necessary).

First, here's what I've learned so far about how auto-scrolling is supposed to work (for those that might be unfamiliar with the code):

How auto-scrolling works

The Scroller class defines a scrollIntoView(element) method, which is used for "programmatically" scrolling the reporter. This method:

  1. Computes a scrollTopGoal that will position the passed element just above the bottom of the container
  2. Sets this._userScroll = false on the Scroller class. This is used to signal that the next scroll event was not user-initiated.
  3. Sets this._container.scrollTop = scrollTopGoal. This scrolls the container, and causes the browser to fire a scroll event.

In the same Scroller class, the _listenToScrolls(onUserScroll) method sets up an event listener on the container to handle scroll events. When a scroll event is handled:

  1. If this._userScroll === false, set it to true and return immediately. That is to say, "if the scroll event was triggered programmatically, then do nothing"
  2. Otherwise, it treats the event as possibly being related to a user-initiated scroll. In this case, it increments a counter (this._userScrollCount++).
  3. If the counter ever reaches 3 (or higher), call the passed onUserScroll callback, reset the counter to zero, and return.
  4. Otherwise, schedule a timer to fire in 50ms that resets the counter to zero. That is all to say "if 3+ user-initiated scroll events are detected within a 50ms time span, the user must be trying to scroll so disable the auto-scroll"

With that brief explainer out of the way, here's what I believe is actually happening...

How auto-scrolling breaks (in theory)

Under perfect conditions, based on the above logic, we would expect things to execute in the following order:

Event loop tick Code executed Resulting value of _userScroll Resulting value of _userScrollCount Scroll assumed to be
1 scrollIntoView() false 0
2 scroll handler true 0 ✅ programmatic
3 scrollIntoView() false 0
4 scroll handler true 0 ✅ programmatic
5 scrollIntoView() false 0
6 scroll handler true 0 ✅ programmatic

That is:

  • A call to scrollIntoView() sets _userScroll = false and programmatically scrolls the container
  • This is immediately followed by the browser queueing a scroll event
  • The scroll event is handled which resets _userScroll = true, and _userScrollCount remains as zero
  • Repeat...

However, as scroll events are fired/queued/handled asynchronously, it seems possible that a succession of scrollIntoView() calls could execute before the browser has a chance to queue the corresponding scroll events.

With some additional logging in both scrollIntoView() and the scroll handler, I've been able to observe things executing in an undesired order such as:

Event loop tick Code executed Resulting value of _userScroll Resulting value of _userScrollCount Scroll assumed to be
1 scrollIntoView() false 0
2 scrollIntoView() false 0
3 scrollIntoView() false 0
4 scrollIntoView() false 0
5 scroll handler true 0 ✅ programmatic
6 scroll handler true 1 ❌ user-initiated
7 scroll handler true 2 ❌ user-initiated
8 scroll handler true 3 ❌ user-initiated
9 onUserScroll callback true 0 ❌ (disable auto-scrolling)

That is:

  • Four successive calls to scrollIntoView() execute before the browser is able to schedule any scroll events.
  • The first scroll event is handled which resets _userScroll = true as normal
  • The next three scroll events are handled, but because _userScroll === true, these are assumed to be user-initiated, and the counter increments until it reaches 3, calls onUserScroll and disables auto-scrolling.

Solution?

Given the asynchronous nature of event handling, and the possibility of things to occur in a different order than you would normally expect; it would seem that using a simple boolean toggle to distinguish between programmatic vs user-initiated scrolls is insufficient.

One possible alternate approach would be to change _userScroll from a boolean to a number; with each call to scrollIntoView incrementing the number, and each handled scroll event decrementing the number:

Event loop tick Code executed Resulting value of _userScroll Resulting value of _userScrollCount Scroll assumed to be
1 scrollIntoView() 1 0
2 scrollIntoView() 2 0
3 scrollIntoView() 3 0
4 scroll handler 2 0 ✅ programmatic
5 scroll handler 1 0 ✅ programmatic
6 scroll handler 0 0 ✅ programmatic
7 scroll handler 0 1 ✅ user-initiated
8 scroll handler 0 2 ✅ user-initiated
9 scroll handler 0 3 ✅ user-initiated
10 onUserScroll callback 0 0 ✅ (disable auto-scrolling)

In this model, _userScrollCount would only increment when there are more scroll events being handled than there are calls to scrollIntoVIew().

Or _userScroll could be removed entirely, and simply allow _userScrollCount to go negative:

Event loop tick Code executed Resulting value of _userScrollCount Scroll assumed to be
1 scrollIntoView() -1
2 scrollIntoView() -2
3 scrollIntoView() -3
4 scroll handler -2 ✅ programmatic
5 scroll handler -1 ✅ programmatic
6 scroll handler 0 ✅ programmatic
7 scroll handler 1 ✅ user-initiated
8 scroll handler 2 ✅ user-initiated
9 scroll handler 3 ✅ user-initiated
10 onUserScroll callback 0 ✅ (disable auto-scrolling)

With more accurate counting of scrollIntoView calls vs handled scroll events; it may even be possible to remove the "3 or more non-programmatic events in a 50ms time span" rubric altogether, and simply assume the user is scrolling as soon as the number of scroll events exceeds the number of calls to scrollIntoView?

I hope this helps.

scottohara avatar May 08 '24 04:05 scottohara

@scottohara Thanks for investigating! This is great. Mostly our problem was even beginning to reliably reproduce. I'll get the team to take a look at this overview.

jennifer-shehane avatar May 08 '24 16:05 jennifer-shehane

@scottohara Thank you so much for this in-depth investigation! We would welcome a PR that addresses the issue with either approach outlined, though a slight preference is granted to the negative-number approach.

cacieprins avatar May 24 '24 19:05 cacieprins

Released in 13.12.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to Cypress v13.12.0, please open a new issue.

cypress-bot[bot] avatar Jun 18 '24 22:06 cypress-bot[bot]