diff --git a/README.md b/README.md index 3a2cf2b..811b9f1 100644 --- a/README.md +++ b/README.md @@ -48,6 +48,7 @@ linters: - [GitHub::Accessibility::NoPositiveTabIndex](./docs/rules/accessibility/no-positive-tab-index.md) - [GitHub::Accessibility::NoRedundantImageAlt](./docs/rules/accessibility/no-redundant-image-alt.md) - [GitHub::Accessibility::NoTitleAttributeCounter](./docs/rules/accessibility/no-title-attribute-counter.md) +- [GitHub::Accessibility::DetailsHasSummary](./docs/rules/accessibility/details-has-summary.md) ## Testing diff --git a/docs/rules/accessibility/details-has-summary.md b/docs/rules/accessibility/details-has-summary.md new file mode 100644 index 0000000..bb671b0 --- /dev/null +++ b/docs/rules/accessibility/details-has-summary.md @@ -0,0 +1,35 @@ +# Details has summary + +## Rule Details +`
` should have a `` element hinting to the user what they'll be expanding by interfacing with the element. `` elements are helpful for screen reader users navigating the website. They are also a good UI pattern since a user agent adds their own `` if a developer omitted it with no context on what UI the details element will expand. + +### 👎 Examples of **incorrect** code for this rule: + +```erb +
+ I don't have a summary tag! +
+``` + +```erb +
+
Expand me!
+ I have a invalid summary tag! The summary tag needs to be a direct child of the details tag. +
+``` + +### 👍 Examples of **correct** code for this rule: + +```erb +
+ Expand me! + I do have a summary tag! +
+```` + +```erb +
+ I do have a summary tag! + Expand me! +
+```` diff --git a/lib/erblint-github/linters/github/accessibility/details_has_summary.rb b/lib/erblint-github/linters/github/accessibility/details_has_summary.rb new file mode 100644 index 0000000..03a6ae7 --- /dev/null +++ b/lib/erblint-github/linters/github/accessibility/details_has_summary.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require_relative "../../custom_helpers" +require_relative "../../tag_tree_helpers" + +module ERBLint + module Linters + module GitHub + module Accessibility + class DetailsHasSummary < Linter + include ERBLint::Linters::CustomHelpers + include ERBLint::Linters::TagTreeHelpers + include LinterRegistry + + MESSAGE = "
elements need to have explicit element" + + def run(processed_source) + has_summary = false + + (tags, tag_tree) = build_tag_tree(processed_source) + + tags.each do |tag| + next if tag.name != "details" || tag.closing? + + details = tag_tree[tag] + + details[:children].each do |child| + if child[:tag].name == "summary" + has_summary = true + break + end + + erb_nodes = extract_erb_nodes(child[:tag].node) + + next if erb_nodes.blank? + + code = "" + + erb_nodes.each do |erb_node| + _, _, code_node = *erb_node + code += code_node.children.first + end + + ast = erb_ast(code) + + ast = ast.children.first if ast.type == :block + next unless ast.method_name == :content_tag + + if ast.arguments.first.value.to_s == "summary" + has_summary = true + break + end + end + + generate_offense(self.class, processed_source, details[:tag]) unless has_summary + + has_summary = false + end + + rule_disabled?(processed_source) + end + + private + + def extract_erb_nodes(node) + return node if node.type == :erb + return nil unless node.type == :text + + node.children.select { |x| x.try(:type) == :erb } + end + + def erb_ast(code) + RuboCop::AST::ProcessedSource.new(code, RUBY_VERSION.to_f).ast + end + end + end + end + end +end diff --git a/lib/erblint-github/linters/tag_tree_helpers.rb b/lib/erblint-github/linters/tag_tree_helpers.rb new file mode 100644 index 0000000..5889f1f --- /dev/null +++ b/lib/erblint-github/linters/tag_tree_helpers.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +module ERBLint + module Linters + # Helpers used by linters to organize HTML tags into abstract syntax trees. + module TagTreeHelpers + # from https://github.com/Shopify/erb-lint/blob/6179ee2d9d681a6ec4dd02351a1e30eefa748d3d/lib/erb_lint/linters/self_closing_tag.rb + SELF_CLOSING_TAGS = %w[ + area base br col command embed hr input keygen + link menuitem meta param source track wbr img + ].freeze + + # This assumes that the AST provided represents valid HTML, where each tag has a corresponding closing tag. + # From the tags, we build a structured tree which represents the tag hierarchy. + # With this, we are able to know where the tags start and end. + def build_tag_tree(processed_source) + nodes = processed_source.ast.children + tag_tree = {} + tags = [] + current_opened_tag = nil + + nodes.each do |node| + tag = BetterHtml::Tree::Tag.from_node(node) + + if node.type == :tag + # get the tag from previously calculated list so the references are the same + tags << tag + + if tag.closing? + if current_opened_tag && tag.name == current_opened_tag.name + tag_tree[current_opened_tag][:closing] = tag + current_opened_tag = tag_tree[current_opened_tag][:parent] + end + + next + end + + self_closing = self_closing?(tag) + + tag_tree[tag] = { + tag: tag, + closing: self_closing ? tag : nil, + parent: current_opened_tag, + children: [] + } + + tag_tree[current_opened_tag][:children] << tag_tree[tag] if current_opened_tag + current_opened_tag = tag unless self_closing + elsif current_opened_tag + tag_tree[current_opened_tag][:children] << { + tag: tag, + closing: tag, + parent: current_opened_tag, + children: [] + } + end + end + + [tags, tag_tree] + end + + def self_closing?(tag) + tag.self_closing? || SELF_CLOSING_TAGS.include?(tag.name) + end + end + end +end diff --git a/test/linters/accessibility/details_has_summary_test.rb b/test/linters/accessibility/details_has_summary_test.rb new file mode 100644 index 0000000..54324c1 --- /dev/null +++ b/test/linters/accessibility/details_has_summary_test.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require "test_helper" + +class DetailsHasSummary < LinterTestCase + def linter_class + ERBLint::Linters::GitHub::Accessibility::DetailsHasSummary + end + + def test_warns_if_details_doesnt_have_a_summary + @file = "
I don't have a summary element
" + @linter.run(processed_source) + + assert_equal 1, @linter.offenses.count + end + + def test_does_not_warn_if_details_has_a_summary_in_a_weird_place + @file = "

Surprise text!

Expand me!
" + @linter.run(processed_source) + + assert_empty @linter.offenses + end + + def test_does_not_warn_if_details_has_a_summary + @file = "
Expand me!
" + @linter.run(processed_source) + + assert_empty @linter.offenses + end + + def test_does_warns_if_details_has_a_summary_as_an_inner_child + @file = "

Expand me!

" + @linter.run(processed_source) + + assert_equal 1, @linter.offenses.count + end + + def test_doesnt_warns_if_details_has_a_summary_content_tag + @file = <<~ERB +
+ <%= content_tag(:summary) { "Expand me!" } %> +

Surprise text!

+
+ ERB + @linter.run(processed_source) + + assert_empty @linter.offenses + end + + def test_doesnt_warns_if_details_has_a_summary_content_tag_with_do_block + @file = <<~ERB +
+ <%= content_tag(:summary) do %> + Expand me! + <% end %> +

Surprise text!

+
+ ERB + @linter.run(processed_source) + + assert_empty @linter.offenses + end + + def test_warns_if_details_has_a_non_summary_content_tag + @file = <<~ERB +
+ <%= content_tag(:div) { "Expand me!" } %> +

Surprise text!

+
+ ERB + @linter.run(processed_source) + + assert_equal 1, @linter.offenses.count + end +end diff --git a/test/linters/tag_tree_helpers_test.rb b/test/linters/tag_tree_helpers_test.rb new file mode 100644 index 0000000..f4fdf82 --- /dev/null +++ b/test/linters/tag_tree_helpers_test.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require "test_helper" +require_relative "../../lib/erblint-github/linters/tag_tree_helpers" + +class TagTreeHelpers < Minitest::Test + include ERBLint::Linters::TagTreeHelpers + + def test_build_tree + file = %{ + + + Hello + + +

Hello

+ Welcome to my page! + + + } + processed_source = ERBLint::ProcessedSource.new("file.rb", file) + tags, tree = build_tag_tree(processed_source) + + html_tag = tags.first + + assert_equal "html", html_tag.name + + children = tree[html_tag][:children].filter do |child| + child[:tag].name + end + + assert_equal 2, children.length + assert_equal "head", children[0][:tag].name + + body_tag = children[1][:tag] + assert_equal "body", body_tag.name + + h1_tag = tree[body_tag][:children].filter{ |child| child[:tag].name }.first[:tag] + assert_equal "h1", h1_tag.name + end +end