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: update visibility for elements with textContent but without width/height #29688

Merged
merged 48 commits into from
Dec 2, 2024

Conversation

senpl
Copy link

@senpl senpl commented Jun 17, 2024

Additional details

  • Fixed visibility issue when the element has no width/height but does have textContent.

Steps to test

See new tests for scenarios.

How has the user experience changed?

Before:
Screenshot 2024-11-26 at 2 10 04 PM

After:
Screenshot 2024-11-26 at 2 09 30 PM

PR Tasks

@cypress-app-bot
Copy link
Collaborator

@senpl senpl changed the title Fixed an issue where parent width 0 signal element hidden when it's visible fix: 29687 an issue where parent width 0 signal element hidden when it's visible Jun 17, 2024
@senpl senpl marked this pull request as ready for review June 17, 2024 07:26
@senpl
Copy link
Author

senpl commented Jun 17, 2024

It was done similar in the past as could be seen in #5974

@senpl
Copy link
Author

senpl commented Jun 17, 2024

One test updated because element that was in test in fact was visible, and incorrectly exected to be hidden and not visible.

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.

@senpl There a failure regarding visibility in the tests.
Screenshot 2024-06-21 at 1 38 47 PM

@senpl senpl requested a review from jennifer-shehane July 8, 2024 10:43
@senpl
Copy link
Author

senpl commented Jul 8, 2024

Now looks like cert errors, so probably something with pipeline.

cli/CHANGELOG.md Outdated Show resolved Hide resolved
@senpl senpl marked this pull request as ready for review September 30, 2024 10:15
@jennifer-shehane jennifer-shehane changed the base branch from develop to release/14.0.0 September 30, 2024 15:29
@jennifer-shehane
Copy link
Member

Updated the base branch to go against release/14.0.0

@jennifer-shehane jennifer-shehane changed the title fix: 29687 an issue where parent width 0 signal element hidden when it's visible fix: 29687 an issue where parent width/height if 0 but with children textContent evaluates as hidden when it's visible Nov 7, 2024
@jennifer-shehane
Copy link
Member

jennifer-shehane commented Nov 13, 2024

I'll leave some notes here on where I left off on this.

Essentially, there are some circumstances where elements should be considered visible/hidden depending on the state of their child node that is a TEXT node. And Cypress is not taking those situations into account today.

  • If an element has a child text node that contains characters other than whitespace characters, that content makes it visible (even if there is no width/height of the element), unless the element itself is explicitly hidden OR the text node is hidden by the element, like the element having overflow: hidden and the text node being clipped.

^ this last part in highlighted text is not currently being accounted for in the logic, which is why there is a failing test. I'm not sure if there are other circumstances that this updated code doesn't account for. Essentially the Cypress logic never seemed to be taking into account TEXT nodes, we've only been accounting for actual element nodes.

@jennifer-shehane jennifer-shehane requested review from mschile and removed request for jennifer-shehane November 13, 2024 16:04
@mschile mschile changed the title fix: 29687 an issue where parent width/height if 0 but with children textContent evaluates as hidden when it's visible fix: update visibility for elements with textContent but without width/height Nov 26, 2024
@mschile mschile dismissed jennifer-shehane’s stale review December 2, 2024 15:36

Dismissing previous review.

@mschile mschile merged commit c3660d4 into cypress-io:release/14.0.0 Dec 2, 2024
2 of 3 checks passed
@jennifer-shehane jennifer-shehane mentioned this pull request Dec 3, 2024
28 tasks
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.

Parent width should not signal element hidden when it's visible
6 participants