Skip to content

Add lint rule to make sure <details> elements have a <summary> element #23

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.

Sign up for GitHub

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

Draft
koddsson wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Add lint rule to make sure <details> elements have a <summary> element #23

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f0de67f
Add test to make sure `<details>` elements have a `<summary>` element
koddsson Mar 14, 2022
a10071a
Use `each_with_index` to get the index of tag
koddsson Mar 15, 2022
de0c088
Simplify iterating through children
koddsson Mar 15, 2022
43bd2da
Update test name
koddsson Mar 15, 2022
963ed23
Add a test for when the `<summary>` is in a weird place
koddsson Mar 15, 2022
8d42625
Add docs on the `DetailsHasSummary` rule
koddsson Mar 15, 2022
8abd503
Update rule description a bit
koddsson Mar 15, 2022
aae4f1f
Remove unused `each_with_index`
koddsson Mar 15, 2022
bb572de
Fix rubocop violations
koddsson Mar 15, 2022
f4d75a9
Add TagTreeHelpers from PVC to actually build a tag tree structure
manuelpuyol Mar 15, 2022
6491bcc
Search for details with summaries using the tag_tree
manuelpuyol Mar 15, 2022
afd2917
Add another incorrect example
koddsson Mar 15, 2022
185195f
fix speling
Mar 16, 2022
3a802dc
Add a test for tag_tree_helpers
koddsson Mar 17, 2022
4e12784
add tests for content_tag
manuelpuyol Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions lib/erblint-github/linters/github/accessibility/details_has_summary.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# frozen_string_literal: true

require_relative "../../custom_helpers"

module ERBLint
module Linters
module GitHub
module Accessibility
class DetailsHasSummary < Linter
include ERBLint::Linters::CustomHelpers
include LinterRegistry

MESSAGE = "<details> elements need to have explict <summary> elements"
Comment thread
koddsson marked this conversation as resolved.
Outdated

def run(processed_source)
tags(processed_source).each do |tag|
Copy link
Copy Markdown
Collaborator

@khiga8 khiga8 Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tags(processed_source).each do |tag|
tags(processed_source).each_with_index do |tag, index|

then you could get rid of line 21!

next if tag.closing?
next unless tag.name == "details"

# Find the details element index in the AST
index = processed_source.ast.to_a.find_index(tag.node)

# Get the next element in the AST
Copy link
Copy Markdown
Collaborator

@khiga8 khiga8 Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if there's any reason to support <summary> not being the immediate next element? I don't know it's valid anyways but worth verifying.

<details>
  <p> some text </p>
  <summary>Toggle</summary>
</details>

Copy link
Copy Markdown
Collaborator

@khiga8 khiga8 Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't an easy way for getting the children of a element with erblint since we only have access to the tag nodes without the actual HTML tree structure.

One approach we've taken is keeping track of the opening/closing tag with variables. Here is an examples where we do this in dotcom:

This approach might make sense if we wanted to allow <summary> that isn't a first child, or if we wanted to give a specific warning message for <summary> that isn't first child.

Copy link
Copy Markdown
Author

@koddsson koddsson Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if there's any reason to support <summary> not being the immediate next element? I don't know it's valid anyways but worth verifying.

<details>
  <p> some text </p>
  <summary>Toggle</summary>
</details>

Hmm, I thought it needed to be the first element but that doesn't seem to be true. I wonder if it's worth supporting or if we should also validate that <summary> is the first element for consistency 🤔

I'll change this to allow <summary> to be anywhere within <details> for now.

next_node = processed_source.ast.to_a[index + 1]
next_tag = BetterHtml::Tree::Tag.from_node(next_node)

# If the next element is a summary, we're good
next if next_tag.name == "summary"

generate_offense(self.class, processed_source, tag)
end

rule_disabled?(processed_source)
end
end
end
end
end
end
23 changes: 23 additions & 0 deletions test/linters/accessibility/details_has_summary_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# 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 = "<details>I don't have a summary element</details>"
@linter.run(processed_source)

assert_equal @linter.offenses.count, 1
end

def test_does_not_warn_if_only_disabled_attribute_is_set
Copy link
Copy Markdown
Collaborator

@khiga8 khiga8 Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test description needs to be updated.

@file = "<details><summary>Expand me!</summary><button>Surprise button!</button></details>"
@linter.run(processed_source)

assert_empty @linter.offenses
end
end