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

core: allow placeholder anchor elements #15894

Conversation

romainmenke
Copy link

Summary

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

See : #15820 (comment)

Related Issues/PRs

Fixes : #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.

@romainmenke romainmenke requested a review from a team as a code owner March 26, 2024 22:26
@romainmenke romainmenke requested review from connorjclark and removed request for a team March 26, 2024 22:26
@adamraine
Copy link
Member

adamraine commented Mar 28, 2024

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

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

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

@romainmenke romainmenke deleted the core-allow-placeholder-anchor-elements--intuitive-vampire-bat-8328f295b7 branch April 11, 2024 16:57
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.

Lighthouse warns when an anchor tag is just a placeholder
4 participants