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(full-page-screenshot): do not render zero size rects #11853

Merged
merged 7 commits into from
Dec 29, 2020
Merged

core(full-page-screenshot): do not render zero size rects #11853

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8e29855
core(full-page-screenshot): do not collect zero size bounding rects
connorjclark Dec 17, 2020
05d8c25
fixtest
connorjclark Dec 17, 2020
c7922d3
update
connorjclark Dec 21, 2020
2027fb4
Merge remote-tracking branch 'origin/master' into bounding-rect-zero
connorjclark Dec 21, 2020
bbae5dd
ok
connorjclark Dec 21, 2020
001dea4
Merge remote-tracking branch 'origin/master' into bounding-rect-zero
connorjclark Dec 28, 2020
7bf6bab
no fallback
connorjclark Dec 28, 2020
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
15 changes: 3 additions & 12 deletions lighthouse-cli/test/smokehouse/test-definitions/a11y/expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,7 @@ const expectations = [
{
node: {
'type': 'node',
'boundingRect': {
'width': '>0',
'height': 0,
},
'boundingRect': undefined,
'selector': 'body > section > div > div#aria-treeitem-name',
'snippet': '<div id="aria-treeitem-name" role="treeitem">',
'explanation': 'Fix any of the following:\n Element does not have text that is visible to screen readers\n aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty\n aria-label attribute does not exist or is empty\n Element has no title attribute',
Expand All @@ -148,10 +145,7 @@ const expectations = [
{
node: {
'type': 'node',
'boundingRect': {
'width': '>0',
'height': 0,
},
'boundingRect': undefined,
'selector': 'body > section > div#aria-command-name',
'snippet': '<div id="aria-command-name" role="button">',
'explanation': 'Fix any of the following:\n aria-label attribute does not exist or is empty\n aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty\n Element has no title attribute\n Element does not have text that is visible to screen readers',
Expand All @@ -168,10 +162,7 @@ const expectations = [
{
node: {
'type': 'node',
'boundingRect': {
'width': '>0',
'height': 0,
},
'boundingRect': undefined,
'selector': 'body > section > div#aria-tooltip-name',
'snippet': '<div id="aria-tooltip-name" role="tooltip">',
'explanation': 'Fix any of the following:\n aria-label attribute does not exist or is empty\n aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty\n Element has no title attribute\n Element does not have text that is visible to screen readers',
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/gather/gatherers/full-page-screenshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ class FullPageScreenshot extends Gatherer {
for (const [node, id] of lhIdToElements.entries()) {
// @ts-expect-error - getBoundingClientRect put into scope via stringification
const rect = getBoundingClientRect(node);
if (rect.width || rect.height) nodes[id] = rect;
if (rect) nodes[id] = rect;
}

return nodes;
Expand Down
4 changes: 3 additions & 1 deletion lighthouse-core/lib/page-functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,12 +409,14 @@ function getNodeLabel(node) {

/**
* @param {HTMLElement} element
* @return {LH.Artifacts.Rect}
* @return {LH.Artifacts.Rect|undefined}
*/
/* istanbul ignore next */
function getBoundingClientRect(element) {
// The protocol does not serialize getters, so extract the values explicitly.
const rect = element.getBoundingClientRect();
if (rect.width === 0 || rect.height === 0) return;
Copy link
Member

Choose a reason for hiding this comment

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

other places use this function and might expect a 0x0 rect returned, though...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

darn @ts-expect-error and stringification! I'll look closer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nope, this is only used in two places: page-function getNodeDetails, and the FPS gatherer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the silliness be avoided through other means like ignoring head contents?

I agree that doing this for all elements that happen to not have size might be a mistake. The location can often still provide value in hidden element cases.

Copy link
Member

@brendankenny brendankenny Dec 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, I guess I meant a function called getBoundingClientRect seems like the wrong place to determine if it's worth keeping the rect from the element or not. getBoundingClientRectIfItHasSize, maybe :)

Agreed it is a little weird that all the uses of NodeDetails keep their boundingRect, though. e.g. LinkElements didn't have any before #11405. Maybe we need an easy way for callers of getNodeDetails to indicate that they know that 0x0 bounding rects aren't important and can be discarded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

points well taken! I've reversed the approach here.


return {
top: Math.round(rect.top),
bottom: Math.round(rect.bottom),
Expand Down