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

core(link-elements): ignore non-HTMLLinkElements #9765

Merged
merged 3 commits into from
Oct 3, 2019

Conversation

patrickhulce
Copy link
Collaborator

Summary
updates link elements gatherer to only consider real link elements and adds a smoketest for it

Related Issues/PRs
fixes #9764

@brendankenny
Copy link
Member

I was only able to test it briefly, so I could have this wrong, but from the test site https://sundaysuppermovement.com/ in #9764, it appears that the only link rel="preload" elements are SVG elements, but they are triggering preloads, so dropping these might lead to unexpected results in things like uses-rel-preload and etc

@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Oct 1, 2019

it appears that the only link rel="preload" elements are SVG elements, but they are triggering preloads, so dropping these might lead to unexpected results in things like uses-rel-preload and etc

No they shouldn't be triggering anything, they're just invalid elements. AFAICT, the preload requests come from the rel="preload" onload="this.rel = 'stylesheet'" pattern the page uses.

@brendankenny
Copy link
Member

AFAICT, the preload requests come from the rel="preload" onload="this.rel = 'stylesheet'" pattern the page uses.

Nice catch!

Ah, sorry, I'm finally getting what you're saying. These end up as SVGUnknownElement (though that's not a public interface) since they're from a foreign namespace, so while they get parsed (and are apparently containers in svg, interesting) and have a link tagName, they are otherwise inert.

In general, the SVG user agent must include the unknown foreign-namespaced elements in the DOM but will ignore and exclude them for rendering purposes.

This sounds good then.

@@ -19,6 +19,7 @@ module.exports = {
'render-blocking-resources',
'errors-in-console',
'efficient-animated-content',
'apple-touch-icon', // pull in apple touch icon to test `LinkElements`
Copy link
Member

Choose a reason for hiding this comment

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

any reason to do this in dbw and not in perf or seo where LinkElements are already being used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just that dbw felt like the dumping ground of error catching appropriate for this sort of thing 😆

the others are so focused it felt mean to pollute them with SVG nonsense

Copy link
Member

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

LGTM!

@@ -217,7 +217,7 @@ declare global {
/** The `as` attribute of the link */
as: string
/** The `crossOrigin` attribute of the link */
crossOrigin: 'anonymous'|'use-credentials'|null
crossOrigin: string | null
Copy link
Member

Choose a reason for hiding this comment

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

this is a loss for documentation but I suppose there's always mdn :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Required LinkElements "Cannot read property 'toLowerCase' of undefined"
3 participants