fix: do not magically set user preferences for auto-scroll #26102

AtofStryker · 2023-03-14T14:15:33Z

Additional details

Fixes a critical bug where the user's auto-scroll preference is being implicitly set by the app when auto-scroll is implicitly toggled. User preferences should be explicitly set by the user and not implicitly mutated by the app.

The application turns off auto-scroll when the user:

scrolls through the command log
presses the a key as a shortcut to toggle the user preference

Before

What was happening, is that we were conflating the user preference's auto scroll switch (explicit setting) with that of implicitly setting auto scroll. When the above behavior to scroll the command log disabled auto scroll, it was then synced to the preference when:

the preference menu was opened
the command log was clicked
If autoscroll was "temporarily" disabled, the preference would get toggled back on at the end of the run (seen in GIF). However, if the user refreshes the run before the test is over, auto scroll would be turned off in preferences

After

Implicit auto scroll behavior and explicit user preferences should be separate configurations/concepts. Now, if auto scroll is enabled as a user preference, auto scroll will happen unless:

the user disables it under preferences (explicitly)
presses the a key as a shortcut to toggle the user preference (explicitly)
the user scrolls the command log (implicitly). With implicit auto scroll disabled, the user preference flag is NOT overridden. If auto scroll is implicitly disabled, and the test is rerun, the test will start auto scrolling again. If the user explicitly turns off auto scroll in preferences and reruns the tests, auto scroll will be disabled. If it is re enabled as the test is running, auto scroll will automatically start happening again. The user preference flag, when set, will override any current behavior

Steps to test

A few edited tests to adjust for the behavior plus manual test as seen in GIF.

How has the user experience changed?

Users should no longer see side effects from auto-scroll implicitly configuring their user preferences. Instead, users now need to explicitly enable / disable auto scroll under user preferences, which is enabled by default. This behavior is more predictable and desired by users.

PR Tasks

Have tests been added/updated?
(n/a) Has a PR for user-facing changes been opened in cypress-documentation?
(n/a) Have API changes been updated in the type definitions?

…havior is disabled when a user scrolls or clicks inside the reporter or opens user preferences. Users must explicitly toggle auto-scroll on/off inside the user preferences menu and any implicit behavior will be overidden when this configuration value is set.

…roll

AtofStryker · 2023-03-14T14:52:40Z

we will likely need to patch this with 12.8.1 later this week

cypress · 2023-03-14T15:21:27Z

Passing run #44768 ↗︎

0	84	0	0	0

Details:

Merge branch 'spike_scroll' of github.com:cypress-io/cypress into spike_scroll
Project: cypress	Commit: `9ad1f6f493`
Status: Passed	Duration: 14:40 💡
Started: Mar 15, 2023 3:42 PM	Ended: Mar 15, 2023 3:57 PM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

mjhenkes · 2023-03-14T15:29:43Z

cli/CHANGELOG.md

@@ -15,6 +15,7 @@ _Released 03/14/2023 (PENDING)_
 - Fixed an issue where using `Cypress.require()` would throw the error `Cannot find module 'typescript'`. Fixes [#25885](https://github.com/cypress-io/cypress/issues/25885).
 - The [`before:spec`](https://docs.cypress.io/api/plugins/before-spec-api) API was updated to correctly support async event handlers in `run` mode. Fixes [#24403](https://github.com/cypress-io/cypress/issues/24403).
 - Updated the Component Testing community framework definition detection logic to take into account monorepo structures that hoist dependencies. Fixes [#25993](https://github.com/cypress-io/cypress/issues/25993)
+- Fixed an issue where the reporter auto-scroll configuration inside user preferences was being set implicitly. User's must now explicitly enable/disable auto-scroll under user preferences, which is enabled by default. Fixes [#24171](https://github.com/cypress-io/cypress/issues/24171) and [#25084](https://github.com/cypress-io/cypress/issues/25084)


was this a regression? if so do you know when it was introduced?

It was a v10 regression

I'm glad to see this fix, but I'm not sure it lets us close those issues. For example in #25084 the setting being changed is a side-effect of another problem. I think it goes like this in the gif for that issue:

Cypress incorrectly detects user scrolling over the command log with no mouse activity when many commands are added close together. The is open-mode version of Auto scroll is turned off sometimes #16098, possibly caused by false positives mentioned here.

Cypress incorrectly mutates the saved user preference for scrolling <-- the thing we are fixing now in this PR

User moves the mouse again and opens the settings panel to show that auto scrolling was turned off

But in #16098 what's being reported is the first problem, because the user has not intended to enter the "temporarily disable autoscrolling" state.

@marktnoonan that's a good point. I don't think we can close #25084 (maybe note on the issue that the config should no longer be mutated?) for the false positive scroll issues, but would we be able to close #24171? I'm not sure #24171 is referring to the false positive scroll listener or the preference behavior.

I think the comment in #24171 refers to the setting being turned off and the person being unaware. But the issue itself refers to "Users are reporting that the command log stops auto-scrolling during test execution unexpectedly."

We could close it as a dupe. Is there any open issue that is just about the preferences being set implicitly? That's what I would link to here, for clarity especially when reading the changelog later. We might need an new issue that isolates this.

@marktnoonan I'll create a new issue and link it here and update the changelog

@marktnoonan created #26113. will link in the PR and update the changelog

packages/reporter/src/lib/app-state.ts

cli/CHANGELOG.md

marktnoonan

Haven't tested yet but looks good.

It's unfortunate that this regression happened in the same release that moved this setting out of view at the top of the command log, the incorrect behavior would have been easier to catch if something visible was changing unexpectedly.

Thinking about it more, it seems odd to me that there is a keyboard hotkey to modify this setting but there's no visible feedback that you have done so. So like, you hit the a key at the wrong time and place and you have saved a preference for scrolling behavior in your tests.

I'm going to suggest we lift this setting back up to the top level instead of being the only item in a menu that needs to be expanded, so users can have a better idea of what their scrolling preference is set to. (Not a suggestion for this PR, just tracking the observation here).

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

AtofStryker · 2023-03-14T17:01:28Z

Thinking about it more, it seems odd to me that there is a keyboard hotkey to modify this setting but there's no visible feedback that you have done so. So like, you hit the a key at the wrong time and place and you have saved a preference for scrolling behavior in your tests.

@marktnoonan I very much agree with this. A user could toggle this off without even knowing.

I'm going to suggest we lift this setting back up to the top level instead of being the only item in a menu that needs to be expanded, so users can have a better idea of what their scrolling preference is set to. (Not a suggestion for this PR, just tracking the observation here).

I also agree with this. I am going to make a github issue for this.

…roll

AtofStryker · 2023-03-14T17:30:39Z

@marktnoonan I created #26107 to track the auto scroll proposed UI changes. Should I route this to CT?

marktnoonan · 2023-03-14T18:04:53Z

Thanks @AtofStryker - no need to route it yet, I'll add it for review as a feature request and hopefully we can move it through pretty quick.

AtofStryker · 2023-03-14T18:43:20Z

should fix the persisting of user preferences for #25084 but does NOT fix the phantom scroll detection

marktnoonan

However, if the user refreshes the run before the test is over

I suspect this detail is why this hasn't presented itself more widely as a bug, if it's largely self correcting unless you refresh at the wrong time. Combined with the fact that the on/off state isn't visible in the runner.

packages/reporter/src/lib/events.ts

astone123

Worked well in my testing 👍🏻

marktnoonan · 2023-03-14T19:00:55Z

This has the expected effect. In the PR description you mention

A few edited tests to adjust for the behavior

But I don't see any test that would catch a regression here. If we want to get this out quickly I understand. Could we get a follow up task for an automated test that would fail on the old code and pass on the new?

Co-authored-by: Mark Noonan <mark@cypress.io>

AtofStryker · 2023-03-14T19:14:27Z

@marktnoonan I'm working on writing a cy-in-cy test currently since circleci is currently down

…plicitly turned off

…ke_scroll

AtofStryker · 2023-03-14T20:26:10Z

@marktnoonan I added a regression test in 19c738b in the form of a cy-in-cy test. It's not the prettiest looking test, but it does function as a e2e regression test for this behavior. @mjhenkes @emilyrohrbough @astone123 would you all mind taking another look as well with this test added. Figured I can add it as we wait for circleCI to recover 😅

develop

this branch

system-tests/projects/cypress-in-cypress/cypress/e2e/dom-content-scrollable-commands.spec.js

…nt-scrollable-commands.spec.js Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

marktnoonan · 2023-03-15T13:46:16Z

packages/app/cypress/e2e/reporter_header.cy.ts

+      cy.get('[data-cy="reporter-panel"] .reporter > header + .container').its('0').then(($runnableContainer) => {
+        // scroll the container to the top.
+        // fire multiple scroll events so our scroller component believes the scroll came from an actual user.
+        [...Array(10).keys()].forEach(() => {


Wonder if it's work a comment linking to https://github.com/cypress-io/cypress/blob/develop/packages/reporter/src/lib/scroller.ts#L37 to explain why this works?

I would think so. My only concern is if the file changed and the reference became out of data

Yeah and thinking about it, if you are doing something that breaks this test, you've almost certainly been in that exact function, probably no need for comment.

…-cy test

…roll

…ke_scroll

AtofStryker added 2 commits March 13, 2023 20:20

[run ci]

9f7f205

AtofStryker changed the title ~~Spike scroll~~ fix: do not magically set user preferences for auto-scroll Mar 14, 2023

AtofStryker self-assigned this Mar 14, 2023

AtofStryker added 2 commits March 14, 2023 10:44

chore: add CHANGELOG entry

0a38681

Merge branch 'develop' of github.com:cypress-io/cypress into spike_sc…

0ded1eb

…roll

AtofStryker marked this pull request as ready for review March 14, 2023 14:51

AtofStryker requested review from astone123, emilyrohrbough and marktnoonan March 14, 2023 14:51

mjhenkes reviewed Mar 14, 2023

View reviewed changes

emilyrohrbough reviewed Mar 14, 2023

View reviewed changes

packages/reporter/src/lib/app-state.ts Outdated Show resolved Hide resolved

emilyrohrbough approved these changes Mar 14, 2023

View reviewed changes

emilyrohrbough reviewed Mar 14, 2023

View reviewed changes

cli/CHANGELOG.md Outdated Show resolved Hide resolved

marktnoonan reviewed Mar 14, 2023

View reviewed changes

Update cli/CHANGELOG.md

14a0957

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

Merge branch 'develop' of github.com:cypress-io/cypress into spike_sc…

41c9154

…roll

AtofStryker requested a review from mjhenkes March 14, 2023 17:19

AtofStryker added 3 commits March 14, 2023 14:17

chore: move bugfix to 12.8.1 in changelog

28c6759

chore: remove issue 25084 from fix

f821276

chore: rename autoScrollingUnderUserPreference to autoScrollingUserPref

aebfa73

AtofStryker requested a review from marktnoonan March 14, 2023 18:23

mjhenkes approved these changes Mar 14, 2023

View reviewed changes

marktnoonan approved these changes Mar 14, 2023

View reviewed changes

packages/reporter/src/lib/events.ts Outdated Show resolved Hide resolved

empty

371050e

astone123 approved these changes Mar 14, 2023

View reviewed changes

Update packages/reporter/src/lib/events.ts

74e3230

Co-authored-by: Mark Noonan <mark@cypress.io>

AtofStryker added 2 commits March 14, 2023 16:21

test: add cy-in-cy regression test to make sure auto scroll is not im…

19c738b

…plicitly turned off

Merge branch 'spike_scroll' of github.com:cypress-io/cypress into spi…

d925a92

…ke_scroll

AtofStryker linked an issue Mar 14, 2023 that may be closed by this pull request

temporary scroll behavior implicitly overrides user preference scrolling #26113

Closed

AtofStryker added 2 commits March 14, 2023 16:59

chore: update changelog entry to include #26113

b3ac61a

Merge branch 'develop' into spike_scroll

88da3ed

emilyrohrbough reviewed Mar 15, 2023

View reviewed changes

system-tests/projects/cypress-in-cypress/cypress/e2e/dom-content-scrollable-commands.spec.js Outdated Show resolved Hide resolved

emilyrohrbough approved these changes Mar 15, 2023

View reviewed changes

Update system-tests/projects/cypress-in-cypress/cypress/e2e/dom-conte…

cc29faf

…nt-scrollable-commands.spec.js Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

marktnoonan reviewed Mar 15, 2023

View reviewed changes

AtofStryker and others added 9 commits March 15, 2023 09:57

Merge branch 'develop' into spike_scroll

9d65937

chore: update the specs list app test since we have added a new cy-in…

e7c19d9

…-cy test

Merge branch 'develop' of github.com:cypress-io/cypress into spike_sc…

fd45631

…roll

Merge branch 'spike_scroll' of github.com:cypress-io/cypress into spi…

32c1b5d

…ke_scroll

chore: update the specChange-subscription tests

cfea4b8

Merge branch 'develop' into spike_scroll

07099fd

Update CHANGELOG.md

9843a91

chore: update missed assertion update in spec change subscription

a1eb8da

Merge branch 'spike_scroll' of github.com:cypress-io/cypress into spi…

9ad1f6f

…ke_scroll

AtofStryker merged commit ef430e9 into develop Mar 15, 2023

AtofStryker deleted the spike_scroll branch March 15, 2023 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: do not magically set user preferences for auto-scroll #26102

fix: do not magically set user preferences for auto-scroll #26102

AtofStryker commented Mar 14, 2023 •

edited

Loading

AtofStryker commented Mar 14, 2023

cypress bot commented Mar 14, 2023 •

edited

Loading

mjhenkes Mar 14, 2023

emilyrohrbough Mar 14, 2023

marktnoonan Mar 14, 2023 •

edited

Loading

AtofStryker Mar 14, 2023

marktnoonan Mar 14, 2023

AtofStryker Mar 14, 2023

AtofStryker Mar 14, 2023

marktnoonan left a comment •

edited

Loading

AtofStryker commented Mar 14, 2023

AtofStryker commented Mar 14, 2023

marktnoonan commented Mar 14, 2023

AtofStryker commented Mar 14, 2023

marktnoonan left a comment •

edited

Loading

astone123 left a comment

marktnoonan commented Mar 14, 2023

AtofStryker commented Mar 14, 2023

AtofStryker commented Mar 14, 2023

marktnoonan Mar 15, 2023

AtofStryker Mar 15, 2023

marktnoonan Mar 15, 2023

fix: do not magically set user preferences for auto-scroll #26102

fix: do not magically set user preferences for auto-scroll #26102

Conversation

AtofStryker commented Mar 14, 2023 • edited Loading

Additional details

Before

After

Steps to test

How has the user experience changed?

PR Tasks

AtofStryker commented Mar 14, 2023

cypress bot commented Mar 14, 2023 • edited Loading

Passing run #44768 ↗︎

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marktnoonan Mar 14, 2023 • edited Loading

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

marktnoonan left a comment • edited Loading

Choose a reason for hiding this comment

AtofStryker commented Mar 14, 2023

AtofStryker commented Mar 14, 2023

marktnoonan commented Mar 14, 2023

AtofStryker commented Mar 14, 2023

marktnoonan left a comment • edited Loading

Choose a reason for hiding this comment

astone123 left a comment

Choose a reason for hiding this comment

marktnoonan commented Mar 14, 2023

AtofStryker commented Mar 14, 2023

AtofStryker commented Mar 14, 2023

develop

this branch

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker commented Mar 14, 2023 •

edited

Loading

cypress bot commented Mar 14, 2023 •

edited

Loading

marktnoonan Mar 14, 2023 •

edited

Loading

marktnoonan left a comment •

edited

Loading

marktnoonan left a comment •

edited

Loading