Merge pull request #34 from github/kh-refine-generic-text-logic

khiga8 · web-flow · commit 8076348f36ed · 2022-06-20T07:55:18.000-07:00
Refine generic link text linter logic
diff --git 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
+<a href="..." aria-label="Learn more about GitHub pricing plans">Learn more</a>
+```
+
+Inaccessible 🚫
+```html
+<a href="..." aria-label="GitHub pricing plans">Learn more</a>
+```
+
+Including the visible text in the ARIA label satisfies [SC 2.5.3: Label in Name](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
+<!-- also bad -->
+<a href="github.com/about" aria-label="Why dogs are awesome">Read more</a>
+```
+
+```erb
+<!-- also bad. `aria-describedby` does not count towards accessible name of an element-->
+<a href="github.com/about" aria-describedby="element123">Read more</a>
+```
+
 ### **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
 <a href="github.com/new">Create a new repository</a>
 ```
+
+```erb
+<!-- not ideal but won't be flagged -->
+<a aria-label="Learn more about GitHub" href="github.com/about">Learn more</a>
+```
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
@@ -18,13 +18,16 @@ 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)
             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]
@@ -36,29 +39,58 @@ 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)
                 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)
+              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
@@ -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
+      <a aria-label="<%= tooltip_text %>">Learn more</a>
+    ERB
+    @linter.run(processed_source)
+
+    assert_empty @linter.offenses
+  end
+
+  def test_flags_when_aria_label_does_not_include_visible_link_text
+    @file = <<~ERB
+      <a aria-label="GitHub Sponsors">Learn more</a>
+    ERB
+    @linter.run(processed_source)
+
+    refute_empty @linter.offenses
+  end
+
+  def test_does_not_flag_when_aria_label_includes_visible_link_text
+    @file = <<~ERB
+      <a aria-label="Learn more about GitHub Sponsors">Learn more</a>
+    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 = "<a aria-labelledby='someElement'>Click here</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') %>"
+    @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
+      <p>
+        <a href="github.com" aria-label='Click here to learn more'>Click here</a>
+        <%= link_to "learn more", billing_path, "aria-label": "something" %>
+        <a href="github.com" aria-label='Some totally different text'>Click here</a>
+        <a href="github.com" aria-labelledby='someElement'>Click here</a>
+      </p>
+      <p>
+        <%= 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" %>
+      </p>
+    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 %>
-      <a>Link</a>
+      <p>
+        <a href="github.com" aria-label='Click here to learn more'>Click here</a>
+        <a href="github.com" aria-label='Some totally different text'>Click here</a>
+        <a href="github.com" aria-labelledby='someElement'>Click here</a>
+      </p>
+      <p>
+        <%= 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" %>
+      </p>
     ERB
     refute_equal @file, corrected_content
 
     expected_content = <<~ERB
-      <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 1 %>
-      <a>Link</a>
+      <%# erblint:counter GitHub::Accessibility::AvoidGenericLinkTextCounter 3 %>
+      <p>
+        <a href="github.com" aria-label='Click here to learn more'>Click here</a>
+        <a href="github.com" aria-label='Some totally different text'>Click here</a>
+        <a href="github.com" aria-labelledby='someElement'>Click here</a>
+      </p>
+      <p>
+        <%= 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" %>
+      </p>
     ERB
     assert_equal expected_content, corrected_content
   end
