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

Correcting technical summary table for <summary> #28602

Merged
merged 9 commits into from
Oct 20, 2023
Merged

Correcting technical summary table for <summary> #28602

merged 9 commits into from
Oct 20, 2023

Conversation

Brian-Pob
Copy link
Contributor

Description

Correcting the implicit ARIA role in the technical summary table for the summary element.

Motivation

To ensure the MDN docs follow the relevant HTML specs.

Additional details

User scottaohara is an editor for the HTML AAM spec and the ARIA in HTML spec. He states in the linked issue that the table should:

  1. Be updated to reference what the ARIA in HTML spec says
  2. Include the ARIA in HTML spec in the Specifications section

Related issues and pull requests

Fixes #28446

@github-actions github-actions bot added the Content:HTML Hypertext Markup Language docs label Aug 17, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2023

Preview URLs

External URLs (1)

URL: /en-US/docs/Web/HTML/Element/summary
Title: <summary>: The Disclosure Summary element

(comment last updated: 2023-10-20 00:32:17)

@Brian-Pob
Copy link
Contributor Author

@scottaohara apologies for the @, I was hoping to get your insight on this. Thank you.

The MDN doc gives the following code as an example for using <summary> as a heading. Summaries as Headings

<details open>
  <summary><h4>Overview</h4></summary>
  <ol>
    <li>Cash on hand: $500.00</li>
    <li>Current invoice: $75.30</li>
    <li>Due date: 5/6/19</li>
  </ol>
</details>

The doc currently has a warning following the example:

Warning: Because the <summary> element has a default role of button (which strips all roles from child elements), this example will not work for users of assistive technologies such as screen readers. The <h4> will have its role removed and thus will not be treated as a heading for these users.

You have established that the "default role of button" statement is incorrect. And I believe the "this example will not work" is also not completely correct.

I did some testing using my Mac's built-in VoiceOver feature. While the screen reader did not recognize the <h4> in the example as a heading, as the warning mentions, it was still able to read the "Overview" text and I could still interact with the <details> element.

My question

How can this warning be made more accurate in terms of its accessibility-related behavior?

@Josh-Cena Josh-Cena requested a review from scottaohara August 17, 2023 03:25
@scottaohara
Copy link
Contributor

hi @Brian-Pob. writing to acknowledge i've seen your ping. I will come back to this asap.

@scottaohara
Copy link
Contributor

@Brian-Pob fortunately i've written quite a bit about this here.

but at a high level, the note could be revised to call out that summary inconsistently maps to the button role, and the button role inconsistently treats its descendant elements as if they've been given a role of presentation | none.

@Brian-Pob
Copy link
Contributor Author

Noted. I will give your article a read. Thank you very much!

@Brian-Pob
Copy link
Contributor Author

The role assigned to the <summary> element varies between different user agents. This inconsistent mapping can cause issues for users of assistive technologies such as screen readers since elements may lose their semantic meaning (see the <h4> in the above example).

@scottaohara Is this an acceptable rewrite of the warning?

Btw, I've read through your whole article. It was very insightful. Thank you for writing and sharing it.

@scottaohara
Copy link
Contributor

sorry for the late reply, got buried in my other notifications. seems good to me with the update

@Brian-Pob Brian-Pob marked this pull request as ready for review October 19, 2023 19:25
@Brian-Pob Brian-Pob requested a review from a team as a code owner October 19, 2023 19:25
@Brian-Pob Brian-Pob requested review from estelle and removed request for a team October 19, 2023 19:25
Copy link
Member

@estelle estelle left a comment

Choose a reason for hiding this comment

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

Thanks! 🎉

@estelle estelle enabled auto-merge (squash) October 20, 2023 00:28
@estelle estelle merged commit 5517a91 into mdn:main Oct 20, 2023
6 checks passed
@Brian-Pob
Copy link
Contributor Author

@estelle Thanks for merging the PR, but it looks like I never actually updated the warning in the document. I updated the technical summary but not the warning.
Sorry about that.

Should I make a new PR?

Screenshot showing the unchanged warning for the summary element

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:HTML Hypertext Markup Language docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implicit role for <summary> element
4 participants