-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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.
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
new_audit(tap-targets): check that tap targets are big enough and don't overlap #5846
Merged
Merged
Changes from 81 commits
Commits
Show all changes
107 commits
Select commit
Hold shift + click to select a range
b0969df
Support multi line outerHTML snippets
mattzeunert 30669ce
WIP
mattzeunert db12c55
Check each target for overlap failure, even if it is already involved…
mattzeunert f44c2fa
Fix table
mattzeunert cd6ec76
makeClientRects helper function
mattzeunert 9fc3125
Clean up gatherer
mattzeunert 366f033
Filter out tiny client rects
mattzeunert 4823e53
Clean up client rect functions
mattzeunert b943ffd
Improve data available for debugging/tweaking finger size
mattzeunert c0ad3b1
Tweak table titles
mattzeunert 34e0257
Rename BCR to client rects
mattzeunert c70ec6e
Move getNodePath to page functions
mattzeunert f80ef46
Add path to tap target node info
mattzeunert 440c86e
Add node selector to tap target node info
mattzeunert 7abf3cc
Show nothing instead of "n/a" when tap target isn't overlapped
mattzeunert 0354ec8
Remove outdated comment
mattzeunert 9da794c
Fix TS error
mattzeunert 3593623
Only report targets that are overlapped
mattzeunert a5c4e2f
Rename targetA and targetB
mattzeunert e5fb2c2
Tweak audit description
mattzeunert b64a67d
Add more data for debugging
mattzeunert 6256103
Move type def
mattzeunert 4048f8a
Fix linting
mattzeunert 52d9b4b
Clean up audit code
mattzeunert ba1490e
Update snaphots
mattzeunert d38f6da
Tweak comment
mattzeunert 8bb5460
Fix accessibility smoke test
mattzeunert 1e45c4c
Ignore missing artifacts error
mattzeunert 409b5be
Don't merge client rects if center of merged rect is outside original…
mattzeunert c705dfd
Fix gatherer
mattzeunert 7e183d9
Change display value to focus less on number of issues
mattzeunert 2fa0ef4
Merge branch 'master' into HEAD
mattzeunert acb63f9
Move rect helpers to client rect functions
3f74c94
Add more comments to audit code
1183b38
Add test case for fully contained tap target
29fb07a
Add tap targets smoke test
0f76dbb
Tidy up audit logic
7bb75b6
Explain convert DOMRect to plain object
45cc9ea
Use getElementsInDocument
9e9af23
Fix code cov error
19a918f
Add extra if branch comments
e4c7791
Don't mark invisible but displayed elements as inVisible
c3b1e2c
Rename checkClientRectsInsideParents param
50d98ed
Fix spelling
mattzeunert a8f039d
New scoring logic
mattzeunert d78bc27
fixup Don't mark invisible but displayed elements as inVisible Matt …
mattzeunert c9d8f2e
WIP fix handling of partially hidden tap targets
mattzeunert 0e80231
Merge branch 'master' into HEAD
mattzeunert 44ad070
Fix audit tap target test
mattzeunert df7615d
wip
mattzeunert 302c04e
core(minification): properly handle regex character classes (#6745)
patrickhulce a7e8ffb
core(tap-targets): helper functions for working with ClientRects (#6703)
mattzeunert e873dbb
report: larger margins for audit group summaries (#6688)
brendankenny 7201f6b
core(service-worker): check that start_url is within SW's scope (#6678)
brendankenny 974e277
Fix after merge
mattzeunert 7c59b8c
Ignore position absolute tap targets
mattzeunert 3b49651
remove old files
mattzeunert 61e0a81
Tweaks
mattzeunert 7121672
Tweak
mattzeunert 1f54523
Merge branch 'master' into HEAD
mattzeunert 05fa8b9
Tidy up code
mattzeunert 2f09fd4
Add smoke test for invisible position absolute tap target
mattzeunert e6c51a8
Remove memoize isVisibleNode since at least in its current form it do…
mattzeunert 64d184b
Add comment
mattzeunert e9de137
singular
mattzeunert 5dd5c40
Ignore tap targets that are nested inside a parent
mattzeunert e6fbf8a
Smoke test for overlapping tap targets with same href
mattzeunert 4701c24
Merge rects more generously
mattzeunert 465fcc0
Fix skipping tap targets when going through ancestors, stop just befo…
mattzeunert 9933b30
Add smoke test for position absolute tap target fully contained by an…
mattzeunert b3228f1
Move allRectsContainedWithinEachOther out of loop
mattzeunert d9a5a57
Tweaks/renaming
mattzeunert d5b1cf9
Don't report symmetric failures twice
mattzeunert 9e1a0ef
Fix type
mattzeunert 1459fb9
Tweak
mattzeunert 767e13e
Merge branch 'master' into HEAD
mattzeunert 13e5193
Update sample json
mattzeunert cd6ee28
Feedback Fixes
mattzeunert 8188e6b
Truncate outerHTML at 300 chars
mattzeunert 21c154e
Rename uniqueOverlapFailures to overlapFailuresForDisplay
mattzeunert ccf0418
Change TapTargetTableItem to interface
mattzeunert a1e134f
Move comment
mattzeunert d3595e2
Merge branch 'master' into HEAD
mattzeunert a33f181
Change Viewport artifact dependency to MetaElements
mattzeunert 1dd2d3d
Node -> Element
mattzeunert 928cf76
Explain why we only check content in text nodes for elementIsInTextBlock
mattzeunert 2c95d32
Type fix
mattzeunert 7793a58
Move ClientRectOverlapFailure/TapTargetOverlapFailure out of audit.d.ts
mattzeunert c2c2896
Don't crash if Element.prototype.matches is overridden
mattzeunert 0138978
Tweaks
mattzeunert 1fc2c8f
Add full audit error to smoke test expectations
mattzeunert 900dd3f
Fix line length
mattzeunert 9f58755
Use useIsolation to handle matches being overwritten
mattzeunert 5bfd792
Move hasTextNodeSiblingsFormingTextBlock outside function block
mattzeunert d320c4f
Remove unnecessary if statement
mattzeunert a001852
Remove ThemeColor/Viewport that was restored during merge
mattzeunert be53ec6
Removed unused window var
mattzeunert ba795c7
Add test case for overwritten matches function
mattzeunert 8fd667c
Make failed viewport audit message in tap targets/font size more spec…
mattzeunert efec03b
Fix tests following previous change
mattzeunert f58fe21
Fix line length for previous changes
mattzeunert 4ae7575
Tweaks
mattzeunert de60ebb
Shorten getTableItems code
mattzeunert 7d497ae
Move TapTargetTableItem type
mattzeunert 1597590
Add test case for no viewport meta tag
mattzeunert 2a33179
Test case for only one tap target in a pair failing
mattzeunert 729eaaf
Rename test options to make more sense
mattzeunert File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
<!doctype html> | ||
<!-- | ||
* Copyright 2018 Google Inc. All Rights Reserved. | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
--> | ||
|
||
<html> | ||
<head> | ||
<title>SEO tap target audit tester</title> | ||
<meta charset="utf-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0"> | ||
<style> | ||
a { | ||
color: blue; | ||
} | ||
body { | ||
margin: 0; | ||
} | ||
</style> | ||
</head> | ||
<body> | ||
<!-- Big tap target, but it's invisible because it's behind the main content div --> | ||
<a style="background: red; position: absolute; top: 0; bottom: 0; left: 0; right: 0; z-index: -1;"></a> | ||
<div style="background: white; padding: 20px;"> | ||
<h1>SEO Tap targets</h1> | ||
|
||
<!-- Invisible nodes don't cause failures --> | ||
<div> | ||
<!-- Scroll container hides the child --> | ||
<div style="height: 1px; overflow: auto;"> | ||
<a style="display: inline-block; height: 1000px; width: 1000px"></a> | ||
</div> | ||
<!-- Various invisible tap targets--> | ||
<a></a> | ||
<a style="width: 0;height:0;display:inline-block;">0</a> | ||
<a style="display: none">display none</a> | ||
<span style="display: none"><a>display none parent</a></span> | ||
<a style="display: inline-block; width: 0; overflow-x: hidden;height: 1px"> | ||
width 0 and overflow x hidden | ||
</a> | ||
<!-- Visible target should not fail because nothing overlaps it --> | ||
<a>visible target</a> | ||
</div> | ||
<br/><br/> | ||
|
||
|
||
<div style="overflow: hidden; position: relative"> | ||
<!-- Should be counted as visible although part of it is outside the scroll container --> | ||
<a> | ||
<div style="position: absolute;top: -100px">invisible</div> | ||
visible | ||
</a> | ||
</div> | ||
|
||
<br/><br/> | ||
|
||
<!-- Link contains large inline block element - no failure because finger | ||
should be placed in the center of the whole link area --> | ||
<a style="background: red;"> | ||
<div style="display: inline-block; height: 30px; width: 30px; background: orange;"></div> | ||
Link | ||
</a> | ||
<br/> | ||
<a style="display: block; padding: 30px; background: #ddf;"> | ||
Link that the top one would overlap with, if it weren't for the inline-block child. | ||
</a> | ||
|
||
<br/><br/> | ||
|
||
<div role="button"> | ||
Tap target with children that are also tap targets should not fail. | ||
(Children should not be counted as independent tap targets that appear | ||
in the list.) | ||
Two children to make sure the two children also don't conflict with each other: | ||
<a>Child 1</a><a>Child2</a> | ||
</div> | ||
|
||
<br/><br/> | ||
|
||
<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> | ||
</div> | ||
|
||
<br/><br/> | ||
|
||
<!-- Only target that's being overlapped should fail, the overlapping one shouldn't --> | ||
<div> | ||
<a style="display: block; width: 100px; height: 30px;background: #ddd;"> | ||
too small target | ||
</a> | ||
<a style="display: block; width: 100px; height: 100px;background: #aaa;"> | ||
big enough target | ||
</a> | ||
</div> | ||
|
||
<br/><br/> | ||
|
||
<!-- Should not fail if the two links have the same link target --> | ||
<div> | ||
<a style="display: block; width: 10px; height: 10px;" href="../seo/"></a> | ||
<a style="display: block; width: 10px; height: 10px;" href="./"></a> | ||
</div> | ||
|
||
<!-- Links in text blocks are exempted from size/overlap requirements and should not fail --> | ||
<p style="width: 100px"> | ||
This is <a>a link in a text block.</a> | ||
This is <a>a link in a text block.</a> | ||
This is <a>a link in a text block.</a> | ||
This is <a>a link in a text block.</a> | ||
This is <a>a link in a text block.</a> | ||
</p> | ||
</div> | ||
</body> | ||
</html> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there more that can be asserted here (like the contents of the item)? This is the only test for a pretty complicated gatherer, and asserting two numbers about the end result feels like it could accidentally miss regressions/changing details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an assertion for the item details.
It would be nice to assert the full list of detected tap targets from the artifacts. Currently smokehouse just provides the LHR though.