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

new_audit(anchor-text-audit): descriptive anchor text audit #3490

Merged
merged 7 commits into from
Oct 19, 2017
Merged
Show file tree
Hide file tree
Changes from 6 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
7 changes: 6 additions & 1 deletion lighthouse-cli/test/fixtures/seo/seo-failure-cases.html
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@
<!-- no <meta name="description" content=""> -->
</head>
<body>
bad crawlz
<h1>SEO</h1>

<h2>Anchor text</h2>
<a href='https://example.com'>click this</a>
<a href='/test.html'> click this </a>
<a href='/test.html'>CLICK THIS</a>
</body>
</html>
8 changes: 7 additions & 1 deletion lighthouse-cli/test/fixtures/seo/seo-tester.html
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@
<meta name="description" content="The premiere destination for testing your SEO audit gathering">
</head>
<body>
crawlz me
<h1>SEO</h1>

<h2>Anchor text</h2>
<a href='https://example.com'>descriptive link</a>
<a href='#non-descriptive-local'>click this</a>
<a href='https://example.com/non-descriptive-no-follow.html' rel="nofollow">click this</a>
<a href='javascript:void(0);'>click this</a>
</body>
</html>
12 changes: 12 additions & 0 deletions lighthouse-cli/test/smokehouse/seo/expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ module.exports = [
'http-status-code': {
score: true,
},
'anchor-text': {
score: true,
},
},
},
{
Expand All @@ -49,6 +52,15 @@ module.exports = [
score: false,
displayValue: '403',
},
'anchor-text': {
score: false,
displayValue: '3 anchors found',
Copy link
Member

Choose a reason for hiding this comment

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

maybe details: { items: {length: 3}} as well to make sure they're in there? (you could also assert the actual items contents if you thought that was worthwhile...the only cost is more verbose/harder to scan test expectations)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checking length SGTM, but checking all contents gets very verbose (showing just one of three items):

        details: {
          items: {
            length: 3,
            0: {
              0: {
                text: 'https://example.com/',
              },
              1: {
                text: 'click this',
              }
            },
          },
        },

I'd leave that to unit tests (in fact, since we are getting rid of extendedInfo, I'll have to use details in unit tests).

Copy link
Member

Choose a reason for hiding this comment

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

I'd leave that to unit tests

SGTM

details: {
items: {
length: 3,
},
},
},
},
},
];
76 changes: 76 additions & 0 deletions lighthouse-core/audits/seo/anchor-text.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/**
* @license Copyright 2017 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const Audit = require('../audit');
const URL = require('../../lib/url-shim');
const BLOCKLIST = new Set([
'click here',
'click this',
'go',
'here',
'this',
'start',
'right here',
'more',
'learn more',
Copy link
Collaborator

Choose a reason for hiding this comment

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

😱 our report would fail like crazy!

Copy link
Member

@rviscomi rviscomi Oct 8, 2017

Choose a reason for hiding this comment

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

The help text for this audit has a "learn more" link 😆

(as bad of an example as it sets, in our defense this tool will not be crawled by search bots)

]);

class AnchorText extends Audit {
/**
* @return {!AuditMeta}
*/
static get meta() {
return {
category: 'Content Best Practices',
name: 'anchor-text',
description: 'Anchors have descriptive text.',
Copy link
Member

Choose a reason for hiding this comment

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

Links have descriptive text

cc @rviscomi you okay with calling these "links" instead of 'anchors' ? IMO its more natural and i dont think the technical correctness is worth it.

Copy link
Member

@rviscomi rviscomi Oct 17, 2017

Choose a reason for hiding this comment

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

"links" or "<a> elements" SGTM

@kdzwinel please also update the name and failureDescription fields for consistency

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changing name kinda forced me to get rid of 'anchor' completely: b710137 . @paulirish PTAL

failureDescription: 'Anchors do not have descriptive text',
Copy link
Member

Choose a reason for hiding this comment

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

Anchors => Links

helpText: 'Descriptive anchor text helps search engines understand your content. ' +
'[Learn more](https://webmasters.googleblog.com/2008/10/importance-of-link-architecture.html)',
requiredArtifacts: ['URL', 'CrawlableAnchors'],
};
}

/**
* @param {!Artifacts} artifacts
* @return {!AuditResult}
*/
static audit(artifacts) {
const failingAnchors = artifacts.CrawlableAnchors
.filter(anchor => {
if (
anchor.href.toLowerCase().startsWith('javascript:') ||
URL.equalWithExcludedFragments(anchor.href, artifacts.URL.finalUrl)
Copy link
Member

Choose a reason for hiding this comment

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

why are links within the page not counted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I understand, these links have no SEO value and often are handled by JS anyway (e.g. <a href='#'>show more</a> to expand some text inline).

) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we decided to skip all links that do not link to another page (e.g. '#')

return false;
}

return BLOCKLIST.has(anchor.text.trim().toLowerCase());
});

const headings = [
{key: 'href', itemType: 'url', text: 'Link destination'},
{key: 'text', itemType: 'text', text: 'Link Text'},
];

const details = Audit.makeTableDetails(headings, failingAnchors);
let displayValue;

if (failingAnchors.length) {
displayValue = failingAnchors.length > 1 ?
`${failingAnchors.length} anchors found` : '1 anchor found';
}

return {
rawValue: failingAnchors.length === 0,
details,
displayValue,
};
}
}

module.exports = AnchorText;
3 changes: 3 additions & 0 deletions lighthouse-core/config/seo.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ module.exports = {
passName: 'defaultPass',
gatherers: [
'seo/meta-description',
'seo/crawlable-anchors',
],
}],
audits: [
'seo/meta-description',
'seo/http-status-code',
'seo/anchor-text',
],
groups: {
'seo-mobile': {
Expand All @@ -41,6 +43,7 @@ module.exports = {
{id: 'document-title', weight: 1, group: 'seo-content'},
{id: 'meta-description', weight: 1, group: 'seo-content'},
{id: 'http-status-code', weight: 1, group: 'seo-crawl'},
{id: 'anchor-text', weight: 1, group: 'seo-content'},
],
},
},
Expand Down
33 changes: 33 additions & 0 deletions lighthouse-core/gather/gatherers/seo/crawlable-anchors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* @license Copyright 2017 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const Gatherer = require('../gatherer');
const DOMHelpers = require('../../../lib/dom-helpers.js');

class CrawlableAnchors extends Gatherer {
/**
* @param {{driver: !Object}} options Run options
* @return {!Promise<!Array<{href: string, text: string}>>}
*/
afterPass(options) {
const expression = `(function() {
${DOMHelpers.getElementsInDocumentFnString}; // define function on page
const selector = 'a[href]:not([rel~="nofollow"])';
Copy link
Member

Choose a reason for hiding this comment

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

would it be better to combine this with anchors-with-no-rel-noopener and sort client-side for nofollow vs noopener into a single anchor gatherer? Would need to collect rel so that audits can filter on it

Copy link
Member

Choose a reason for hiding this comment

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

could be all-anchors or something (we could do it in a followup PR if you want to keep this one focused)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all-anchors SGTM, but since this will require changing anchors-with-no-rel-noopener I think we should do that in a followup PR as you suggested

Copy link
Member

Choose a reason for hiding this comment

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

sg. filed as #3581

const elements = getElementsInDocument(selector);
return elements
.map(node => ({
href: node.href,
text: node.innerText
Copy link
Member

Choose a reason for hiding this comment

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

s/innerText/textContent

Copy link
Member

Choose a reason for hiding this comment

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

disagree. :)

Copy link
Member

Choose a reason for hiding this comment

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

jk I'm wrong :)

}));
})()`;

return options.driver.evaluateAsync(expression);
}
}

module.exports = CrawlableAnchors;

82 changes: 82 additions & 0 deletions lighthouse-core/test/audits/seo/anchor-text-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/**
* @license Copyright 2017 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const AnchorTextAudit = require('../../../audits/seo/anchor-text.js');
const assert = require('assert');

/* eslint-env mocha */

describe('SEO: anchor text audit', () => {
it('fails when anchor with non descriptive text is found', () => {
const invalidAnchor = {href: 'https://example.com/otherpage.html', text: 'click here'};
const artifacts = {
URL: {
finalUrl: 'https://example.com/page.html',
},
CrawlableAnchors: [
{href: 'https://example.com/otherpage.html', text: 'legit anchor text'},
invalidAnchor,
{href: 'https://example.com/otherpage.html', text: 'legit anchor text'},
],
};

const auditResult = AnchorTextAudit.audit(artifacts);
assert.equal(auditResult.rawValue, false);
assert.equal(auditResult.details.items.length, 1);
assert.equal(auditResult.details.items[0][0].text, invalidAnchor.href);
assert.equal(auditResult.details.items[0][1].text, invalidAnchor.text);
});

it('ignores links pointing to the main document', () => {
const artifacts = {
URL: {
finalUrl: 'https://example.com/page.html',
},
CrawlableAnchors: [
{href: 'https://example.com/otherpage.html', text: 'legit anchor text'},
{href: 'https://example.com/page.html', text: 'click here'},
{href: 'https://example.com/page.html#test', text: 'click here'},
{href: 'https://example.com/otherpage.html', text: 'legit anchor text'},
],
};

const auditResult = AnchorTextAudit.audit(artifacts);
assert.equal(auditResult.rawValue, true);
});

it('ignores javascript: links', () => {
const artifacts = {
URL: {
finalUrl: 'https://example.com/page.html',
},
CrawlableAnchors: [
{href: 'javascript:alert(1)', text: 'click here'},
{href: 'JavaScript:window.location="/otherpage.html"', text: 'click here'},
{href: 'JAVASCRIPT:void(0)', text: 'click here'},
],
};

const auditResult = AnchorTextAudit.audit(artifacts);
assert.equal(auditResult.rawValue, true);
});

it('passes when all anchors have descriptive texts', () => {
const artifacts = {
URL: {
finalUrl: 'https://example.com/page.html',
},
CrawlableAnchors: [
{href: 'https://example.com/otherpage.html', text: 'legit anchor text'},
{href: 'http://example.com/page.html?test=test', text: 'legit anchor text'},
{href: 'file://Users/user/Desktop/file.png', text: 'legit anchor text'},
],
};

const auditResult = AnchorTextAudit.audit(artifacts);
assert.equal(auditResult.rawValue, true);
});
});
Copy link
Member

Choose a reason for hiding this comment

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

add a javascript: unit test?