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: #29093 for elements with length in rem #29224
base: develop
Are you sure you want to change the base?
The head ref may contain hidden characters: "issue-29093-fix-Visible-elements-are-\"not-visible\"-for-Cypress"
fix: #29093 for elements with length in rem #29224
Conversation
|
…-visible"-for-Cypress
changelog update
fix pipeline bug
fix pipeline for line2
fix pipeline3
@senpl Thanks! Can you add a test around this new behavior? There is a Contributing Guide that covers how to contribute and get Cypress running locally in generally here: https://github.com/cypress-io/cypress/blob/develop/.github/CONTRIBUTING.md Here's an example of another issue's test code:
To run the tests:
Instructions for running the |
…-visible"-for-Cypress
…-visible"-for-Cypress
…-visible"-for-Cypress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I manually tested the change against the 3 examples this is expected to fix. It does fix them.
There are some other tests failing however, so it looks like this logic has broken some other visibility checks:
I had some concern around performance of this, and whether this would mean slower performance. Running benchmarks against offsetWidth
vs getBoundingClientRect
seem to indicate that getBoundingClientRect
is actually faster.
Before
After
3 failed and 10 flaky tests on run #55315 ↗︎
Details:
src/components/code/FileRow.cy.tsx • 1 failed test • launchpad-ct
cypress/e2e/specs_list_e2e.cy.ts • 1 failed test • app-e2e
cypress/e2e/dom/visibility.cy.ts • 1 failed test • 5x-driver-firefox |
mocha added to launchpad package
…-visible"-for-Cypress
make it pipeline ok
pipeline
make pipeline view visible
out of bound fix
…-visible"-for-Cypress
@@ -117,10 +117,10 @@ describe('FileRow', () => { | |||
cy.contains(changesRequiredDescription).should('be.visible') | |||
cy.get('pre').should('have.length', 2) | |||
|
|||
cy.get('.shiki').should('be.visible') | |||
cy.get('div.rounded > div.text-left.cursor-text').should('be.visible') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@senpl Could you explain the need for this change? If users are required to update their tests based on this change, this would be considered a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.shiki is not precise selector. In moment of search in this test there are two elements with this locator. One berly visible but still possible to locate.
This selector better explain what test intended to search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be breaking change for poorly created locators. Still it's closer to what user can actually see in browser. So it's probably good change. If I could locate element in browser then it's visible, but there should be some rules to determine if something is visible or not.
fix for existing test
kitchensink fix
go back to previously working
pipeline speed improvement
…-visible"-for-Cypress
…-visible"-for-Cypress
Visible elements are "not visible" for Cypress #29093
Additional details
Change was necessary because previous visibility check version works only with integer values for visibility. Current fix provide it with getting real width.
In theory it now fixes issue when visible element throws error as hidden/invisible or without width/height.
Still how visibility should behave when grid is used is not really clear, so there might be cases that this check is not perfect. Still I guess it's better than it was before.
Steps to test
e2e or component test
How has the user experience changed?
User no longer has to use {force: true} fix.
PR Tasks
cypress-documentation
?type definitions
?