core: normalize creation of NodeValue by connorjclark · Pull Request #11877 · GoogleChrome/lighthouse

connorjclark · 2020-12-22T23:16:41Z

Background

NodeDetails are collected by Gatherers. They are part of artifacts, typically as a node: NodeDetails property.
NodeValue is a detail type, and is created by audits.
NodeValue and NodeDetails differ trivially:
- NodeDetails['devtoolsNodePath'] is called NodeValue['path']
- In NodeDetails only optional fields are lhId and boundingRect. In NodeValue, everything is optional.
All NodeDetails are created by using the page function getNodeDetails.
Most audits create a NodeValue by using a NodeDetails, but some audits create their own NodeValue without the backing of a NodeDetails. These few non-standard usages is what forces us to have NodeValue be all-optional.

(the following categories are not mutually exclusive)

A) Audits that create a `NodeValue` directly from a `NodeDetail` (1:1)

all axe audits
largest-contentful-paint-element
layout-shift-element

B) Audits that create a `NodeValue` directly from a `NodeDetail` (but misses something. All of these are at least missing `boundingRect`/`lhId`, so no element screenshots):

autocomplete
crawlable-anchors
dom-size
external-anchors-use-rel-noopener
hreflang (where applicable for LinkElements)
is-crawlable
non-composited-animations
password-inputs-can-be-pasted-into
plugins
unsized-images

C) Audits that create a `NodeValue` directly from a `NodeDetail`, but override a property

hreflang (ex)
is-crawlable
plugins
tap-targets (

lighthouse/lighthouse-core/audits/seo/tap-targets.js

Line 256 in 2bdf6cb

boundingRect,

this is just getting the bounding rect from somewhere else... This must have been b/c when the audit was made, node detail did not have a bounding rect. Today, I think this is basically ignored because of how FullPageScreenshot works.)

D) Audits that create a `NodeValue` from something OTHER than a `NodeDetail`

font-size (

lighthouse/lighthouse-core/audits/seo/font-size.js

Line 120 in 2bdf6cb

type: 'node',

)
hreflang (sometimes, only if LinkElement is from response headers)

This PR

For audits creating a NodeValue from a NodeDetails, use new function Audit.makeNodeValue. This simplifies category A), fixes category B), and makes category C) much more obvious.
Make snippet required in NodeValue. This is the strictest we can get with this, without changing anything about usages in category D). It also conceptually make sense... if you want to show something as a node, but aren't using NodeDetails, you should be able to provide something HTML-ish.
Since all NodeDetails are created via the page function, and that function always returns lhId and boundingRect, we can make those properties required. This didn't require any code changes, I think it was an oversight in core(full-page-screenshot): resolve node rects during emulation #11536.
To be extra sure that NodeDetails properties are all present, for the string values do ... || ''. I'm not super convinced about this (and it seems we'd really want to wrap each with a try...catch to catch the errors we've seen before).

brendankenny · 2020-12-23T01:04:40Z

tap-targets (

lighthouse/lighthouse-core/audits/seo/tap-targets.js

Line 256 in 2bdf6cb

boundingRect,

this is just getting the bounding rect from somewhere else... This must have been b/c when the audit was made, node detail did not have a bounding rect. Today, I think this is basically ignored because of how FullPageScreenshot works.)

this is the bounding rect for all of the client rects of the tap target, plus some extra:

lighthouse/lighthouse-core/lib/rect-helpers.js

Lines 96 to 127 in d584194

    
           function getBoundingRectWithPadding(rects, padding) { 
        
             if (rects.length === 0) { 
        
               throw new Error('No rects to take bounds of'); 
        
             } 
        
             let left = Number.MAX_VALUE; 
        
             let right = -Number.MAX_VALUE; 
        
             let top = Number.MAX_VALUE; 
        
             let bottom = -Number.MAX_VALUE; 
        
             for (const rect of rects) { 
        
               left = Math.min(left, rect.left); 
        
               right = Math.max(right, rect.right); 
        
               top = Math.min(top, rect.top); 
        
               bottom = Math.max(bottom, rect.bottom); 
        
             } 
        
             // Pad on all sides. 
        
             const halfMinSize = padding / 2; 
        
             left -= halfMinSize; 
        
             right += halfMinSize; 
        
             top -= halfMinSize; 
        
             bottom += halfMinSize; 
        
             return { 
        
               left, 
        
               right, 
        
               top, 
        
               bottom, 
        
               width: right - left, 
        
               height: bottom - top, 
        
             }; 
        
           }

not really the same as all the other bounding rects, the danger of trying to unify things in one type :)

brendankenny

nice! Will definitely make it easier to make things real NodeValues

brendankenny · 2020-12-23T01:17:59Z

    lhId,
-    devtoolsNodePath: getNodePath(element),
-    selector: getNodeSelector(htmlElement),
+    devtoolsNodePath: getNodePath(element) || '',


maybe the fallbacks (if needed) should go in the individual functions rather than here?

connorjclark · 2020-12-23T18:18:02Z

would splitting up

Sure, let's split out the changes to MetaElements.

adamraine · 2020-12-23T18:31:37Z

-          snippet: axeNode.node.snippet,
-          boundingRect: axeNode.node.boundingRect,
+          ...Audit.makeNodeValue(axeNode.node),
          explanation: axeNode.failureSummary,


Doesn't look like this gets included in the report 🤔 . Is it supposed to?

adamraine

LGTM

brendankenny · 2021-01-05T20:33:13Z

-          snippet: axeNode.node.snippet,
-          boundingRect: axeNode.node.boundingRect,
+          ...Audit.makeNodeValue(axeNode.node),
          explanation: axeNode.failureSummary,


Why do we add explanation here?

#5402

brendankenny · 2021-01-05T20:45:25Z

        boundingRect?: Artifacts.Rect;
        /** An HTML snippet used to identify the node. */
-        snippet?: string;
+        snippet: string;


the possible bugs should be relatively minor, but unless we update snippet to always be a string in Util.prepareReportResult, this won't be typesafe for old LHRs (and if we have to retain the current if (item.snippet) { instead, there's not much benefit of making this non-optional)

brendankenny · 2021-01-05T20:52:42Z

          source: {
-            type: /** @type {'node'} */ ('node'),
+            ...Audit.makeNodeItem(plugin.node),
            snippet: `<${tagName}${attributes}>${params}</${tagName}>`,


this ends up rendering completely differently in the report (it was basically just <span class="lh-node"><div class="lh-node__snippet">${snippet}</div></span> before). Does it work well for this audit (with these sometimes very long snippets)? Should it use the default NodeDetails snippet instead of its custom one?

sometimes very long snippets

examples?

Should it use the default NodeDetails snippet instead of its custom one?

Maybe, if we wanted to we could parameterize getNodeDetails/Snippet to accept a list of attribute names (give it SOURCE_PARAMS).

Does it work well for this audit (with these sometimes very long snippets)?

examples?

unlike the getNodeDetails().snippet, the plugin attributes and params aren't ever truncated in number or string length, so it could be as long as the original page wanted

Should it use the default NodeDetails snippet instead of its custom one?

Maybe, if we wanted to we could parameterize getNodeDetails/Snippet to accept a list of attribute names (give it SOURCE_PARAMS).

yeah, though I guess do we care about the <param> elements or are the element attributes (plus all the other node details stuff) sufficient to identify it?

ehhh let's just use the default snippet so we can keep this simple. it'll be more identifiable now with the devtools node path and everything else, and truncation will prevent the worse cases.

brendankenny · 2021-01-05T21:08:46Z

-    path: target.node.devtoolsNodePath,
-    selector: target.node.selector,
+    ...Audit.makeNodeItem(target.node),
    boundingRect,


Which boundingRect should be used here? Using the boundingRect of all clientRects goes back to the first commit of #10716, but getNodeDetails also didn't exist back then, so that might have just been the most convenient way to do it at the time. Should it be the just node.boundingRect? When can the two differ? getBoundingClientRect should normally contain all child rects, but does it ever differ (transforms or something?)

I'll followup with this in a different PR.

brendankenny · 2021-01-05T21:19:35Z

      [{
        tagName: 'APPLET',
        params: [],
+        node: {},


since this PR is adding a new EmbeddedContent artifact property and changing the audit details almost completely and we're trying to reach toward a typed test future (#11728), can these be generated so they're more like the real thing and the output tested to some degree?

I'm struggling to find any actual pages with these elements. The one I found isn't included in our plugin list: https://www.quackit.com/html_5/tags/html_object_tag.cfm (video/quicktime). Should that type be included?

EDIT: well video/quicktime is expected to pass in a test. I don't understand the purpose of this audit if a quicktime plugin is acceptable. Is the idea that any video is OK? I suppose since this is an SEO audit video is out of scope, it's just that this audit also could double as a user/compat/browser-friendliness check.

Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>

… into node-details-yay

connorjclark added 30 commits May 19, 2020 18:35

core(driver): create page code using structured interface

90018b2

rename type

f7e38f5

pr feedback

78f386a

no strings

32856e7

test

e2dd63d

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced

f38bf41

test

ce9839b

restrucutre

4c903de

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced

0413a4a

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced

d4bd2bf

remove obj

02dc215

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced

ce2827a

update master

c81a636

rm dead code

70d8210

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced

df5e8dd

adam feedback

4e59476

fix tests

147b98c

fix mangle issues

a4051a1

wip

9eea206

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2

d79b23f

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2

b8c76ed

fix

2088109

wip

7b01e8c

fix

4cc1eea

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2

aae0a9f

fix tests

77af88d

fix

d35fd5e

tmp

eb897cf

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2

9c10cda

update

d643baf

brendankenny reviewed Dec 23, 2020

View reviewed changes

rename

098411b

vercel Bot deployed to Preview December 23, 2020 18:08 View deployment

no meta els

cc8cdb3

adamraine reviewed Dec 23, 2020

View reviewed changes

adamraine added waiting4committer and removed waiting4reviewer labels Dec 23, 2020

tweak

f763a1f

vercel Bot deployed to Preview December 23, 2020 18:45 View deployment

revert

25210c5

vercel Bot deployed to Preview December 23, 2020 19:11 View deployment

connorjclark added waiting4reviewer and removed waiting4committer labels Dec 28, 2020

adamraine approved these changes Jan 5, 2021

View reviewed changes

Comment thread lighthouse-core/gather/gatherers/seo/embedded-content.js Outdated

brendankenny reviewed Jan 5, 2021

View reviewed changes

update

ba30ab5

vercel Bot deployed to Preview January 11, 2021 17:51 View deployment

Apply suggestions from code review

0679ff5

Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>

vercel Bot deployed to Preview January 11, 2021 19:34 View deployment

connorjclark added 2 commits January 11, 2021 13:40

kiss

68ee66f

Merge branch 'node-details-yay' of github.com:GoogleChrome/lighthouse…

d7c562c

… into node-details-yay

vercel Bot deployed to Preview January 11, 2021 19:46 View deployment

connorjclark merged commit 2e242d6 into master Jan 12, 2021

connorjclark deleted the node-details-yay branch January 12, 2021 23:33

connorjclark mentioned this pull request Jan 12, 2021

use normal node rect for tap targets #11948

Closed

This was referenced Mar 18, 2021

use NodeDetails in MetaElements #12263

Closed

core(hreflang): use Audit.makeNodeItem #12273

Merged

paulirish pushed a commit that referenced this pull request Mar 23, 2021

core: normalize creation of NodeValue (#11877)

144689a

Conversation

connorjclark commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

A) Audits that create a NodeValue directly from a NodeDetail (1:1)

B) Audits that create a NodeValue directly from a NodeDetail (but misses something. All of these are at least missing boundingRect/lhId, so no element screenshots):

C) Audits that create a NodeValue directly from a NodeDetail, but override a property

D) Audits that create a NodeValue from something OTHER than a NodeDetail

This PR

Uh oh!

brendankenny commented Dec 23, 2020

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark commented Dec 23, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamraine left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

connorjclark commented Dec 22, 2020 •

edited

Loading

A) Audits that create a `NodeValue` directly from a `NodeDetail` (1:1)

B) Audits that create a `NodeValue` directly from a `NodeDetail` (but misses something. All of these are at least missing `boundingRect`/`lhId`, so no element screenshots):

C) Audits that create a `NodeValue` directly from a `NodeDetail`, but override a property

D) Audits that create a `NodeValue` from something OTHER than a `NodeDetail`