cypress-io / cypress

Fast, easy and reliable testing for anything that runs in a browser.
https://cypress.io
MIT License
47.03k stars 3.19k forks source link

click() has side effect of moving DOM elements vertically within non-scrollable container #9739

Open danatemple opened 3 years ago

danatemple commented 3 years ago

Current behavior

Applying a .click() to an element can in some situations result in the page layout changing in that a containing element of the click target moves vertically.

In this case, we have a simple page rendered by React with a page root (green border) and a div (yellow border) with

height: 100%
position: fixed;
padding-top: 60px;

The padding leaves space for some top nav (light grey)

Inside this container there is some content, (red border) which is set to height: 100%

I.e. it fills the remaining viewport space (apart from the padding) and therefore is positioned 60px from viewport top. All is well:

image

Then, we apply a Cypress click to the button element, and:

image

DOM inspection shows that the padding still exists, no attributes of the key elements appear to have changed. It has been captured by the Cypress screenshots, it's clear to see that it is the click() that causes the move. Inserting a pause() or wait() before the click means that no movement is seen until the click is called. Clicking other elements also has the same effect.

Desired behavior

No side-effects on click. Needless to say, a real click does not have this effect, neither does the app ever exhibit this behavior in real life, only in Cypress.

Test code to reproduce

This will take quite some effort to strip down but can be done if the above does not give any "aha" ideas, or private access can be given to a repo.

Versions

Cypress 6.1.0 and many past versions at least to 3.x.x (this has been a problem for a while) Electron, Firefox and Chrome browser with Cypress.

dverbiest commented 3 years ago

Interesting!

@danatemple: What do you mean by a page container, a div? or other html elements?

Because I saw this behaviour on a page using shadow dom, in a vaadin-grid.

danatemple commented 3 years ago

Yes, they are all just divs. Have edited OP.

I have investigated further, and (I guess of course) it does not happen if the HTML is static, only when it is generated with React.

However, Cypress must be doing something to the DOM (or doing something that has a side-effect) BEFORE the click - as it moves before the click (evidenced by the fact the click fails as the element has moved and is now hidden).

danatemple commented 3 years ago

Aha. Just found that what is happening is that the yellow container div has had a value set in its scrollTop property.

So, Cypress is making a change to the scrollTop before attempting the click(). This seems to be the problem described in #871 . I have confirmed that setting the new 6.1.0 scrollBehavior global config to 'false' prevents the problem.

However this is not very optimal, as either I lose the nice "scroll into view when necessary" function, or have to use force: true and lose all the value of the visibility checking.

There is no overflow-y on any of these elements, BTW. They should be non-scrolling. Surely it must be an option for Cypress to not attempt to scroll a non-scrollable (by the user) element?

jennifer-shehane commented 3 years ago

Cypress does a few checks before clicking an element (or performing any action). I suggest reading https://on.cypress.io/interacting-with-elements for a full explanation.

One of the things Cypress does before a click is it automatically scrolls the element into the viewport. This is to prevent having to scroll to every single element before performing an action (which could get annoying pretty quickly).

The default behavior is to scroll the element to the very top of the viewport and while we try to not scroll elements under other elements, it does happen sometimes. This behavior can be changed by setting scrollBehavior to either 'center', 'top', 'bottom', 'nearest', or false. false disables scrolling.

cy.click({ scrollBehavior: 'center' })
danatemple commented 3 years ago

@jennifer-shehane

I can't see anywhere that it should be scrolling an element that is not scrollable by the user? I.e. does not have overflow-y set to 'auto' or 'scroll'.

If such elements are non-visible, then there is no way the user would have been able to scroll them into view, and hence the click should either succeed (good) or fail (in which case the test SHOULD fail, as the user had no way to click the element).

jennifer-shehane commented 3 years ago

@danatemple Can you provide the HTML/CSS example page with a Cypress test against it? It's possible there's an edge case that we're not handling correctly, but we'd need that to confirm. Just a single page HTMl file to load in the browser is fine.

danatemple commented 3 years ago

I have tried to isolate to static HTML/CSS but can only re-create the problem when the page is rendered by React. I can give you access to a private repo (on GitLab) which is cut-down enough to just render the few divs in the example. Let me know if this would be useful. I can post an email address here.

jennifer-shehane commented 3 years ago

Sure, you can email to support@cypress.io.

danatemple commented 3 years ago

Email has been sent.

jennifer-shehane commented 3 years ago

Thanks for sending over the reproducible example. I've narrowed it down to the test below. This reminds me of this issue, but I don't think it's the same case: https://github.com/cypress-io/cypress/issues/4233

Reproducible Example

index.html

<html>
<head>
</head>
<body style="margin: 0;">
  <div style="opacity: 0.5; z-index: 100; position: relative; height: 60px;">
    <div class="middle aligned row" style="background-color: blue; height: 60px;">
      <div>Header height: 60px</div>
    </div>
  </div>
  <div style="height: 100%; position: fixed; top: 0; padding-top: 60px; overflow-y: hidden; overflow-x: hidden; width: 100%;">
    <div style="background-color: red">
      <button>Button</button>
      <div style="height: 817px;"></div>
    </div>
  </div>
</body>
</html>

spec.js

it('button scrolls behind el on click', () => {
  cy.visit('index.html')
  cy.wait(1000) // just to see where button is at start
  cy.get('button').click({ timeout: 1000 })
})

Outside Cypress

Normal click of button does not move page. Also the page is not scrollable by itself.

Inside Cypress

Button moves below header bar, even though element should not be scrollable.

Workaround

Pass { scrollBehavior: false } to the .click() command to avoid Cypress auto-scrolling on click.

it('button scrolls behind el on click', () => {
  cy.visit('index.html')
  cy.get('button').click({ scrollBehavior: false })
})
danatemple commented 3 years ago

@jennifer-shehane Any progress on this? Just running into it again having rearranged the DOM somewhat.

Andarist commented 3 years ago

@jennifer-shehane I have a somewhat different problem but it feels sufficiently overlapping, so I'll just put it here - if you feel that it deserves an issue of its own then I could create a new one.

I believe that scrolling is only needed when the element is not visible. For already visible elements scrollIntoView may move the container - which feels like something that could/should be avoided as it's not required and might change something in the application's behavior by accident. WDYT?

On the attached recording the "terefere" button is being clicked on - it is fully visible and thus I would expect the container to not be moved.

https://user-images.githubusercontent.com/9800850/113202458-0ea85180-926b-11eb-8335-7e2b6a13d6a3.mp4

danatemple commented 3 years ago

@jennifer-shehane I'm wondering if you can remove the "workaround-available" tag from this issue, as it really is not a very good workaround.

Essentially you can turn off Cypress auto-scroll-to-click globally, but then you have to pepper your code with overrides, all of which will have to be removed at some point. Tests start failing because a list got one more member. etc etc. And once you've removed it, you can still hit the bug as some scroll events that are necessary can also have this side-effect.

Or, you can (even worse) turn off the scroll at each click() call that seems to be hitting this bug.

@Andarist I think this is something else. Let's keep one issue per case or they will never get closed :-)

cypress-app-bot commented 1 year ago

This issue has not had any activity in 180 days. Cypress evolves quickly and the reported behavior should be tested on the latest version of Cypress to verify the behavior is still occurring. It will be closed in 14 days if no updates are provided.

danatemple commented 1 year ago

BUMP for the bot - this still can happen in Cypress 12 - i.e. that elements that are not scrollable get scrolled. Not sure that the exact case in the OP still fails, but it does happen.

The essence is that it seems to be possible for an element within a non-scrollable parent can still get its scrollTop changed. There should maybe be a check within Cypress for this situation and prevent ever scrolling an element that the user would not be able to scroll.

marktnoonan commented 1 year ago

The essence is that it seems to be possible for an element within a non-scrollable parent can still get its scrollTop changed. There should maybe be a check within Cypress for this situation and prevent ever scrolling an element that the user would not be able to scroll.

Thanks for the update @danatemple. I've added a label to prevent the bot flagging this in the future. This would certainly still be a good one to address.

fionnachan commented 1 year ago

@marktnoonan FYI there's a relevant issue also in Triage with sscce https://github.com/cypress-io/cypress/issues/23898

jensb commented 1 month ago

Still seeing this issue (scrolling of unscrollable content with top padding upon click()) with Cypress 13.14.2. Is there a fix or workaround except setting scrollBehavior: false (which works, but is cumbersome)?