core: normalize creation of NodeValue

[connorjclark]

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]

• 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]

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

[connorjclark]

would splitting up

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

[adamraine]

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

[adamraine]

LGTM

[brendankenny]

Why do we add explanation here?

#5402

[brendankenny]

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]

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?

[connorjclark]

sometimes very long snippets

examples?

[connorjclark]

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

[brendankenny]

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?

[connorjclark]

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]

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?)

[connorjclark]

I'll followup with this in a different PR.

[brendankenny]

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?

[connorjclark]

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.