:visible/:hidden selectors unreliable #2227

Closed
shimonenator opened this Issue Apr 21, 2015 · 22 comments

Comments

Projects
None yet
7 participants
@shimonenator

Issue

This issue is present in both version 1 and 2 of jQuery.

It is happening on inline elements wrapping block elements, which is valid in HTML5, such as a header and an image wrapped in a single anchor.

Two issues arise:

  1. Different browsers report offsetWidth/Height of the anchor differently. For example Firefox would report one visible anchor in the example below, and Chrome will report 0. This also highly depends on the CSS styles applied, as on the page where this issue was discovered missing images are not displayed at all instead of the common missing image icon.
  2. While a browser may report an anchor as invisible, child elements are still visible and clickable as they inherit anchor properties.

Example

<!doctype html>
<html>
<head>
    <meta charset="utf-8">
    <script src="http://code.jquery.com/jquery-2.1.3.min.js"></script>
</head>
<body>
    <a href=#>
        <h1>Header</h1>
    </a>
    <script>
        $(function () {
            alert('Visible anchors: ' + $('a:visible').length);
        })
    </script>
</body>
</html>

Solution

There are 2 potential solutions that come to mind, but each has flaws

  1. Loop through child elements and check each for visibility. Potentially harmful for performance, although the selector is already marked as such in the documentation.
  2. Check parent for visibility. Probably not enough on its own, as it will report incorrectly, if anchor doesn't have visible children.
@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Apr 21, 2015

Member

Thank you for opening an issue. While you are correct that the behavior may be unexpected, the selector is working as documented. While the behavior differs between Firefox and Chrome, Chrome reports zero width and zero height, which technically means it is not visible according to our docs...

Visible elements have a width or height that is greater than zero.

The fact the hidden elements can have visible content can seem strange, but it's the way it is in Chrome. But more importantly, the cost of working around this would be far greater than we are willing to deal with. I don't think this is the only edge case where the :visible selector behaves in a way that can be unexpected, but I would prefer to keep the selector as simple as possible and let the edge cases be covered by other means.

Member

timmywil commented Apr 21, 2015

Thank you for opening an issue. While you are correct that the behavior may be unexpected, the selector is working as documented. While the behavior differs between Firefox and Chrome, Chrome reports zero width and zero height, which technically means it is not visible according to our docs...

Visible elements have a width or height that is greater than zero.

The fact the hidden elements can have visible content can seem strange, but it's the way it is in Chrome. But more importantly, the cost of working around this would be far greater than we are willing to deal with. I don't think this is the only edge case where the :visible selector behaves in a way that can be unexpected, but I would prefer to keep the selector as simple as possible and let the edge cases be covered by other means.

@timmywil timmywil closed this Apr 21, 2015

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 21, 2015

Member

This seems like a Chrome bug, does Safari have it as well?

Member

dmethvin commented Apr 21, 2015

This seems like a Chrome bug, does Safari have it as well?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 21, 2015

Member

This seems like a Chrome bug, does Safari have it as well?

Yes.

Member

mgol commented Apr 21, 2015

This seems like a Chrome bug, does Safari have it as well?

Yes.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 21, 2015

Member

Here's a test case: http://jsfiddle.net/2tgw2yr3/

Both jsbin.com and browserstack.com are down for me at the moment.

Member

dmethvin commented Apr 21, 2015

Here's a test case: http://jsfiddle.net/2tgw2yr3/

Both jsbin.com and browserstack.com are down for me at the moment.

@shimonenator

This comment has been minimized.

Show comment
Hide comment
@shimonenator

shimonenator Apr 22, 2015

@timmywil thank you for a quick reply.

First of all I'd like to apologize, I did file a duplicate issue. It seems that there's at least one earlier issue #1855 reporting this problem, which includes a solution.

Also, while it doesn't seem strange to that not visible elements can bleed visible child elements.

For the edge case degree, perhaps there should be a note in the documentation then. IMO people use jQuery to get the same results in different browsers no matter what, and they may not even realize that their selectors will sometime behave differently.

@timmywil thank you for a quick reply.

First of all I'd like to apologize, I did file a duplicate issue. It seems that there's at least one earlier issue #1855 reporting this problem, which includes a solution.

Also, while it doesn't seem strange to that not visible elements can bleed visible child elements.

For the edge case degree, perhaps there should be a note in the documentation then. IMO people use jQuery to get the same results in different browsers no matter what, and they may not even realize that their selectors will sometime behave differently.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 22, 2015

Member

Funny, I thought I remembered an issue like this where getBoundingClientRect came into play but I couldn't find it because I was looking in the old tracker which was down. Maybe I'm crashing all these sites, I dunno.

The :visible and :hidden selectors aren't a good idea because they can make the browser do a lot of work to determine the answer when the layout has been changed. In many cases you can track that information in other ways and it will be a lot faster.

Member

dmethvin commented Apr 22, 2015

Funny, I thought I remembered an issue like this where getBoundingClientRect came into play but I couldn't find it because I was looking in the old tracker which was down. Maybe I'm crashing all these sites, I dunno.

The :visible and :hidden selectors aren't a good idea because they can make the browser do a lot of work to determine the answer when the layout has been changed. In many cases you can track that information in other ways and it will be a lot faster.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Apr 22, 2015

Member

@dmethvin How old was it? @shimonenator already found #1855, and the only other case that comes to my mind is the soon-to-be-merged #2043 (which uses getClientRects to decide if .offset is meaningful). In fact, that work in particular leaves me open to a redefinition of :visible as !!(elem.offsetWidth || elem.offsetHeight || elem.getClientRects().length).

Member

gibson042 commented Apr 22, 2015

@dmethvin How old was it? @shimonenator already found #1855, and the only other case that comes to my mind is the soon-to-be-merged #2043 (which uses getClientRects to decide if .offset is meaningful). In fact, that work in particular leaves me open to a redefinition of :visible as !!(elem.offsetWidth || elem.offsetHeight || elem.getClientRects().length).

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Apr 22, 2015

Member

The :visible and :hidden selectors aren't a good idea because they can make the browser do a lot of work to determine the answer when the layout has been changed. In many cases you can track that information in other ways and it will be a lot faster.

I know you love to say this as much as possible, but it's really not helpful for developers writing general purpose code, and the performance problems are irrelevant for many use cases.

Member

scottgonzalez commented Apr 22, 2015

The :visible and :hidden selectors aren't a good idea because they can make the browser do a lot of work to determine the answer when the layout has been changed. In many cases you can track that information in other ways and it will be a lot faster.

I know you love to say this as much as possible, but it's really not helpful for developers writing general purpose code, and the performance problems are irrelevant for many use cases.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Apr 22, 2015

Member

In fact, that work in particular leaves me open to a redefinition of :visible as !!(elem.offsetWidth || elem.offsetHeight || elem.getClientRects().length).

If it's that simple and getClientRects doesn't cost too much, let's do it.

Member

timmywil commented Apr 22, 2015

In fact, that work in particular leaves me open to a redefinition of :visible as !!(elem.offsetWidth || elem.offsetHeight || elem.getClientRects().length).

If it's that simple and getClientRects doesn't cost too much, let's do it.

@timmywil timmywil reopened this Apr 22, 2015

@shimonenator

This comment has been minimized.

Show comment
Hide comment
@shimonenator

shimonenator Apr 22, 2015

@scottgonzalez

I think you meant :visible as

return !!((elem.offsetWidth && elem.offsetHeight) || elem.getClientRects().length)

As such, following the current implementation where :hidden is the functional method, it would be:

return (!elem.offsetWidth || !elem.offsetHeight) && !elem.getClientRects().length

Thus, it seems to me that :visible is a more performant solution, as it would only fall onto getClientRects() when either offsetWidth or offsetHeight is 0. In case with :hidden, getClientRects()will always be executed. That's for v2. It's a little more complicated in v1 due to an old Opera behavior.

@scottgonzalez

I think you meant :visible as

return !!((elem.offsetWidth && elem.offsetHeight) || elem.getClientRects().length)

As such, following the current implementation where :hidden is the functional method, it would be:

return (!elem.offsetWidth || !elem.offsetHeight) && !elem.getClientRects().length

Thus, it seems to me that :visible is a more performant solution, as it would only fall onto getClientRects() when either offsetWidth or offsetHeight is 0. In case with :hidden, getClientRects()will always be executed. That's for v2. It's a little more complicated in v1 due to an old Opera behavior.

@shimonenator

This comment has been minimized.

Show comment
Hide comment
@shimonenator

shimonenator Apr 22, 2015

Just discovered that there's another solution #2212 with a PR that was approved. It seems that in line with current discussion about performance, there should be some additional consideration added to #2212.

Just discovered that there's another solution #2212 with a PR that was approved. It seems that in line with current discussion about performance, there should be some additional consideration added to #2212.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Apr 23, 2015

Member

Upon deeper analysis, this would not be a behavior change if it makes it in for 3.0, which I strongly recommend. The git builds include 10399dd (defining :hidden by elem.offsetWidth <= 0 || elem.offsetHeight <= 0, essentially noWidth OR noHeight), but that is unreleased—all releases from 1.4 forward use logic like noWidth AND noHeight. A fix for this issue would refine the released logic and revert 10399dd.

Member

gibson042 commented Apr 23, 2015

Upon deeper analysis, this would not be a behavior change if it makes it in for 3.0, which I strongly recommend. The git builds include 10399dd (defining :hidden by elem.offsetWidth <= 0 || elem.offsetHeight <= 0, essentially noWidth OR noHeight), but that is unreleased—all releases from 1.4 forward use logic like noWidth AND noHeight. A fix for this issue would refine the released logic and revert 10399dd.

@gibson042 gibson042 added this to the 3.0.0 milestone Apr 23, 2015

@timmywil timmywil self-assigned this May 5, 2015

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 8, 2015

Member

Actually, it looks like the proposed solution breaks the width: 0; height: 0; overflow: hidden; case, which jQuery considers hidden. http://jsbin.com/cokeku/1/edit?css,js,console,output

Member

timmywil commented May 8, 2015

Actually, it looks like the proposed solution breaks the width: 0; height: 0; overflow: hidden; case, which jQuery considers hidden. http://jsbin.com/cokeku/1/edit?css,js,console,output

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 8, 2015

Member

Ah, a slight behavior change, then. I'm still in favor of the getClientRects().length approach.

Member

gibson042 commented May 8, 2015

Ah, a slight behavior change, then. I'm still in favor of the getClientRects().length approach.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 8, 2015

Member

In that case, what about dropping offsetWidth and offsetHeight?

Member

timmywil commented May 8, 2015

In that case, what about dropping offsetWidth and offsetHeight?

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 8, 2015

Member

I would make such a decision entirely on performance. My untested supposition is that offset properties are faster than getClientRects, and therefore worth including even though redundant.

Member

gibson042 commented May 8, 2015

I would make such a decision entirely on performance. My untested supposition is that offset properties are faster than getClientRects, and therefore worth including even though redundant.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 8, 2015

Member

I'll do some testing.

Member

timmywil commented May 8, 2015

I'll do some testing.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 8, 2015

Member

Ok, looks like performance necessitates keeping offsetWidth and offsetHeight. http://jsperf.com/visible-hidden-and-getclientrects

There's no difference for hidden elements, but it's about 2x slower in Chrome for visible elements to only use .getClientRects().

Member

timmywil commented May 8, 2015

Ok, looks like performance necessitates keeping offsetWidth and offsetHeight. http://jsperf.com/visible-hidden-and-getclientrects

There's no difference for hidden elements, but it's about 2x slower in Chrome for visible elements to only use .getClientRects().

timmywil added a commit to timmywil/jquery that referenced this issue May 8, 2015

CSS: fix :visible/:hidden selectors for inline element w/ content
- Reverts behavior from 10399dd, which we never released.
  BR and inline elements are considered visible.
- The possibility of dropping .offsetWidth and .offsetHeight
  was debunked by this perf:
  http://jsperf.com/visible-hidden-and-getclientrects

Fixes gh-2227
@timmywil

This comment has been minimized.

Show comment
Hide comment
Member

timmywil commented May 8, 2015

PR

@timmywil timmywil closed this in 79bcb29 May 12, 2015

timmywil added a commit that referenced this issue May 12, 2015

CSS: fix :visible/:hidden selectors for inline element w/ content
- Reverts behavior from 10399dd, which we never released.
  BR and inline elements are considered visible.
- The possibility of dropping .offsetWidth and .offsetHeight
  was debunked by this perf:
  http://jsperf.com/visible-hidden-and-getclientrects

Fixes gh-2227
Close gh-2281
@shimonenator

This comment has been minimized.

Show comment
Hide comment
@shimonenator

shimonenator May 14, 2015

Sorry for being late to the proposed and accepted PR, but it does not fix the issue. Please refer to my original issue description and a follow up comment.

Sorry for being late to the proposed and accepted PR, but it does not fix the issue. Please refer to my original issue description and a follow up comment.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 14, 2015

Member

@shimonenator Had we wanted to keep the logic that was there, your suggestion would be right. However, the change was intentional.

Member

timmywil commented May 14, 2015

@shimonenator Had we wanted to keep the logic that was there, your suggestion would be right. However, the change was intentional.

@FesterCluck

This comment has been minimized.

Show comment
Hide comment
@FesterCluck

FesterCluck Sep 20, 2015

My apologies for commenting on a closed issue, but I believe this case warrants review.

Using ClientRectList.prototype.length an anchor tag with no text is "visible", not to mention one with child elements which have no dimension either.

Please consider https://github.com/FesterCluck/jquery/blob/master/src/css/hiddenVisibleSelectors.js

If this is not part of the new intended behavior, my apologies.

My apologies for commenting on a closed issue, but I believe this case warrants review.

Using ClientRectList.prototype.length an anchor tag with no text is "visible", not to mention one with child elements which have no dimension either.

Please consider https://github.com/FesterCluck/jquery/blob/master/src/css/hiddenVisibleSelectors.js

If this is not part of the new intended behavior, my apologies.

timmywil added a commit that referenced this issue Nov 10, 2015

CSS: fix :visible/:hidden selectors for inline element w/ content
- Reverts behavior from 10399dd, which we never released.
  BR and inline elements are considered visible.
- The possibility of dropping .offsetWidth and .offsetHeight
  was debunked by this perf:
  http://jsperf.com/visible-hidden-and-getclientrects

Fixes gh-2227
Close gh-2281

@cssmagic cssmagic referenced this issue in cssmagic/ChangeLog May 18, 2016

Open

jQuery #5

@jennifer-shehane jennifer-shehane referenced this issue in cypress-io/cypress Jan 31, 2018

Open

Element visibility fixes #1242

@jquery jquery locked as resolved and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.