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(crawlable-anchors): adds an anchor-href audit #10662

Merged
merged 18 commits into from
May 20, 2020

Conversation

umaar
Copy link
Contributor

@umaar umaar commented Apr 28, 2020

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:

image

image

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

Related issue: #10590

@umaar umaar requested a review from a team as a code owner April 28, 2020 23:33
@umaar umaar requested review from exterkamp and removed request for a team April 28, 2020 23:33
@umaar umaar marked this pull request as draft April 28, 2020 23:33
@connorjclark
Copy link
Collaborator

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
Copy link
Contributor Author

umaar commented Apr 29, 2020

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:


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

@umaar umaar marked this pull request as ready for review April 29, 2020 21:25
@connorjclark
Copy link
Collaborator

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
Copy link
Member

brendankenny commented May 1, 2020

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 umaar changed the title new_audit(anchor-href): adds an anchor-href audit new_audit(crawlable-anchors): adds an anchor-href audit May 4, 2020
@vercel vercel bot temporarily deployed to Preview May 18, 2020 22:26 Inactive
@umaar
Copy link
Contributor Author

umaar commented May 18, 2020

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
Copy link
Collaborator

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
Copy link
Collaborator

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 😞

image

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

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 awesome job @umaar! 👍

@umaar
Copy link
Contributor Author

umaar commented May 19, 2020

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
Copy link
Collaborator

Thanks @umaar! :)

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

@AVGP
Copy link
Collaborator

AVGP commented May 20, 2020

Thanks @umaar! :)

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

LvGTM! 👍 :shipit:

@patrickhulce patrickhulce merged commit 575e29b into GoogleChrome:master May 20, 2020
@patrickhulce
Copy link
Collaborator

@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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants