core(tap-targets): don't exclude visible position absolute elements from audit #7778

mattzeunert · 2019-03-28T16:29:29Z

Summary

include tap targets that are position absolute or have a position absolute ancestor
check that tap targets are visible using elementFromPoint

Also catching this now:

Related Issues/PRs

This reverts commit 90404ff.

patrickhulce

high level looks pretty good! did you have the stats on sites that start failing more now in the issue or was that something else I was remembering?

lighthouse-core/gather/gatherers/seo/tap-targets.js

mattzeunert · 2019-03-28T17:49:24Z

high level looks pretty good! did you have the stats on sites that start failing more now in the issue or was that something else I was remembering?

I did a few of those recently, so you're probably thinking of something else. But I do plan to get some stats for this one too.

mattzeunert · 2019-04-02T14:25:58Z

DZL, do a barrel roll!

mattzeunert · 2019-04-03T14:49:11Z

Here are some examples of how scores are impacted. Generally it looks good, not just in terms of including position absolute elements, we're also detecting much better whether elements are visible. Do you think we need to do anything about the overflow: hidden issue on Godaddy?

For 100 sites I tested there's a change of ±5.7 point on average. Positive and negative changes even out overall and there's a drop of just over 1 point in the average score.

mattzeunert · 2019-04-03T16:35:11Z

@patrickhulce Should be good for some more review now 🙂

devtools-bot · 2019-04-03T16:39:00Z

DZL is done! Go check it out http://lh-dzl-7778.surge.sh

patrickhulce

LGTM!

patrickhulce · 2019-04-18T16:19:28Z

lighthouse-cli/test/fixtures/seo/seo-tap-targets.html

-    <div style="position: relative">
-      <a style="display: block; position: absolute; top:0; height: 40px; width: 40px;">inner</a>
-      <a style="display: block; height: 100px; width: 100px; background: yellowgreen">outer</a>
+    <!-- The left tap target has a large client rect that would normally overlap with the right tap target, 


😍 LOVIN' these tests :D

patrickhulce · 2019-04-18T16:20:40Z

lighthouse-cli/test/smokehouse/seo/expectations.js

            const SCORE_FACTOR = 0.89;
-            return Math.floor(PASSING_TAP_TARGETS / TOTAL_TAP_TARGETS * SCORE_FACTOR * 100) / 100;
+            return Math.round(passingTapTargets / totalTapTargets * SCORE_FACTOR * 100) / 100;


a tad worried by how much logic ends up living in our expectations, but I sympathize with the tedium of updating these and would rather have it be more precise so 👍

a tad worried by how much logic ends up living in our expectations, but I sympathize with the tedium of updating these and would rather have it be more precise so 👍

I think this might be going too far in this direction :/ It's one thing when it's broken down but still defined by constants, but since this is pulling live from the test file, it would be difficult to notice e.g. if we broke one of the overlapping tap target examples in the test page. So long as it also no longer matched the regex it would still pass and we wouldn't notice we were losing coverage.

It's also nice to have a place we can point to with "here's what we expect a failing/real value for this audit to look like", and with this approach the only way to do that is to put in a breakpoint and open up a debugger.

lighthouse-core/gather/gatherers/seo/tap-targets.js

brendankenny

left a couple real comments and some nitpicky ones that came up as I was reading :)

Higher level, though: to sidestep the need for switching on/off pointer-events, did you consider document.elementsFromPoint()? Since the returned array is in paint order, seems like it would be possible to shift off any starting elements that are fixed or sticky?

Or just not feasible?

brendankenny · 2019-04-18T23:03:25Z

lighthouse-cli/test/smokehouse/seo/expectations.js

            const SCORE_FACTOR = 0.89;
-            return Math.floor(PASSING_TAP_TARGETS / TOTAL_TAP_TARGETS * SCORE_FACTOR * 100) / 100;
+            return Math.round(passingTapTargets / totalTapTargets * SCORE_FACTOR * 100) / 100;


a tad worried by how much logic ends up living in our expectations, but I sympathize with the tedium of updating these and would rather have it be more precise so 👍

I think this might be going too far in this direction :/ It's one thing when it's broken down but still defined by constants, but since this is pulling live from the test file, it would be difficult to notice e.g. if we broke one of the overlapping tap target examples in the test page. So long as it also no longer matched the regex it would still pass and we wouldn't notice we were losing coverage.

It's also nice to have a place we can point to with "here's what we expect a failing/real value for this audit to look like", and with this approach the only way to do that is to put in a breakpoint and open up a debugger.

lighthouse-core/lib/rect-helpers.js

brendankenny · 2019-04-18T23:06:27Z

lighthouse-core/lib/rect-helpers.js

@@ -208,13 +204,15 @@ function getRectAtCenter(rect, centerRectSize) {
 /**
 * @param {LH.Artifacts.Rect} rect
 */
+/* istanbul ignore next */


same with this and getLargestRect, which are called from the tap-targets.js audit?

lighthouse-core/gather/gatherers/seo/tap-targets.js

Co-Authored-By: mattzeunert <matt@mostlystatic.com>

…gleChrome/lighthouse into tap-target-position-absolute

mattzeunert · 2019-04-21T12:12:37Z

to sidestep the need for switching on/off pointer-events, did you consider document.elementsFromPoint()? Since the returned array is in paint order, seems like it would be possible to shift off any starting elements that are fixed or sticky?

No I didn't consider it, that's a good idea!

I tried it now and elementsFP is a lot slower than elementFP. (200x slower in a simple loop for the same position over and over.) The gatherer as a whole is 2.5x slower on the List of 2017 albums.

devtools-bot · 2019-04-25T17:32:14Z

DZL is done! Go check it out http://lh-dzl-7778.surge.sh

paulirish · 2019-05-14T18:05:18Z

@brendankenny want another pass or is this g2g?

brendankenny

I have some lingering doubts about modifying the DOM like this, but the increased functionality and tests are great, so lets land and then take a look at it again in the future

…rom audit (#7778)

Filter pos absolute elements using elementFromPoint

728c003

mattzeunert requested review from brendankenny, patrickhulce and paulirish as code owners March 28, 2019 16:29

mattzeunert added 3 commits March 28, 2019 16:31

Restore formatting

90404ff

Revert "Restore formatting"

d3a9a8d

This reverts commit 90404ff.

Restore formatting

06a624a

patrickhulce reviewed Mar 28, 2019

View reviewed changes

lighthouse-core/gather/gatherers/seo/tap-targets.js Show resolved Hide resolved

lighthouse-core/gather/gatherers/seo/tap-targets.js Outdated Show resolved Hide resolved

lighthouse-core/gather/gatherers/seo/tap-targets.js Show resolved Hide resolved

scroll window instead of document

659fa88

mattzeunert added 4 commits April 3, 2019 13:31

Make disable point-events !imporant

0f8e2df

Position absolute wrapper for smoke test failure

233ee73

Fix lint

d9d40bf

Fix code cov tests

76cc42f

mattzeunert added 3 commits April 3, 2019 15:55

Fix code cov

c2767c3

Go further up down when checking if within scrollable area

993dd77

Always use fn.toString instead of fnString for rect helpers

a6d9932

GoogleChrome deleted a comment from devtools-bot Apr 3, 2019

mattzeunert added 3 commits April 3, 2019 17:11

Code style tweaks

463622c

Scroll to target position instead of using while loop

7f38635

while loop -> for of loop

4b9f808

mattzeunert added 2 commits April 3, 2019 17:48

Fix exception

31d8251

Don't make unreasonable scrollY check

8622bf4

patrickhulce approved these changes Apr 18, 2019

View reviewed changes

brendankenny reviewed Apr 18, 2019

View reviewed changes

Use textContent instead of innerHTML

f4a9ee4

Co-Authored-By: mattzeunert <matt@mostlystatic.com>

vercel bot requested a deployment to staging April 21, 2019 11:26 Abandoned

brendankenny and others added 2 commits April 21, 2019 12:27

No need for Array.from

8fd5e2d

Co-Authored-By: mattzeunert <matt@mostlystatic.com>

Spelling

b5a86d6

Co-Authored-By: mattzeunert <matt@mostlystatic.com>

vercel bot requested a deployment to staging April 21, 2019 11:28 Abandoned

mattzeunert added 2 commits April 21, 2019 12:50

Make tap targets in SEO smoke test more explicit

f20fc20

Merge branch 'tap-target-position-absolute' of https://github.com/Goo…

befe66e

…gleChrome/lighthouse into tap-target-position-absolute

vercel bot deployed to staging April 21, 2019 11:50 View deployment

doc

fda7c9e

vercel bot requested a deployment to staging April 21, 2019 12:21 Abandoned

Fix smoke coverage

aafa240

vercel bot deployed to staging April 25, 2019 12:49 View deployment

patrickhulce mentioned this pull request Apr 25, 2019

core(gather-runner): always reset scroll position #8625

Merged

mattzeunert mentioned this pull request Apr 26, 2019

How should we handle content changing when scrolling down the page? #8647

Closed

paulirish assigned brendankenny May 14, 2019

brendankenny added 2 commits May 24, 2019 18:28

Merge branch 'master' into tap-target-position-absolute

5e3c2f8

more

6689f0d

vercel bot deployed to staging May 25, 2019 02:24 View deployment

brendankenny approved these changes May 25, 2019

View reviewed changes

brendankenny added the land-when-ci-is-green label May 25, 2019

patrickhulce merged commit 631bbe2 into master May 25, 2019

patrickhulce deleted the tap-target-position-absolute branch May 25, 2019 03:43

brendankenny removed the land-when-ci-is-green label May 25, 2019

paulirish pushed a commit that referenced this pull request May 28, 2019

core(tap-targets): don't exclude visible position absolute elements f…

78963ff

…rom audit (#7778)

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(tap-targets): don't exclude visible position absolute elements from audit #7778

core(tap-targets): don't exclude visible position absolute elements from audit #7778

mattzeunert commented Mar 28, 2019 •

edited

Loading

patrickhulce left a comment

mattzeunert commented Mar 28, 2019

mattzeunert commented Apr 2, 2019

mattzeunert commented Apr 3, 2019

mattzeunert commented Apr 3, 2019

devtools-bot commented Apr 3, 2019

patrickhulce left a comment

patrickhulce Apr 18, 2019

patrickhulce Apr 18, 2019

brendankenny Apr 18, 2019

brendankenny left a comment

brendankenny Apr 18, 2019

brendankenny Apr 18, 2019

mattzeunert commented Apr 21, 2019

devtools-bot commented Apr 25, 2019

paulirish commented May 14, 2019

brendankenny left a comment

core(tap-targets): don't exclude visible position absolute elements from audit #7778

core(tap-targets): don't exclude visible position absolute elements from audit #7778

Conversation

mattzeunert commented Mar 28, 2019 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

mattzeunert commented Mar 28, 2019

mattzeunert commented Apr 2, 2019

mattzeunert commented Apr 3, 2019

mattzeunert commented Apr 3, 2019

devtools-bot commented Apr 3, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce Apr 18, 2019

Choose a reason for hiding this comment

patrickhulce Apr 18, 2019

Choose a reason for hiding this comment

brendankenny Apr 18, 2019

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Apr 18, 2019

Choose a reason for hiding this comment

brendankenny Apr 18, 2019

Choose a reason for hiding this comment

mattzeunert commented Apr 21, 2019

devtools-bot commented Apr 25, 2019

paulirish commented May 14, 2019

brendankenny left a comment

Choose a reason for hiding this comment

mattzeunert commented Mar 28, 2019 •

edited

Loading