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: allow placeholder anchor elements #16292

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions core/audits/seo/crawlable-anchors.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@ const UIStrings = {
columnFailingLink: 'Uncrawlable Link',
};

const hrefAssociatedAttributes = [
'target',
'download',
'ping',
'rel',
'hreflang',
'type',
'referrerpolicy',
];

const str_ = i18n.createIcuMessageFn(import.meta.url, UIStrings);

class CrawlableAnchors extends Audit {
Expand Down Expand Up @@ -45,10 +55,14 @@ class CrawlableAnchors extends Audit {
role = '',
id,
href,
attributeNames = [],
listeners = [],
ancestorListeners = [],
}) => {
rawHref = rawHref.replace( /\s/g, '');
name = name.trim();
role = role.trim();
const hasListener = Boolean(listeners.length || ancestorListeners.length);

if (role.length > 0) return;
// Ignore mailto links even if they use one of the failing patterns. See https://github.com/GoogleChrome/lighthouse/issues/11443#issuecomment-694898412
Expand All @@ -62,6 +76,16 @@ class CrawlableAnchors extends Audit {
if (rawHref.startsWith('file:')) return true;
if (name.length > 0) return;

// https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element
// If the a element has no href attribute, then the element represents a placeholder for where a link might otherwise have been placed, if it had been relevant, consisting of just the element's contents.
// The target, download, ping, rel, hreflang, type, and referrerpolicy attributes must be omitted if the href attribute is not present.
if (
!attributeNames.includes('href') &&
!hrefAssociatedAttributes.some(attribute => attributeNames.includes(attribute))
) {
return hasListener;
}

if (href === '') return true;
if (javaScriptVoidRegExp.test(rawHref)) return true;

Expand Down
20 changes: 20 additions & 0 deletions core/gather/gatherers/anchor-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ function collectAnchorElements() {
rel: node.rel,
target: node.target,
id: node.getAttribute('id') || '',
attributeNames: node.getAttributeNames(),
// @ts-expect-error - getNodeDetails put into scope via stringification
node: getNodeDetails(node),
};
Expand All @@ -68,6 +69,7 @@ function collectAnchorElements() {
rel: '',
target: node.target.baseVal || '',
id: node.getAttribute('id') || '',
attributeNames: node.getAttributeNames(),
// @ts-expect-error - getNodeDetails put into scope via stringification
node: getNodeDetails(node),
};
Expand Down Expand Up @@ -119,9 +121,27 @@ class AnchorElements extends BaseGatherer {
const anchorsWithEventListeners = anchors.map(async anchor => {
const listeners = await getEventListeners(session, anchor.node.devtoolsNodePath);

/** @type {Set<{type: string}>} */
const ancestorListeners = new Set();
const splitPath = anchor.node.devtoolsNodePath.split(',');
const ancestorListenerPromises = [];
while (splitPath.length >= 2) {
splitPath.length -= 2;
const path = splitPath.join(',');
const promise = getEventListeners(session, path).then(listeners => {
for (const listener of listeners) {
ancestorListeners.add(listener);
}
}).catch(() => {});
ancestorListenerPromises.push(promise);
}

await Promise.all(ancestorListenerPromises);

return {
...anchor,
listeners,
ancestorListeners: Array.from(ancestorListeners),
};
});

Expand Down
39 changes: 35 additions & 4 deletions core/test/audits/seo/crawlable-anchors-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ function runAudit({
onclick = '',
name = '',
id = '',
attributeNames = [
(rawHref || href) ? 'href' : null, role ? 'role' : null, name ? 'name' : null,
].filter(Boolean),
listeners = onclick.trim().length ? [{type: 'click'}] : [],
ancestorListeners = [],
node = {
snippet: '',
devtoolsNodePath: '',
Expand All @@ -30,10 +34,12 @@ function runAudit({
href,
name,
listeners,
ancestorListeners,
onclick,
role,
node,
id,
attributeNames,
}],
URL: {
finalDisplayedUrl: 'http://example.com',
Expand All @@ -56,12 +62,13 @@ describe('SEO: Crawlable anchors audit', () => {
assert.equal(runAudit({rawHref: '//example.com'}), 1, 'protocol relative link');
assert.equal(runAudit({rawHref: 'tel:5555555'}), 1, 'email link with a tel URI');
assert.equal(runAudit({rawHref: '#'}), 1, 'link with only a hash symbol');
assert.equal(runAudit({}), 1, 'placeholder anchor element');
assert.equal(runAudit({
rawHref: '?query=string',
}), 1, 'relative link which specifies a query string');

assert.equal(runAudit({rawHref: 'ftp://'}), 0, 'invalid FTP links fails');
assert.equal(runAudit({rawHref: '', href: 'https://example.com'}), 1, 'empty attribute that links to current page');
assert.equal(runAudit({rawHref: '', href: 'https://example.com', attributeNames: ['href']}), 1, 'empty attribute that links to current page');
});

it('allows anchors acting as an ID anchor', () => {
Expand All @@ -79,7 +86,11 @@ describe('SEO: Crawlable anchors audit', () => {
});
assert.equal(auditResult, 1, 'Href value has no effect when a role is present');
assert.equal(runAudit({role: 'a'}), 1, 'Using a role attribute value is an immediate pass');
assert.equal(runAudit({role: ' '}), 0, 'A role value of a space character fails the audit');
assert.equal(
runAudit({role: ' ', attributeNames: ['rel']}),
0,
'A role value of a space character fails the audit'
);
});

it('handles anchor elements which use event listeners', () => {
Expand All @@ -96,16 +107,36 @@ describe('SEO: Crawlable anchors audit', () => {
});
assert.equal(auditResultWithInvalidHref, 0, 'invalid href with any event listener is a faile');

const auditResultWithNoHref = runAudit({
listeners: [{type: 'nope'}, {type: 'another'}, {type: 'click'}],
});
assert.equal(auditResultWithNoHref, 0, 'no href with any event listener is a fail');

const auditResultWithParentListenerNoHref = runAudit({
ancestorListeners: [{type: 'nope'}, {type: 'another'}, {type: 'click'}],
});
assert.equal(auditResultWithParentListenerNoHref, 0,
'no href with any event listener on a parent is a fail');

const auditResultNoListener = runAudit({
rawHref: '/validPath',
});
assert.equal(auditResultNoListener, 1, 'valid href with no event listener is a pass');
});

it('disallows uncrawlable anchors', () => {
assert.equal(runAudit({}), 0, 'link with no meaningful attributes and no event handlers');
assert.equal(
runAudit({attributeNames: ['href']}),
0,
'link with an empty href and no other meaningful attributes and no event handlers'
);
assert.equal(runAudit({attributeNames: ['target']}), 0, 'link with only a target attribute');
assert.equal(runAudit({rawHref: 'file:///image.png'}), 0, 'hyperlink with a `file:` URI');
assert.equal(runAudit({name: ' '}), 0, 'name attribute with only space characters');
assert.equal(
runAudit({name: ' ', attributeNames: ['rel']}),
0,
'name attribute with only space characters'
);
assert.equal(runAudit({rawHref: ' '}), 1, 'href attribute with only space characters');
const assertionMessage = 'onclick attribute with only space characters';
assert.equal(runAudit({rawHref: ' ', onclick: ' '}), 1, assertionMessage);
Expand Down
4 changes: 4 additions & 0 deletions types/artifacts.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -365,9 +365,13 @@ declare module Artifacts {
node: NodeDetails
onclick: string
id: string
attributeNames: Array<string>
listeners?: Array<{
type: Crdp.DOMDebugger.EventListener['type']
}>
ancestorListeners?: Array<{
type: Crdp.DOMDebugger.EventListener['type']
}>
}

type BFCacheReasonMap = {
Expand Down
Loading