From 1b233febc89a7b53fd9d8356f94d78df37764437 Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Fri, 17 Jun 2022 14:04:04 -0700 Subject: [PATCH 01/11] Add logic to consider ARIA label attributes --- .../avoid_generic_link_text_counter.rb | 42 +++- .../avoid_generic_link_text_counter_test.rb | 187 ++++++++++++------ 2 files changed, 169 insertions(+), 60 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 00930f2..c243bcb 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 @@ -36,8 +36,15 @@ def run(processed_source) prev_node_tag = BetterHtml::Tree::Tag.from_node(prev_node) next_node_tag = BetterHtml::Tree::Tag.from_node(next_node) + aria_label = possible_attribute_values(prev_node_tag, "aria-label") + aria_labelledby = possible_attribute_values(prev_node_tag, "aria-labelledby") + # 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? + # Cannot statically check label from aria-labelledby or label from variable aria-label so we skip + next if aria_labelledby.present? || (aria_label.present? && aria_label.join.include?("<%=")) + 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) @@ -54,11 +61,34 @@ def run(processed_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| - 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) + banned_text = child_node.children.join if child_node.methods.include?(:type) && child_node.type == :str && banned_text?(child_node.children.join) + + next unless banned_text.present? && 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) + + # Don't flag if link has aria-labelledby which we cannot evaluate with ERB alone. + banned_text = nil if pair_node.children.first.children.join == "aria-labelledby" || pair_node.children.first.children.join == "aria-label" + next unless pair_node.children.first.children.join == "aria-label" + + aria_label_value_node = pair_node.children[1] + if aria_label_value_node.type == :str + binding.irb + aria_label_text = aria_label_value_node.children.join + banned_text = nil if valid_accessible_name?(aria_label_text, banned_text) + else + # Don't flag if aria-label is not string (e.g. is a variable) since we cannot evaluate with ERB alone. + banned_text = nil end + end + end + if banned_text.present? + tag = BetterHtml::Tree::Tag.from_node(code_node) + generate_offense(self.class, processed_source, tag) end end counter_correct?(processed_source) @@ -84,6 +114,12 @@ def banned_text?(text) BANNED_GENERIC_TEXT.map(&:downcase).include?(text.downcase) end + # We flag if accessible name does not start with visible text. + # Related: Success Criteria 2.5.3 + def valid_accessible_name?(accessible_name, visible_text) + accessible_name.downcase.start_with?(visible_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..bbb6a7d 100644 --- a/test/linters/accessibility/avoid_generic_link_text_counter_test.rb +++ b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb @@ -7,83 +7,156 @@ 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) + # def test_warns_when_link_text_is_click_here + # @file = "Click here" + # @linter.run(processed_source) - refute_empty @linter.offenses - end + # refute_empty @linter.offenses + # end - def test_warns_when_link_text_is_learn_more - @file = "Learn more" - @linter.run(processed_source) + # def test_warns_when_link_text_is_learn_more + # @file = "Learn more" + # @linter.run(processed_source) - refute_empty @linter.offenses - end + # refute_empty @linter.offenses + # end - def test_warns_when_link_text_is_read_more - @file = "Read more" - @linter.run(processed_source) + # def test_warns_when_link_text_is_read_more + # @file = "Read more" + # @linter.run(processed_source) - refute_empty @linter.offenses - end + # refute_empty @linter.offenses + # end - def test_warns_when_link_text_is_more - @file = "More" - @linter.run(processed_source) + # def test_warns_when_link_text_is_more + # @file = "More" + # @linter.run(processed_source) - refute_empty @linter.offenses - end + # refute_empty @linter.offenses + # end - def test_warns_when_link_text_is_link - @file = "Link" - @linter.run(processed_source) + # def test_warns_when_link_text_is_link + # @file = "Link" + # @linter.run(processed_source) - refute_empty @linter.offenses - end + # 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) + # 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 + # 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) + # def test_ignores_when_aria_label_with_variable_is_set_on_link_tag + # @file = <<~ERB + # Learn more + # ERB + # @linter.run(processed_source) - refute_empty @linter.offenses - end + # assert_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) + # def test_flags_when_aria_label_does_not_include_visible_link_text + # @file = <<~ERB + # Learn more + # ERB + # @linter.run(processed_source) - assert_empty @linter.offenses - end + # refute_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) + # def test_does_not_flag_when_aria_label_includes_visible_link_text + # @file = <<~ERB + # Learn more + # ERB + # @linter.run(processed_source) - assert_equal 0, @linter.offenses.count - end + # assert_empty @linter.offenses + # end + + # def test_ignores_when_aria_labelledby_is_set_on_link_tag + # @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') %>" + # @linter.run(processed_source) + + # refute_empty @linter.offenses + # end - def test_autocorrects_when_ignores_are_not_correct + # def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_labelled_by + # @file = "<%= link_to('learn more', 'aria-labelledby': 'element1234', id: 'redirect') %>" + # @linter.run(processed_source) + + # assert_empty @linter.offenses + # end + + # def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_that_cannot_be_parsed_by_linter + # @file = <<~ERB + # <%= link_to('learn more', 'aria-label': some_variable, id: 'redirect') %> + # ERB + # @linter.run(processed_source) + + # assert_empty @linter.offenses + # end + + def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_that_cannot_be_parsed_by_linter_because_interpolated @file = <<~ERB - <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 2 %> - Link + <%= link_to('learn more', 'aria-label': "Learn #{@variable}", id: 'redirect') %> ERB - refute_equal @file, corrected_content + @linter.run(processed_source) - expected_content = <<~ERB - <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 1 %> - Link - ERB - assert_equal expected_content, corrected_content + assert_empty @linter.offenses end + + # def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_that_contains_visible_text + # @file = "<%= link_to('learn more', 'aria-label': 'learn more about GitHub', id: 'redirect') %>" + # @linter.run(processed_source) + + # assert_empty @linter.offenses + # end + + # def test_warns_when_link_rails_helper_text_is_banned_text_with_aria_label_hash + # @file = "<%= link_to('learn more', aria: { label: 'learn more about GitHub' }, id: 'redirect') %>" + # @linter.run(processed_source) + + # assert_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 6ee53c241da87f20466872131011354cd80d190e Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Fri, 17 Jun 2022 15:06:23 -0700 Subject: [PATCH 02/11] Do not evaluate link_to helpers with ARIA label attributes --- .../avoid_generic_link_text_counter.rb | 39 ++-- .../avoid_generic_link_text_counter_test.rb | 218 +++++++++--------- 2 files changed, 130 insertions(+), 127 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 c243bcb..14362d2 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 @@ -25,6 +25,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] @@ -39,10 +40,12 @@ def run(processed_source) aria_label = possible_attribute_values(prev_node_tag, "aria-label") aria_labelledby = possible_attribute_values(prev_node_tag, "aria-labelledby") - # We only report if the text is nested between two link tags. + # Checks if nested between two link tags. if link_tag?(prev_node_tag) && link_tag?(next_node_tag) && next_node_tag.closing? - # Cannot statically check label from aria-labelledby or label from variable aria-label so we skip + # We skip because we cannot reliably check accessible name given 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?("<%=")) + # We skip because aria-label contains visible text. Relates 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 @@ -71,19 +74,16 @@ def run(processed_source) child_node.descendants(:pair).each do |pair_node| next unless pair_node.children.first.type?(:sym) - # Don't flag if link has aria-labelledby which we cannot evaluate with ERB alone. - banned_text = nil if pair_node.children.first.children.join == "aria-labelledby" || pair_node.children.first.children.join == "aria-label" - next unless pair_node.children.first.children.join == "aria-label" - - aria_label_value_node = pair_node.children[1] - if aria_label_value_node.type == :str - binding.irb - aria_label_text = aria_label_value_node.children.join - banned_text = nil if valid_accessible_name?(aria_label_text, banned_text) - else - # Don't flag if aria-label is not string (e.g. is a variable) since we cannot evaluate with ERB alone. - banned_text = nil + if pair_node.children.first.children.join == "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 + + # Skip if `link_to` has `aria-labelledby` or `aria-label` which we 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 + # be `Learn` which is unreliable so we have to skip entirely unlike with HTML :( + banned_text = nil if pair_node.children.first.children.join == "aria-labelledby" || pair_node.children.first.children.join == "aria-label" end end if banned_text.present? @@ -114,10 +114,13 @@ def banned_text?(text) BANNED_GENERIC_TEXT.map(&:downcase).include?(text.downcase) end - # We flag if accessible name does not start with visible text. - # Related: Success Criteria 2.5.3 - def valid_accessible_name?(accessible_name, visible_text) - accessible_name.downcase.start_with?(visible_text.downcase) + def valid_accessible_name?(aria_label, text) + aria_label.start_with?(text) + end + + # Checks if Ruby node contains `aria-label` or `aria-labelledby` + def ruby_node_contains_aria_label_attributes?(pair_node) + pair_node.children.first.children.join == "aria-labelledby" || pair_node.children.first.children.join == "aria-label" end def extract_ruby_node(source) 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 bbb6a7d..1da2388 100644 --- a/test/linters/accessibility/avoid_generic_link_text_counter_test.rb +++ b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb @@ -7,106 +7,106 @@ 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) + def test_warns_when_link_text_is_click_here + @file = "Click here" + @linter.run(processed_source) - # refute_empty @linter.offenses - # end + refute_empty @linter.offenses + end - # def test_warns_when_link_text_is_learn_more - # @file = "Learn more" - # @linter.run(processed_source) + def test_warns_when_link_text_is_learn_more + @file = "Learn more" + @linter.run(processed_source) - # refute_empty @linter.offenses - # end + refute_empty @linter.offenses + end - # def test_warns_when_link_text_is_read_more - # @file = "Read more" - # @linter.run(processed_source) + def test_warns_when_link_text_is_read_more + @file = "Read more" + @linter.run(processed_source) - # refute_empty @linter.offenses - # end + refute_empty @linter.offenses + end - # def test_warns_when_link_text_is_more - # @file = "More" - # @linter.run(processed_source) + def test_warns_when_link_text_is_more + @file = "More" + @linter.run(processed_source) - # refute_empty @linter.offenses - # end + refute_empty @linter.offenses + end - # def test_warns_when_link_text_is_link - # @file = "Link" - # @linter.run(processed_source) + def test_warns_when_link_text_is_link + @file = "Link" + @linter.run(processed_source) - # refute_empty @linter.offenses - # end + 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) + 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 + 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) + 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 + 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) + 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 + 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) + 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 + assert_empty @linter.offenses + end - # def test_ignores_when_aria_labelledby_is_set_on_link_tag - # @file = "Click here" - # @linter.run(processed_source) + def test_ignores_when_aria_labelledby_is_set_on_link_tag + @file = "Click here" + @linter.run(processed_source) - # assert_empty @linter.offenses - # end + 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) + 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 + refute_empty @linter.offenses + end - # def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_labelled_by - # @file = "<%= link_to('learn more', 'aria-labelledby': 'element1234', id: 'redirect') %>" - # @linter.run(processed_source) + def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_labelled_by + @file = "<%= link_to('learn more', 'aria-labelledby': 'element1234', id: 'redirect') %>" + @linter.run(processed_source) - # assert_empty @linter.offenses - # end + assert_empty @linter.offenses + end - # def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_that_cannot_be_parsed_by_linter - # @file = <<~ERB - # <%= link_to('learn more', 'aria-label': some_variable, id: 'redirect') %> - # ERB - # @linter.run(processed_source) + def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_that_cannot_be_parsed_by_linter + @file = <<~ERB + <%= link_to('learn more', 'aria-label': some_variable, id: 'redirect') %> + ERB + @linter.run(processed_source) - # assert_empty @linter.offenses - # end + assert_empty @linter.offenses + end - def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_that_cannot_be_parsed_by_linter_because_interpolated + def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_since_cannot_be_parsed_accurately_by_linter @file = <<~ERB <%= link_to('learn more', 'aria-label': "Learn #{@variable}", id: 'redirect') %> ERB @@ -115,48 +115,48 @@ def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_that assert_empty @linter.offenses end - # def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_that_contains_visible_text - # @file = "<%= link_to('learn more', 'aria-label': 'learn more about GitHub', id: 'redirect') %>" - # @linter.run(processed_source) + def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label + @file = "<%= link_to('learn more', 'aria-label': 'learn more about GitHub', id: 'redirect') %>" + @linter.run(processed_source) - # assert_empty @linter.offenses - # end + assert_empty @linter.offenses + end - # def test_warns_when_link_rails_helper_text_is_banned_text_with_aria_label_hash - # @file = "<%= link_to('learn more', aria: { label: 'learn more about GitHub' }, id: 'redirect') %>" - # @linter.run(processed_source) + def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_hash + @file = "<%= link_to('learn more', aria: { label: 'learn more about GitHub' }, id: 'redirect') %>" + @linter.run(processed_source) - # assert_empty @linter.offenses - # end + assert_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) + 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 + 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) + 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 + 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 + 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 + expected_content = <<~ERB + <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 1 %> + Link + ERB + assert_equal expected_content, corrected_content + end end From af0ab513fcc4e229911e2e377a53cad526295cc6 Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Fri, 17 Jun 2022 15:17:32 -0700 Subject: [PATCH 03/11] Add test case to ignore only if label attributes and not description --- .../accessibility/avoid_generic_link_text_counter.rb | 5 +++-- .../avoid_generic_link_text_counter_test.rb | 9 +++++++++ 2 files changed, 12 insertions(+), 2 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 14362d2..8cb7498 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 @@ -68,8 +68,9 @@ def run(processed_source) 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 banned_text.present? && child_node.methods.include?(:type) && child_node.type == :hash + 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) @@ -82,7 +83,7 @@ def run(processed_source) # Skip if `link_to` has `aria-labelledby` or `aria-label` which we 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 - # be `Learn` which is unreliable so we have to skip entirely unlike with HTML :( + # only be `Learn` which is unreliable so we can't do checks :( banned_text = nil if pair_node.children.first.children.join == "aria-labelledby" || pair_node.children.first.children.join == "aria-label" 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 index 1da2388..9daf10c 100644 --- a/test/linters/accessibility/avoid_generic_link_text_counter_test.rb +++ b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb @@ -90,6 +90,15 @@ def test_warns_when_link_rails_helper_text_is_banned_text refute_empty @linter.offenses end + def test_warns_when_link_rails_helper_text_is_banned_text_with_aria_description + @file = <<~ERB + <%= link_to('click here', '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_aria_labelled_by @file = "<%= link_to('learn more', 'aria-labelledby': 'element1234', id: 'redirect') %>" @linter.run(processed_source) From 29dc51684d66b0debf80d8176b2ae9d6e9221648 Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Fri, 17 Jun 2022 15:22:15 -0700 Subject: [PATCH 04/11] Update test code and comments --- .../avoid_generic_link_text_counter.rb | 6 +++--- .../avoid_generic_link_text_counter_test.rb | 14 +++++++------- 2 files changed, 10 insertions(+), 10 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 8cb7498..dcb1e10 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 @@ -42,10 +42,10 @@ def run(processed_source) # Checks if nested between two link tags. if link_tag?(prev_node_tag) && link_tag?(next_node_tag) && next_node_tag.closing? - # We skip because we cannot reliably check accessible name given aria-labelledby, or an aria-label that is set to a variable + # 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?("<%=")) - # We skip because aria-label contains visible text. Relates to Success Criterion 2.5.3: Label in Name + # 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 @@ -81,7 +81,7 @@ def run(processed_source) end end - # Skip if `link_to` has `aria-labelledby` or `aria-label` which we cannot be evaluated accurately with ERB lint alone. + # Skips if `link_to` has `aria-labelledby` or `aria-label` which we 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 :( banned_text = nil if pair_node.children.first.children.join == "aria-labelledby" || pair_node.children.first.children.join == "aria-label" 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 9daf10c..3ed4a38 100644 --- a/test/linters/accessibility/avoid_generic_link_text_counter_test.rb +++ b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb @@ -84,7 +84,7 @@ def test_ignores_when_aria_labelledby_is_set_on_link_tag 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 @@ -92,7 +92,7 @@ def test_warns_when_link_rails_helper_text_is_banned_text def test_warns_when_link_rails_helper_text_is_banned_text_with_aria_description @file = <<~ERB - <%= link_to('click here', 'aria-describedby': 'element123', id: 'redirect') %> + <%= link_to('click here', redirect_url, 'aria-describedby': 'element123', id: 'redirect') %> ERB @linter.run(processed_source) @@ -100,7 +100,7 @@ def test_warns_when_link_rails_helper_text_is_banned_text_with_aria_description end def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_labelled_by - @file = "<%= link_to('learn more', 'aria-labelledby': 'element1234', id: 'redirect') %>" + @file = "<%= link_to('learn more', redirect_url, 'aria-labelledby': 'element1234', id: 'redirect') %>" @linter.run(processed_source) assert_empty @linter.offenses @@ -108,7 +108,7 @@ def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_labelled_b def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_that_cannot_be_parsed_by_linter @file = <<~ERB - <%= link_to('learn more', 'aria-label': some_variable, id: 'redirect') %> + <%= link_to('learn more', redirect_url, 'aria-label': some_variable, id: 'redirect') %> ERB @linter.run(processed_source) @@ -117,7 +117,7 @@ def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_that def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_since_cannot_be_parsed_accurately_by_linter @file = <<~ERB - <%= link_to('learn more', 'aria-label': "Learn #{@variable}", id: 'redirect') %> + <%= link_to('learn more', redirect_url, 'aria-label': "Learn #{@variable}", id: 'redirect') %> ERB @linter.run(processed_source) @@ -125,14 +125,14 @@ def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_sinc end def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label - @file = "<%= link_to('learn more', 'aria-label': 'learn more about GitHub', id: 'redirect') %>" + @file = "<%= link_to('learn more', redirect_url, 'aria-label': 'learn more about GitHub', id: 'redirect') %>" @linter.run(processed_source) assert_empty @linter.offenses end def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_hash - @file = "<%= link_to('learn more', aria: { label: 'learn more about GitHub' }, id: 'redirect') %>" + @file = "<%= link_to('learn more', redirect_url, aria: { label: 'learn more about GitHub' }, id: 'redirect') %>" @linter.run(processed_source) assert_empty @linter.offenses From e5adc3b484042b134b6fe0643f2eb937bb012844 Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Fri, 17 Jun 2022 15:54:34 -0700 Subject: [PATCH 05/11] Make sure we're iterating through all ERB nodes --- .../avoid_generic_link_text_counter.rb | 54 +++++++++---------- .../avoid_generic_link_text_counter_test.rb | 18 +++++++ 2 files changed, 45 insertions(+), 27 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 dcb1e10..fbc0a0c 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 @@ -55,41 +55,41 @@ 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 + 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 - _, _, 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 - 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? - 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 - 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) - child_node.descendants(:pair).each do |pair_node| - next unless pair_node.children.first.type?(:sym) - - if pair_node.children.first.children.join == "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" + if pair_node.children.first.children.join == "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 - # Skips if `link_to` has `aria-labelledby` or `aria-label` which we 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 :( - banned_text = nil if pair_node.children.first.children.join == "aria-labelledby" || pair_node.children.first.children.join == "aria-label" + # Skips if `link_to` has `aria-labelledby` or `aria-label` which we 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 :( + banned_text = nil if pair_node.children.first.children.join == "aria-labelledby" || pair_node.children.first.children.join == "aria-label" + end end - end - if banned_text.present? - tag = BetterHtml::Tree::Tag.from_node(code_node) - generate_offense(self.class, processed_source, tag) + if banned_text.present? + tag = BetterHtml::Tree::Tag.from_node(code_node) + generate_offense(self.class, processed_source, tag) + end + banned_text = nil end end counter_correct?(processed_source) 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 3ed4a38..5bbba8f 100644 --- a/test/linters/accessibility/avoid_generic_link_text_counter_test.rb +++ b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb @@ -145,6 +145,24 @@ def test_does_not_warn_when_generic_text_is_link_rails_helper_sub_text 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" %> + <%= 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 + assert_equal 3, @linter.offenses.count + end + def test_does_not_warns_if_element_has_correct_counter_comment @file = <<~ERB <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 1 %> From 848ce2e09fa83d9d3b5bfa5980091ae73130f731 Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Fri, 17 Jun 2022 16:17:12 -0700 Subject: [PATCH 06/11] Update docs --- .../avoid-generic-link-text-counter.md | 28 +++++++++++++++++++ .../avoid_generic_link_text_counter_test.rb | 7 +++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/docs/rules/accessibility/avoid-generic-link-text-counter.md b/docs/rules/accessibility/avoid-generic-link-text-counter.md index e2fd646..44e6a5d 100644 --- a/docs/rules/accessibility/avoid-generic-link-text-counter.md +++ b/docs/rules/accessibility/avoid-generic-link-text-counter.md @@ -11,6 +11,24 @@ 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 + +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: +``` +Read more +``` + +This is acceptable: +``` +Read more +``` + +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. + ## Resources - [Primer: Links](https://primer.style/design/accessibility/links) @@ -43,6 +61,11 @@ Learn more about how to write descriptive link text at [Access Guide: Write desc <%= link_to "Learn more", "#" %> ``` +```erb + +Read more +``` + ### **Correct** code for this rule 👍 ```erb @@ -52,3 +75,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/test/linters/accessibility/avoid_generic_link_text_counter_test.rb b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb index 5bbba8f..f223025 100644 --- a/test/linters/accessibility/avoid_generic_link_text_counter_test.rb +++ b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb @@ -148,7 +148,9 @@ def test_does_not_warn_when_generic_text_is_link_rails_helper_sub_text def test_handles_files_with_various_links @file = <<~ERB

- Click here + Click here + Click here + Click here

<%= link_to "learn more", billing_path, "aria-label": "something" %> @@ -160,7 +162,8 @@ def test_handles_files_with_various_links @linter.run(processed_source) refute_empty @linter.offenses - assert_equal 3, @linter.offenses.count + # 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 From f40b8dd5be04aa0bb677b22ed4e96f30dfa76cef Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Fri, 17 Jun 2022 16:37:00 -0700 Subject: [PATCH 07/11] Switch to include? instead of start_with? since it's not technically a requirement though recommended --- .../github/accessibility/avoid_generic_link_text_counter.rb | 2 +- 1 file changed, 1 insertion(+), 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 fbc0a0c..da0f671 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 @@ -116,7 +116,7 @@ def banned_text?(text) end def valid_accessible_name?(aria_label, text) - aria_label.start_with?(text) + aria_label.downcase.include?(text.downcase) end # Checks if Ruby node contains `aria-label` or `aria-labelledby` From 1c3a86726804b8bafe079efd53d66ce47ebc89b6 Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Fri, 17 Jun 2022 16:59:47 -0700 Subject: [PATCH 08/11] Clean up code a bit --- .../avoid-generic-link-text-counter.md | 5 ++++ .../avoid_generic_link_text_counter.rb | 24 +++++++---------- .../avoid_generic_link_text_counter_test.rb | 27 ++++++++++++++++--- 3 files changed, 38 insertions(+), 18 deletions(-) diff --git a/docs/rules/accessibility/avoid-generic-link-text-counter.md b/docs/rules/accessibility/avoid-generic-link-text-counter.md index 44e6a5d..c9e9720 100644 --- a/docs/rules/accessibility/avoid-generic-link-text-counter.md +++ b/docs/rules/accessibility/avoid-generic-link-text-counter.md @@ -66,6 +66,11 @@ This linter will raise a flag when it is able to detect that a generic link has Read more ``` +```erb + +Read more +``` + ### **Correct** code for this rule 👍 ```erb 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 da0f671..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) @@ -67,22 +69,21 @@ def run(processed_source) 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) - if pair_node.children.first.children.join == "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 - - # Skips if `link_to` has `aria-labelledby` or `aria-label` which we cannot be evaluated accurately with ERB lint alone. + # 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 :( - banned_text = nil if pair_node.children.first.children.join == "aria-labelledby" || pair_node.children.first.children.join == "aria-label" + 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? @@ -119,11 +120,6 @@ def valid_accessible_name?(aria_label, text) aria_label.downcase.include?(text.downcase) end - # Checks if Ruby node contains `aria-label` or `aria-labelledby` - def ruby_node_contains_aria_label_attributes?(pair_node) - pair_node.children.first.children.join == "aria-labelledby" || pair_node.children.first.children.join == "aria-label" - 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 f223025..dd2539b 100644 --- a/test/linters/accessibility/avoid_generic_link_text_counter_test.rb +++ b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb @@ -178,14 +178,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 From 1968894479a8a173ce4aa1a04d3cfac73a37f974 Mon Sep 17 00:00:00 2001 From: Kate Higa Date: Fri, 17 Jun 2022 17:11:33 -0700 Subject: [PATCH 09/11] Combine multiple tests to one --- .../avoid_generic_link_text_counter_test.rb | 45 +++---------------- 1 file changed, 6 insertions(+), 39 deletions(-) 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 dd2539b..966de85 100644 --- a/test/linters/accessibility/avoid_generic_link_text_counter_test.rb +++ b/test/linters/accessibility/avoid_generic_link_text_counter_test.rb @@ -76,7 +76,7 @@ def test_does_not_flag_when_aria_label_includes_visible_link_text assert_empty @linter.offenses end - def test_ignores_when_aria_labelledby_is_set_on_link_tag + 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) @@ -99,61 +99,28 @@ def test_warns_when_link_rails_helper_text_is_banned_text_with_aria_description refute_empty @linter.offenses end - def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_labelled_by - @file = "<%= link_to('learn more', redirect_url, 'aria-labelledby': 'element1234', id: 'redirect') %>" - @linter.run(processed_source) - - assert_empty @linter.offenses - end - - def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_that_cannot_be_parsed_by_linter + 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') %> - ERB - @linter.run(processed_source) - - assert_empty @linter.offenses - end - - def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_since_cannot_be_parsed_accurately_by_linter - @file = <<~ERB <%= 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_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label - @file = "<%= link_to('learn more', redirect_url, 'aria-label': 'learn more about GitHub', id: 'redirect') %>" - @linter.run(processed_source) - - assert_empty @linter.offenses - end - - def test_ignores_when_link_rails_helper_text_is_banned_text_with_aria_label_hash - @file = "<%= link_to('learn more', redirect_url, aria: { label: 'learn more about GitHub' }, id: 'redirect') %>" - @linter.run(processed_source) - - assert_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_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: { label: "something" } %> <%= link_to "learn more", billing_path, aria: { describedby: "element123" } %> <%= link_to "learn more", billing_path, "aria-describedby": "element123" %> From 401496b912b1d6fbcf4bfa1254c816ea44da9734 Mon Sep 17 00:00:00 2001 From: Kate Higa <16447748+khiga8@users.noreply.github.com> Date: Mon, 20 Jun 2022 07:37:52 -0700 Subject: [PATCH 10/11] Update avoid-generic-link-text-counter.md Co-authored-by: Andri Alexandrou --- .../avoid-generic-link-text-counter.md | 30 ++++++++++++------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/docs/rules/accessibility/avoid-generic-link-text-counter.md b/docs/rules/accessibility/avoid-generic-link-text-counter.md index c9e9720..c22976f 100644 --- a/docs/rules/accessibility/avoid-generic-link-text-counter.md +++ b/docs/rules/accessibility/avoid-generic-link-text-counter.md @@ -11,23 +11,31 @@ 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. -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. +For example, on a pricing plans page, the following are good: +- Visible text: `Learn more` +- Accessible label: `Learn more about GitHub pricing plans` -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: -``` -Read more +Accessible ✅ +```html +Learn more ``` -This is acceptable: -``` -Read more +Inaccessible 🚫 +```html +Learn more ``` -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. +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 From dc663188510735a4ca2f25a682e19d024863988b Mon Sep 17 00:00:00 2001 From: Kate Higa <16447748+khiga8@users.noreply.github.com> Date: Mon, 20 Jun 2022 07:38:28 -0700 Subject: [PATCH 11/11] Update avoid-generic-link-text-counter.md --- docs/rules/accessibility/avoid-generic-link-text-counter.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/rules/accessibility/avoid-generic-link-text-counter.md b/docs/rules/accessibility/avoid-generic-link-text-counter.md index c22976f..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,8 @@ 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: