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: 29687 an issue where parent width/height if 0 but with children textContent evaluates as hidden when it's visible #29688

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

Conversation

senpl
Copy link

@senpl senpl commented Jun 17, 2024

Additional details

This is necessery to be consistent with what users see in browsers.

Steps to test

<div id="id2">
	<div style="width: 0">
		<p class="big">IN SECTION 2 </p>
	</div>
</div>
expect(cy.$$('#id2 >div > p')).to.be.visible
    
cy.get('#id2 >div > p').click()

How has the user experience changed?

Some elements that previously was marked as hidden now are will correctly be marked as visible

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
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
@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:13
@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: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
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
4 participants