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.

Sign up for GitHub

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

Jump to bottom

core(perf): speed up tap-target's isVisible() #9056

Merged
merged 7 commits into from
Jun 5, 2019
Merged

core(perf): speed up tap-target's isVisible() #9056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d7c7c39
core(perf): speed up tap-target's isVisible()
paulirish May 25, 2019
ec81ea2
Merge branch 'master' into isvisible
mattzeunert May 25, 2019
9f58389
Fix types
mattzeunert May 25, 2019
f012ce6
Smoke test for failing tap targets with width 0
mattzeunert May 25, 2019
19f2e55
Fix lint
mattzeunert May 28, 2019
b0d621d
Merge branch 'master' into isvisible
mattzeunert May 29, 2019
0e3dace
Fix element height in test
mattzeunert May 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lighthouse-cli/test/fixtures/seo/seo-tap-targets.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,24 @@ <h1>SEO Tap targets</h1>

<br/><br/>

<!-- even if a tap target's size is 0x0px it can still fail if overflowing content is visible -->
<div>
<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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

right now we don't try to get the client rects of children that are text nodes -->
<span style="display: inline-block; width: 110px; height: 18px">zero width target</span>
</a>
<a
data-gathered-target="passing-tap-target-next-to-zero-width-target"
style="display: block; width: 110px; height: 100px;background: #aaa;">
passing target
</a>
</div>

<br/><br/>

<!-- Should not fail if the two links have the same link target -->
<div>
<a
Expand Down
33 changes: 33 additions & 0 deletions lighthouse-cli/test/smokehouse/seo/expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ const expectedGatheredTapTargets = [
{
snippet: /large-enough-tap-target-next-to-too-small-tap-target/,
},
{
snippet: /zero-width-tap-target-with-overflowing-child-content/,
shouldFail: true,
},
{
snippet: /passing-tap-target-next-to-zero-width-target/,
},
{
snippet: /links-with-same-link-target-1/,
},
Expand Down Expand Up @@ -251,6 +258,30 @@ module.exports = [
})(),
details: {
items: [
{
'tapTarget': {
'type': 'node',
/* eslint-disable max-len */
'snippet': '<a data-gathered-target="zero-width-tap-target-with-overflowing-child-content" style="display: block; width: 0; white-space: nowrap">\n <!-- TODO: having the span should not be necessary to cause a failure here, but\n right now we don\'t try to get the client rects of children that …',
'path': '2,HTML,1,BODY,14,DIV,0,A',
'selector': 'body > div > a',
'nodeLabel': 'zero width target',
},
'overlappingTarget': {
'type': 'node',
/* eslint-disable max-len */
'snippet': '<a data-gathered-target="passing-tap-target-next-to-zero-width-target" style="display: block; width: 110px; height: 100px;background: #aaa;">\n passing target\n </a>',
'path': '2,HTML,1,BODY,14,DIV,1,A',
'selector': 'body > div > a',
'nodeLabel': 'passing target',
},
'tapTargetScore': 864,
'overlappingTargetScore': 720,
'overlapScoreRatio': 0.8333333333333334,
'size': '110x18',
'width': 110,
'height': 18,
},
{
'tapTarget': {
'type': 'node',
Expand All @@ -276,11 +307,13 @@ module.exports = [
'type': 'node',
'path': '2,HTML,1,BODY,3,DIV,24,A',
'selector': 'body > div > a',
'nodeLabel': 'left',
},
'overlappingTarget': {
'type': 'node',
'path': '2,HTML,1,BODY,3,DIV,25,A',
'selector': 'body > div > a',
'nodeLabel': 'right',
},
'tapTargetScore': 1920,
'overlappingTargetScore': 560,
Expand Down
32 changes: 3 additions & 29 deletions lighthouse-core/gather/gatherers/seo/tap-targets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,38 +37,12 @@ const TARGET_SELECTORS = [
const tapTargetsSelector = TARGET_SELECTORS.join(',');

/**
* @param {Element} element
* @param {HTMLElement} element
* @returns {boolean}
*/
/* istanbul ignore next */
function elementIsVisible(element) {
const {overflowX, overflowY, display, visibility} = getComputedStyle(element);

if (
display === 'none' ||
(visibility === 'collapse' && ['TR', 'TBODY', 'COL', 'COLGROUP'].includes(element.tagName))
) {
// Element not displayed
return false;
}

// only for block and inline-block, since clientWidth/Height are always 0 for inline elements
if (display === 'block' || display === 'inline-block') {
// if height/width is 0 and no overflow in that direction then
// there's no content that the user can see and tap on
if ((element.clientWidth === 0 && overflowX === 'hidden') ||
(element.clientHeight === 0 && overflowY === 'hidden')) {
return false;
}
}

const parent = element.parentElement;
if (parent && parent.tagName !== 'BODY') {
// if a parent is invisible then the current element is also invisible
return elementIsVisible(parent);
}

return true;
return !!(element.offsetWidth || element.offsetHeight || element.getClientRects().length);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand Down Expand Up @@ -238,7 +212,7 @@ function gatherTapTargets() {
// Capture element positions relative to the top of the page
window.scrollTo(0, 0);

/** @type {Element[]} */
/** @type {HTMLElement[]} */
// @ts-ignore - getElementsInDocument put into scope via stringification
const tapTargetElements = getElementsInDocument(tapTargetsSelector);

Expand Down
6 changes: 3 additions & 3 deletions lighthouse-core/lib/page-functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,15 @@ function checkTimeSinceLastLongTask() {
/**
* @param {string=} selector Optional simple CSS selector to filter nodes on.
* Combinators are not supported.
* @return {Array<Element>}
* @return {Array<HTMLElement>}
*/
/* istanbul ignore next */
function getElementsInDocument(selector) {
const realMatchesFn = window.__ElementMatches || window.Element.prototype.matches;
/** @type {Array<Element>} */
/** @type {Array<HTMLElement>} */
const results = [];

/** @param {NodeListOf<Element>} nodes */
/** @param {NodeListOf<HTMLElement>} nodes */
const _findAllElements = nodes => {
for (let i = 0, el; el = nodes[i]; ++i) {
if (!selector || realMatchesFn.call(el, selector)) {
Expand Down