From 3480742750224651b14c3e2be7c47ac255e99048 Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Wed, 8 Jun 2022 22:50:43 -0500 Subject: [PATCH 1/6] Introduce generic link text linter --- README.md | 1 + .../avoid-generic-link-text-counter.md | 47 ++++++++ .../avoid_generic_link_text_counter.rb | 100 ++++++++++++++++++ .../avoid_generic_link_text_counter_test.rb | 89 ++++++++++++++++ 4 files changed, 237 insertions(+) create mode 100644 docs/rules/accessibility/avoid-generic-link-text-counter.md create mode 100644 lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb create mode 100644 test/linters/accessibility/avoid_generic_link_text_counter_test.rb diff --git a/README.md b/README.md index 3a2cf2b..bae57b8 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,7 @@ linters: ## Rules - [GitHub::Accessibility::AvoidBothDisabledAndAriaDisabled](./docs/rules/accessibility/avoid-both-disabled-and-aria-disabled.md) +- [GitHub::Accessibility::AvoidGenericLinkTextCounter](./docs/rules/accessibility/avoid-generic-link-text-counter.md) - [GitHub::Accessibility::IframeHasTitle](./docs/rules/accessibility/iframe-has-title.md) - [GitHub::Accessibility::ImageHasAlt](./docs/rules/accessibility/image-has-alt.md) - [GitHub::Accessibility::NoAriaLabelMisuseCounter](./docs/rules/accessibility/no-aria-label-misuse-counter.md) diff --git a/docs/rules/accessibility/avoid-generic-link-text-counter.md b/docs/rules/accessibility/avoid-generic-link-text-counter.md new file mode 100644 index 0000000..7098f06 --- /dev/null +++ b/docs/rules/accessibility/avoid-generic-link-text-counter.md @@ -0,0 +1,47 @@ +# 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. + +## Resources + +- [Primer: Links](https://primer.style/design/accessibility/link) +- [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 +Learn more +``` + +```erb + +Read more +``` + +```erb + + + Click here to create a new repository. + +``` + +```erb + +<%= link_to "Learn more", "#" %> +``` + +### **Correct** code for this rule 👍 + +```erb +Learn more about GitHub +``` + +```erb +Create a new repository +``` diff --git a/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb b/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb new file mode 100644 index 0000000..4766561 --- /dev/null +++ b/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb @@ -0,0 +1,100 @@ +# 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 = [ + "Read more", + "Learn more", + "Click here", + "More", + "Link" + ].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?(prev_node) && tag?(next_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? + generate_offense(self.class, processed_source, prev_node_tag) + 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 + if 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 + if banned_text?(child_node.children.join) + tag = BetterHtml::Tree::Tag.from_node(code_node) + generate_offense(self.class, processed_source, tag) + end + end + 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?(node) + node.methods.include?(:type) && node.type == :tag + end + end + end + end + end +end diff --git a/test/linters/accessibility/avoid_generic_link_text_counter_test.rb b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb new file mode 100644 index 0000000..8a8411a --- /dev/null +++ b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb @@ -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 = "Click here" + @linter.run(processed_source) + + refute_empty @linter.offenses + end + + def test_warns_when_link_text_is_learn_more + @file = "Learn more" + @linter.run(processed_source) + + refute_empty @linter.offenses + end + + def test_warns_when_link_text_is_read_more + @file = "Read more" + @linter.run(processed_source) + + refute_empty @linter.offenses + end + + def test_warns_when_link_text_is_more + @file = "More" + @linter.run(processed_source) + + refute_empty @linter.offenses + end + + def test_warns_when_link_text_is_link + @file = "Link" + @linter.run(processed_source) + + refute_empty @linter.offenses + end + + def test_does_not_warn_when_banned_text_is_part_of_more_text + @file = "Learn more about GitHub Stars" + @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 %> + Link + 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 %> + Link + ERB + refute_equal @file, corrected_content + + expected_content = <<~ERB + <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 1 %> + Link + ERB + assert_equal expected_content, corrected_content + end +end From 837dba160cc6e8b1f0f05b52e2aa885282c1c725 Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Thu, 9 Jun 2022 00:16:37 -0500 Subject: [PATCH 2/6] Add helper for showing offense given a source range --- lib/erblint-github/linters/custom_helpers.rb | 7 +++++++ .../accessibility/avoid_generic_link_text_counter.rb | 9 ++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/erblint-github/linters/custom_helpers.rb b/lib/erblint-github/linters/custom_helpers.rb index 3dd711b..86b5d8a 100644 --- a/lib/erblint-github/linters/custom_helpers.rb +++ b/lib/erblint-github/linters/custom_helpers.rb @@ -64,6 +64,13 @@ def generate_offense(klass, processed_source, tag, message = nil, replacement = add_offense(processed_source.to_source_range(tag.loc), offense, replacement) end + def generate_offense_from_source_range(klass, source_range, message = nil, replacement = nil) + message ||= klass::MESSAGE + message += "\nLearn more at https://github.com/github/erblint-github#rules.\n" + offense = ["#{simple_class_name}:#{message}", source_range.source].join("\n") + add_offense(source_range, offense, replacement) + end + def possible_attribute_values(tag, attr_name) value = tag.attributes[attr_name]&.value || nil basic_conditional_code_check(value || "") || [value].compact diff --git a/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb b/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb index 4766561..58899aa 100644 --- a/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb +++ b/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb @@ -28,14 +28,17 @@ def run(processed_source) prev_node = processed_source.ast.children[index-1] next_node = processed_source.ast.children[index+1] - next unless tag?(prev_node) && tag?(next_node) + 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? - generate_offense(self.class, processed_source, prev_node_tag) + 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 @@ -90,7 +93,7 @@ def link_tag?(tag_node) tag_node.name == "a" end - def tag?(node) + def tag_type?(node) node.methods.include?(:type) && node.type == :tag end end From 4379446d49a9d152cb243bb30785dbdb5e296011 Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Thu, 9 Jun 2022 08:15:50 -0500 Subject: [PATCH 3/6] Run rubocop lint --- .../avoid_generic_link_text_counter.rb | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb b/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb index 58899aa..d283295 100644 --- a/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb +++ b/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb @@ -22,14 +22,15 @@ class AvoidGenericLinkTextCounter < Linter 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] - + 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) @@ -41,23 +42,22 @@ def run(processed_source) 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 - if 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 - if banned_text?(child_node.children.join) - tag = BetterHtml::Tree::Tag.from_node(code_node) - generate_offense(self.class, processed_source, tag) - end + 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 end counter_correct?(processed_source) @@ -82,7 +82,7 @@ def autocorrect(processed_source, offense) 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 From 80ebd6bbbba39dd2dce94b1ce32038837bd288e8 Mon Sep 17 00:00:00 2001 From: Kate Higa <16447748+khiga8@users.noreply.github.com> Date: Thu, 9 Jun 2022 14:19:45 -0500 Subject: [PATCH 4/6] Update avoid_generic_link_text_counter.rb Add @lindseywild suggestion --- .../github/accessibility/avoid_generic_link_text_counter.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb b/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb index d283295..00930f2 100644 --- a/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb +++ b/lib/erblint-github/linters/github/accessibility/avoid_generic_link_text_counter.rb @@ -15,7 +15,8 @@ class AvoidGenericLinkTextCounter < Linter "Learn more", "Click here", "More", - "Link" + "Link", + "Here" ].freeze MESSAGE = "Avoid using generic link text such as #{BANNED_GENERIC_TEXT.join(', ')} which do not make sense in isolation." From 595f5a33be53d29c746e360598ba51469137b9d1 Mon Sep 17 00:00:00 2001 From: Kate Higa <16447748+khiga8@users.noreply.github.com> Date: Fri, 10 Jun 2022 07:47:35 -0700 Subject: [PATCH 5/6] Update docs/rules/accessibility/avoid-generic-link-text-counter.md Co-authored-by: Andri Alexandrou --- docs/rules/accessibility/avoid-generic-link-text-counter.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/accessibility/avoid-generic-link-text-counter.md b/docs/rules/accessibility/avoid-generic-link-text-counter.md index 7098f06..902c383 100644 --- a/docs/rules/accessibility/avoid-generic-link-text-counter.md +++ b/docs/rules/accessibility/avoid-generic-link-text-counter.md @@ -6,7 +6,7 @@ Avoid setting generic link text like, "Click here", "Read more", and "Learn more ## Resources -- [Primer: Links](https://primer.style/design/accessibility/link) +- [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) From c077f6d7d3b920add47290040c7ad2f0c216b69e Mon Sep 17 00:00:00 2001 From: Kate Higa <16447748+khiga8@users.noreply.github.com> Date: Fri, 10 Jun 2022 08:24:13 -0700 Subject: [PATCH 6/6] Update avoid-generic-link-text-counter.md --- .../accessibility/avoid-generic-link-text-counter.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/rules/accessibility/avoid-generic-link-text-counter.md b/docs/rules/accessibility/avoid-generic-link-text-counter.md index 902c383..e2fd646 100644 --- a/docs/rules/accessibility/avoid-generic-link-text-counter.md +++ b/docs/rules/accessibility/avoid-generic-link-text-counter.md @@ -2,7 +2,14 @@ ## 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. +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