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.

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

misc: include SVG elements by default in typed querySelector #12307

Merged
merged 2 commits into from
Mar 30, 2021
Merged

Conversation

brendankenny
Copy link
Member

fixes #12011

To recap: we need gatherers to deal with svg elements where they overlap in tag names with HTML elements (at least <a>, <script>, <style>, and <title>) since real pages contain those elements and those svg elements have different attributes and behavior than their HTML counterparts (causing bugs like #6481). This PR makes querySelector include those svg elements by default instead of only including them if no matching HTML element was found.

So now document.querySelector('a') returns a value of type HTMLAnchorElement | SVGAElement instead of just HTMLAnchorElement, and the subsequent code has to identify which of the two it's dealing with before proceeding.

However, we also use querySelector a lot in the report (usually via dom.find()), and it's annoying to have to do that extra work when we were also the ones that just wrote the template being queried and it only includes an HTMLAnchorElement. This PR maintains the current behavior of dom.find() to make report authoring easier.

This wasn't quite the idea laid out in #12011 (comment) ("make dom.find() only return HTMLElements (the same way it already filters out null)") because it turns out we already query into svg at least twice in the report (for chevrons and clipping the full-page screenshot), there may be more (e.g. queries without a tag name in them), and there's no strong reason for us to prevent doing any svg querying in the future.

As a result, for dom.find() this PR maintains the old behavior of pretending there's no such thing as an SVGStyleElement (only in the world of the report), assuming that there's no reason we'd ever want an SVGStyleElement in there (you can just style svg from html-level css). If that ever changes, we can always make dom.find() stricter.

@brendankenny brendankenny requested a review from a team as a code owner March 29, 2021 18:32
@brendankenny brendankenny requested review from paulirish and removed request for a team March 29, 2021 18:32
@google-cla google-cla bot added the cla: yes label Mar 29, 2021
@brendankenny brendankenny requested review from connorjclark and removed request for paulirish March 29, 2021 18:32
Copy link
Member Author

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

R: @connorjclark because I'm pretty sure he's the only other person who cares about this :)

// Because we control the report layout and templates, use the simpler
// `typed-query-selector` types that don't require differentiating between
// e.g. HTMLAnchorElement and SVGAElement. See https://github.com/GoogleChrome/lighthouse/issues/12011.
return /** @type {ParseSelector<T>} */ (result);
Copy link
Member Author

@brendankenny brendankenny Mar 29, 2021

Choose a reason for hiding this comment

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

I'd love to not cast here, but the types aren't quite compatible, testing is totally broken if you try to filter out SVG elements at runtime (jsdom/jsdom#2734), and if you try to do naive filtering of the type, tsc will give you things like HTMLElementTagNameMap[keyof HTMLElementTagNameMap] & HTMLAnchorElement, which, when parameterized, gives you leftovers like SVGAElement & HTMLAnchorElement, which is not the empty set of properties so isn't eliminating anything from the union of types :)

type QuerySelectorParse<I extends string> = ParseSelectorToTagNames<I> extends infer TagNames ?
TagNames extends Array<string> ?
HtmlAndSvgElementTagNameMap[TagNames[number]] :
Element: // Fall back for queries typed-query-selector fails to parse, e.g. `'[alt], [aria-label]'`.
Copy link
Member Author

Choose a reason for hiding this comment

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

typed-query-selector returns unknown from ParseSelectorToTagNames for both situations it can't handle and known error states (like document.querySelector('')). As a result, with Element here it'll still give that type for those bad queries instead of never, but it has to be included for queries like '[alt], [aria-label]' (from page-functions) which need the Element fall back.

Not much to do to get around this except write our own version, but no worse than the current state of things or the default typescript querySelector types :)

lighthouse-treemap/app/src/util.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/dom.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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