-
Notifications
You must be signed in to change notification settings - Fork 8
Add linter against generic link text #32
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
Changes from all commits
3480742
837dba1
4379446
80ebd6b
595f5a3
c077f6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| # Avoid generic link text | ||
|
|
||
| ## Rule Details | ||
|
|
||
| Avoid setting generic link text like, "Click here", "Read more", and "Learn more" which do not make sense when read out of context. | ||
|
|
||
| Screen reader users often tab through links on a page to quickly find content without needing to listen to the full page. When link text is too generic, it becomes difficult to quickly identify the destination of the link. While it is possible to provide a more specific link text by setting the `aria-label`, this results in divergence between the label and the text and is not an ideal, future-proof solution. | ||
|
|
||
| Additionally, generic link text can also problematic for heavy zoom users where the link context is out of view. | ||
|
|
||
| Ensure that your link text is descriptive and the purpose of the link is clear even when read out of context of surrounding text. | ||
| Learn more about how to write descriptive link text at [Access Guide: Write descriptive link text](https://www.accessguide.io/guide/descriptive-link-text) | ||
|
|
||
| ## Resources | ||
|
|
||
| - [Primer: Links](https://primer.style/design/accessibility/links) | ||
| - [Understanding Success Criterion 2.4.4: Link Purpose (In Context)](https://www.w3.org/WAI/WCAG21/Understanding/link-purpose-in-context.html) | ||
| - [WebAim: Links and Hypertext](https://webaim.org/techniques/hypertext/) | ||
| - [Deque: Use link text that make sense when read out of context](https://dequeuniversity.com/tips/link-text) | ||
|
|
||
| ## Examples | ||
|
|
||
| ### **Incorrect** code for this rule 👎 | ||
|
|
||
| ```erb | ||
| <a href="github.com/about">Learn more</a> | ||
| ``` | ||
|
|
||
| ```erb | ||
| <!-- also bad --> | ||
| <a href="github.com/about">Read more</a> | ||
| ``` | ||
|
|
||
| ```erb | ||
| <!-- also bad --> | ||
| <span> | ||
| <a href="github.com/new">Click here</a> to create a new repository. | ||
| </span> | ||
| ``` | ||
|
|
||
| ```erb | ||
| <!-- also bad --> | ||
| <%= link_to "Learn more", "#" %> | ||
| ``` | ||
|
|
||
| ### **Correct** code for this rule 👍 | ||
|
|
||
| ```erb | ||
| <a href="github.com/about">Learn more about GitHub</a> | ||
| ``` | ||
|
|
||
| ```erb | ||
| <a href="github.com/new">Create a new repository</a> | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require_relative "../../custom_helpers" | ||
|
|
||
| module ERBLint | ||
| module Linters | ||
| module GitHub | ||
| module Accessibility | ||
| class AvoidGenericLinkTextCounter < Linter | ||
| include ERBLint::Linters::CustomHelpers | ||
| include LinterRegistry | ||
|
|
||
| BANNED_GENERIC_TEXT = [ | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there anything to remove from here? Anything else we should add in here?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is amazing 🤩 Thank you for doing this! What do you think about adding "Here" as well? I've seen text such as `Read more about it here
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great idea!!! <3
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in: 80ebd6b ❤️ |
||
| "Read more", | ||
| "Learn more", | ||
| "Click here", | ||
| "More", | ||
| "Link", | ||
| "Here" | ||
| ].freeze | ||
| MESSAGE = "Avoid using generic link text such as #{BANNED_GENERIC_TEXT.join(', ')} which do not make sense in isolation." | ||
|
|
||
| def run(processed_source) | ||
| processed_source.ast.children.each_with_index do |node, index| | ||
| next unless node.methods.include?(:type) && node.type == :text | ||
|
|
||
| text = node.children.join.strip | ||
| # Checks HTML tags | ||
| if banned_text?(text) | ||
| prev_node = processed_source.ast.children[index - 1] | ||
| next_node = processed_source.ast.children[index + 1] | ||
|
|
||
| next unless tag_type?(prev_node) && tag_type?(next_node) | ||
|
|
||
| text_node_tag = BetterHtml::Tree::Tag.from_node(node) | ||
| prev_node_tag = BetterHtml::Tree::Tag.from_node(prev_node) | ||
| next_node_tag = BetterHtml::Tree::Tag.from_node(next_node) | ||
|
|
||
| # We only report if the text is nested between two link tags. | ||
| if link_tag?(prev_node_tag) && link_tag?(next_node_tag) && next_node_tag.closing? | ||
| range = prev_node_tag.loc.begin_pos...text_node_tag.loc.end_pos | ||
| source_range = processed_source.to_source_range(range) | ||
| generate_offense_from_source_range(self.class, source_range) | ||
| end | ||
| end | ||
|
|
||
| # Checks Rails link helpers like `link_to` | ||
| erb_node = node.type == :erb ? node : node.descendants(:erb).first | ||
| next unless erb_node | ||
|
|
||
| _, _, code_node = *erb_node | ||
| source = code_node.loc.source | ||
| ruby_node = extract_ruby_node(source) | ||
| send_node = ruby_node&.descendants(:send)&.first | ||
| next unless send_node.methods.include?(:method_name) && send_node.method_name == :link_to | ||
|
|
||
| send_node.child_nodes.each do |child_node| | ||
| if child_node.methods.include?(:type) && child_node.type == :str && banned_text?(child_node.children.join) | ||
| tag = BetterHtml::Tree::Tag.from_node(code_node) | ||
| generate_offense(self.class, processed_source, tag) | ||
| end | ||
| end | ||
| end | ||
| counter_correct?(processed_source) | ||
| end | ||
|
|
||
| def autocorrect(processed_source, offense) | ||
| return unless offense.context | ||
|
|
||
| lambda do |corrector| | ||
| if processed_source.file_content.include?("erblint:counter #{simple_class_name}") | ||
| # update the counter if exists | ||
| corrector.replace(offense.source_range, offense.context) | ||
| else | ||
| # add comment with counter if none | ||
| corrector.insert_before(processed_source.source_buffer.source_range, "#{offense.context}\n") | ||
| end | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def banned_text?(text) | ||
| BANNED_GENERIC_TEXT.map(&:downcase).include?(text.downcase) | ||
| end | ||
|
|
||
| def extract_ruby_node(source) | ||
| BetterHtml::TestHelper::RubyNode.parse(source) | ||
| rescue ::Parser::SyntaxError | ||
| nil | ||
| end | ||
|
|
||
| def link_tag?(tag_node) | ||
| tag_node.name == "a" | ||
| end | ||
|
|
||
| def tag_type?(node) | ||
| node.methods.include?(:type) && node.type == :tag | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "test_helper" | ||
|
|
||
| class AvoidGenericLinkTextCounterTest < LinterTestCase | ||
| def linter_class | ||
| ERBLint::Linters::GitHub::Accessibility::AvoidGenericLinkTextCounter | ||
| end | ||
|
|
||
| def test_warns_when_link_text_is_click_here | ||
| @file = "<a>Click here</a>" | ||
| @linter.run(processed_source) | ||
|
|
||
| refute_empty @linter.offenses | ||
| end | ||
|
|
||
| def test_warns_when_link_text_is_learn_more | ||
| @file = "<a>Learn more</a>" | ||
| @linter.run(processed_source) | ||
|
|
||
| refute_empty @linter.offenses | ||
| end | ||
|
|
||
| def test_warns_when_link_text_is_read_more | ||
| @file = "<a>Read more</a>" | ||
| @linter.run(processed_source) | ||
|
|
||
| refute_empty @linter.offenses | ||
| end | ||
|
|
||
| def test_warns_when_link_text_is_more | ||
| @file = "<a>More</a>" | ||
| @linter.run(processed_source) | ||
|
|
||
| refute_empty @linter.offenses | ||
| end | ||
|
|
||
| def test_warns_when_link_text_is_link | ||
| @file = "<a>Link</a>" | ||
| @linter.run(processed_source) | ||
|
|
||
| refute_empty @linter.offenses | ||
| end | ||
|
|
||
| def test_does_not_warn_when_banned_text_is_part_of_more_text | ||
| @file = "<a>Learn more about GitHub Stars</a>" | ||
| @linter.run(processed_source) | ||
|
|
||
| assert_empty @linter.offenses | ||
| end | ||
|
|
||
| def test_warns_when_link_rails_helper_text_is_banned_text | ||
| @file = "<%= link_to('click here', redirect_url, id: 'redirect') %>" | ||
| @linter.run(processed_source) | ||
|
|
||
| refute_empty @linter.offenses | ||
| end | ||
|
|
||
| def test_does_not_warn_when_generic_text_is_link_rails_helper_sub_text | ||
| @file = "<%= link_to('click here to learn about github', redirect_url, id: 'redirect') %>" | ||
| @linter.run(processed_source) | ||
|
|
||
| assert_empty @linter.offenses | ||
| end | ||
|
|
||
| def test_does_not_warns_if_element_has_correct_counter_comment | ||
| @file = <<~ERB | ||
| <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 1 %> | ||
| <a>Link</a> | ||
| ERB | ||
| @linter.run(processed_source) | ||
|
|
||
| assert_equal 0, @linter.offenses.count | ||
| end | ||
|
|
||
| def test_autocorrects_when_ignores_are_not_correct | ||
| @file = <<~ERB | ||
| <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 2 %> | ||
| <a>Link</a> | ||
| ERB | ||
| refute_equal @file, corrected_content | ||
|
|
||
| expected_content = <<~ERB | ||
| <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 1 %> | ||
| <a>Link</a> | ||
| ERB | ||
| assert_equal expected_content, corrected_content | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper allows the output of the ERB lint message to be something like:
where we can specify a range of the code source to be output. In this case we include the first anchor tag and the text content for ease of identification.
The other helper we commonly use,
generate_offenseonly allows a single tag to be output in the message.