Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lint rule against .tooltipped and introduce accessibility.yml config #2086

Merged
merged 16 commits into from
Jun 22, 2023
Merged
5 changes: 5 additions & 0 deletions .changeset/cuddly-laws-peel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/view-components": minor
---

Add lint rule against .tooltipped and introduce accessibility.yml config
4 changes: 4 additions & 0 deletions .erb-lint.yml
Original file line number Diff line number Diff line change
@@ -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:
Expand Down
7 changes: 7 additions & 0 deletions lib/primer/view_components/linters/accessibility.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Accessibility-focused lint rule configurations, tracked by accessibility scorecard.
camertron marked this conversation as resolved.
Show resolved Hide resolved
---
linters:
NoUnusedDisable:
enabled: true
Primer::Accessibility::TooltippedMigration:
enabled: true
4 changes: 2 additions & 2 deletions lib/primer/view_components/linters/argument_mappers/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Expand Down Expand Up @@ -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|
Expand Down
14 changes: 2 additions & 12 deletions lib/primer/view_components/linters/base_linter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
require "primer/view_components/constants"

require_relative "tag_tree_helpers"

require_relative "helpers/rule_helpers"
# :nocov:

module ERBLint
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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?

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
39 changes: 39 additions & 0 deletions lib/primer/view_components/linters/helpers/rule_helpers.rb
Original file line number Diff line number Diff line change
@@ -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
37 changes: 37 additions & 0 deletions lib/primer/view_components/linters/tooltipped_migration.rb
Original file line number Diff line number Diff line change
@@ -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. <span>, <div>, <li>). " \
"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
20 changes: 20 additions & 0 deletions test/lib/erblint/linter_config_works_test.rb
Original file line number Diff line number Diff line change
@@ -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
48 changes: 48 additions & 0 deletions test/lib/erblint/tooltipped_migration_test.rb
Original file line number Diff line number Diff line change
@@ -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 = "<li class='tooltipped'>List item</li>"
@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 %>
camertron marked this conversation as resolved.
Show resolved Hide resolved
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