search

GoogleChrome / lighthouse

Automated auditing, performance metrics, and best practices for the web.
https://developer.chrome.com/docs/lighthouse/overview/
Apache License 2.0
28.27k stars 9.35k forks source link

core: allow placeholder anchor elements #15894

Closed romainmenke closed 5 months ago

romainmenke commented 6 months ago

Summary

Anchor elements without an href or specific attributes are considered placeholder elements and should not produce audit failures.

See : https://github.com/GoogleChrome/lighthouse/issues/15820#issuecomment-1977583434

Related Issues/PRs

Fixes : https://github.com/GoogleChrome/lighthouse/issues/15820

The tests will fail because I am not sure how to update all the artifacts. I tried this but the diff was very large and produced an unexpected number of new files.

adamraine commented 6 months ago

Looking into the test failures, I think the solution is going to be much more complicated than I hoped...

The following case should be an uncrawlable anchor:

<a class="some-link">Some link</a>
<script>
  document.querySelector('.some-link').addEventListener('mousedown', () => {});
</script>

However this PR would not flag this anchor because our analysis doesn't actually look at event listeners. We would previously use the absence of the href as an indication that this anchor is not crawlable.

I think a "proper" solution would require us to look for mousedown/click listeners on the element and any of it's parents so we can continue to fail those cases. Unfortunately, this change is looking more complicate than I hoped.

This PR is what I was originally hoping for though! if you are interested in working on this the test failures are coming from our smoke tests which you can run with yarn smoke seo-failing seo-passing. The test expectations files are here:

https://github.com/GoogleChrome/lighthouse/blob/main/cli/test/smokehouse/test-definitions/seo-failing.js https://github.com/GoogleChrome/lighthouse/blob/main/cli/test/smokehouse/test-definitions/seo-passing.js

romainmenke commented 6 months ago

Anchors with event listeners but without an href warn because they are an anti pattern? Stuff that should be buttons but aren't. Right?

Interesting! Thank you for this feedback!

romainmenke commented 5 months ago

Closing this because I don't currently have the bandwidth to continue work on this.