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

Improve ambiguous-anchor-text false negatives (via extending getAccessibleChildText) #879

Open
2 tasks done
mattxwang opened this issue Jul 22, 2022 · 2 comments
Open
2 tasks done

Comments

@mattxwang
Copy link
Contributor

mattxwang commented Jul 22, 2022

I implemented ambiguous-anchor-text in #873, which uses a new util getAccessibleChildText to determine what counts as screenreader-accessible text inside an anchor tag. However, I think there are extra cases that would be valuable for the rule to catch.

Current Approach

The current approach is simple: recursively add children, with base cases for:

  • elements with an aria-label, returning the label - note that in this case, a label of "click here" is still an error
  • elements that are hidden from a screenreader (via isHiddenFromScreenReader), returning the empty string
  • the literal value for literals and text (Literal and JSXText types)

Proposed Extensions

However, this misses some nuance in how screenreader text is interpreted, leading to false negatives. I have some specific examples that I think should also be in-scope, all of which seem reasonable to implement:

There's also a more challenging (but important) case of aria-labelledby. This is tricker since:

  • we're not guaranteed that the id referenced by the attribute is a child of the anchor tag
  • or that it exists at all!
  • aria-labelledby supports reference lists, and order matters, though needs to ignore repeated IDs; chaining is disallowed

It also seems that aria-labelledby overrides aria-label, so we should be sure to implement that.

I imagine that aria-describedby is out of scope, since it's a long-form description (and may not make sense for a screenreader to read).

What do we think? I don't have great familiarity in this field, so perhaps I am missing some nuance or other motivating examples. I may also split up the PRs so that it's easier to review (i.e. the low hanging fruit first).

@mattxwang
Copy link
Contributor Author

I've thought about this a bit more (and done some local testing) - I don't actually think the <svg> override is necessary, since <title> is parsed anyways as a child and there should not be any other text inside it.

I think that addresses my shallow follow-ups, and I'm curious once we release this rule how the community responds / how important the referential checks are. @ljharb do you know if you're planning on cutting a new release soon? Would be very helpful for me to then resolve this issue (of course, no rush!).

@ljharb
Copy link
Member

ljharb commented Aug 23, 2022

@mattxwang no schedule, but i can try to do it in the next few weeks.

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

No branches or pull requests

2 participants