core(perf): speed up tap-target's isVisible() #9056
Conversation
|
Looking at the failure... This implementation includes this node from the smoketest, but it should not: <a style="display: inline-block; width: 0; overflow-x: hidden;height: 1px">
width 0 and overflow x hidden
</a> |
| } | ||
|
|
||
| return true; | ||
| return !!(element.offsetWidth || element.offsetHeight || element.getClientRects().length); |
There was a problem hiding this comment.
to fix the failure can we check if any of these dimensions are 0?
it feels like everything was so carefully written the first time around though I'm not sure I fully understand how these compare...
There was a problem hiding this comment.
Yeahh... something with a 1px height but 0x width is still invisible. doesnt matter the height.
Not sure what the test case was supposed to be for. @mattzeunert
There was a problem hiding this comment.
The test failure is fixed after merging master, because the node is filtered out by elementCenterIsAtZAxisTop. The position absolute PR also generally improves the smoke test a bunch.
Not sure what the test case was supposed to be for.
The tap target can't be tapped on because the content is hidden by the overflow: hidden. We care about the total number of tap targets in the artifacts because the pass rate is 1 - failureCount/totalTapTargets, and we don't want invisible elements to bring the score up.
to fix the failure can we check if any of these dimensions are 0?
Just because the element has a width of 0 doesn't mean the tap target is untappable, because there could be overflowing child content that the user can tap on.
I added a test case for a width: 0 element with visible child content.
it feels like everything was so carefully written the first time around though I'm not sure I fully understand how these compare...
I think this change is fine, we have reasonable smoke test coverage and I tried it on 50 sites and didn't notice any changes.
I think the new elementIsVisible will deem more elements visible than before, but they'll then be filtered out by elementCenterIsAtZAxisTop.
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the ℹ️ Googlers: Go here for more info. |
| <a | ||
| data-gathered-target="zero-width-tap-target-with-overflowing-child-content" | ||
| style="display: block; width: 0; white-space: nowrap"> | ||
| <!-- TODO: having the span should not be necessary to cause a failure here, but |
There was a problem hiding this comment.
I think we could do this with https://stackoverflow.com/a/28457044/1290545. Konrad's original code actually used it, but then neither of us could figure out what it was needed for 🙂
There was a problem hiding this comment.
If it's good enough for @mattzeunert, it's good enough for me! 😃
Is the test failure expected given your smoketest changes though @mattzeunert? Or is that a bug that needs fixing?
Seems like CI rendered the link 1px less tall than on my local. |
thanks jquery!
https://github.com/jquery/jquery/blob/master/src/css/hiddenVisibleSelectors.js
Seems good...