misc: return specific html element type for dom.find #11526

connorjclark · 2020-10-06T23:46:38Z

~~Adds an optional third parameter to dom.find/findAll which is plugged into typescript to return a specific HTML element type. Also does a runtime check.~~ Allows us to remove some ugly type coercion.

~~found all occurrences with \*/ \(.*dom\.find. maybe more?~~

An alternative API would be to keep the parameters the same, but use TypeScript magic to extract the tag name from a CSS selector. But I don't think typescript yet has the meta-programming capabilities for this (the template strings update is moving closer to this... microsoft/TypeScript#40336 ).

ended up using TypeScript magic via typed-query-selector :)

lighthouse-core/report/html/renderer/category-renderer.js

brendankenny · 2020-10-07T18:51:10Z

An alternative API would be to keep the parameters the same, but use TypeScript magic to extract the tag name from a CSS selector. But I don't think typescript yet has the meta-programming capabilities for this (the template strings update is moving closer to this... microsoft/TypeScript#40336).

FWIW there's been a few people already doing this with the 4.1 beta, e.g.

https://twitter.com/MikeRyanDev/status/1308472279010025477

TS Playground example

connorjclark · 2020-10-07T19:26:46Z

holy cow. gonna dig into that magic, thanks for sharing

do we want to do that instead? we wouldn't be able to assert at runtime (not without duplicating the query parsing in JS...), but that's nbd. just like today, things should still error if incorrectly using the wrong html element.

If we do, we gotta updates the TS. and wait for 4.1 stable. I saw a few tricky looking errors last time I checked out the latest tsc.

paulirish

a few more cases of this

reportuifeatures
L141
L323
L327

cat renderer L339 - svg if you wanna tackle it...

lighthouse-core/report/html/renderer/dom.js

paulirish · 2020-10-08T01:33:06Z

do we want to do that instead?

personally, i wouldn't do either of these.

doing it solely in TS seems cool, but i can't rationalize that time investment on a problem this size.

IMO the current casts fixed in the current PR aren't a huge deal, so removing them doesnt feel too valuable. But.. I'm not gonna block it if others think otherwise.

connorjclark · 2020-10-08T19:52:21Z

reportuifeatures
L141
L323
L327

Fixed L141, not sure what you are referring to w/ the others.

cat renderer L339 - svg if you wanna tackle it...

doesn't use dom.find so i don't wanna change anything here.

paulirish · 2020-10-09T21:32:50Z

Fixed L141, not sure what you are referring to w/ the others.

dunno about both but one of them was:

l325

      /** @type {HTMLElement|null} */
      const urlItem = rowEl.querySelector('.lh-text__url');

obv a querySelector use, not find(), but.. there are others.. no idea why they're not using find/findAll but... they have the same issue

l231

    /** @type {Array<HTMLTableElement>} */
    const tables = Array.from(this._document.querySelectorAll('.lh-table'));

dom.js L110

    const template = /** @type {?HTMLTemplateElement} */ (context.querySelector(selector));

paulirish

🤷‍♂️

connorjclark · 2020-11-21T00:21:44Z

@brendankenny I much prefer the api enabled by that magic TS you shared. When #11690 lands should we do that instead? @paulirish you mention maintainer burden but I don't think there is any with a copy/paste, and if a future TS makes it untenable then we can just do what this PR does now.

I'd prefer the solution that requires no actual API change.

brendankenny · 2020-11-21T00:33:22Z

It's under consideration for core typescript types (microsoft/TypeScript#29037) though there is skepticism (microsoft/TypeScript#41538, scroll down to "Better Types for querySelector").

There's also at least one library (https://www.npmjs.com/package/typed-query-selector) which would make it easy to include, though I haven't looked at the quality of implementation or anything :)

fregante · 2020-12-28T07:15:59Z

If it were possible, what do you think about using querySelector's own types directly? I mean in a way that when/if "Better Types for querySelector" is merged, dom.find will automatically be enhanced. Something like this:

// This is a demo only, it's not enough
interface Dom {
	find: ParentNode['QuerySelector'];
}

This would have over 9000 advantages:

no API change
no complex types in lighthouse
no external libraries required
if a future TS version implements "Better Types for querySelector", updating to it would automatically enhance lighthouse as well
in userland, you could already suggest using typed-query-selector without having to directly deal with it

connorjclark · 2021-01-20T23:22:06Z

If it were possible, what do you think about using querySelector's own types directly?

Yeah, we'll definitely do this when available.

@fregante I just tried using typed-query-selector but I get typescript errors, even for the latest 4.1.3.

Strange, I suppose you don't see these errors?

brendankenny · 2021-01-21T00:03:06Z

but I get typescript errors, even for the latest 4.1.3.

looks like those are real errors from the library, hidden when directly testing the library because of "skipLibCheck": true in the project's base tsconfig

brendankenny · 2021-01-21T00:07:21Z

(I suspect everything's ok, the bounds just might need to be relaxed to unknown in a few places and possibly add a few more conditions in order to keep the compiler happy)

brendankenny · 2021-01-21T00:28:13Z

types/externs.d.ts

+    ? HTMLElementTagNameMap[Tag]
+    : Tag extends keyof SVGElementTagNameMap
+    ? SVGElementTagNameMap[Tag]
+    : HTMLElement; // typed-query-selector use Element here, which we don't want.


I think we should maybe do the right thing and have it be Element here. Maybe we stuck to a default HTMLElement because too many things needed to be fixed simultaneously?

Playing with it for a few minutes to see how bad the transition would be and I already found a bug in our renderer :)

lighthouse/lighthouse-core/report/html/renderer/category-renderer.js

Lines 146 to 153 in b2aa480

/**

* @return {HTMLElement}

*/

_createChevron() {

const chevronTmpl = this.dom.cloneTemplate('#tmpl-lh-chevron', this.templateContext);

const chevronEl = this.dom.find('.lh-chevron', chevronTmpl);

return chevronEl;

}

is allowed to return HTMLElement because dom.find() is defaulting to returning HTMLElement, but .lh-chevron is an SVGElement:

lighthouse/lighthouse-core/report/html/templates.html

Lines 35 to 37 in b2aa480



<template id="tmpl-lh-chevron">

<svg class="lh-chevron" title="See audits" xmlns="http://www.w3.org/2000/svg" viewbox="0 0 100 100">

As a result, when we try to add a title to the chevron for a "Show audits" tooltip:

lighthouse/lighthouse-core/report/html/renderer/category-renderer.js

Lines 291 to 292 in b2aa480

const chevronEl = summaryInnerEl.appendChild(this._createChevron());

chevronEl.title = Util.i18n.strings.auditGroupExpandTooltip;

the property is set but it doesn't do anything...because SVGElement doesn't support title for tooltips (I guess you need <title> for that or just set it on a parent HTMLElement, like we do for the chevron on perf audits)

good catch, changed to use Element as fallback.

required adding a lot of div prefixes

fregante · 2021-01-21T00:31:56Z

Strange, I suppose you don't see these errors?

Where is the number coming from? The selector should be a string in every case.

brendankenny · 2021-01-21T01:32:34Z

Strange, I suppose you don't see these errors?

Where is the number coming from? The selector should be a string in every case.

I believe it's from https://github.com/g-plane/typed-query-selector/blob/2efb75b8cf6b671332bc4c9e62a101f276902814/parser.d.ts#L106-L108

fregante · 2021-01-21T01:40:44Z

cc @g-plane

g-plane · 2021-01-21T01:58:14Z

I can reproduce with setting skipLibCheck to false.

g-plane · 2021-01-21T06:42:24Z

I've released v2.4.0 which contains changes:

Fix errors with disabled skipLibCheck
Allow to specify fallback type, so you don't need to make your own TagNameToElement type any more

brendankenny

Nice! Personally I love it. Safer and really no development overhead at all (it might reduce DX overhead, really).

@paulirish, seem better to you?

@fregante and @g-plane, thanks so much for your help!

brendankenny · 2021-01-21T23:49:20Z

lighthouse-core/report/html/renderer/category-renderer.js

-    chevronEl.title = Util.i18n.strings.auditGroupExpandTooltip;
+    const summaryInnerEl = this.dom.find('div.lh-audit-group__summary', clumpElement);
+    summaryInnerEl.appendChild(this._createChevron());
+    summaryInnerEl.title = Util.i18n.strings.auditGroupExpandTooltip;


is this weird having the tooltip on the full width of the <summary> for the group? Technically clicking on any of it does expand the group.

lighthouse/lighthouse-core/report/html/templates.html

Lines 57 to 58 in 73af3c8

<summary>

<div class="lh-audit-group__summary">

The alternative would be like the perf audits we have the tooltip on the parent .lh-chevron-container that's only the size of the chevron.

cc @paulirish as the person who would probably care the most :)

I looked at https://www.w3.org/TR/wai-aria-practices-1.1/#disclosure and a few other things and i'm convinced we're better completely without this title.

none of the examples here or elsewhere have an extra tooltip written as a command, instead they all rely on descriptive text to describe whats behind the disclosure

in our impl, it still says show audits even after you expand. we probably meant to switch to hide but never cared enough.

let's delete!

also @connorjclark you can delete the title in the HTML since we now know it's invalid https://github.com/GoogleChrome/lighthouse/blob/dom-find-types/lighthouse-core/report/html/templates.html#L37

brendankenny · 2021-01-22T00:00:47Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

@@ -70,15 +70,16 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
    }

    // Overwrite the displayValue with opportunity's wastedMs
-    const displayEl = this.dom.find('.lh-audit__display-text', element);
+    const displayEl =
+      this.dom.find('span.lh-audit__display-text, div.lh-audit__display-text', element);


this one's kind of unfortunate :/

I wonder why we have <span> in one place and a <div> in the other?

this one's kind of unfortunate :/

I wonder why we have <span> in one place and a <div> in the other?

actually, they always end up display: flex from .lh-audit__header, so they could both be <span> or both be <div> and not have to have this double selector. The minor improvement is that the next person looking at this line doesn't have to wonder if it's significant that they're different types of elements, but up to you if that's worth anything :)

Why use the tag name at all?

Suggested change

this.dom.find('span.lh-audit__display-text, div.lh-audit__display-text', element);

this.dom.find('.lh-audit__display-text', element);

or

Suggested change

this.dom.find('span.lh-audit__display-text, div.lh-audit__display-text', element);

this.dom.find<HTMLElement>('.lh-audit__display-text', element);

The minor improvement is that the next person looking at this line doesn't have to wonder if it's significant that they're different types of elements, but up to you if that's worth anything :)

div changed the layout.

I guess you mistook .lh-audit__header to be the parent of this element, but it's the grandparent.

Why use the tag name at all?

Because title doesn't exist on Element.

(also, this file is JS, not TS).

they always end up display: flex from .lh-audit__header, so they could both be <span> or both be <div> and not have to have this double selector.

div changed the layout.

I guess you mistook .lh-audit__header to be the parent of this element, but it's the grandparent.

Ah, whoops, sorry. Well, the <div> can be a <span> because that's the one of those two cases I actually tested to see if it would work :)

edit: though, again, the TODO you added SGTM too :)

I wasn't able to confidently test it (quickly..) so I just tossed a TODO there. Also leaving the "remove .title" work to a different PR, so figured keeping this all to just type changes / should-be-noop runtime changes is ideal.

brendankenny · 2021-01-22T00:05:53Z

lighthouse-core/report/html/renderer/dom.js

   */
  find(query, context) {
-    /** @type {?HTMLElement} */
+    /** @type {?import('typed-query-selector/parser').ParseSelector<T>} */


what are your thoughts about doing it this way vs using the querySelector shim provided by default?

I think you're suggesting deleting this jsdoc line and relying on the types the lib injects. however, the T type doesn't transfer. quick to test: just delete the line and see how the function always returns Element then.

brendankenny · 2021-01-22T00:11:28Z

If it were possible, what do you think about using querySelector's own types directly? I mean in a way that when/if "Better Types for querySelector" is merged, dom.find will automatically be enhanced. Something like this:
...

if a future TS version implements "Better Types for querySelector", updating to it would automatically enhance lighthouse as well

in userland, you could already suggest using typed-query-selector without having to directly deal with it

@fregante this approach is a good one, but for us this will be a devDep, not exposed to end users of Lighthouse. It's making the internal generation of the report easier/(type)safer, not a report feature or anything.

misc: return specific html element type for find

cd4301d

connorjclark requested a review from a team as a code owner October 6, 2020 23:46

connorjclark requested review from paulirish and removed request for a team October 6, 2020 23:46

google-cla bot added the cla: yes label Oct 6, 2020

connorjclark changed the title ~~misc: return specific html element type for find~~ misc: return specific html element type for dom.find Oct 6, 2020

connorjclark commented Oct 6, 2020

View reviewed changes

lighthouse-core/report/html/renderer/category-renderer.js Show resolved Hide resolved

devtools-bot assigned paulirish Oct 6, 2020

devtools-bot added the waiting4reviewer label Oct 6, 2020

paulirish requested changes Oct 8, 2020

View reviewed changes

lighthouse-core/report/html/renderer/dom.js Outdated Show resolved Hide resolved

lighthouse-core/report/html/renderer/dom.js Outdated Show resolved Hide resolved

lighthouse-core/report/html/renderer/dom.js Outdated Show resolved Hide resolved

pr

8b17322

vercel bot deployed to Preview October 8, 2020 19:53 View deployment

paulirish approved these changes Oct 9, 2020

View reviewed changes

paulirish added waiting4committer and removed waiting4reviewer labels Oct 9, 2020

connorjclark added 2 commits January 20, 2021 17:01

Merge remote-tracking branch 'origin/master' into dom-find-types

1bbdb44

upddate

53a4536

vercel bot deployed to Preview January 20, 2021 23:21 View deployment

brendankenny reviewed Jan 21, 2021

View reviewed changes

connorjclark added 2 commits January 21, 2021 16:43

ok

15f9270

update

504ec41

vercel bot deployed to Preview January 21, 2021 23:16 View deployment

findAll

85f17f6

vercel bot deployed to Preview January 21, 2021 23:23 View deployment

brendankenny approved these changes Jan 22, 2021

View reviewed changes

comment

27b788f

vercel bot deployed to Preview January 22, 2021 18:55 View deployment

connorjclark merged commit 066ac9f into master Jan 22, 2021

connorjclark deleted the dom-find-types branch January 22, 2021 19:42

connorjclark mentioned this pull request Jan 22, 2021

report: remove title from audit clump expand #11989

Merged

This was referenced Jan 22, 2021

misc: use typed-query-selector for native querySelector #11990

Merged

Should the typed querySelector() return mixed HTML and SVG results? #12011

Closed

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

misc: return specific html element type for dom.find (#11526)

390b729

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: return specific html element type for dom.find #11526

misc: return specific html element type for dom.find #11526

connorjclark commented Oct 6, 2020 •

edited by brendankenny

Loading

brendankenny commented Oct 7, 2020 •

edited

Loading

connorjclark commented Oct 7, 2020

paulirish left a comment

paulirish commented Oct 8, 2020

connorjclark commented Oct 8, 2020

paulirish commented Oct 9, 2020

paulirish left a comment

connorjclark commented Nov 21, 2020

brendankenny commented Nov 21, 2020

fregante commented Dec 28, 2020 •

edited

Loading

connorjclark commented Jan 20, 2021 •

edited

Loading

brendankenny commented Jan 21, 2021

brendankenny commented Jan 21, 2021

brendankenny Jan 21, 2021

connorjclark Jan 21, 2021

fregante commented Jan 21, 2021

brendankenny commented Jan 21, 2021

fregante commented Jan 21, 2021

g-plane commented Jan 21, 2021

g-plane commented Jan 21, 2021

brendankenny left a comment

brendankenny Jan 21, 2021

paulirish Jan 22, 2021

brendankenny Jan 22, 2021

brendankenny Jan 22, 2021

fregante Jan 22, 2021 •

edited

Loading

connorjclark Jan 22, 2021

brendankenny Jan 22, 2021 •

edited

Loading

connorjclark Jan 22, 2021

brendankenny Jan 22, 2021

connorjclark Jan 22, 2021

brendankenny commented Jan 22, 2021

	/**
	* @return {HTMLElement}
	*/
	_createChevron() {
	const chevronTmpl = this.dom.cloneTemplate('#tmpl-lh-chevron', this.templateContext);
	const chevronEl = this.dom.find('.lh-chevron', chevronTmpl);
	return chevronEl;
	}

	<!-- Toggle arrow chevron -->
	<template id="tmpl-lh-chevron">
	<svg class="lh-chevron" title="See audits" xmlns="http://www.w3.org/2000/svg" viewbox="0 0 100 100">

	const chevronEl = summaryInnerEl.appendChild(this._createChevron());
	chevronEl.title = Util.i18n.strings.auditGroupExpandTooltip;

	this.dom.find('span.lh-audit__display-text, div.lh-audit__display-text', element);
	this.dom.find('.lh-audit__display-text', element);

	this.dom.find('span.lh-audit__display-text, div.lh-audit__display-text', element);
	this.dom.find<HTMLElement>('.lh-audit__display-text', element);

misc: return specific html element type for dom.find #11526

misc: return specific html element type for dom.find #11526

Conversation

connorjclark commented Oct 6, 2020 • edited by brendankenny Loading

brendankenny commented Oct 7, 2020 • edited Loading

connorjclark commented Oct 7, 2020

paulirish left a comment

Choose a reason for hiding this comment

paulirish commented Oct 8, 2020

connorjclark commented Oct 8, 2020

paulirish commented Oct 9, 2020

paulirish left a comment

Choose a reason for hiding this comment

connorjclark commented Nov 21, 2020

brendankenny commented Nov 21, 2020

fregante commented Dec 28, 2020 • edited Loading

connorjclark commented Jan 20, 2021 • edited Loading

brendankenny commented Jan 21, 2021

brendankenny commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante commented Jan 21, 2021

brendankenny commented Jan 21, 2021

fregante commented Jan 21, 2021

g-plane commented Jan 21, 2021

g-plane commented Jan 21, 2021

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jan 22, 2021

connorjclark commented Oct 6, 2020 •

edited by brendankenny

Loading

brendankenny commented Oct 7, 2020 •

edited

Loading

fregante commented Dec 28, 2020 •

edited

Loading

connorjclark commented Jan 20, 2021 •

edited

Loading

fregante Jan 22, 2021 •

edited

Loading

brendankenny Jan 22, 2021 •

edited

Loading