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

[brendankenny]

Since #11526 and #11990, if you do document.querySelector('a') you get back a value with the type HTMLAnchorElement | null, which is usually what you'd expect and want. However we've gotten burned (at least) once before on this where we're also selecting SVG elements in a page but not handling them in the code that follows.

In a page with anchors in SVG, document.querySelector('a') should be returning a value of type HTMLAnchorElement | SVGAElement | null, and the difference in property/attribute behavior in HTML vs SVG can cause bugs. The actual issue we ran into was #6481, fixed in #6483, and then eventually became the explicit handling of HTMLAnchorElement vs SVGAElement in anchor-elements.

The reason for the current types is typed-query-selector looks first for a valid HTMLElement matching the found element identifier, if nothing is found it falls back to see if there are any matching SVGElements, and only then falls back to the generic Element (one of the shared base classes of both HTMLElement and SVGElement):

export type TagNameToElement<Tag extends string, Fallback extends Element = Element> =
  Tag extends keyof HTMLElementTagNameMap ? HTMLElementTagNameMap[Tag] :
  Tag extends keyof SVGElementTagNameMap ? SVGElementTagNameMap[Tag] :
  Fallback

https://github.com/g-plane/typed-query-selector/blob/823e4b850280dab7192cd0468b8e97c196e9e8ec/parser.d.ts#L121-L125

The tags shared between HTMLElements and SVGElements are <a>, <script>, <style>, and <title>, and it does seem like they could run into similar issues in various gatherers that we aren't accounting for and it would be nice if the type system forced us to account for them. So we could change the type (and/or get the upstream changed) so it returns the union of found HTMLElements and SVGElements. Something like:

type TypeOrFallback<T, Fallback> = [T] extends [never] ? Fallback : T;

type TagNameToElement<Tag extends string> = TypeOrFallback<
  (Tag extends keyof HTMLElementTagNameMap ? HTMLElementTagNameMap[Tag] : never) |
  (Tag extends keyof SVGElementTagNameMap ? SVGElementTagNameMap[Tag] : never),
  Element
>;

(seems like there has to be a better pattern for that fallback, but that's the best I can come up with right now :)

Buuuut, the downside is having to deal with that in situations where we know there aren't e.g. SVG anchors, like in a template we wrote in the report. For example:

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

Lines 328 to 329 in 52b12cf

	const wrapper = this.dom.find('a.lh-gauge__wrapper', tmpl);
	wrapper.href = `#${category.id}`;

this would error, because href is a readonly property on SVGAElement (you have to set the attribute). There aren't that many errors, but it does seem like a somewhat annoying situation to put on ourselves.

Current 9 errors when trying this on master:

$ tsc -p . && tsc -p lighthouse-viewer/ && tsc -p lighthouse-treemap/
lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js:91:36 - error TS2339: Property 'src' does not exist on type 'HTMLOrSVGScriptElement'.
  Property 'src' does not exist on type 'SVGScriptElement'.

91           !/^data:/.test(scriptTag.src) &&
                                      ~~~

lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js:92:36 - error TS2339: Property 'src' does not exist on type 'HTMLOrSVGScriptElement'.
  Property 'src' does not exist on type 'SVGScriptElement'.

92           !/^blob:/.test(scriptTag.src) &&
                                      ~~~

lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js:99:20 - error TS2339: Property 'src' does not exist on type 'HTMLOrSVGScriptElement'.
  Property 'src' does not exist on type 'SVGScriptElement'.

99           url: tag.src,
                      ~~~

lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js:100:20 - error TS2339: Property 'src' does not exist on type 'HTMLOrSVGScriptElement'.
  Property 'src' does not exist on type 'SVGScriptElement'.

100           src: tag.src,
                       ~~~

lighthouse-core/report/html/renderer/category-renderer.js:329:13 - error TS2540: Cannot assign to 'href' because it is a read-only property.

329     wrapper.href = `#${category.id}`;
                ~~~~

lighthouse-core/report/html/renderer/pwa-category-renderer.js:62:13 - error TS2540: Cannot assign to 'href' because it is a read-only property.

62     wrapper.href = `#${category.id}`;
               ~~~~

lighthouse-core/report/html/renderer/pwa-category-renderer.js:81:13 - error TS2339: Property 'title' does not exist on type 'HTMLAnchorElement | SVGAElement'.
  Property 'title' does not exist on type 'SVGAElement'.

81     wrapper.title = this._getGaugeTooltip(category.auditRefs, groupDefinitions);
               ~~~~~

lighthouse-core/report/html/renderer/report-renderer.js:74:17 - error TS2540: Cannot assign to 'href' because it is a read-only property.

74     metadataUrl.href = metadataUrl.textContent = report.finalUrl;
                   ~~~~

lighthouse-core/report/html/renderer/report-renderer.js:75:17 - error TS2339: Property 'title' does not exist on type 'HTMLAnchorElement | SVGAElement'.
  Property 'title' does not exist on type 'SVGAElement'.

75     metadataUrl.title = report.finalUrl;
                   ~~~~~


Found 9 errors.

[brendankenny]

@connorjclark mentioned that one solution would be to fix the querySelector return type but make dom.find() be stricter and only return HTMLElements (the same way it already filters out null) since we have complete control over the templates and generated markup that we're using dom.find() to search through. That would eliminate most of those renderer errors while still doing the right thing, so SGTM if/when we get around to being stricter on this.

Those tags-blocking-first-paint.js errors (asking to account for sites using <script> inside of SVG) also shouldn't be an issue, because we're selecting for head script[src] and a) I don't believe you can have SVG inside <head> and b) we're only looking for scripts with a defined src attribute, which SVGScriptElement doesn't use (they use href)...

[patrickhulce]

one solution would be to fix the querySelector return type but make dom.find() be stricter and only return HTMLElements (the same way it already filters out null) since we have complete control over the templates and generated markup that we're using dom.find() to search through. That would eliminate most of those renderer errors while still doing the right thing, so SGTM if/when we get around to being stricter on this.

I like this 👍 Doing the if (element instanceof SVGScriptElement) return doesn't seem like a big deal if it means future audits get reminded of the SVGAnchor problem