diff --git a/.changeset/cuddly-laws-peel.md b/.changeset/cuddly-laws-peel.md new file mode 100644 index 0000000000..9b2f7d4081 --- /dev/null +++ b/.changeset/cuddly-laws-peel.md @@ -0,0 +1,5 @@ +--- +"@primer/view-components": minor +--- + +Add lint rule against .tooltipped and introduce accessibility.yml config diff --git a/.erb-lint.yml b/.erb-lint.yml index 14c6fd2cb1..2b4533c690 100644 --- a/.erb-lint.yml +++ b/.erb-lint.yml @@ -1,9 +1,13 @@ --- EnableDefaultLinters: true +inherit_from: + - lib/primer/view_components/linters/accessibility.yml inherit_gem: erblint-github: - config/accessibility.yml linters: + NoUnusedDisable: + enabled: true DeprecatedComponentsCounter: enabled: true ButtonComponentMigrationCounter: diff --git a/lib/primer/view_components/linters/accessibility.yml b/lib/primer/view_components/linters/accessibility.yml new file mode 100644 index 0000000000..bf0b7d9ee7 --- /dev/null +++ b/lib/primer/view_components/linters/accessibility.yml @@ -0,0 +1,7 @@ +# Accessibility-focused lint rule configurations, tracked by accessibility scorecard. +--- +linters: + NoUnusedDisable: + enabled: true + Primer::Accessibility::TooltippedMigration: + enabled: true diff --git a/lib/primer/view_components/linters/argument_mappers/base.rb b/lib/primer/view_components/linters/argument_mappers/base.rb index 8edc4f8da2..76ccba653d 100644 --- a/lib/primer/view_components/linters/argument_mappers/base.rb +++ b/lib/primer/view_components/linters/argument_mappers/base.rb @@ -53,7 +53,7 @@ def map_classes(classes_node) system_arguments = system_arguments_to_args(classes_node.value) args = classes_to_args(system_arguments[:classes]&.split || []) - invalid_classes = args[:classes].select { |class_name| Primer::Classify::Validation.invalid?(class_name) } + invalid_classes = args[:classes].select { |class_name| ::Primer::Classify::Validation.invalid?(class_name) } raise ConversionError, "Cannot convert #{'class'.pluralize(invalid_classes.size)} #{invalid_classes.join(',')}" if invalid_classes.present? @@ -81,7 +81,7 @@ def classes_to_args(classes) end def system_arguments_to_args(classes) - system_arguments = Primer::Classify::Utilities.classes_to_hash(classes) + system_arguments = ::Primer::Classify::Utilities.classes_to_hash(classes) # need to transform symbols to strings with leading `:` system_arguments.transform_values do |v| diff --git a/lib/primer/view_components/linters/base_linter.rb b/lib/primer/view_components/linters/base_linter.rb index 7fad24eb07..e25eb5f3c1 100644 --- a/lib/primer/view_components/linters/base_linter.rb +++ b/lib/primer/view_components/linters/base_linter.rb @@ -5,7 +5,7 @@ require "primer/view_components/constants" require_relative "tag_tree_helpers" - +require_relative "helpers/rule_helpers" # :nocov: module ERBLint @@ -17,6 +17,7 @@ module Linters # * `REQUIRED_ARGUMENTS` - optional - A list of HTML attributes that are required by the component. class BaseLinter < Linter include TagTreeHelpers + include Helpers::RuleHelpers DUMP_FILE = ".erblint-counter-ignore.json" DISALLOWED_CLASSES = [].freeze @@ -134,10 +135,6 @@ def correct_counter(corrector, processed_source, offense) end end - def tags(processed_source) - processed_source.parser.nodes_with_type(:tag).map { |tag_node| BetterHtml::Tree::Tag.from_node(tag_node) } - end - def counter_correct?(processed_source) comment_node = nil expected_count = 0 @@ -178,13 +175,6 @@ def counter_correct?(processed_source) end end - def generate_offense(klass, processed_source, tag, message = nil, replacement = nil) - message ||= klass::MESSAGE - klass_name = klass.name.demodulize - offense = ["#{klass_name}:#{message}", tag.node.loc.source].join("\n") - add_offense(processed_source.to_source_range(tag.loc), offense, replacement) - end - def dump_data(processed_source) return if @total_offenses.zero? diff --git a/lib/primer/view_components/linters/helpers/deprecated_components_helpers.rb b/lib/primer/view_components/linters/helpers/deprecated_components_helpers.rb index 65509f3473..5bc3dba4a5 100644 --- a/lib/primer/view_components/linters/helpers/deprecated_components_helpers.rb +++ b/lib/primer/view_components/linters/helpers/deprecated_components_helpers.rb @@ -8,11 +8,11 @@ module Helpers # Helpers to share between DeprecatedComponents ERB lint and Rubocop cop module DeprecatedComponentsHelpers def message(component_name) - Primer::Deprecations.deprecation_message(component_name) + ::Primer::Deprecations.deprecation_message(component_name) end def deprecated_components - Primer::Deprecations.deprecated_components + ::Primer::Deprecations.deprecated_components end end end diff --git a/lib/primer/view_components/linters/helpers/rule_helpers.rb b/lib/primer/view_components/linters/helpers/rule_helpers.rb new file mode 100644 index 0000000000..808e4ee704 --- /dev/null +++ b/lib/primer/view_components/linters/helpers/rule_helpers.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module ERBLint + module Linters + module Helpers + # Helpers for writing rules + module RuleHelpers + def tags(processed_source) + processed_source.parser.nodes_with_type(:tag).map { |tag_node| BetterHtml::Tree::Tag.from_node(tag_node) } + end + + def generate_offense(klass, processed_source, tag, message = nil, replacement = nil) + message ||= klass::MESSAGE + klass_name = klass.name.demodulize + offense = ["#{klass_name}:#{message}", tag.node.loc.source].join("\n") + add_offense(processed_source.to_source_range(tag.loc), offense, replacement) + end + + # Generate offense for ERB node tag + def generate_node_offense(klass, processed_source, node, message = nil) + message ||= klass::MESSAGE + offense = ["#{klass.name}:#{message}", node.loc.source].join("\n") + add_offense(processed_source.to_source_range(node.loc), offense) + end + + def erb_nodes(processed_source) + processed_source.parser.ast.descendants(:erb) + end + + def extract_ruby_from_erb_node(erb_node) + return unless erb_node.type == :erb + + _, _, code_node = *erb_node + code_node.loc.source.strip + end + end + end + end +end diff --git a/lib/primer/view_components/linters/tooltipped_migration.rb b/lib/primer/view_components/linters/tooltipped_migration.rb new file mode 100644 index 0000000000..fee0d71a62 --- /dev/null +++ b/lib/primer/view_components/linters/tooltipped_migration.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require_relative "helpers/rule_helpers" +module ERBLint + module Linters + module Primer + module Accessibility + # Flag when `.tooltipped` is being used and offer alternatives. + class TooltippedMigration < Linter + include LinterRegistry + include Helpers::RuleHelpers + + MIGRATE_TO_NEWER_TOOLTIP = ".tooltipped has been deprecated. Tooltip should never be set on a non-interactive element (e.g. ,
,
  • ). " \ + "Find alternatives in https://primer.style/design/guides/accessibility/tooltip-alternatives/. " \ + "If you're setting a tooltip on an interactive element, use the latest tooltip in https://primer.style/view-components." + TOOLTIPPED_RUBY_PATTERN = /classes:.*tooltipped|class:.*tooltipped/.freeze + + def run(processed_source) + # HTML tags + tags(processed_source).each do |tag| + next if tag.closing? + + classes = tag.attributes["class"]&.value + generate_offense(self.class, processed_source, tag, MIGRATE_TO_NEWER_TOOLTIP) if classes&.include?("tooltipped") + end + + # ERB nodes + erb_nodes(processed_source).each do |node| + code = extract_ruby_from_erb_node(node) + generate_node_offense(self.class, processed_source, node, MIGRATE_TO_NEWER_TOOLTIP) if code.match?(TOOLTIPPED_RUBY_PATTERN) + end + end + end + end + end + end +end diff --git a/test/lib/erblint/linter_config_works_test.rb b/test/lib/erblint/linter_config_works_test.rb new file mode 100644 index 0000000000..4d8f8fb8a1 --- /dev/null +++ b/test/lib/erblint/linter_config_works_test.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require "lib/erblint_test_case" + +class LinterConfigWorksTest < ErblintTestCase + # The ability to share rules and configs from other gems in erb_lint is not well-documented. + # This test validates that our recommended setup works. + def test_asserts_recommended_setup_works + erb_lint_config = ERBLint::RunnerConfig.new(file_loader.yaml(".erb-lint.yml"), file_loader) + + rules_enabled_in_accessibility_config = 0 + erb_lint_config.to_hash["linters"].each do |linter| + rules_enabled_in_accessibility_config += 1 if linter[0].include?("Primer::Accessibility") && linter[1]["enabled"] == true + end + known_linter_names ||= ERBLint::LinterRegistry.linters.map(&:simple_name) + known_linter_names_count = known_linter_names.count { |linter| linter.include?("Primer::Accessibility") } + assert_equal 1, rules_enabled_in_accessibility_config + assert_equal 1, known_linter_names_count + end +end diff --git a/test/lib/erblint/tooltipped_migration_test.rb b/test/lib/erblint/tooltipped_migration_test.rb new file mode 100644 index 0000000000..fd17ac181a --- /dev/null +++ b/test/lib/erblint/tooltipped_migration_test.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require "lib/erblint_test_case" + +class TooltippedMigrationTest < ErblintTestCase + def linter_class + ERBLint::Linters::Primer::Accessibility::TooltippedMigration + end + + def test_warns_if_tooltipped_class_is_used_on_html_tag + @file = "
  • List item
    • " + @linter.run(processed_source) + assert_equal 1, @linter.offenses.count + assert_match(/.tooltipped has been deprecated./, @linter.offenses.first.message) + end + + def test_warns_if_tooltipped_class_is_used_on_view_component + @file = <<~HTML + <%= content_tag :span, + class: "tooltipped tooltipped-nw cursor-pointer" do %> + HTML + @linter.run(processed_source) + assert_equal 1, @linter.offenses.count + assert_match(/.tooltipped has been deprecated./, @linter.offenses.first.message) + end + + def test_warns_if_tooltipped_class_is_used_on_ruby + @file = '<%= render SomeComponent.new(classes: "tooltipped tooltipped-multiline tooltipped-s") do %>' + @linter.run(processed_source) + assert_equal 1, @linter.offenses.count + assert_match(/.tooltipped has been deprecated./, @linter.offenses.first.message) + end + + def test_does_not_warn_if_tooltipped_is_not_used + @file = "<%= render SomeComponent.new do %>" + @linter.run(processed_source) + assert_equal 0, @linter.offenses.count + end + + def test_does_not_warn_if_inline_disable_comment + @file = <<~HTML + <%= render SomeComponent.new(classes: "tooltipped") do %><%# erblint:disable Primer::Accessibility::TooltippedMigration %> + HTML + @linter.run_and_update_offense_status(processed_source) + offenses = @linter.offenses.reject(&:disabled?) # Reject offenses marked as disabled. (This happens at the runner level so we repliace it here) + assert_equal 0, offenses.count + end +end