Merge pull request #64 from github/kh-fix-landmark-logic

Fix and rename LandmarkHasLabel rule to NavigationHasLabel

Author: khiga8

README.md

@@ -35,7 +35,7 @@ linters:
     enabled: true
   GitHub::Accessibility::ImageHasAlt:
     enabled: true
-  GitHub::Accessibility::LandmarkHasLabel:
+  GitHub::Accessibility::NavigationHasLabel:
     enabled: true
   GitHub::Accessibility::LinkHasHref:
     enabled: true
@@ -61,7 +61,7 @@ linters:
 - [GitHub::Accessibility::AvoidBothDisabledAndAriaDisabled](./docs/rules/accessibility/avoid-both-disabled-and-aria-disabled.md)
 - [GitHub::Accessibility::AvoidGenericLinkText](./docs/rules/accessibility/avoid-generic-link-text.md)
 - [GitHub::Accessibility::DisabledAttribute](./docs/rules/accessibility/disabled-attribute.md)
-- [GitHub::Accessibility::LandmarkHasLabel](./docs/rules/accessibility/landmark-has-label.md)
+- [GitHub::Accessibility::NavigationHasLabel](./docs/rules/accessibility/navigation-has-label.md)
 - [GitHub::Accessibility::LinkHasHref](./docs/rules/accessibility/link-has-href.md)
 - [GitHub::Accessibility::NestedInteractiveElements](./docs/rules/accessibility/nested-interactive-elements.md)
 - [GitHub::Accessibility::IframeHasTitle](./docs/rules/accessibility/iframe-has-title.md)

docs/rules/accessibility/landmark-has-label.md

@@ -0,0 +1,30 @@
+# Navigation Has Label
+
+## Rule Details
+
+This rule enforces that a navigation landmark (a `<nav>` or a `role="navigation"`) has an accessible name. This rule is helpful to enforce for sites (like GitHub) where multiple navigation is common.
+
+The navigation landmark element should have an `aria-label` attribute, or `aria-labelledby` to distinguish it from other elements.
+
+## Resources
+
+- [ARIA Landmarks Example](https://www.w3.org/WAI/ARIA/apg/example-index/landmarks/index.html)
+
+## Examples
+### **Incorrect** code for this rule 👎
+
+```erb
+<!-- incorrect -->
+<nav>
+  <h1>This is a text</h1>
+</nav>
+```
+
+### **Correct** code for this rule  👍
+
+```erb
+<!-- correct -->
+<nav aria-labelledby="title_id"t>
+  <h1 id="title_id">This is a text</h1>
+</nav>
+```

docs/rules/accessibility/navigation-has-label.md

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+require_relative "../../custom_helpers"
+
+module ERBLint
+  module Linters
+    module GitHub
+      module Accessibility
+        class NavigationHasLabel < Linter
+          include ERBLint::Linters::CustomHelpers
+          include LinterRegistry
+
+          MESSAGE = "The navigation landmark should have a unique accessible name via `aria-label` or `aria-labelledby`. Remember that the name does not need to include `navigation` or `nav` since it will already be announced."
+
+          def run(processed_source)
+            tags(processed_source).each do |tag|
+              next if tag.closing?
+              next unless possible_attribute_values(tag, "role").include?("navigation") || tag.name == "nav"
+              if possible_attribute_values(tag, "aria-label").empty? && possible_attribute_values(tag, "aria-labelledby").empty?
+                message = MESSAGE
+                if tag.name != "nav"
+                  message += "Additionally, you can safely drop the `role='navigation'` and replace it with the native HTML `nav` element."
+                end
+                generate_offense(self.class, processed_source, tag, message)
+              end
+
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/erblint-github/linters/github/accessibility/landmark_has_label.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+class NavigationHasLabelTest < LinterTestCase
+  def linter_class
+    ERBLint::Linters::GitHub::Accessibility::NavigationHasLabel
+  end
+
+  def test_warns_if_navigation_landmark_has_no_label
+    @file = <<~ERB
+      <nav>
+      </nav>
+    ERB
+    @linter.run(processed_source)
+
+    assert_equal(1, @linter.offenses.count)
+    assert_match(/The navigation landmark should have a unique accessible name via `aria-label` or `aria-labelledby`./, @linter.offenses.first.message)
+  end
+
+  def test_warns_if_navigation_role_landmark_has_no_label
+    @file = <<~ERB
+      <div role="navigation">
+      </div>
+    ERB
+    @linter.run(processed_source)
+
+    assert_equal(1, @linter.offenses.count)
+    assert_match(/The navigation landmark should have a unique accessible name via `aria-label` or `aria-labelledby`./, @linter.offenses.first.message)
+  end
+
+  def test_does_not_warn_if_navigation_landmark_has_aria_labelled_by
+    @file = <<~ERB
+      <nav aria-labelledby="title_id"t>
+      </nav>
+    ERB
+    @linter.run(processed_source)
+
+    assert_empty @linter.offenses
+  end
+
+  def test_does_not_warn_if_navigation_landmark_has_aria_label
+    @file = <<~ERB
+      <nav aria-label="Repos"t>
+      </nav>
+    ERB
+    @linter.run(processed_source)
+
+    assert_empty @linter.offenses
+  end
+end