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.

Sign up for GitHub

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

Jump to bottom

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

Open
klarzynskik opened this issue Jan 4, 2024 · 7 comments
Open

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

klarzynskik opened this issue Jan 4, 2024 · 7 comments
Labels
stale no activity on this issue for a long period topic: visibility 👁 type: bug

Comments

@klarzynskik
Copy link
Contributor

klarzynskik commented Jan 4, 2024 •
edited
Loading

Current behavior

It appears "isVisible" is incorrectly assessed for the absolutely positioned elements if the ancestor has an overflow and static position. For example, for the following document the #visible-button is considered as not-visible:

<body>
  <div style="height: 200px; position: relative; display: flex">
    <div style="border: 5px solid red">
      <div
        id="breaking-container"
        style="overflow: auto; border: 5px solid green"
      >
        <div>
          <h1>Example</h1>
          <div style="position: absolute; bottom: 5px">
            <button id="visible-button" style="position: relative">
              Try me
            </button>
          </div>
        </div>
      </div>
    </div>
  </div>
</body>

Here's the screenshot from the test:

button shown as not visible despite being there

It appears that in this case, while assessing the isVisible, the button goes particularly into the isHidddenByAncestors. Then while calculating the elIsOutOfBoundsOfAncestorsOverflow for the #breaking-container div element from the example, the canClipContent is incorrectly assessed. That element has a default position: static, thus does not affect the visibility of the button. However, this is not checked in the canClipContent, resulting in the incorrectly assessed visibility via calculating the button's position compared to that div.

The documentation in that particular states that:

Any of its ancestors hides overflow*
AND the element is position: relative
AND it is positioned outside that ancestor's bounds

However, as mentioned above, I think that this should be true only if the considered ancestor's position is not static.

Desired behavior

Element visibility is correctly assessed.

Test code to reproduce

Here's the repository: https://github.com/klarzynskik/cypress-visibility-issue-demo

The simplest example is the following code:

<div id="breaking-container" style="overflow: auto">
      <div>
        <div style="position: absolute; bottom: 5px">
          <button id="visible-button">Try me</button>
        </div>
      </div>
</div>

Cypress Version

13.6.2

Node version

18.16.1

Operating System

macOS 13.4.1

Debug Logs

No response

Other

No response
@jennifer-shehane
Copy link
Member

@klarzynskik We would be open to accepting a PR to update our visibility algorithm to address this. The code for visibility is here: https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/dom/visibility.ts

@klarzynskik
Copy link
Contributor Author

@jennifer-shehane, thanks for the reply! Indeed, I was already looking into that, and it seems that the most straightforward option would be to move to the native Element.checkVisibility (chromium feature description: https://chromestatus.com/feature/5163102852087808), which has been widely adopted in other frameworks (i.e. Playwright) and is supported in all modern browsers.

Would you be open to such a refactor? I would be happy to work on that.

@jennifer-shehane
Copy link
Member

@klarzynskik Yes! I had spent about 20 minutes trying to throw this check in place of our visibility checks one day and some of our tests were failing, but I didn't have time to investigate further.

As long as our current visibility tests all pass - any updates and refactors are welcome, especially if they address any of the outstanding visibility issues with added tests and do not impact performance negatively.

@senpl
Copy link

senpl commented Apr 3, 2024

I checked in my version and seems like:
#29224
fix also this issue.

@jennifer-shehane jennifer-shehane mentioned this issue Apr 3, 2024
Closed
@jennifer-shehane
Copy link
Member

@senpl Thanks, I'll note that in the PR for us to verify also.

@senpl senpl mentioned this issue Jun 17, 2024
Merged
1 task
@senpl
Copy link

senpl commented Jun 28, 2024 •
edited
Loading

Would you be open to such a refactor? I would be happy to work on that.

I already did that refactor in #29741 . But your comment make me aware of this solution.

@cypress-app-bot
Copy link
Collaborator

This issue has not had any activity in 180 days. Cypress evolves quickly and the reported behavior should be tested on the latest version of Cypress to verify the behavior is still occurring. It will be closed in 14 days if no updates are provided.

@cypress-app-bot cypress-app-bot added the stale no activity on this issue for a long period label Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
stale no activity on this issue for a long period topic: visibility 👁 type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: #29093 for elements with length in rem senpl/cypress
4 participants
@jennifer-shehane @senpl @klarzynskik @cypress-app-bot