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

Why doesn't _isProbablyVisible check visibility: hidden? #816

Closed
mikesnare opened this issue Oct 27, 2023 · 3 comments
Closed

Why doesn't _isProbablyVisible check visibility: hidden? #816

mikesnare opened this issue Oct 27, 2023 · 3 comments

Comments

@mikesnare
Copy link
Contributor

We've run into this bug: #769 and the reason seems to be that visibility: hidden isn't being checked in _isProbablyVisible. The site has duplicate images hidden somewhere using that approach to hiding.

Since you're looking for display: none, it seems odd that you are not also looking for visibility: hidden. But it's so odd that I figure there must be a reason, so before I monkey patch I wanted to check to see what that reason might be.

Thanks!

@aehlke
Copy link

aehlke commented Oct 27, 2023

I tried looking through older tickets but couldn't find any leads on why this is omitted. I would recommend just adding it and checking test results.

The method was originally added here it looks like #283 which was solving a specific issue that didn't include visibility: hidden. I think it just wasn't relevant to the problem the author was solving at the time.

@gijsk
Copy link
Contributor

gijsk commented Oct 27, 2023

I think we've just not tried particularly hard. Once you go down the rabbithole (what about opacity? What about overflow: hidden combined with a 0 width/height? What about...) it's pretty deep.

I've no objections to adding visibility: hidden checks, though.

@gijsk
Copy link
Contributor

gijsk commented Dec 15, 2023

We merged #817 so let's close this out. Thanks!

@gijsk gijsk closed this as completed Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants