fix: #29093 for elements with length in rem #29224

senpl · 2024-03-29T11:43:33Z

Closes
Visible elements are "not visible" for Cypress #29093
Closes "isVisible" is incorrectly assessed for the absolutely positioned elements if the ancestor has overflow and static position #28638

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

describe('Repro', () => {
    it('Skeleton Fail without changes', () => {
        cy.viewport('macbook-16');
        cy.visit('https://www.skeleton.dev/docs/get-started');
        cy.get('#sidebar-left > div > section').click();
      });
  })

How has the user experience changed?

User no longer has to use {force: true} fix.

PR Tasks

[] Have tests been added/updated?
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?

CLAassistant · 2024-03-29T11:43:38Z

All committers have signed the CLA.

cypress-app-bot · 2024-03-29T11:43:48Z

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.

…-visible"-for-Cypress

…Cypress' into cypress-io#29093-fix

changelog update

fix pipeline bug

fix pipeline for line2

fix pipeline3

jennifer-shehane · 2024-03-29T13:51:50Z

@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:

HTML template replicating bug: https://github.com/cypress-io/cypress/blob/develop/packages/driver/cypress/fixtures/issue-565.html
Test spec: https://github.com/cypress-io/cypress/blob/develop/packages/driver/cypress/e2e/issues/565.cy.js

To run the tests:

within cypress root, run yarn
run yarn workspace @packages/driver cypress:open
click on the test you're writing to run it within Cypress

Instructions for running the driver tests can always be found here: https://github.com/cypress-io/cypress/blob/develop/packages/driver/README.md

cypress-io#29093 fix tests

…-visible"-for-Cypress

jennifer-shehane

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

pipeline speed improvement

…-visible"-for-Cypress

Dismissing previous review

id for precise locator gind

…-visible"-for-Cypress

cacieprins · 2024-05-28T15:57:12Z

packages/launchpad/src/components/code/FileRow.cy.tsx

@@ -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')


Hi @senpl ,

There are two .shiki elements in this component. One of them is always hidden, and one of them that should show/hide on click. The hidden state for this element is caused by a parent element with maxHeight of 0, and overflow:hidden.

I do agree that the .shiki selector here should be more specific. I edited it to assert the expected visibility state of both elements:

// A FileRow with status=valid should not be initially open cy.get('[data-cy=valid] .shiki').should('not.be.visible') // A FileRow with status=changes should be initially open cy.get('[data-cy=changes] .shiki').should('be.visible') // Clicking the header should collapse the shiki code view in the FileRow cy.contains('cypress/integration/command.js').click() cy.get('[data-cy=valid] .shiki').should('not.be.visible') cy.get('[data-cy=changes] .shiki').should('not.be.visible')

Unfortunately, while this test passes on the develop, it does not pass with the visibility changes in this branch. Can you look into why?

Thank you!

…-visible"-for-Cypress

cacieprins

It looks like the issue being tested in the fixture added with this PR is related to display: contents, rather than rem units.

This seems like a separate issue from #29093 and #28638

For fixing the display:contents issue directly, I recommend checking for this css rule in canClipContent, as elements with display:contents cannot clip content. I've opened an issue to track that here: #29605

The rem issue should have a fixture that uses rem units. I do believe there is a useful fix here regarding them and using getClientBoundingRect() to calculate float dimensions rather than int dimensions. Fixing the rem issue and the display: contents issue should be separated into two different pull requests. But disregarding certain combinations of clipping ancestors will cause many more issues, giving lots of false positives.

Thank you so much for contributing!

cacieprins · 2024-06-03T15:28:34Z

packages/driver/src/dom/visibility.ts


-    if (elHasPositionAbsolute($el) && (ancestorProps.width === 0 || ancestorProps.height === 0)) {
-      return elIsOutOfBoundsOfAncestorsOverflow($el, getParent($ancestor))
+    if ((elHasPositionAbsolute($el) || elHasBlockStyle($el)) && (ancestorProps.width === 0 || ancestorProps.height === 0)) {


Block elements that have ancestors that hide them via overflow clipping will assert as visible even if they are not.

For example, with this DOM:

<div style="overflow: hidden; width: 0; height: 0"> <div style="height:100px; width:100px;"> <div id="hidden" style="height: 100px; width: 100px;">This content is hidden</div> </div> </div>

this assertion will fail:

cy.get('#hidden').should('not.be.visible')

However, this DOM:

<div style="overflow: hidden; width: 0; height: 0"> <div style="height:100px; width:100px;"> <span id="hidden">This content is hidden</div> </div> </div>

Will succeed with the same assertion.

As will this DOM:

<div style="overflow: hidden; width: 0; height: 0"> <div id="hidden">This content is hidden</div> </div>

Removing the elHasBlockStyle portion of this if block should resolve the issue.

To verify, you can add additional test cases in visibility.cy.ts for parent vs. grandparent visibility.

Modified. elHasBlockStyle removed. Test in visibility.cy.ts added.

packages/driver/cypress/e2e/dom/visibility.cy.ts

cacieprins · 2024-06-03T16:56:07Z

packages/driver/src/dom/visibility.ts

  // no ancestor, not out of bounds!
  // if we've reached the top parent, which is not a normal DOM el
  // then we're in bounds all the way up, return false
  if (isUndefinedOrHTMLBodyDoc($ancestor)) {
    return false
  }

+  //fix for 28638
+  if (elHasPositionRelative($el) && elHasPositionAbsolute($ancestor)) {


Relative elements can be clipped by absolute ancestors that have height or width of 0 and overflow:hidden. canClipContent (L#230) checks for various edge cases with offset parents before comparing the dimensions of an element and their ancestor to determine if the element is actually obscured by clipping.

moved to canClipContent. This is just first working fix. Assumption is: when El has position relative and it's ancestor Absolute than this can not be clipped. But if this assumption is not exactly correct then more complicated check could be done. It's just better then existing version for this case.

cacieprins · 2024-06-03T17:42:08Z

packages/driver/src/dom/visibility.ts

@@ -308,47 +322,54 @@ const elIsNotElementFromPoint = function ($el) {
  return true
 }

-const elIsOutOfBoundsOfAncestorsOverflow = function ($el, $ancestor = getParent($el)) {
+const elIsOutOfBoundsOfAncestorsOverflow = function ($el, el: HTMLElement, $ancestor = getParent($el)) {


$el.get(0) and el refer to the same element - let's remove the extraneous argument to make this function easier to follow. For getting rid of the implicit any on $el, JQuery<HTMLElement> works.

Changed. I guess it was just naive aproach to pass pipeline limitations in hugeJS file and not calculating jQuery when it was necessery. Still probably not needed and not that optimal anyway. Suggested getBoundingCalculation is probably better optimalisation anyway.

cacieprins · 2024-06-03T17:49:53Z

packages/driver/src/dom/visibility.ts

-const elOffsetWidth = ($el) => {
-  return $el[0].offsetWidth
+const elClientHeight = ($el) => {
+  return $el[0].getBoundingClientRect().height


Each call to .getBoundingClientRect() forces a layout/reflow of the browser. Calling this many times for each element/ancestor comparison in a complicated DOM may cause performance issues.

Changed into function. So result could be stored.

cacieprins · 2024-06-03T18:15:49Z

packages/driver/cypress/fixtures/issue-29093.html

+    }
+
+    .contents {
+      display: contents


This is what is causing Cypress to not properly determine that #sidebar-left is not hidden in the fixture. display: contents causes the element to not generate its own box, which prevents overflow:hidden from being applied as expected - the element is effectively replaced by its children, and its box model (and overflow settings) should be disregarded. See: https://drafts.csswg.org/css-display/#valdef-display-contents.

It's true. Check moved to canClip.

cypress-io#29093 fix

…-visible"-for-Cypress

cacieprins

Thank you for the latest changes!

The next step for this PR is to decide which issue you would like it to fix. We like to keep PRs narrowly focused on one issue at a time.

If you wish to fix the display: contents issue (#29605) in this PR, please remove any changes that are unnecessary to fix it. I think the only change necessary to fix this is the addition of the CSS rule check in canClipContent. If you wish to continue fixing any rem/em issues, please open a second PR to address it

If you wish to fix issues that are arising due to rounding dimensions with rem and em units, please add a test case for that in this PR and remove any changes that are unnecessary to fix that specific test case. If you wish, open a second PR to address the display:contents issue.

Thank you again!

senpl · 2024-06-14T11:12:59Z

I choose second approach, added tests and tried to demonstrate need of this change. Still it's not that simple because it's only fix for not correctly detected as visible element that are with display: contents so they are detected as with 0 width without it. Anyway this change is needed in some cases. I just not found good test to demonstrate it yet.

senpl · 2024-07-01T08:26:04Z

This changes are now part of other PR where they are needed to work. When their are accepted that one will no longer be needed.

senpl added 3 commits March 29, 2024 11:11

first working version

8b1182f

clean commented code

da6825f

clean

f4278d0

senpl added 10 commits March 29, 2024 12:50

changelog update

7d08741

Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…

cc86762

…-visible"-for-Cypress

Merge branch 'issue-29093-fix-Visible-elements-are-"not-visible"-for-…

cf5353a

…Cypress' into cypress-io#29093-fix

Merge pull request #2 from senpl/cypress-io#29093-fix

c2e4f99

changelog update

fix pipeline bug

bd82c5d

Merge pull request #3 from senpl/cypress-io#29093-fix

b5c97d7

fix pipeline bug

fix pipeline for line2

bbeb1bb

Merge pull request #4 from senpl/cypress-io#29093-fix

6d48369

fix pipeline for line2

fix pipeline3

71cdb93

Merge pull request #5 from senpl/cypress-io#29093-fix

f85f307

fix pipeline3

senpl marked this pull request as ready for review March 29, 2024 13:13

jennifer-shehane assigned senpl Mar 29, 2024

jennifer-shehane self-requested a review March 29, 2024 13:53

senpl added 3 commits April 2, 2024 13:14

error reproduction

b15254c

go back to previous commend for tests

0e52c90

Merge pull request #6 from senpl/cypress-io#29093-fix

55ac75f

cypress-io#29093 fix tests

senpl mentioned this pull request Apr 3, 2024

"isVisible" is incorrectly assessed for the absolutely positioned elements if the ancestor has overflow and static position #28638

Open

jennifer-shehane added 4 commits April 3, 2024 11:41

Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…

8ef291b

…-visible"-for-Cypress

Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…

47d2948

…-visible"-for-Cypress

Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…

9d84c25

…-visible"-for-Cypress

fix changelog

2a4d554

jennifer-shehane requested changes Apr 11, 2024

View reviewed changes

senpl added 2 commits April 12, 2024 08:43

fix for test that catched not correct span

347ca9e

fix for visibility compatible backward

5d57f92

senpl added 3 commits May 13, 2024 15:13

Merge pull request #23 from senpl/cypress-io#29093-fix

04efcf5

pipeline speed improvement

Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…

5a279d8

…-visible"-for-Cypress

Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…

95c624c

…-visible"-for-Cypress

senpl requested a review from jennifer-shehane May 20, 2024 06:17

jennifer-shehane removed their request for review May 20, 2024 13:45

mschile requested a review from cacieprins May 21, 2024 15:51

senpl added 3 commits May 22, 2024 09:57

id for precise locator gind

093c8e0

Merge pull request #24 from senpl/cypress-io#29093-fix

98c56de

id for precise locator gind

Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…

09fe08f

…-visible"-for-Cypress

cacieprins requested changes May 28, 2024

View reviewed changes

senpl requested a review from cacieprins May 29, 2024 07:43

Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…

53c7242

…-visible"-for-Cypress

cacieprins requested changes Jun 3, 2024

View reviewed changes

senpl added 8 commits June 4, 2024 09:15

removed unussed id

3862c48

display contents fix

bf76534

additional tests

bcea9bd

move to correct place in canClip

cad469d

28638 in correct place

58f2e9d

readability and performance improvements

7cf025a

Merge pull request #25 from senpl/cypress-io#29093-fix

d6ad12d

cypress-io#29093 fix

Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…

9561099

…-visible"-for-Cypress

cacieprins self-requested a review June 4, 2024 17:10

cacieprins requested changes Jun 4, 2024

View reviewed changes

This was referenced Jun 12, 2024

fix: #29605 for display: contents parent visibility #29658

Closed

fix: #29605 for display: contents parent visibility with correct width check #29680

Merged

jennifer-shehane requested a review from cacieprins June 18, 2024 14:25

senpl closed this Jul 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: #29093 for elements with length in rem #29224

fix: #29093 for elements with length in rem #29224

senpl commented Mar 29, 2024 •

edited by jennifer-shehane

Loading

CLAassistant commented Mar 29, 2024 •

edited

Loading

cypress-app-bot commented Mar 29, 2024

jennifer-shehane commented Mar 29, 2024

jennifer-shehane left a comment

cacieprins May 28, 2024 •

edited

Loading

cacieprins left a comment

cacieprins Jun 3, 2024

senpl Jun 4, 2024

cacieprins Jun 3, 2024

senpl Jun 4, 2024

cacieprins Jun 3, 2024

senpl Jun 4, 2024 •

edited

Loading

cacieprins Jun 3, 2024

senpl Jun 4, 2024

cacieprins Jun 3, 2024

senpl Jun 4, 2024

cacieprins left a comment

senpl commented Jun 14, 2024

senpl commented Jul 1, 2024

fix: #29093 for elements with length in rem #29224

fix: #29093 for elements with length in rem #29224

Conversation

senpl commented Mar 29, 2024 • edited by jennifer-shehane Loading

Additional details

Steps to test

How has the user experience changed?

PR Tasks

CLAassistant commented Mar 29, 2024 • edited Loading

cypress-app-bot commented Mar 29, 2024

jennifer-shehane commented Mar 29, 2024

jennifer-shehane left a comment

Choose a reason for hiding this comment

Before

After

cacieprins May 28, 2024 • edited Loading

Choose a reason for hiding this comment

cacieprins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

senpl Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cacieprins left a comment

Choose a reason for hiding this comment

senpl commented Jun 14, 2024

senpl commented Jul 1, 2024

senpl commented Mar 29, 2024 •

edited by jennifer-shehane

Loading

CLAassistant commented Mar 29, 2024 •

edited

Loading

cacieprins May 28, 2024 •

edited

Loading

senpl Jun 4, 2024 •

edited

Loading