new_audit(crawlable-anchors): adds an anchor-href audit

[umaar]

Hey, still have more work to do here but thought the general approach could be sanity checked in the meantime. I guess things to check here:

• are we ok with the blocklist?
• are we ok with the approach of exposing a new anchorHref property from the anchor elements gatherer?
• messages/strings
• tests
• displaying results to the user - what do we want to show?

Quick q around the tests, typically for a new audit we have a unit test for the gatherer + audit, update the jest snapshots, then run a yarn update:sample-json? Does the protobuf stuff come into play here, as in would yarn compile-proto && yarn build-proto-roundtrip be needed also? I also see an Updating fixture dumps doc and a yarn update:sample-artifacts command.

I think it'd be cool to have a doc like, "you added/removed an audit, here's what you need to update" (that may exist somewhere and I've missed it though!)

Screenshots:

Run with ./lighthouse-cli/index.js --view --only-audits=crawlable-anchors http://example.com

Related issue: #10590

[connorjclark]

thanks for the PR! couple of quickfire comments (leaving the full review to shane):

Quick q around the tests, typically for a new audit we have a unit test for the gatherer + audit, update the jest snapshots, then run a yarn update:sample-json? Does the protobuf stuff come into play here, as in would yarn compile-proto && yarn build-proto-roundtrip be needed also? I also see an Updating fixture dumps doc and a yarn update:sample-artifacts command.

We recently moved the proto roundtrip to happen entirely in CI. #10557 . You still need to do yarn update:sample-json to update the sample json with the new audit results.

Since you modified a gather, you will also need to update results/artifacts.json so we are testing your new feature in the sample json. That's what we mean by "update the fixutres". It's best to run that command and only save the changes you need. Sometimes we just manually modify the file if that is not feasible.

---

All of our audits have a web.dev article linked in the description. I don't know what the turnaround time for that might be, but we shouldn't request it of @kaycebasques until we're good with the audit. Maybe there's good alternative we can use in the meantime, do you know of any?

---

Can you provide screenshots in the original comment?

[umaar]

Thanks for explaining @connorjclark. Curious why we should discard the other changes, but in the end I did yarn update:sample-artifacts and discarded changes to defaultPass.devtoolslog.json, defaultPass.trace.json, offlinePass.devtoolslog.json, redirectPass.devtoolslog.json, but kept the changes in artifacts.json in the AnchorElements section.

---

For the web.dev article, here's a quick draft text which could be a starting point. There are also these existing pages:

• Google webmasters: Make your links crawlable
• axe-core Anchors must only be used as links with valid URLs or URL fragments

---

Screenshots added in the original comment

---

Would be cool to see this working in DevTools, wondering if there are any tips for getting a local version of Lighthouse integrated with chrome/chromium during development

[connorjclark]

Curious why we should discard the other changes

Avoid churn, keep git blame a useful archaeological tool, and doing wholesale updates will make unrelated tests fail/require updating.

For the web.dev article, here's a quick draft text which could be a starting point.

Cool! Seems like we can block this PR on that new article being published, shouldn't be much of a delay.

Would be cool to see this working in DevTools, wondering if there are any tips for getting a local version of Lighthouse integrated with chrome/chromium during development

#10241 mentions that there are some necessary changes in Chromium for 6.0 to work as intended. Probably should still work enough to test things out. assuming you have a local build of chromium, you run yarn build-devtools && yarn devtools to roll Lighthouse to the CDT frontend repo. see lighthouse-core/scripts/roll-to-devtools.sh

doc for chromium devtools: https://docs.google.com/document/d/1WNF-KqRSzPLUUfZqQG5AFeU_Ll8TfWYcJasa_XGf7ro/edit#heading=h.xz439gqj1lwr I think it may be possible these days to use a release build of Chromium, pass a flag to it pointing to a development build of the devtools–skipping the burden of building Chromium.

[brendankenny]

Should maybe be named something more specific like crawlable-anchors or something like that?

Also it would be good to try to address the questions brought up at the end of #10590 (comment)

• What does the crawler do (and does it penalise or simply ignore some of those above examples)
• How strict should this audit be considering the popularity of those techniques

From the fail examples, it seems like a lot of sites are going to get a lot of hits on this :) How much would fixing them help the site?

It seems like if it's trivial to switch a js-only "anchor" that behaves like a real anchor to a real anchor (possibly augmented with js) it's worth it so search engines better know the connections, but OTOH if a site is (ab)using anchors for more button-like things, the ROI for them to fix might be really low and this will come off more as nagging for Lighthouse users.

[umaar]

Hey @patrickhulce thanks again for the tips, messages are updated.

Maybe add a few of the passing and failing examples we discussed to the DBW tester?

Unless I'm mistaken on what the DBW tester is, I think because this is around SEO, it should go into the SEO smoke tests? Anyway have done that (smoke tests in the SEO section) for now, but happy to move them elsewhere if that's better. Kept the smokes a little light to adhere to the testing pyramid. I think the unit tests cover a good amount. That being said the anchor-elements.js gatherer never had a unit test to begin with it seems.

[patrickhulce]

Unless I'm mistaken on what the DBW tester is, I think because this is around SEO, it should go into the SEO smoke tests?

Sure even better :) I think there's still a little bleed over from before the SEO were split out, but good call 👍

[patrickhulce]

So good @umaar , great job! My mouse was literally hovering over the approve button, but then I decided to try one more site and I have now waffled on one minor point. I'm sorry 😞

I know I said we should look at click handler when you asked if we should ignore role=button but the cases in CNN reconvinced me otherwise.

I think we should ignore role=X for this audit regardless of click handler.

1. It's an obvious signal that this thing is not a link, so for SEO it clearly doesn't need to be crawlable. An anchor element might be the wrong choice for whatever it is, but it's not the job of this audit to police that. We should solve this problem with other a11y audits.
2. In the CNN case, they are definitely button-like clickable elements we would normally ignore but they're hidden offscreen, so I assume the handlers are attached upon render.

Hopefully this is just 2 lines for exposure, 1 line for the audit check and a few lines for the test and we're done if you agree :D

[patrickhulce]

LGTM 🎉 awesome job @umaar! 👍

[umaar]

Hey thanks for being so thorough with this PR @patrickhulce . I think you're right, it's better to be flexible and help users see the value in this audit, rather than it turning into an audit which gets habitually ignored. And it's really nice how you take time to explain + justify each suggestion, it means I get to understand the reasoning behind things and learn a new perspective!

@AVGP we now have a well defined list of fail criteria. Following this web.dev template, shall I make a PR documenting exactly when this audit will fail? E.g., referring to the javascript:void(0) case and other similar ones? Since we already have a [Learn More] link in this audit, we can update the link in a separate PR.

[patrickhulce]

Thanks @umaar! :)

I'd love a quick LGTM from @AVGP as well that the criteria look good before merging 👍

[AVGP]

Thanks @umaar! :)

I'd love a quick LGTM from @AVGP as well that the criteria look good before merging 👍

LvGTM! 👍

[patrickhulce]

@connorjclark I'm curious what your take on the commit message would've been here for a new audit where the score is basically repeating the message :)