core(seo): properly handle anchors in SVG #6483
Conversation
| const expression = `(function() { | ||
| ${pageFunctions.getElementsInDocumentString}; // define function on page | ||
| const resolveURLOrNull = url => { | ||
| try { return new URL(url, window.location.href).href; } |
There was a problem hiding this comment.
I'll do ya one better, I'll enable useIsolation :)
| try { return new URL(url, window.location.href).href; } | ||
| catch (_) { return null; } | ||
| }; | ||
|
|
||
| const selector = 'a[href]:not([rel~="nofollow"])'; | ||
| const elements = getElementsInDocument(selector); |
There was a problem hiding this comment.
how about
getElementsInDocument(selector).filter(e => __ElementMatches.call(e, 'svg a'))There was a problem hiding this comment.
not 100% sure what you're requesting, like we separate them in two different arrays? or just that we use .matches to replace the instanceof check?
There was a problem hiding this comment.
hmmm that was the idea but the fact that resolving these baseVal is so annoying makes it kinda not worth it.
actually how about
const elements = getElementsInDocument(selector).filter(e => __ElementMatches.call(e, 'svg a'))
return elements.map(node => ({
href: new __URL(node.getAttribute('href'), window.location.href).toString(), // but with try/catch
text: // ...
// oh but then links in SVG don't have innerText
// node.innerText || node.textContent ??okay yeah this is all terrible.
There was a problem hiding this comment.
yeah I agree it's all terrible, I started with node.innerText || node.textContent but then got worried about potential accidental fallbacks with script/style tag contents when it's empty and whatnot, wanted to keep it as narrow as possible.
| try { return new URL(url, window.location.href).href; } | ||
| catch (_) { return null; } | ||
| }; | ||
|
|
||
| const selector = 'a[href]:not([rel~="nofollow"])'; | ||
| const elements = getElementsInDocument(selector); |
There was a problem hiding this comment.
hmmm that was the idea but the fact that resolving these baseVal is so annoying makes it kinda not worth it.
actually how about
const elements = getElementsInDocument(selector).filter(e => __ElementMatches.call(e, 'svg a'))
return elements.map(node => ({
href: new __URL(node.getAttribute('href'), window.location.href).toString(), // but with try/catch
text: // ...
// oh but then links in SVG don't have innerText
// node.innerText || node.textContent ??okay yeah this is all terrible.
| @@ -8,6 +8,7 @@ | |||
| const Audit = require('../audit'); | |||
| const URL = require('../../lib/url-shim'); | |||
| const BLOCKLIST = new Set([ | |||
| '', // no description is also bad | |||
There was a problem hiding this comment.
I think this will have a big effect on a lot of anchors. Not sure if we want to pull this trigger without doing more research first. For now can we fix the svg > a failure without changing the audit scoring? We can open a separate FR to weigh in on auditing for empty anchor text.
There was a problem hiding this comment.
Alright, I'll revert 👍
patrickhulce
changed the title
|
I see that some changes have been merged here but the issue I brought up in my original ticket (#6481) is still occurring. I'm wondering how long it will be before this fix is available for the lighthouse software that ships with Chrome. Do I need to wait until the next release? If so, can I just download the master branch and use it as-is with NPM? |
|
You'll have to wait for the next release with this commit merged, it will likely hit Chrome Canary in a few weeks and Chrome stable in a few months. If you're feeling especially brave you could try installing LH HEAD directly from GitHub, but proceed at your own risk :) |
Summary
Links inside SVG here throwing the error here and links without any text content were passing the audit too, which seemed wrong, @rviscomi does that check out to you too? I couldn't find any discussion in the original PR (#3490) about it.
Related Issues/PRs
fixes #6481