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

New rule: Summary element has non-empty accessible name #2202

Merged
merged 16 commits into from
Aug 26, 2024

Conversation

WilcoFiers
Copy link
Member

New rule to test that summary elements have an accessible name.

I don't think this is covered by any other rule today, since summary elements do not have a semantic role.

Need for Call for Review: 2 weeks


How to Review And Approve

  • Go to the “Files changed” tab
  • Here you will have the option to leave comments on different lines.
  • Once the review is completed, find the “Review changes” button in the top right, select “Approve” (if you are really confident in the rule) or "Request changes" and click “Submit review”.
  • Make sure to also review the proposed Call for Review period. In case of disagreement, the longer period wins.

Copy link
Collaborator

@tombrunet tombrunet left a comment

Choose a reason for hiding this comment

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

An inapplicable example for two summary elements to catch the third applicability bullet would be useful.

_rules/summary-non-empty-accessible-name-2t702h.md Outdated Show resolved Hide resolved
_rules/summary-non-empty-accessible-name-2t702h.md Outdated Show resolved Hide resolved
tombrunet
tombrunet previously approved these changes Jul 18, 2024
kengdoj
kengdoj previously approved these changes Jul 18, 2024
Jym77
Jym77 previously requested changes Jul 18, 2024
Copy link
Collaborator

@Jym77 Jym77 left a comment

Choose a reason for hiding this comment

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

I like it 👍

_rules/summary-non-empty-accessible-name-2t702h.md Outdated Show resolved Hide resolved
_rules/summary-non-empty-accessible-name-2t702h.md Outdated Show resolved Hide resolved
@WilcoFiers WilcoFiers dismissed Jym77’s stale review July 25, 2024 13:40

Needs another review

@WilcoFiers WilcoFiers dismissed stale reviews from kengdoj and tombrunet July 25, 2024 16:26

Couple tweaks, please have another look

Co-authored-by: Jean-Yves Moyen <[email protected]>
Co-authored-by: Giacomo Petri <[email protected]>
@Jym77 Jym77 dismissed their stale review August 1, 2024 12:36

Changes done.

@giacomo-petri
Copy link
Collaborator

In Firefox, the ::marker CSS pseudo-element is part of the accessibility tree, which means the accessible name cannot be left empty in Firefox.

Should we address this, clarifying that the "triangle" icon is not enough?


## Accessibility Support

Implementation of [Presentational Roles Conflict Resolution][] varies from one browser or assistive technology to another. Depending on this, some elements can have a [semantic role][] of `button` and fail this rule with some technology but users of other technologies would not experience any accessibility issue.
Copy link
Member

Choose a reason for hiding this comment

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

Why would the element having a semantic role of button make it fail this rule? Is this copy/paste from another rule?

</details>
```

### Failed
Copy link
Member

Choose a reason for hiding this comment

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

Add failed example with two summary elements, being the first empty

@WilcoFiers WilcoFiers added the Review call 2 weeks Call for review for new rules and big changes label Aug 1, 2024
@WilcoFiers WilcoFiers merged commit 7e71738 into develop Aug 26, 2024
2 checks passed
@WilcoFiers WilcoFiers deleted the summary-has-name branch August 26, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review call 2 weeks Call for review for new rules and big changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants