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

Re-query elements that are found 'detached' from the DOM #7306

Closed
jennifer-shehane opened this issue May 12, 2020 · 315 comments
Closed

Re-query elements that are found 'detached' from the DOM #7306

jennifer-shehane opened this issue May 12, 2020 · 315 comments
Assignees
Labels
Epic Requires breaking up into smaller issues pkg/driver This is due to an issue in the packages/driver directory type: feature New feature that does not currently exist v12.0.0 🐛

Comments

@jennifer-shehane
Copy link
Member

jennifer-shehane commented May 12, 2020

Current status

Update 12/06/2022: This work as been complete and will be released in Cypress v12 today.

This is currently in progress. See #7306 (comment), #7306 (comment) and #7306 (comment) for a brief description of the plan and updates on current status.

Current behavior:

Currently when Cypress queries an element that was passed from a parent element (say in .get(), .click() or .find()), if the element at some point becomes detached from the DOM, we throw an error:

CypressError: cy.click() failed because this element is detached from the DOM.

<element>

Cypress requires elements be attached in the DOM to interact with them.

The previous command that ran was:

  cy.get()

This DOM element likely became detached somewhere between the previous and current command.

Common situations why this happens:
  - Your JS framework re-rendered asynchronously
  - Your app code reacted to an event firing and removed the element

You typically need to re-query for the element or add 'guards' which delay Cypress from running new commands.

https://on.cypress.io/element-has-detached-from-dom

Screen Shot 2020-11-02 at 10 27 54 AM

Desired behavior:

Really users just want to re-query the DOM for the previously found element and click on the new element. Because it's more often than not that the framework underneath has created a completely new DOM element and inserted it into the DOM.

The proposal is to re-query the previously found element and continue the command with any newfound elements. Perhaps this could be an option passed to the command to allow this or not.

Reproducible Example

Provided in #9032

index.html

<form>
    <select onchange="document.forms[0].submit()">
        <option value="1">First</option>
    </select>
    <input />
</form>

spec.js

it('this will fail but should not', () => {
    cy.visit('index.html')
    cy.get("select").select("First")
    // adding any wait time will make it pass
    // cy.wait(0)
    cy.get("input").type("Hallo")
})

Related issues

@mf-alex-tsepkov
Copy link

Is there a workaround available in the meantime? Perhaps writing a custom uncaught:exception handler?

@ghost

This comment was marked as duplicate.

@jhanzeb1

This comment was marked as duplicate.

@stevenvachon
Copy link

Per #5743 (comment):

.click({force: true})

... is a decent enough solution, but it's vague without a comment.

@ghost
Copy link

ghost commented May 21, 2020

@stevenvachon I tried that several times in several of the scripts and it still hasn't worked for me.

@FayP
Copy link

FayP commented May 22, 2020

We ran into this issue so many times it was making our tests very flakey. We've started using the waitUntil command this week and has made our tests much more reliable, I'd recommend giving it a go. https://github.com/NoriSte/cypress-wait-until#readme

@mf-alex-tsepkov
Copy link

Per #5743 (comment):

.click({force: true})

... is a decent enough solution, but it's vague without a comment.

force may work but is not ideal here, since you're forcing your test to interact with an element the user may ultimately not be able to interact with (what if the element never gets reattached?). You want to wait until the element can be interacted with, not force interaction on a detached element. I currently have a cy.wait(5000) in our test, but that hack goes against the best practices of Cypress.

@ghost
Copy link

ghost commented May 22, 2020

@FayP how can you use waitUntil with an each loop? here is one of my each loops that I like to loop through.

cy.get(".mx-auto > .checkbox-col > .form-check")
          .each(($el, index, $list) => {
              cy.wrap($el).click({force: true});
              cy.get(".col-md-4 > .actions-contianer > .btn-secondary").click();
              cy.get(".mb-0 > :nth-child(2) > .btn").click();
              cy.get(".jobs-list-nav > :nth-child(1) > .nav-link").click();
          })

@FayP
Copy link

FayP commented May 22, 2020

@eabusharkh0 I'm not sure which element you're trying to target in that code snippet. But in theory you could target anything with a waitUntil. For example cy.waitUntil(() => cy.wrap($el))).click(); I would expect to work. Best bet is to give it a try.

@TomaszG
Copy link
Contributor

TomaszG commented May 29, 2020

So I managed to get it working (at least in few of my tests) with wait-until plugin:

cy.waitUntil(() =>
      cy.get('.someSelector')
        .as('someAlias')
        .wait(10) // for some reason this is needed, otherwise next line returns `true` even if click() fails due to detached element in the next step
        .then($el => Cypress.dom.isAttached($el)),
    { timeout: 1000, interval: 10 })

      .get('@someAlias')
      .click()

@JonDum

This comment was marked as duplicate.

@serge-shaldanov
Copy link

So I managed to get it working (at least in few of my tests) with wait-until plugin:

cy.waitUntil(() =>
      cy.get('.someSelector')
        .as('someAlias')
        .wait(1) // for some reason this is needed, otherwise next line returns `true` even if click() fails due to detached element in the next step
        .then($el => Cypress.dom.isAttached($el)),
    { timeout: 1000, interval: 10 })

      .get('@someAlias')
      .click()

Unfortunately, it does not work for me.
It works fine for the cy.get(...) chain but fails for the cy.get(...).find(...) chain because Cypress cannot find the parent element.

I solved my issue using the JQuery.click():

cy
  .get('[data-cy="user_row"]')
  .find('[data-cy="user_row_cell"]')
  .should('be.visible')
  .then((e) => {
    Cypress.$(e).click();
  })

It does not work for all cases but it works fine for my case. Maybe it will be useful to someone.

@pmaoui
Copy link

pmaoui commented Jun 6, 2020

All solutions here are at best hacky workarounds.
The Cypress spirit is to interact with UIs like an user would do.
This issue is indeed very technical and obviously an user is not supposed to know if an element is render one or multiple times, so as the test writer.

IMO, This re-query should be done systematically, without option to enforce the Retry-ability core-concept of Cypress.

For now, my only not-100%-safe workaround is a cy.wait(xxx) before any risky selector. It goes against core-concepts of this project.

@marmite22
Copy link

marmite22 commented Jun 10, 2020

AngularJS and Angular users can use https://angular.io/api/core/Testability

We have something like this in our legacy AngularJS tests. I don't know if React has an equivilent api.

Cypress.Commands.add('waitUntilAngularStable', () => {
    const getAngular = () => cy.window().its('angular', {log: false});
    getAngular().then((angular) => {
        return new Cypress.Promise(resolve => {
            angular.getTestability('body').whenStable(() => {
                resolve();
            });
        });
    });
});

Used like

cy.get('.back-btn').click();
cy.waitUntilAngularStable();
cy.get('.container').should('not.be.visible');

@Adil-Iqbal
Copy link

Adil-Iqbal commented Jun 11, 2020

Nothing works for me. Even chains fail.
Снимок экрана 2020-06-10 в 16 46 29

One viable alternative is to use Cypress.$ instead of cy.get. It's something we've used to great affect in our team. The side effect is that it bypasses a lot of the checks that come with using the official selection method.

Copy-paste the following to your ./cypress/support/commands.js file.

Cypress.Commands.add("get$", (selector) => {
  return cy.wrap(Cypress.$(selector)).should("have.length.gte", 1);
});

And now you should be able to click the button using the cy.get$ command:

cy.get$("#btnId").click();

This works because cy.wrap automatically retries when the .should assertion passes. If you wanted to explicity wait until the DOM element is attached, you can try the solution here: #5743 (comment)

@jupiterspot
Copy link

All solutions here are at best hacky workarounds.
The Cypress spirit is to interact with UIs like an user would do.
This issue is indeed very technical and obviously an user is not supposed to know if an element is render one or multiple times, so as the test writer.

IMO, This re-query should be done systematically, without option to enforce the Retry-ability core-concept of Cypress.

For now, my only not-100%-safe workaround is a cy.wait(xxx) before any risky selector. It goes against core-concepts of this project.

The time of a machine's interactions with elements on a site differ significantly than of a regular user's interactions. By a lot, in fact! That is why such things happen.

I am currently having the same issue with a <ul> list element. This particular list element updates its contents (<li> children) by the time a search box (text input element) receives input and basically acts as a search result suggestions list for the user.

As the user is typing into the searchbox, the suggestions refresh rather quickly on the server, probably on every character input, but does not manage to update in real-time for the user as well, which is why it takes a bit of time for the suggestions to refresh after the user is done typing. A user would typically wait a bit before the different suggestions calm down and stay in place, then click on the right one to advance.

This is why cy.wait(n); should not be ignored! Let's put the machine (test) into action now.

As the machine is done typing, the suggestions list might not have updated for the cilent (machine) instantly, however, the particular suggestion that meets the criteria is found at the bottom of the list, but would appear at the top right after the suggestions list finally updates for the client. The machine should now wait a bit until it is assumed that the list has been updated for the client and ready for its elements to be interacted with.
Otherwise, the machine would select the suggestion found at the bottom and by the time the suggestion has been "moved" to the top, the machine will try and interact with it (or click on it), but the element is not there anymore. It has been moved, but in reality, its current instance has been removed from the DOM and a new instance has appeared on the top of the list as the list has been fully updated for the client.

Sorry for my bad english, but I'm hoping I covered some benefits of cy.wait(n); :)
It is still not-100% safe, but is the safest approach if your assumptions are relatively correct.

@remjx
Copy link

remjx commented Jun 16, 2020

@uberwire that is not a valid use of Cy.wait. Cy.get has a built in wait so if your css selector is correct it will wait till it becomes the first element in the list.

@azgnetov
Copy link

I was not sure that my Cy ran synchronously, so I deleted my previous comment and rebuilt my tests. I separated actions and asserts to different describe/it sections and now it must work synchronously. But I get the error from time to time.
Also I tried force click, it adds more problems, than solves.

@jupiterspot
Copy link

jupiterspot commented Jun 16, 2020

@uberwire that is not a valid use of Cy.wait. Cy.get has a built in wait so if your css selector is correct it will wait till it becomes the first element in the list.

According to my context, cy.get gets called once on the object before it gets detached from the DOM, which terminates its "waiting". So, the point is to prevent getting the object until we assure that it will stay in place for an actual user to interact with it.

@remjx
Copy link

remjx commented Jun 16, 2020

@uberwire in your example, you should be able to wait for the element and position within the container since it should be deterministic that once you've entered X characters should it appear at the top of the list.

@jupiterspot
Copy link

@uberwire in your example, you should be able to wait for the element and position within the container since it should be deterministic that once you've entered X characters should it appear at the top of the list.

Safer and more practical approach in that case. Unless, the element an actual user is looking for gets found at the bottom of the list while another element that meets the same criteria gets found at the top of the list before the list fully refreshes, getting caught in the get method before the actual element we want.

The user can stop typing mid-way after seeing the desired result at the bottom of the list and interact, but that is not what is being tested in my case.

@kkukelka
Copy link

cy.contains('elementToRequery') seems to work for us, since we rely a lot on loading components asynchronously. 🥳 this doesn't work however in combination with cy.get() (as in cy.get.contains('elementToRequery')

more on this behaviour here

@berazur

This comment was marked as duplicate.

@mikepetrusenko
Copy link

mikepetrusenko commented Jun 23, 2020

Same problem even if using the waitUntil library:

cy.get('[data-cy=acceptButton]').click({ force: true })
cy.waitUntil(  () => cy.get('[data-cy=acceptButton]')  ).click()
cy.waitUntil(  () => cy.get('[data-cy=acceptButton]')  ).click({ force: true })

element is DETACHED from the DOM (not always, but randomly)

@berazur
Copy link

berazur commented Jun 23, 2020

How can we trust the cypress test if it doesn't work as expected?

cy.get('[data-cy=acceptOfferButton]').as('acceptOfferButton')
cy.get('@acceptOfferButton').should('be.visible')
cy.get('@acceptOfferButton').click({ force: true })

all commands pass successfully except last, sometimes is clicking but most of the time not.

How can we work with react component if the framework re-rendendered asynchronously?

@jpike88
Copy link

jpike88 commented Nov 16, 2022

automatic spec load balancing with PW wasn't that hard to implement I've found. a simple mechanism to auto-balance your specs will do the trick, all you have to do is decide how many shards you want.

  • decide how many shards you like.
  • run the test using the JSON reporter option, which spits out a JSON containing all test results (timings and names of each spec file).
  • with a simple algorithm, you can easily spit out an instructions file that dictates how to divide the specs for the next run. e.g. (let x = total sum of time/ number of shards, and nominate a shard for that test one by one until it's 'full' (using x as the watermark)
  • save that output to cache
  • next run, check that file, and shard according the instructions
  • etc

@AlexDaniel
Copy link

AlexDaniel commented Nov 16, 2022

To bring this discussion back on topic, I had to dive into a legacy Cypress file today and here's a fix that I did.

We had a series of checks like this:

          cy.contains('.v-card', 'three three').contains( '98 / 333')
          cy.contains('.v-card',     'two two').contains( '98 / 222')
          cy.contains('.v-card',     'one one').contains(  '0 / 111')

However, the test was failing on that first line, saying that 98 / 333 cannot be found. Upon video inspection, I could see that “98 / 333” is clearly on the screen, yet Cypress is not able to see it. The reason is that the v-card found by contains('.v-card', 'three three') reloads during the test, and is probably detached.

In this case (and in many others), the workaround is:

          cy.contains('98 / 333') // wait for data to load
          cy.contains('.v-card', 'three three').contains( '98 / 333')
          cy.contains('.v-card',     'two two').contains( '98 / 222')
          cy.contains('.v-card',     'one one').contains(  '0 / 111')

Basically, you just search for the final text first (assuming it's unique on the page). This way Cypress will wait for data to appear, and after that it's less likely that DOM will be detached. At least, this has fixed the issue in this case, but yes, Cypress wouldn't've had this issue if it was working with selectors under the hood (and I guess soon it won't have this problem, which is good).

Also, in my opinion, screenshots can be a lot more helpful than videos

Actually, Playwright doesn't exactly write videos, it records traces. Traces include console logs, network requests, screenshots before/after every action, DOM snapshots, and also the videos (though the videos are indeed least useful out of everything listed, but it depends).

@asmyshlyaev177
Copy link

This was the final straw, I've wasted an entire day trying to work around this issue, among others. I moved our suite to playwright today and it JUST WORKS, and does so a million times faster. To those still struggling and waiting for an outcome here I recommend the same until it is resolved.

Yeah, if not so many tests to rewrite many people will do that.
Reason is ridiculous amount of bugs and issues with cypress, and most tickets just ignored.
Basically people end up testing Cypress itself instead of testing code with Cypress.

@romankhomitskyi
Copy link

Haahh)) true
We all have been there

@SalahAdDin
Copy link

This was the final straw, I've wasted an entire day trying to work around this issue, among others. I moved our suite to playwright today and it JUST WORKS and does so a million times faster. To those still struggling and waiting for an outcome here I recommend the same until it is resolved.

Yeah, if not so many tests to rewrite many people will do that. The reason is the ridiculous amount of bugs and issues with cypress, and most tickets are just ignored. Basically, people end up testing Cypress itself instead of testing code with Cypress.

Hahahaha, really?

@emilyrohrbough
Copy link
Member

This work has been completed! Thank you @BlueWinds!!! 🎉 These changes will be making its debut in the Cypress 12 release.

@karlhorky
Copy link
Contributor

karlhorky commented Dec 6, 2022

It's been so long I've forgotten why this is good / needed 😅

Is there a blog post / docs about why this is needed and how to enable it? (or maybe it's just automatically enabled?)

@karlhorky
Copy link
Contributor

karlhorky commented Dec 6, 2022

Maybe this will happen when a framework such as Next.js / another React metaframework switches pages asynchronously and Cypress does not know about this change?

We are continuously having problems still with this in Cypress, definitely. Currently we usually just add an intercept for this network request to /_next/data/**, but this blows up the size of the code a lot:

cy.intercept('GET', '/_next/data/**').as('nextPageJson');

cy.contains('a', 'Upgrade').should('be.visible').click();

// Wait for page JSON to load to avoid Cypress continuing too quickly
// with next steps before Next.js page navigation has completed
cy.wait('@nextPageJson').then((getInterception) => {
  expect(getInterception.response?.statusCode).to.equal(200, 'response.statusCode');
});

// Without the intercept above, Cypress does not know that something has
// changed and this pathname check will fail
cy.location('pathname').should('equal', '/p/cohort-access-levels');

@emilyrohrbough
Copy link
Member

@karlhorky This has been a fundamental change to how Cypress stores and managed DOM element resolution. No changes are needed on your end.

Before these changes, Cypress stored a single DOM element (or window) reference that is passed between a set of commands to assert on, which could become stale as the App under test updated.

With these changes, Cypress will now re-query for the DOM element (window object) to ensure its always the most up-to-date reference to verify.

Blue has done a great job of re-vamp our Retry-ability guide to explain how this works. This is currently out for review - but will be live very soon.

@karlhorky
Copy link
Contributor

karlhorky commented Dec 6, 2022

Nice, so I guess I'm understanding correctly that after Cypress 12, we will be able to remove these intercepts related to clicking on a link that will cause a navigation and async page update eg. using History.pushState?

-cy.intercept('GET', '/_next/data/**').as('nextPageJson');

cy.contains('a', 'Upgrade').should('be.visible').click();

-// Wait for page JSON to load to avoid Cypress continuing too quickly
-// with next steps before Next.js page navigation has completed
-cy.wait('@nextPageJson').then((getInterception) => {
-  expect(getInterception.response?.statusCode).to.equal(200, 'response.statusCode');
-});
-
cy.location('pathname').should('equal', '/p/cohort-access-levels');

@karlhorky
Copy link
Contributor

karlhorky commented Dec 6, 2022

Blue has done a great job of re-vamp our Retry-ability guide to explain how this works. This is currently out for review - but will be live very soon.

Ok great, I guess it's this file here: https://github.com/cypress-io/cypress-documentation/blob/5501639a14a5778e25da776893a9c14e7a106434/content/guides/core-concepts/retry-ability.md

@emilyrohrbough
Copy link
Member

Nice, so I guess I'm understanding correctly that after Cypress 12, we will be able to remove these intercepts related to clicking on a link that will cause a navigation and async page update eg. using History.pushState?

I believe so, but I'll def to @BlueWinds .

@BlueWinds
Copy link
Contributor

It's been so long I've forgotten why this is good / needed sweat_smile

Is there a blog post / docs about why this is needed and how to enable it? (or maybe it's just automatically enabled?)

There will be a blog post - I'll link here when it goes up. It will be automatically enabled - no changes needed to get the functionality.

Maybe this will happen when a framework such as Next.js / another React metaframework switches pages asynchronously and Cypress does not know about this change?

Yep, exactly so. @emilyrohrbough explained it a bit above; I'll leave a fuller description to the docs. You found the right page, https://github.com/cypress-io/cypress-documentation/blob/5501639a14a5778e25da776893a9c14e7a106434/content/guides/core-concepts/retry-ability.md. I'll also link this again once we merge in the docs.

Nice, so I guess I'm understanding correctly that after Cypress 12, we will be able to remove these intercepts related to clicking on a link that will cause a navigation and async page update eg. using History.pushState?

Exactly so!

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 6, 2022

Released in 12.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v12.0.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Dec 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Epic Requires breaking up into smaller issues pkg/driver This is due to an issue in the packages/driver directory type: feature New feature that does not currently exist v12.0.0 🐛
Projects
Status: Generally Available
Development

Successfully merging a pull request may close this issue.