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: 28638 parent absolute #29689

Open
wants to merge 35 commits into
base: release/14.0.0
Choose a base branch
from

Conversation

senpl
Copy link

@senpl senpl commented Jun 17, 2024

Fix for parent overflow and element position absolute detection

Additional details

Some changes to detect of visibility as in other of my PR. Still without them this fix not work.

Steps to test

as described in 28638.

How has the user experience changed?

Element now should be clickable without force: true walkaround.

PR Tasks

@cypress-app-bot
Copy link
Collaborator

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

There are some tests failing here. Can you please address those?

@senpl
Copy link
Author

senpl commented Jun 26, 2024

Unit test -Delegate cloud is probably not related.
Integrated works except Firefox. I will check it when I will find time.

cli/CHANGELOG.md Outdated
@@ -33,6 +34,7 @@ _Released 6/18/2024_
- Fixed an issue where `inlineSourceMaps` was still being used when `sourceMaps` was provided in a users typescript config for typescript version 5. Fixes [#26203](https://github.com/cypress-io/cypress/issues/26203).
- When capture protocol script fails verification, an appropriate error is now displayed. Previously, an error regarding Test Replay archive location was shown. Addressed in [#29603](https://github.com/cypress-io/cypress/pull/29603).
- Fixed an issue where receiving HTTP responses with invalid headers raised an error. Now cypress removes the invalid headers and gives a warning in the console with debug mode on. Fixes [#28865](https://github.com/cypress-io/cypress/issues/28865).
- Fixed an issue where "isVisible" is incorrectly assessed for the absolutely positioned elements if the ancestor has overflow and static position [#29689](https://github.com/cypress-io/cypress/pull/29689).
Copy link
Member

Choose a reason for hiding this comment

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

Does this belong here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. And it's required by pipeline. Of course position could be discussed but it's fix issue reported by user so probably should be in Bugfixes

@@ -367,7 +367,7 @@ describe('src/cypress/dom/visibility', () => {
`)

this.$elOutOfParentBoundsAbove = add(`\
<div style='width: 100px; height: 100px; overflow: hidden; position: relative;'>
<div style='width: 100px; height: 100px; overflow: hidden; position: fixed;'>
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain this change more fully?

Copy link
Author

Choose a reason for hiding this comment

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

This is needed to firefox to behave like other browsers in this tests. Test check el out of bound and this not changed. And in browser it could be seen as same. But Firefox do not like combination of relative and absolute. Maybe in future it will behave same as other browsers. But for pipeline Firefox version this seems like simple walkaround.

Copy link
Author

Choose a reason for hiding this comment

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

Alternatively test could stay not changed and { browser: '!firefox' } added to this test.

@senpl
Copy link
Author

senpl commented Jul 1, 2024

Now looks like pipeline fail on unrelated certificate.

@jennifer-shehane
Copy link
Member

@senpl There are still failures here that need addressing. Please check the test files locally before committing, to ensure they pass.

@senpl
Copy link
Author

senpl commented Jul 8, 2024

All tests passed. What else is needed ???

Copy link
Author

@senpl senpl left a comment

Choose a reason for hiding this comment

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

All seems fixed

@jennifer-shehane
Copy link
Member

@senpl We'll need to spend some time reviewing the changes.

@jennifer-shehane
Copy link
Member

We've decided to prioritize looking into these visibility fixes in Quarter 4 of this year, when we intend to spend time on Cypress 14 release. It will ease any concerns we have around releasing breaking changes with these fixes and also help us plan the proper time to review these. Tracking here: #29925

@jennifer-shehane jennifer-shehane marked this pull request as draft August 14, 2024 15:14
@jennifer-shehane jennifer-shehane removed their request for review August 15, 2024 14:02
@jennifer-shehane jennifer-shehane added the Cypress 14 Issues scoped for Cypress 14 label Sep 27, 2024
@senpl senpl marked this pull request as ready for review September 30, 2024 10:16
@jennifer-shehane jennifer-shehane changed the base branch from develop to release/14.0.0 September 30, 2024 15:29
@jennifer-shehane jennifer-shehane requested review from mschile and removed request for jennifer-shehane November 13, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cypress 14 Issues scoped for Cypress 14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants