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

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

Merged
merged 25 commits into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e172e39
fix: Do not magically mutate the user preferences when auto scroll be…
AtofStryker Mar 13, 2023
9f7f205
[run ci]
AtofStryker Mar 13, 2023
0a38681
chore: add CHANGELOG entry
AtofStryker Mar 14, 2023
0ded1eb
Merge branch 'develop' of github.com:cypress-io/cypress into spike_sc…
AtofStryker Mar 14, 2023
14a0957
Update cli/CHANGELOG.md
AtofStryker Mar 14, 2023
41c9154
Merge branch 'develop' of github.com:cypress-io/cypress into spike_sc…
AtofStryker Mar 14, 2023
28c6759
chore: move bugfix to 12.8.1 in changelog
AtofStryker Mar 14, 2023
f821276
chore: remove issue 25084 from fix
AtofStryker Mar 14, 2023
aebfa73
chore: rename autoScrollingUnderUserPreference to autoScrollingUserPref
AtofStryker Mar 14, 2023
371050e
empty
AtofStryker Mar 14, 2023
74e3230
Update packages/reporter/src/lib/events.ts
AtofStryker Mar 14, 2023
19c738b
test: add cy-in-cy regression test to make sure auto scroll is not im…
AtofStryker Mar 14, 2023
d925a92
Merge branch 'spike_scroll' of github.com:cypress-io/cypress into spi…
AtofStryker Mar 14, 2023
b3ac61a
chore: update changelog entry to include #26113
AtofStryker Mar 14, 2023
88da3ed
Merge branch 'develop' into spike_scroll
AtofStryker Mar 15, 2023
cc29faf
Update system-tests/projects/cypress-in-cypress/cypress/e2e/dom-conte…
AtofStryker Mar 15, 2023
9d65937
Merge branch 'develop' into spike_scroll
AtofStryker Mar 15, 2023
e7c19d9
chore: update the specs list app test since we have added a new cy-in…
AtofStryker Mar 15, 2023
fd45631
Merge branch 'develop' of github.com:cypress-io/cypress into spike_sc…
AtofStryker Mar 15, 2023
32c1b5d
Merge branch 'spike_scroll' of github.com:cypress-io/cypress into spi…
AtofStryker Mar 15, 2023
cfea4b8
chore: update the specChange-subscription tests
AtofStryker Mar 15, 2023
07099fd
Merge branch 'develop' into spike_scroll
AtofStryker Mar 15, 2023
9843a91
Update CHANGELOG.md
emilyrohrbough Mar 15, 2023
a1eb8da
chore: update missed assertion update in spec change subscription
AtofStryker Mar 15, 2023
9ad1f6f
Merge branch 'spike_scroll' of github.com:cypress-io/cypress into spi…
AtofStryker Mar 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a v10 regression

Copy link
Contributor

@marktnoonan marktnoonan Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. Cypress incorrectly mutates the saved user preference for scrolling <-- the thing we are fixing now in this PR
  3. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

AtofStryker marked this conversation as resolved.
Show resolved Hide resolved

**Misc:**

Expand Down
2 changes: 1 addition & 1 deletion packages/reporter/cypress/e2e/unit/events.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ describe('events', () => {
})

it('emits save:state on save:state', () => {
appState.autoScrollingEnabled = false
appState.autoScrollingEnabledUnderUserPreferences = false
appState.isSpecsListOpen = true
events.emit('save:state')
expect(runner.emit).to.have.been.calledWith('save:state', {
Expand Down
22 changes: 22 additions & 0 deletions packages/reporter/src/lib/app-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const defaults: DefaultAppState = {
}

class AppState {
@observable autoScrollingEnabledUnderUserPreferences = true
AtofStryker marked this conversation as resolved.
Show resolved Hide resolved
@observable autoScrollingEnabled = true
@observable isSpecsListOpen = false
@observable isPaused = defaults.isPaused
Expand Down Expand Up @@ -69,6 +70,14 @@ class AppState {
this.setAutoScrolling(!this.autoScrollingEnabled)
}

/**
* Toggles the auto-scrolling user preference to true|false. This method should only be called from the
* preferences menu itself.
*/
toggleAutoScrollingUnderUserPreferences () {
this.setAutoScrollingEnabledUnderUserPreferences(!this.autoScrollingEnabledUnderUserPreferences)
}

toggleSpecList () {
this.isSpecsListOpen = !this.isSpecsListOpen
}
Expand All @@ -88,6 +97,19 @@ class AppState {
}
}

/**
* Sets the auto scroll user preference to true|false.
* When this preference is set, it overrides any temporary auto scrolling behaviors that may be in effect.
* @param {boolean | null | undefined} isEnabled - whether or not auto scroll should be enabled or disabled.
* If not a boolean, this method is a no-op.
*/
setAutoScrollingEnabledUnderUserPreferences (isEnabled?: boolean | null) {
if (isEnabled != null) {
this.autoScrollingEnabledUnderUserPreferences = isEnabled
this.setAutoScrolling(isEnabled)
}
}

setStudioActive (studioActive: boolean) {
this.studioActive = studioActive
}
Expand Down
3 changes: 2 additions & 1 deletion packages/reporter/src/lib/events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,8 @@ const events: Events = {

localBus.on('save:state', () => {
runner.emit('save:state', {
autoScrollingEnabled: appState.autoScrollingEnabled,
// the "autoScrollingEnabled" key refers to the preference value itself, not the "autoScrollingEnabled" variable stored in application state
AtofStryker marked this conversation as resolved.
Show resolved Hide resolved
autoScrollingEnabled: appState.autoScrollingEnabledUnderUserPreferences,
isSpecsListOpen: appState.isSpecsListOpen,
})
})
Expand Down
2 changes: 1 addition & 1 deletion packages/reporter/src/lib/shortcuts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class Shortcuts {
case 'n': events.emit('next')
break
case 'a': action('set:scrolling', () => {
appState.setAutoScrolling(!appState.autoScrollingEnabled)
appState.toggleAutoScrollingUnderUserPreferences()
events.emit('save:state')
})()

Expand Down
3 changes: 2 additions & 1 deletion packages/reporter/src/main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ class Reporter extends Component<SingleReporterProps> {
}

action('set:scrolling', () => {
appState.setAutoScrolling(autoScrollingEnabled)
// set the initial enablement of auto scroll configured inside the user preferences when the app is loaded
appState.setAutoScrollingEnabledUnderUserPreferences(autoScrollingEnabled)
})()

action('set:specs:list', () => {
Expand Down
8 changes: 4 additions & 4 deletions packages/reporter/src/preferences/testing-preferences.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ const TestingPreferences = observer(({
events = defaultEvents,
appState,
}: Props) => {
const toggleAutoScrolling = () => {
appState.toggleAutoScrolling()
const toggleAutoScrollingUnderUserPreferences = () => {
appState.toggleAutoScrollingUnderUserPreferences()
events.emit('save:state')
}

Expand All @@ -31,8 +31,8 @@ const TestingPreferences = observer(({
Auto-scrolling
<Switch
data-cy="auto-scroll-switch"
value={appState.autoScrollingEnabled}
onUpdate={action('toggle:auto:scrolling', toggleAutoScrolling)}
value={appState.autoScrollingEnabledUnderUserPreferences}
onUpdate={action('toggle:auto:scrolling', toggleAutoScrollingUnderUserPreferences)}
/>
</div>
<div>
Expand Down