new_audit(anchor-text-audit): descriptive anchor text audit by kdzwinel · Pull Request #3490 · GoogleChrome/lighthouse

kdzwinel · 2017-10-05T21:53:28Z

Failing audit:

Successful audit:

kdzwinel · 2017-10-05T21:56:53Z

lighthouse-core/audits/seo/anchor-text.js

+          url.origin == pageHost.origin &&
+          url.pathname == pageHost.pathname &&
+          url.search == pageHost.search
+        ) {


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

kdzwinel · 2017-10-05T22:04:54Z

lighthouse-core/audits/seo/anchor-text.js

+          url.protocol.toLowerCase() === 'javascript:' ||
+          (url.origin == pageHost.origin &&
+            url.pathname == pageHost.pathname &&
+            url.search == pageHost.search)


we are skipping links that point to the same website (e.g. '#' or 'javascript:void(0)')

we should have a helper method for the non-javascript case

lighthouse/lighthouse-core/lib/url-shim.js

Lines 116 to 129 in 05030ad

URL.equalWithExcludedFragments = function(url1, url2) {

[url1, url2] = [url1, url2].map(rewriteChromeInternalUrl);

try {

url1 = new URL(url1);

url1.hash = '';

url2 = new URL(url2);

url2.hash = '';

return url1.href === url2.href;

} catch (e) {

return false;

}

};

does that work for you?

Include this as an inline comment in the code if it's purpose might be unclear.

@patrickhulce that works perfectly!
@rviscomi with equalWithExcludedFragments this will probably won't need more explanation. I'll skip the comment.

patrickhulce

nice! easy win here 🎉 🔍 💣

patrickhulce · 2017-10-05T22:13:22Z

lighthouse-core/audits/seo/anchor-text.js

+  'start',
+  'right here',
+  'more',
+  'learn more',


😱 our report would fail like crazy!

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)

patrickhulce · 2017-10-05T22:14:31Z

lighthouse-core/audits/seo/anchor-text.js

+          return false;
+        }
+
+        return BLACKLIST.includes(anchor.text.trim());


need to toLowerCase this too? maybe add a test case to catch it too :)

👍 Good point, fixed.

patrickhulce · 2017-10-05T22:15:27Z

lighthouse-core/audits/seo/anchor-text.js

+
+        return BLACKLIST.includes(anchor.text.trim());
+      })
+      .map(anchor => ({


does the anchor have extra data you're trying to trim out here? if not, let's just drop since the prop names are the same

based on the return value of the gather, looks like they only have these two props... so agree we can drop the .map().

Correct, these are the only two props. Good catch!

patrickhulce · 2017-10-05T22:16:31Z

lighthouse-core/audits/seo/anchor-text.js

+        value: failingAnchors,
+      },
+      details,
+      displayValue: failingAnchors.length ? `${failingAnchors.length} anchors found` : undefined,


we usually add the qualifier here for display value, how about 2 non-descriptive anchors found?

That's the copy that I got from here: Anchors do not have descriptive text: {#} anchors found. Not sure if Anchors do not have descriptive text: {#} non-descriptive anchors found sounds equally good. @rviscomi WDYT?

oh, that's fine then 👍 ignore me :)

patrickhulce · 2017-10-05T22:18:17Z

lighthouse-core/audits/seo/anchor-text.js

+
+const Audit = require('../audit');
+const URL = require('../../lib/url-shim');
+const BLACKLIST = [


not sure it makes a big difference with such a small list but how about a Set since we just basically want .has

Good point! We will be checking 100s of links and these will be mostly misses against the blocklist, so it turns out it can actually make a difference: https://github.com/GoogleChrome/lighthouse/pull/3490/files#r143292224

rviscomi · 2017-10-05T22:18:43Z

lighthouse-core/audits/seo/anchor-text.js

+          url.protocol.toLowerCase() === 'javascript:' ||
+          (url.origin == pageHost.origin &&
+            url.pathname == pageHost.pathname &&
+            url.search == pageHost.search)


Include this as an inline comment in the code if it's purpose might be unclear.

rviscomi · 2017-10-05T22:24:02Z

lighthouse-core/audits/seo/anchor-text.js

+          url.protocol.toLowerCase() === 'javascript:' ||
+          (url.origin == pageHost.origin &&
+            url.pathname == pageHost.pathname &&
+            url.search == pageHost.search)


Do we need to compare the search properties? #foo and #bar are the same web page all else being equal. Unless the page is using ajaxy #! URLs :-\

rviscomi · 2017-10-05T22:25:57Z

lighthouse-core/audits/seo/anchor-text.js

+          return false;
+        }
+
+        return BLACKLIST.includes(anchor.text.trim());


Performance nit: maybe make BLACKLIST a Set and use Set.has()? The blacklist may grow, and this is inside a loop with potentially many iterations, so it could be significant. Ok to leave it as is if you don't feel it'll be significant.

i'd leave it alone. we have bigger fish to fry. :)

I thought it's probably nothing but then I checked… and damn, Array.includes is slow:

This is for 100 elements in array/set and 1000 checks. Bottom two results are probably most important in our case.
https://jsperf.com/set-array-access

rviscomi · 2017-10-05T22:27:12Z

lighthouse-core/audits/seo/anchor-text.js

+        value: failingAnchors,
+      },
+      details,
+      displayValue: failingAnchors.length ? `${failingAnchors.length} anchors found` : undefined,


If the length is 1, we should say "1 anchor found".

paulirish · 2017-10-06T00:15:02Z

lighthouse-core/audits/seo/anchor-text.js

@@ -0,0 +1,82 @@
+/**
+ * @license Copyright 2016 Google Inc. All Rights Reserved.


🔎 good catch!

paulirish · 2017-10-06T00:15:39Z

lighthouse-core/audits/seo/anchor-text.js

+
+const Audit = require('../audit');
+const URL = require('../../lib/url-shim');
+const BLACKLIST = [


We'll be slowly doing a s/Whitelist/Safelist/g and s/Blacklist/Blocklist/g, but i'd love to get a headstart here. :)

CS lingo is the worst. Thanks for pointing that out.

paulirish · 2017-10-06T00:18:48Z

lighthouse-core/audits/seo/anchor-text.js

+          return false;
+        }
+
+        return BLACKLIST.includes(anchor.text.trim());


paulirish · 2017-10-06T00:19:18Z

lighthouse-core/audits/seo/anchor-text.js

+          return false;
+        }
+
+        return BLACKLIST.includes(anchor.text.trim());


i'd leave it alone. we have bigger fish to fry. :)

paulirish · 2017-10-06T00:20:49Z

lighthouse-core/audits/seo/anchor-text.js

+
+        return BLACKLIST.includes(anchor.text.trim());
+      })
+      .map(anchor => ({


based on the return value of the gather, looks like they only have these two props... so agree we can drop the .map().

brendankenny · 2017-10-06T22:20:14Z

lighthouse-core/gather/gatherers/seo/crawlable-anchors.js

+  afterPass(options) {
+    const expression = `(function() {
+      ${DOMHelpers.getElementsInDocumentFnString}; // define function on page
+      const selector = 'a[href]:not([rel~="nofollow"])';


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

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

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

sg. filed as #3581

paulirish

Overall this looks great. A few wording changes..

paulirish · 2017-10-16T19:47:08Z

lighthouse-core/audits/seo/anchor-text.js

+      });
+
+    const headings = [
+      {key: 'href', itemType: 'url', text: 'URL'},


URL => Link destination

paulirish · 2017-10-16T19:47:16Z

lighthouse-core/audits/seo/anchor-text.js

+
+    const headings = [
+      {key: 'href', itemType: 'url', text: 'URL'},
+      {key: 'text', itemType: 'text', text: 'Text'},


Text => Link Text

paulirish · 2017-10-16T19:48:39Z

lighthouse-core/audits/seo/anchor-text.js

+    return {
+      category: 'Content Best Practices',
+      name: 'anchor-text',
+      description: 'Anchors have descriptive text.',


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.

"links" or "<a> elements" SGTM

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

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

paulirish · 2017-10-16T19:48:54Z

lighthouse-core/audits/seo/anchor-text.js

+      category: 'Content Best Practices',
+      name: 'anchor-text',
+      description: 'Anchors have descriptive text.',
+      failureDescription: 'Anchors do not have descriptive text',


Anchors => Links

brendankenny · 2017-10-16T19:52:22Z

lighthouse-core/gather/gatherers/seo/crawlable-anchors.js

+      return elements
+        .map(node => ({
+          href: node.href,
+          text: node.innerText


s/innerText/textContent

disagree. :)

jk I'm wrong :)

brendankenny · 2017-10-16T19:57:11Z

lighthouse-core/audits/seo/anchor-text.js

+    return {
+      rawValue: failingAnchors.length === 0,
+      extendedInfo: {
+        value: failingAnchors,


in theory this could be a fairly large set of links. Is there a benefit to having a copy in extendedInfo as well as in details? If not we should probably just keep details

extendedInfo is a bit easier to work with in unit tests:

assert.equal(auditResult.extendedInfo.value.length, 1); assert.equal(auditResult.extendedInfo.value.includes(invalidAnchor), true);

will have to became:

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);

But yeah, it doesn't justify keeping it around.

brendankenny · 2017-10-16T19:58:08Z

lighthouse-cli/test/smokehouse/seo/expectations.js

      },
+      'anchor-text': {
+        score: false,
+        displayValue: '3 anchors found',


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)

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

I'd leave that to unit tests

SGTM

brendankenny · 2017-10-16T20:02:58Z

lighthouse-core/audits/seo/anchor-text.js

+      .filter(anchor => {
+        if (
+          anchor.href.toLowerCase().startsWith('javascript:') ||
+          URL.equalWithExcludedFragments(anchor.href, artifacts.URL.finalUrl)


why are links within the page not counted?

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

brendankenny · 2017-10-16T20:03:52Z

lighthouse-core/test/audits/seo/anchor-text-test.js

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


add a javascript: unit test?

…t for javascript: links.

kdzwinel · 2017-10-17T00:54:22Z

@brendankenny all done, thank you!

@rviscomi I'll need you to sign off on these description changes that Paul suggested.

paulirish · 2017-10-17T20:21:37Z

@rviscomi is calling this "Link text" rather than "Anchor text" ok with you? (we've already made the change)

rviscomi · 2017-10-17T21:30:17Z

👍 Anchor -> Link SGTM, reviewed earlier

brendankenny

LGTM

paulirish · 2017-10-19T02:33:21Z

ⒸⓁⒾⒸⓀ ⒽⒺⓇⒺ

💥

kdzwinel changed the title ~~Seo anchor text~~ new SEO audit: descriptive status text Oct 5, 2017

kdzwinel changed the title ~~new SEO audit: descriptive status text~~ new SEO audit: descriptive anchor text Oct 5, 2017

kdzwinel commented Oct 5, 2017

View reviewed changes

patrickhulce reviewed Oct 5, 2017

View reviewed changes

rviscomi reviewed Oct 5, 2017

View reviewed changes

paulirish reviewed Oct 6, 2017

View reviewed changes

brendankenny reviewed Oct 6, 2017

View reviewed changes

paulirish changed the title ~~new SEO audit: descriptive anchor text~~ new SEO audit: descriptive anchor text. Oct 11, 2017

kdzwinel changed the title ~~new SEO audit: descriptive anchor text.~~ feature(anchor-text-audit): Adding descriptive anchor text audit Oct 11, 2017

kdzwinel changed the title ~~feature(anchor-text-audit): Adding descriptive anchor text audit~~ Adding(anchor-text-audit): descriptive anchor text audit Oct 11, 2017

kdzwinel changed the title ~~Adding(anchor-text-audit): descriptive anchor text audit~~ new-audit(anchor-text-audit): descriptive anchor text audit Oct 11, 2017

kdzwinel force-pushed the seo-anchor-text branch from 668f28c to b16c181 Compare October 11, 2017 22:18

paulirish changed the title ~~new-audit(anchor-text-audit): descriptive anchor text audit~~ new_audit(anchor-text-audit): descriptive anchor text audit Oct 11, 2017

paulirish modified the milestones: Sprint Uno: Oct 2-13, Sprint Dos: Oct 16-27 Oct 16, 2017

paulirish mentioned this pull request Oct 16, 2017

Create generalized all-anchors gatherer #3581

Closed

paulirish requested changes Oct 16, 2017

View reviewed changes

brendankenny suggested changes Oct 16, 2017

View reviewed changes

kdzwinel added 6 commits October 17, 2017 01:15

new SEO audit: descriptive status text

b9e8bda

Display value should be undefined on audit success.

2c1600e

Ignore 'javascript:' links

5794d61

Address review comments.

068da8e

Check number of items in smoke test. Get rid of extendedInfo. Add tes…

f4a7989

…t for javascript: links.

Change details table headings.

db23054

kdzwinel force-pushed the seo-anchor-text branch from b16c181 to db23054 Compare October 17, 2017 00:35

karanjthakkar mentioned this pull request Oct 17, 2017

core(audit): Ignore href=javascript:.* for rel=noopener audit #3574

Merged

Anchor -> Link - all the things

b710137

paulirish added the waiting4reviewer label Oct 17, 2017

brendankenny approved these changes Oct 17, 2017

View reviewed changes

brendankenny assigned paulirish Oct 17, 2017

paulirish approved these changes Oct 19, 2017

View reviewed changes

paulirish merged commit f911e84 into GoogleChrome:master Oct 19, 2017

ghost mentioned this pull request Dec 17, 2017

Update lighthouse in / from 2.5.0 to 2.7.0 chauncey-garrett/dotfiles#57

Open

paulirish removed the waiting4reviewer label Mar 6, 2018

patrickhulce mentioned this pull request Nov 6, 2018

core(seo): properly handle anchors in SVG #6483

Merged

	URL.equalWithExcludedFragments = function(url1, url2) {
	[url1, url2] = [url1, url2].map(rewriteChromeInternalUrl);
	try {
	url1 = new URL(url1);
	url1.hash = '';

	url2 = new URL(url2);
	url2.hash = '';

	return url1.href === url2.href;
	} catch (e) {
	return false;
	}
	};

		@@ -0,0 +1,82 @@
		/**
		* @license Copyright 2016 Google Inc. All Rights Reserved.

Conversation

kdzwinel commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rviscomi Oct 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

LEARN MORE

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

LEARN MORE

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rviscomi Oct 8, 2017 •

edited

Loading

patrickhulce Oct 6, 2017 •

edited

Loading

rviscomi Oct 17, 2017 •

edited

Loading