core(artifacts): unify AnchorElements into single gatherer#7101
Conversation
| @@ -33,7 +33,9 @@ class ExternalAnchorsUseRelNoopenerAudit extends Audit { | |||
| const warnings = []; | |||
| const pageHost = new URL(artifacts.URL.finalUrl).host; | |||
| // Filter usages to exclude anchors that are same origin | |||
There was a problem hiding this comment.
move the comment down to the second filter?
| @@ -0,0 +1,77 @@ | |||
| /** | |||
There was a problem hiding this comment.
lot of lint stuff in here :)
| const auditResult = ExternalAnchorsAudit.audit({ | ||
| AnchorElements: [ | ||
| {href: 'https://other.com/test', target: '_blank', rel: 'nofollow noopener'}, | ||
| {href: 'https://other.com/test1', target: '_blank', rel: 'noreferrer'}, |
There was a problem hiding this comment.
add one for just noopener as well?
| {href: 'https://example.com/test1'}, | ||
| AnchorElements: [ | ||
| {href: 'https://example.com/test', target: '_blank', rel: ''}, | ||
| {href: 'https://example.com/test1', target: '_blank', rel: ''}, |
There was a problem hiding this comment.
maybe vary the rel values to give that some exercise?
| })()` | ||
|
|
||
| // We'll use evaluateAsync because the `node.getAttribute` method doesn't actually normalize | ||
| // the values like access from JavaScript does. |
There was a problem hiding this comment.
nit: this comment seems like it makes more sense before the definition of expression
|
|
||
| return anchorElements.map(node => { | ||
| /** @type {HTMLAnchorElement} */ | ||
| const anchorNode = (node); |
There was a problem hiding this comment.
the () thing is a bug in tsc's js support, I think. It's hard to notice the different behavior than without the parens (non-coercive assertion). Can we do inline instead (const anchorNode = /** @type {HTMLAnchorElement} */ (node);) if going this route?
What about an instanceof HTMLAnchorElement check instead, though? No need to cast for type checking in that case, and can do an early return.
There was a problem hiding this comment.
wdyt about something like
return anchorElements.map(node => {
// @ts-ignore - put into scope via stringification
const outerHTML = getOuterHTMLSnippet(node); // eslint-disable-line no-undef
if (node instanceof HTMLAnchorElement) {
return {
href: node.href,
text: node.innerText,
rel: node.rel,
target: node.target,
outerHTML,
};
}
return {
href: resolveURLOrEmpty(node.href.baseVal),
text: node.textContent || '',
rel: '',
target: node.target.baseVal || '',
outerHTML,
};
});(as far as I can tell, SVGAElement.href is always a SVGAnimatedString, see SVGAElement implementing SVGURIReference)
There was a problem hiding this comment.
yeah I like it!
| const href = (anchorNode.href); | ||
| /** @type {SVGAElement} */ | ||
| const svgNode = (node); | ||
| if (href instanceof SVGAnimatedString) { |
There was a problem hiding this comment.
although considering this is testing the href, maybe there's a reason this isn't doing instanceof against the whole node?
There was a problem hiding this comment.
correct, it can be either or
| node.href, | ||
| text: node.href instanceof SVGAnimatedString ? | ||
| node.textContent : | ||
| node.innerText, |
There was a problem hiding this comment.
also, do you know why the original code is innerText for HTMLAnchorElement vs textContent for SVGAElement? We should keep it, just seems weird :)
There was a problem hiding this comment.
I think we didn't want to return invisible text, IIRC there were some situations where we started sticking large amounts of nonsense text in the table
brendankenny
left a comment
There was a problem hiding this comment.
LGTM, take or leave the suggestion :)
Summary
Unifies all our usage of
<a>elements under a singleAnchorElementsgatherer/artifact. Replaces the two other artifacts (AnchorsWithNoRelOpener and CrawableLinks)Related Issues/PRs
#6747