fix(driver): Sticky elements within a fixed container will not prevent an element from being scrolled to #18441

davidmunechika · 2021-10-11T17:29:06Z

Closes Sticky elements within a fixed container are not properly taken into account when scrolling to an element #4233

User facing changelog

Sticky elements within a fixed container will now be able to be scrolled to.

Additional details

Flowchart detailing the new actionability behavior: https://whimsical.com/actionability-J38eY9K2Y3vA6uCMWtmLVA

Previously, sticky elements could cover up elements that user would try to scroll to, causing failed tests such as the one below:

This error would happen because cy.ensureVisibility() would fail before our nudging algorithm would run. This PR extracts some of the visibility checks related to being covered up by ancestors into a new ensure function and runs these checks after the nudging algorithm scrolls the element into view.

PR Tasks

Have tests been added/updated?
Has the original issue or this PR been tagged with a release in ZenHub?
Has a PR for user-facing changes been opened in cypress-documentation? --> Sticky elements within a fixed container will not prevent an element from being scrolled to cypress-documentation#4153 (wip)
Have API changes been updated in the type definitions? --> N/A
Have new configuration options been added to the cypress.schema.json? --> N/A

cypress-bot · 2021-10-11T17:29:08Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2021-10-11T17:38:04Z

Test summary

4227 • 1 • 50 • 2 • 1

Run details


Project	cypress
Status	Failed
Commit	`5aa6dc7`
Started	Nov 2, 2021 3:30 PM
Ended	Nov 2, 2021 3:42 PM
Duration	11:14 💡
OS	Linux Debian - 10.9
Browser	Electron 93

View run in Cypress Dashboard ➡️

Failures

	cypress/integration/commands/navigation_spec.js		1
1	... > throws when qs is null

Flakiness

	cypress/integration/commands/net_stubbing_spec.ts		1
1	network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

jennifer-shehane

I don't think that we should use the rotate scrollBehavior if the user has specified a scrollBehavior to overwrite the default. They may be testing that an element is clickable with a specific scroll behavior - that you can scroll to bottom and always click for example. What would be the use of passing scrollBehavior if it is always overridden?

I do however think that most people use the scrollBehavior option as a fix for our issues of not finding the thing to click on, but this would be a breaking change imo.

This does fix the initial issue.

chrisbreiding

We should also add a test covering when the user specifies the scroll behavior.

I think it could basically be the same as the test you added, but pass in a scrollBehavior to cy.click() that makes it fail. Then test it's the right failure. If you're not sure how to do that, look at tests that use cy.on('fail', ...). There are a bunch of them in the driver tests.

packages/driver/src/cy/actionability.ts

packages/driver/src/cy/commands/actions/click.ts

Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>

chrisbreiding

Looks great 👍 Nice work!

chrisbreiding

Sorry, I approved this too soon. We need to take into account all of the commands that can receive scrollBehavior as an option. That means setting the userSetScrollBehavior in each command and adding tests for each one too.

I think these are all the other commands that accept scrollBehavior:

check
clear
dblclick
trigger
type
uncheck

packages/driver/cypress/integration/commands/actions/click_spec.js

…cypress-io/cypress into issue-4233-click-sticky-element

packages/driver/cypress/integration/commands/actions/click_spec.js

Dismissing previous review

* develop: (40 commits) fix(driver): Sticky elements within a fixed container will not prevent an element from being scrolled to (#18441) chore: make `create` function on server.ts obsolete (#18615) docs: Add instructions to squash commits to develop in Contributing (#18728) fix(@cypress/react): throw if using Next.js swc-loader without nodeVersion=system (#18686) refactor: remove Ramda (#18723) chore: Increase paralleled machines for desktop-gui tests (#18725) chore: Update Chrome (stable) to 95.0.4638.69 (#18696) chore: release @cypress/vue-v3.0.4 chore: release @cypress/react-v5.10.2 chore: release @cypress/schematic-v1.5.3 fix: remove outdated registry link (#18710) chore: release @cypress/schematic-v1.5.2 chore: release create-cypress-tests-v1.1.3 chore: Update Chrome (beta) to 96.0.4664.27 (#18676) chore(tests): Remove flaky assertion that relies on png how compression (#18668) fix: make sure to go back to no-specs when delete spec file (#17760) fix: Next.JS 12 components testing failing with ` TypeError: Cannot read property 'traceChild' of undefined` (#18648) Backport .gitignore from unified-desktop-gui chore(docs): add 'Upgrading Electron' instructions (#18594) release 8.7.0 [skip ci] ...

…ve-activeProject * unified-desktop-gui: (57 commits) chore: Add e2e tests for global mode (#18719) chore: add percy to app and launchpad package (#18781) chore: update test refactor: move settings in app (#18729) feat: setup launchpad lifecycle (#18734) feat(app): decouple event manager from driver (#18695) chore: Force single resolution for core modules, infinite loop guard (#18764) fix(driver): Sticky elements within a fixed container will not prevent an element from being scrolled to (#18441) chore: cleaning up the runner container pattern (#18741) feat: Use .config files (#18578) chore(app): basic style and example to stop scrollIntoView bug (#18736) chore: make `create` function on server.ts obsolete (#18615) feat: add codegen utility (#18708) docs: Add instructions to squash commits to develop in Contributing (#18728) fix(@cypress/react): throw if using Next.js swc-loader without nodeVersion=system (#18686) refactor: remove Ramda (#18723) fix: support using create-cypress-tests as part of build process (#18714) chore: Increase paralleled machines for desktop-gui tests (#18725) fix(app): do not cache graphql (#18716) chore: Update Chrome (stable) to 95.0.4638.69 (#18696) ...

…t an element from being scrolled to (#18441) Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>

davidmunechika added 3 commits October 6, 2021 11:19

add test

271dcbd

try other scroll behavior options

43fb341

refactor scroll until visible

496945b

davidmunechika self-assigned this Oct 11, 2021

davidmunechika marked this pull request as ready for review October 12, 2021 14:17

davidmunechika requested a review from a team as a code owner October 12, 2021 14:17

davidmunechika requested review from BlueWinds and emilyrohrbough and removed request for a team October 12, 2021 14:17

davidmunechika mentioned this pull request Oct 12, 2021

Sticky elements within a fixed container will not prevent an element from being scrolled to cypress-io/cypress-documentation#4153

Closed

jennifer-shehane requested review from chrisbreiding and jennifer-shehane and removed request for BlueWinds and emilyrohrbough October 12, 2021 14:42

jennifer-shehane previously requested changes Oct 12, 2021

View reviewed changes

davidmunechika and others added 2 commits October 12, 2021 15:19

allow overriding with scrollBehavior

f3d05c6

Merge branch 'develop' into issue-4233-click-sticky-element

c833f0b

davidmunechika requested a review from jennifer-shehane October 12, 2021 19:29

chrisbreiding requested changes Oct 13, 2021

View reviewed changes

packages/driver/src/cy/actionability.ts Outdated Show resolved Hide resolved

packages/driver/src/cy/commands/actions/click.ts Outdated Show resolved Hide resolved

davidmunechika and others added 4 commits October 13, 2021 11:23

add comment

95d2211

Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>

rename userSetScrollBehavior

0195ca0

Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>

refactor if else

4f254e6

add custom scroll behavior test

5af31e0

davidmunechika requested a review from chrisbreiding October 13, 2021 15:46

chrisbreiding approved these changes Oct 13, 2021

View reviewed changes

Merge branch 'develop' into issue-4233-click-sticky-element

d57dd85

chrisbreiding requested changes Oct 13, 2021

View reviewed changes

chrisbreiding reviewed Oct 14, 2021

View reviewed changes

packages/driver/cypress/integration/commands/actions/click_spec.js Outdated Show resolved Hide resolved

davidmunechika and others added 3 commits October 18, 2021 11:54

Merge branch 'develop' into issue-4233-click-sticky-element

f0ddb9b

refactor ensure visibility

28834b5

Merge branch 'issue-4233-click-sticky-element' of https://github.com/…

3d8eb4c

…cypress-io/cypress into issue-4233-click-sticky-element

davidmunechika dismissed chrisbreiding’s stale review via 3d8eb4c October 27, 2021 15:32

davidmunechika added 3 commits October 27, 2021 11:37

remove chai import

2f9a14f

fix trigger test

981f297

remove user set scroll behavior

7a0b1c1

davidmunechika requested a review from chrisbreiding October 27, 2021 15:46

davidmunechika and others added 9 commits October 27, 2021 11:46

Merge branch 'develop' into issue-4233-click-sticky-element

2e9207c

fix clear spec

15a22b5

fix error message

2bccbff

fix select case

b0b348b

Merge branch 'develop' into issue-4233-click-sticky-element

8330767

update tests

f2a4b1f

Merge branch 'issue-4233-click-sticky-element' of https://github.com/…

dbbe481

…cypress-io/cypress into issue-4233-click-sticky-element

add comment

1de8681

Merge branch 'develop' into issue-4233-click-sticky-element

1d0f1f7

flotwig requested a review from BlueWinds November 1, 2021 15:34

BlueWinds approved these changes Nov 1, 2021

View reviewed changes

packages/driver/cypress/integration/commands/actions/click_spec.js Show resolved Hide resolved

chrisbreiding approved these changes Nov 2, 2021

View reviewed changes

Merge branch 'develop' into issue-4233-click-sticky-element

5aa6dc7

jennifer-shehane removed their request for review November 2, 2021 14:32

chrisbreiding merged commit 28ae1c1 into develop Nov 2, 2021

chrisbreiding deleted the issue-4233-click-sticky-element branch November 2, 2021 18:22

davidmunechika mentioned this pull request Nov 2, 2021

Sticky elements within a fixed container are not properly taken into account when scrolling to an element #4233

Closed

flotwig pushed a commit that referenced this pull request Nov 8, 2021

fix(driver): Sticky elements within a fixed container will not preven…

bbb825e

…t an element from being scrolled to (#18441) Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(driver): Sticky elements within a fixed container will not prevent an element from being scrolled to #18441

fix(driver): Sticky elements within a fixed container will not prevent an element from being scrolled to #18441

davidmunechika commented Oct 11, 2021 •

edited by chrisbreiding

cypress-bot bot commented Oct 11, 2021

cypress bot commented Oct 11, 2021 •

edited

jennifer-shehane left a comment

chrisbreiding left a comment

chrisbreiding left a comment

chrisbreiding left a comment

fix(driver): Sticky elements within a fixed container will not prevent an element from being scrolled to #18441

fix(driver): Sticky elements within a fixed container will not prevent an element from being scrolled to #18441

Conversation

davidmunechika commented Oct 11, 2021 • edited by chrisbreiding

User facing changelog

Additional details

PR Tasks

cypress-bot bot commented Oct 11, 2021

cypress bot commented Oct 11, 2021 • edited

Test summary

Run details

Failures

Flakiness

jennifer-shehane left a comment

Choose a reason for hiding this comment

chrisbreiding left a comment

Choose a reason for hiding this comment

chrisbreiding left a comment

Choose a reason for hiding this comment

chrisbreiding left a comment

Choose a reason for hiding this comment

davidmunechika commented Oct 11, 2021 •

edited by chrisbreiding

cypress bot commented Oct 11, 2021 •

edited