Refine generic link text linter logic #34
Conversation
…a requirement though recommended
| if link_tag?(prev_node_tag) && link_tag?(next_node_tag) && next_node_tag.closing? | ||
| # Skip because we cannot reliably check accessible name from aria-labelledby, or an aria-label that is set to a variable | ||
| # with static code analysis. | ||
| next if aria_labelledby.present? || (aria_label.present? && aria_label.join.include?("<%=")) |
There was a problem hiding this comment.
We skip flagging the link if it has aria-labelledby or an aria-label that is set to a variable like:
<a href="github.com" aria-label="<%= some_variable%>" >since it may be an acceptable usecase and we cannot evaluate with ERB lint.
khiga8
force-pushed
the
kh-refine-generic-text-logic
branch
from
6f01605 to
1c3a867
Compare
andrialexandrou
left a comment
andrialexandrou
left a comment
There was a problem hiding this comment.
Ship to learn ✨
Nonblocking:
I put a suggested change to the documentation in a comment. Bc of code blocks and suggestions not playing nice together, I'll leave it to you to add it in a commit or not. I don't want to push to your branch w/o approval.
| It is acceptable to use `aria-label` or `aria-labelledby` to provide a more descriptive text in some cases. As note above, this is not the preferred solution and one should strive to make the visible text to be as descriptive as the design allows. | ||
|
|
||
| If you _must_ use this technique, you need to ensure that the accessible name completely contains the visible text. Otherwise, this is a failure of [SC 2.5.3: Label in Name](https://www.w3.org/WAI/WCAG21/Understanding/label-in-name.html). | ||
|
|
||
| This is not acceptable: | ||
| ``` | ||
| <a href="..." aria-label="GitHub announces something">Read more</a> | ||
| ``` | ||
|
|
||
| This is acceptable: | ||
| ``` | ||
| <a href="..." aria-label="Read more about the new accesibility feature">Read more</a> | ||
| ``` | ||
|
|
||
| This linter will raise a flag when it is able to detect that a generic link has an accessible name that doesn't contain the visible text. Due to the restrictions of static code analysis, this may not catch all violations so it is important to supplement this check with other techniques like browser tests. For instance, ERB lint will not be able to evaluate the accessible name set by `aria-labelledby` or when a variable is set to `aria-label` since this requires runtime evaluation. |
There was a problem hiding this comment.
If you must use ARIA to replace the visible link text, include the visible text at the beginning.
For example, on a pricing plans page, the following are good:
- Visible text:
Learn more - Accessible label:
Learn more about GitHub pricing plans
Accessible ✅
<a href="..." aria-label="Learn more about GitHub pricing plans">Learn more</a>Inaccessible 🚫
<a href="..." aria-label="GitHub pricing plans">Learn more</a>Including the visible text in the ARIA label satisfies SC 2.5.3: Label in Name.
False negatives
Caution: because of the restrictions of static code analysis, we may not catch all violations.
Please perform browser tests and spot checks:
- when
aria-labelis set dynamically - when using
aria-labelledby
There was a problem hiding this comment.
Love this! Thank you!
khiga8
force-pushed
the
kh-refine-generic-text-logic
branch
from
4ca53cc to
6a79f49
Compare
Co-authored-by: Andri Alexandrou <andrialexandrou@github.com>
khiga8
force-pushed
the
kh-refine-generic-text-logic
branch
from
6a79f49 to
dc66318
Compare
Relates to: https://github.com/github/accessibility/issues/1291
@github/accessibility-reviewers before you review, please check the discussion in the linked issue :)
Changes
This refines the generic linter text introduced in #32 based on internal discussions.
Currently, this linter will flag a link with generic text even if it has a descriptive accessible name set by
aria-labelledbyoraria-label. This is a sometimes acceptable technique even if it may not be the most preferable, so we don't want to raise a flag because it could be a false positive.This updates the linter to not evaluate links with generic text that have ARIA attributes since linters should never raise false positives. One nuance we discussed is that when using ARIA to set a more descriptive accessible name, one must ensure that the visible text is contained in the accessible name output. This is because this would be a violation of Success Criterion 2.5.3: Label in Name.
If the linter is able to access the
aria-labelstring and can accurately determine that thearia-labeltext does not contain the visible text at all, it will raise a flag. Notably, because this is static code analysis, we have limitations when it comes to evaluating the actual runtime values of the accessible name output, so this flag won't always be raised.Examples of accessible name that ERB lint can and cannot evaluate
ERB lint can evaluate the accessible name when
aria-labelis a string:ERB lint cannot evaluate the accessible name when
aria-labelledbyso we skip:ERB lint cannot evaluate the accessible name since it's a variable so we skip:
ERB lint cannot accurately evaluate the accessible name of
link_tohelpers witharia-labeloraria-labelledbyattributes so we skip completely:Even with these limitations around not being able to comprehensively evaluate links if they have ARIA label attributes set, this linter should catch quite a number of issues.
I've added a note in the doc that further informs these changes. The code is a bit complex so I added comments and additional tests which should help.
References
GitHub Staff only - Discussion on nuanced uses of ARIA to provide more descriptive text