diff --git a/docs/rules/accessibility/avoid-generic-link-text-counter.md b/docs/rules/accessibility/avoid-generic-link-text-counter.md index e2fd646..773c362 100644 --- a/docs/rules/accessibility/avoid-generic-link-text-counter.md +++ b/docs/rules/accessibility/avoid-generic-link-text-counter.md @@ -11,6 +11,34 @@ Additionally, generic link text can also problematic for heavy zoom users where 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) +### Use of ARIA attributes + +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 ✅ +```html +Learn more +``` + +Inaccessible 🚫 +```html +Learn more +``` + +Including the visible text in the ARIA label satisfies [SC 2.5.3: Label in Name](https://www.w3.org/WAI/WCAG21/Understanding/label-in-name.html). + +#### 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-label` is set dynamically +- when using `aria-labelledby` + ## Resources - [Primer: Links](https://primer.style/design/accessibility/links) @@ -43,6 +71,16 @@ Learn more about how to write descriptive link text at [Access Guide: Write desc <%= link_to "Learn more", "#" %> ``` +```erb + +Read more +``` + +```erb + +Read more +``` + ### **Correct** code for this rule 👍 ```erb @@ -52,3 +90,8 @@ Learn more about how to write descriptive link text at [Access Guide: Write desc ```erb Create a new repository ``` + +```erb + +Learn more +``` 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 00930f2..ad97d0c 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 @@ -18,6 +18,8 @@ class AvoidGenericLinkTextCounter < Linter "Link", "Here" ].freeze + ARIA_LABEL_ATTRIBUTES = %w[aria-labelledby aria-label].freeze + MESSAGE = "Avoid using generic link text such as #{BANNED_GENERIC_TEXT.join(', ')} which do not make sense in isolation." def run(processed_source) @@ -25,6 +27,7 @@ def run(processed_source) 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] @@ -36,8 +39,17 @@ def run(processed_source) 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. + aria_label = possible_attribute_values(prev_node_tag, "aria-label") + aria_labelledby = possible_attribute_values(prev_node_tag, "aria-labelledby") + + # Checks if nested between two link tags. 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?("<%=")) + # Skip because aria-label starts with visible text which we allow. Related to Success Criterion 2.5.3: Label in Name + next if aria_label.present? && valid_accessible_name?(aria_label.join, text) + 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) @@ -45,20 +57,40 @@ def run(processed_source) 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) + node.descendants(:erb).each do |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 + + banned_text = nil + + send_node.child_nodes.each do |child_node| + banned_text = child_node.children.join if child_node.methods.include?(:type) && child_node.type == :str && banned_text?(child_node.children.join) + next if banned_text.blank? + next unless child_node.methods.include?(:type) && child_node.type == :hash + + child_node.descendants(:pair).each do |pair_node| + next unless pair_node.children.first.type?(:sym) + + # Skips if `link_to` has `aria-labelledby` or `aria-label` which cannot be evaluated accurately with ERB lint alone. + # ERB lint removes Ruby string interpolation so the `aria-label` for "<%= link_to 'Learn more', "aria-label": "Learn #{@some_variable}" %>" will + # only be `Learn` which is unreliable so we can't do checks :( + key_value = pair_node.children.first.children.join + banned_text = nil if ARIA_LABEL_ATTRIBUTES.include?(key_value) + next unless key_value == "aria" + + pair_node.children[1].descendants(:sym).each do |sym_node| + banned_text = nil if sym_node.children.join == "label" || sym_node.children.join == "labelledby" + end + end + end + if banned_text.present? tag = BetterHtml::Tree::Tag.from_node(code_node) generate_offense(self.class, processed_source, tag) - end + end + banned_text = nil end end counter_correct?(processed_source) @@ -84,6 +116,10 @@ def banned_text?(text) BANNED_GENERIC_TEXT.map(&:downcase).include?(text.downcase) end + def valid_accessible_name?(aria_label, text) + aria_label.downcase.include?(text.downcase) + end + def extract_ruby_node(source) BetterHtml::TestHelper::RubyNode.parse(source) rescue ::Parser::SyntaxError diff --git a/test/linters/accessibility/avoid_generic_link_text_counter_test.rb b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb index 8a8411a..966de85 100644 --- a/test/linters/accessibility/avoid_generic_link_text_counter_test.rb +++ b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb @@ -49,20 +49,90 @@ def test_does_not_warn_when_banned_text_is_part_of_more_text assert_empty @linter.offenses end + def test_ignores_when_aria_label_with_variable_is_set_on_link_tag + @file = <<~ERB + Learn more + ERB + @linter.run(processed_source) + + assert_empty @linter.offenses + end + + def test_flags_when_aria_label_does_not_include_visible_link_text + @file = <<~ERB + Learn more + ERB + @linter.run(processed_source) + + refute_empty @linter.offenses + end + + def test_does_not_flag_when_aria_label_includes_visible_link_text + @file = <<~ERB + Learn more + ERB + @linter.run(processed_source) + + assert_empty @linter.offenses + end + + def test_ignores_when_aria_labelledby_is_set_on_link_tag_since_cannot_be_evaluated_accurately_by_erb_linter + @file = "Click here" + @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') %>" + @file = "<%= link_to('click here', redirect_url) %>" @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') %>" + def test_warns_when_link_rails_helper_text_is_banned_text_with_aria_description + @file = <<~ERB + <%= link_to('click here', redirect_url, 'aria-describedby': 'element123', id: 'redirect') %> + ERB + @linter.run(processed_source) + + refute_empty @linter.offenses + end + + def test_ignores_when_link_rails_helper_text_is_banned_text_with_any_aria_label_attributes_since_cannot_be_evaluated_accurately_by_erb_linter + @file = <<~ERB + <%= link_to('learn more', redirect_url, 'aria-labelledby': 'element1234', id: 'redirect') %> + <%= link_to('learn more', redirect_url, 'aria-label': some_variable, id: 'redirect') %> + <%= link_to('learn more', redirect_url, 'aria-label': "Learn #{@variable}", id: 'redirect') %> + <%= link_to('learn more', redirect_url, 'aria-label': 'learn more about GitHub', id: 'redirect') %> + <%= link_to('learn more', redirect_url, aria: { label: 'learn more about GitHub' }, id: 'redirect') %> + ERB @linter.run(processed_source) assert_empty @linter.offenses end + def test_handles_files_with_various_links + @file = <<~ERB +

+ Click here + <%= link_to "learn more", billing_path, "aria-label": "something" %> + Click here + Click here +

+

+ <%= link_to "learn more", billing_path, aria: { label: "something" } %> + <%= link_to "learn more", billing_path, aria: { describedby: "element123" } %> + <%= link_to "learn more", billing_path, "aria-describedby": "element123" %> +

+ ERB + @linter.run(processed_source) + + refute_empty @linter.offenses + # 3 offenses, 1 related to matching counter comment not present despite violations + assert_equal 4, @linter.offenses.count + end + def test_does_not_warns_if_element_has_correct_counter_comment @file = <<~ERB <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 1 %> @@ -75,14 +145,33 @@ def test_does_not_warns_if_element_has_correct_counter_comment def test_autocorrects_when_ignores_are_not_correct @file = <<~ERB - <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 2 %> - Link +

+ Click here + Click here + Click here +

+

+ <%= link_to "learn more", billing_path, "aria-label": "something" %> + <%= link_to "learn more", billing_path, aria: { label: "something" } %> + <%= link_to "learn more", billing_path, aria: { describedby: "element123" } %> + <%= link_to "learn more", billing_path, "aria-describedby": "element123" %> +

ERB refute_equal @file, corrected_content expected_content = <<~ERB - <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 1 %> - Link + <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 3 %> +

+ Click here + Click here + Click here +

+

+ <%= link_to "learn more", billing_path, "aria-label": "something" %> + <%= link_to "learn more", billing_path, aria: { label: "something" } %> + <%= link_to "learn more", billing_path, aria: { describedby: "element123" } %> + <%= link_to "learn more", billing_path, "aria-describedby": "element123" %> +

ERB assert_equal expected_content, corrected_content end