fix isVisible false positives from deep nesting or alternate means

[RyanBerliner]

fixes #33914

[alpadev]

Can't judge for the if-condition, but everything else LGTM, thank you! 👍

[alpadev]

@RyanBerliner Hm wouldn't be checking getClientRects() enough to see if an element (or an ancestor of it) has display: none? 🤔

[RyanBerliner]

@RyanBerliner Hm wouldn't be checking getClientRects() enough to see if an element (or an ancestor of it) has display: none? 🤔

99% of the time, yes! However, so long as we keep the element.tagName !== 'AREA', then we'll still need to check the display property for supporting that specific tag name. I know we could also squeeze in a few more edge cases to support here, though I'd imagine it's preferred only to improve it as much as we think the framework will need it. I'm looking at this function through the lens of the most rigorous use case bootstrap will be using it for, which is probably #33865.

This is why I've decided to support the <area> edge case, and not (for example) the svg edge case. <area> is focusable by default, and I'd think someone is more likely to put an image map in a modal (still unlikely though lol) than an svg with an explicit tabindex. If you all have more insight into what edge cases you'd imagine should be supported I can adjust accordingly.

[GeoSot]

Maybe window.getComputedStyle(x).getPropertyValue('visibility') would work without tricks
Example wrong link sorry
Example

const eligibleElement = el => el && el.style could by replaced be isElement?

[alpadev]

@RyanBerliner From my observations getClientRects() seem to work for <area>. That's true at least with the latest versions of Chromium, FF and Safari. (The values of the DOMRect that is returned are wrong but it's not like we check those). I did some research and found a Chromium bug, that mentions, that they eventually changed that some years ago to match the FF behaviour. (https://bugs.chromium.org/p/chromium/issues/detail?id=416195#c6)

I couldn't find any informations in the spec tho, that would confirm if an HTMLAreaElement should or shouldn't have a bounding box.

Also I found that even when you apply display: none to an <area> or <map> element, you can still tab to it and click it. So not sure if it makes sense even, to check if they are visible or not.

Maybe we should just risk it, and fix it if some issue pops up.

[RyanBerliner]

Interesting - I'm starting to think we should just completely remove the edge case handling of <area> as it seems it's a little all over the place with browser behavior. Here is where I got my initial inspiration to handle <area> separately, fyi. Then we can completely remove the loop, rely entirely on getClientRect(), and do a final visibility check with the computed style property like @GeoSot suggested. Would that work with everyone or should I proceed with the other suggested changes while maintaining separate <area> checking?

[alpadev]

To be honest, I don't really have a preference here. It's hard to argue when neither the browser nor the specification is clear about this behavior. I feel like image maps may be some abandoned thing since they lack of responsiveness or something.

I had a talk with Geo yesterday, and he asked about opacity. My guess that would require to have a loop, if we want to take care about that too.

Hard to say what's the best approach here, but I'm a bit concerned, that with that many iterations we will block you or kill your mood a bit. ❤️

[GeoSot]

IMO kill the 'AREA' thing. Is so rare.
The checks are already better than before.

As @alpadev said:

Maybe we should just risk it, and fix it if some issue pops up.

I cannot agree more.