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

feat(new-rule): summary elements must have an accessible name #4511

Merged
merged 4 commits into from
Jul 1, 2024

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented Jun 21, 2024

This rule checks that summary elements have an accessible name, through text content, aria-label(ledby) or title. It skips summary elements that are not used as controls for details, or if its details element has no content.

Closes: #4510

@WilcoFiers WilcoFiers requested a review from a team as a code owner June 21, 2024 14:18
@WilcoFiers WilcoFiers marked this pull request as draft June 21, 2024 15:40
@WilcoFiers WilcoFiers marked this pull request as ready for review June 24, 2024 12:55
assert.isFalse(rule.matches(null, vNode));
});

it('is false for s details parent in a different DOM tree', () => {
Copy link
Contributor

@straker straker Jun 24, 2024

Choose a reason for hiding this comment

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

I don't think this is setup up correctly. This creates a summary element who has a shadowroot div as the parent and the slot isn't doing anything.

screenshot of Chrome DevTools Elements panel showing a div with a summary element as a shadowroot child and a summary element as sibling with a slot child

Did you mean instead to setup a case where the summary gets used as the slotted content of the details?

screenshot of Chrome DevTools Elements panel showing a custom element with a details element as a shadowroot child and a summary element being slotted into the details element

assert.isFalse(rule.matches(null, vNode));
});

it('is false for s details parent in a different DOM tree', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('is false for s details parent in a different DOM tree', () => {
it('is false for details parent in a different DOM tree', () => {

const parent = virtualNode.parent;
if (
parent.props.nodeName !== 'details' ||
virtualNode.shadowId !== parent.shadowId
Copy link
Contributor

@straker straker Jun 24, 2024

Choose a reason for hiding this comment

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

Can details and summary every not share the same shadowId? details cannot have a shadow root attached to it and if you put a summary as slotted content to the details they'll both have the same shadowId

<custom-details>
  <summary>slotted content</summary>
</custom-details>

<script>
customElements.define('custom-details',
  class extends HTMLElement {
    constructor() {
      super();
      const shadowRoot = this.attachShadow({ mode: 'open' });
      shadowRoot.innerHTML = '<details><slot></slot></details>';
    }
  }
);
</script>
screenshot of the axe virtual tree structure showing that both the details element and summary element have the same shadowId when using the previous code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gd catch. Do have a way to see in virtual tree that two nodes are in the same tree?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if we have a way to distinguish between light DOM tree and shadow DOM tree.

return true;
}

function hasDetailsContent(vDetails) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ignore summary elements of details that don't have content? They are still interactive and screen readers read them out. They don't seem to be treated any differently if the details has text content.

<details>
  <summary></summary>
</details>
  • JAWS / Chrome - "unlabeled zero button, collapsed"
  • NVDA / Firefox - "filled right pointing small triangle, collapsed"
  • VO / Safari - "collapsed, details"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not less sure what's going on when there's nothing in the details element to expand. Its just an empty tab stop at that point. Those aren't great, but they're not an WCAG violation IMO.

@WilcoFiers WilcoFiers merged commit 0d8a99e into develop Jul 1, 2024
21 checks passed
@WilcoFiers WilcoFiers deleted the summary-name branch July 1, 2024 09:31
WilcoFiers added a commit that referenced this pull request Jul 29, 2024
##
[4.10.0](v4.9.1...v4.10.0)
(2024-07-29)

### Features

- **new-rule:** summary elements must have an accessible name
([#4511](#4511))
([0d8a99e](0d8a99e)),
closes [#4510](#4510)

### Bug Fixes

- **all-rules:** fix flakey all-rules firefox test
([#4467](#4467))
([3f13aa1](3f13aa1))
- **aria-allowed-attr:** allow aria-multiline=false for element with
contenteditable
([#4537](#4537))
([f019068](f019068))
- **aria-allowed-attr:** allow aria-required=false when normally not
allowed ([#4532](#4532))
([2e242e1](2e242e1))
- **aria-prohibited-attr:** allow aria-label/ledby on decendants of
widget ([#4541](#4541))
([07c5d91](07c5d91))
- **aria-roledescription:** keep disabled with { runOnly: 'wcag2a' }
([#4526](#4526))
([5b4cb9d](5b4cb9d)),
closes [#4523](#4523)
- **autocomplete-valid:** incomplete for invalid but safe values
([#4500](#4500))
([e31a974](e31a974)),
closes [#4492](#4492)
- **build:** limit locales to valid files when using the --all-lang
option ([#4486](#4486))
([d3db593](d3db593)),
closes [#4485](#4485)
- colorio.js patch mocking CSS
([#4456](#4456))
([3ef9353](3ef9353)),
closes [#4400](#4400)
- correct typos in texts
([#4499](#4499))
([11fad59](11fad59))
- **landmark-unique:** follow spec, aside -> landmark
([#4469](#4469))
([e32f803](e32f803)),
closes [#4460](#4460)
- **required-attr:** allow aria-valuetext on slider instead of valuenow
([#4518](#4518))
([135898b](135898b)),
closes [#4515](#4515)

This PR was opened by a robot 🤖 🎉
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.

Add summary-name rule
2 participants