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

Auto scrolling is not working #25084

Closed
mohanbabu-zumen opened this issue Dec 9, 2022 · 21 comments · Fixed by #29574
Closed

Auto scrolling is not working #25084

mohanbabu-zumen opened this issue Dec 9, 2022 · 21 comments · Fixed by #29574
Assignees
Labels
pkg/reporter This is due to an issue in the packages/reporter directory Triaged Issue has been routed to backlog. This is not a commitment to have it prioritized by the team. type: bug type: regression A bug that didn't appear until a specific Cy version release type: unexpected behavior User expected result, but got another

Comments

@mohanbabu-zumen
Copy link

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

chrome_KYAzJ7OCeX.mp4

Cypress Version

11.2.0

Node version

16.17.0

Operating System

Windows 11

Debug Logs

No response

Other

No response

@WORMSS
Copy link

WORMSS commented Dec 10, 2022

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.

@romankhomitskyi
Copy link

Yeah 10.11 same issue so annoying

@mohanbabu-zumen
Copy link
Author

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

@emilyrohrbough emilyrohrbough added the pkg/reporter This is due to an issue in the packages/reporter directory label Dec 29, 2022
@emilyrohrbough
Copy link
Member

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 emilyrohrbough added stage: awaiting response Potential fix was proposed; awaiting response CT Issue related to component testing labels Dec 29, 2022
@romankhomitskyi
Copy link

romankhomitskyi commented Dec 29, 2022

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
Copy link

romankhomitskyi commented Dec 29, 2022

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
Copy link

It might have smth to do with this issue Issue

@emilyrohrbough
Copy link
Member

@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?

@mohanbabu-zumen
Copy link
Author

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
Copy link
Author

@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.

@romankhomitskyi
Copy link

@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

@emilyrohrbough
Copy link
Member

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

@romankhomitskyi
Copy link

romankhomitskyi commented Dec 30, 2022

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

I gave a details a couple of comments above

@emilyrohrbough
Copy link
Member

@romankhomitskyi whoops. Missed those details. thanks.

@emilyrohrbough
Copy link
Member

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 emilyrohrbough added routed-to-ct type: unexpected behavior User expected result, but got another and removed stage: awaiting response Potential fix was proposed; awaiting response labels Jan 3, 2023
@marktnoonan
Copy link
Contributor

Linking up to the previous issue that mentioned this: #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.

@emilyrohrbough emilyrohrbough added type: bug type: regression A bug that didn't appear until a specific Cy version release labels Mar 13, 2023
@nagash77 nagash77 added Triaged Issue has been routed to backlog. This is not a commitment to have it prioritized by the team. and removed routed-to-ct labels Apr 19, 2023
@scottohara
Copy link
Contributor

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
Copy link
Contributor

scottohara commented May 8, 2024

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.

@jennifer-shehane
Copy link
Member

@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 jennifer-shehane removed the CT Issue related to component testing label May 15, 2024
@cacieprins
Copy link
Contributor

@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.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 18, 2024

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 cypress-bot bot locked as resolved and limited conversation to collaborators Jun 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg/reporter This is due to an issue in the packages/reporter directory Triaged Issue has been routed to backlog. This is not a commitment to have it prioritized by the team. type: bug type: regression A bug that didn't appear until a specific Cy version release type: unexpected behavior User expected result, but got another
Projects
None yet