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 to make sure <details> elements have a <summary> element #23

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

Conversation

koddsson
Copy link

Hey 👋🏻

Here's a rule that makes sure that <details> elements have a <summary> element as well. While a <details> element doesn't need to have a <summary> element, it's good practice to provide one and not let the user agent inject one. Details elements should have a descriptive <summary> element instead.

I haven't written a rule that checks the child elements of a element before so I'm happy to make revisions if I'm doing the child querying incorrectly.

@koddsson koddsson requested a review from a team as a code owner March 14, 2022 19:54
@khiga8
Copy link
Collaborator

khiga8 commented Mar 14, 2022

@koddsson thank you for this 💪

Would you mind adding a corresponding rule doc in docs/rules/accessibility and linking to it in the README? (side note: we should introduce a workflow action to check this)

assert_equal @linter.offenses.count, 1
end

def test_does_not_warn_if_only_disabled_attribute_is_set
Copy link
Collaborator

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.

# 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
Collaborator

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
Collaborator

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
Author

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.

MESSAGE = "<details> elements need to have explict <summary> elements"

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

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!

@koddsson
Copy link
Author

koddsson commented Mar 15, 2022

[...] we should introduce a workflow action to check this

We have a meta test in https://github.com/github/eslint-plugin-custom-elements/blob/main/test/check-rules.js that makes sure that each rule is documented in a specific way and that the documentation is linked from the README if you're looking for inspiration.

@koddsson koddsson requested a review from khiga8 March 15, 2022 13:23
@koddsson koddsson changed the title Add test to make sure <details> elements have a <summary> element Add lint rule to make sure <details> elements have a <summary> element Mar 15, 2022
Copy link
Collaborator

@khiga8 khiga8 left a comment

Choose a reason for hiding this comment

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

A few minor comments/questions.
Feel free to merge when ready! 💪

```erb
<details>
<div><summary>Expand me!</summary></div>
I have a invalid summary tag! The summary tag needs to be a direct child of the details tag.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that you call this out!

# 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is awesome!!

This assumes that the AST provided represents valid HTML

What happens when the AST doesn't represent valid HTML? I'm guessing ERB lint rules like SelfClosingTag and ParserErrors ensures we don't end up in that state?

Should we add tests for this method, either in this PR or as a follow up?

Copy link
Author

Choose a reason for hiding this comment

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

What happens when the AST doesn't represent valid HTML? I'm guessing ERB lint rules like SelfClosingTag and ParserErrors ensures we don't end up in that state?

Yes, I believe this is the case. cc @manuelpuyol for confirmation.

Should we add tests for this method, either in this PR or as a follow up?

Good shout. I'll add some tests.

Kristján Oddsson and others added 2 commits March 16, 2022 10:26
Co-authored-by: Kate Higa  <16447748+khiga8@users.noreply.github.com>
@koddsson koddsson marked this pull request as draft March 18, 2022 14:41
@koddsson
Copy link
Author

Running this on github/github, I'm getting false positives for <details> elements that have content_tag or ViewComponents with tag: :summary.

I feel like I gotta fix those before merging.

@smockle smockle removed their request for review July 25, 2024 20:15
@smockle
Copy link
Contributor

smockle commented Jul 25, 2024

Removing my assignment.

cc @github/accessibility-reviewers (this project’s CODEOWNER)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants