Details preprocess incorrectly adds role to summary

Hi, Issues for Drupal core should be targeted to the 'main' branch, our primary development branch. Changes are made on the main branch first, and are then back ported as needed according to the Core change policies. The version the problem was discovered on should be stated in the issue summary Problem/Motivation section. Thanks.

Thank you for your work to improve Drupal's accessibility!

Unfortunately, the MR is failing a Functional test for Views. The FunctionalJavascript test failure is probably random and unrelated. But that Views test is legitimate. It explicitly tests for the role attribute and is annotated with the comment // Check that the summary has the right accessibility settings. So clearly someone thought it belonged there. I don't doubt what you say in the issue summary about it being incorrect. It just means that we need to adjust our expectations. As a result, this probably has to undergo accessibility review before being committed.

In addition to fixing the existing test, bug fixes require automated tests to be added to ensure there are no regressions in the future. So I'm tagging this issue as needing tests.

I did my best to fix the broken tests. Everything has gone green!

Thanks for giving it a shot! Left some comments on the MR but main point is think we can revert most of the test coverage and just add a single assertion to an existing test.

Based on feedback, I undid my test. I am not sure how to fix the broken test, so I have left it alone. Maybe someone else can fix the test.

@rkoller pointed me to some articles that bring into question whether we should be using details at all instead of a custom disclosure widget:

• https://www.scottohara.me/blog/2022/09/12/details-summary.html
• https://www.matuzo.at/blog/2023/details-summary

tl;dr:

It really just all comes down to what sort of experience you want to provide to people, and how OK you might be knowing that the native elements are going to be exposed differently, have different gaps but also different features to them. Or, you can roll your own disclosure widgets and provide a consistent experience that you are fully in charge of… but it may not be as feature rich as what the browsers could provide, and your disclosure widgets need to degrade gracefully if viewed in a browser’s reading mode.

@rkoller did some testing and found that Safari didn't announce a state change, which is consistent with the matuzo.at article: #3579845-17: Remove aria-expanded from summary in details element.

In a comment on another issue, @cwilcox808 implied that adding role="button" is OK, but that it should be done with JavaScript: #3571628-12: The aria-expanded attribute doesn't change once it became true on a node edit forms detail summary element. He hinted at this in the Slack thread referenced in #4.

How it should work is the summary elements have no ARIA attributes (including role) so that if JavaScript doesn't run, the element's inherent role and state can still work. When the JavaScript runs, it adds the role="button" and the aria-expanded with the state that matches whether the details element is open or not. It should probably only add the role attribute if the element doesn't already have one.

I flagged this for accessibility review to get more input from the accessibility team on the recommended solution.

Correction:

The matuzo.at article said that Safari / VoiceOver on iOS doesn't announce a state change (or role).

Voice Over iOS Safari is also very consistent but in a bad way. It doesn’t announce any role or state.

Also wondering if keeping the role and the custom behavior on the summary (but fixing it) accomplishes the "custom disclosure widget" recommended by http://matuzo.at/ and Scott O'Hara and thereby fixes the Safari issues with native details / summary.

In the other issue, @cwilcox808 also said (emphasis mine):

I didn't realize this script was being used on a summary with role="button", effectively changing the details / summary into a generic "disclosure widget."

Bug ID: DRU-3d2c0738 (instance) / DRU-93a3d129 (pattern)
URL: http://localhost/form-test/group-details
XPath: //summary[@data-summary-attribute="test"][@role="button"]
Full DOM path: /html/body//details[summary[@data-summary-attribute="test"]]/summary
WCAG SC: 1.3.1 - Info and Relationships (Level A)
Rule: manual semantic check - redundant-role-summary
Severity: Medium Frequency:
Pattern-level; applies to details summaries rendered by core preprocess on pages using #type details
Screen type: desktop

<!-- Current output (problematic): role is redundantly added on native summary. -->
<summary data-summary-attribute="test" role="button" aria-expanded="false">
  Details element with summary attributes
</summary>

<!-- Expected output after fix: native summary semantics preserved, aria-expanded retained. -->
<summary data-summary-attribute="test" aria-expanded="false">
  Details element with summary attributes
</summary>

Removed redundant role assignment from summary.

This contribution was prepared with assistance from an AI coding tool.

https://github.com/mgifford/drupal-core/blob/main/patches/ISSUE-QUEUE-CO...

@kentr Great points to consider in the comments above..

How do we make the code better today than it was yesterday? I think the main documentation is around:

• https://www.matuzo.at/blog/2023/details-summary
• https://www.scottohara.me/blog/2022/09/12/details-summary.html

I haven't dug into this but an AI summary of this matuzo's article suggests:

1. Keep native summary semantics as baseline (do not add role=button in preprocess).
2. Avoid trying to normalize AT output by forcing ARIA role overrides on native summary.
3. If any ARIA state is used, require strict sync with open/closed state and test it, but do not reintroduce role=button as a compensation strategy.
4. Document explicitly that cross-AT wording differences are expected and not, by themselves, proof of failure.

Is this the right one? Is it being read wrong? The easiest improve still seems to be removing the button.

Here's a patch to review https://git.drupalcode.org/project/drupal/-/merge_requests/16325.diff

@mgifford

The easiest improve still seems to be removing the button.

My interpretation is that removing the role won't improve it, and we should probably ditch native details / summary by keeping the role and JavaScript behaviors to turn it into a generic disclosure widget. #3571628: The aria-expanded attribute doesn't change once it became true on a node edit forms detail summary element should take care of that.

More points from the Scott O'Hara article:

The <summary> element is ‘supposed to be’ exposed to assistive technologies as having a “button” role. However, its role may also be announced as “summary”, or even as a “disclosure triangle” because that’s a choice someone made on purpose… and even sometimes (unfortunately) no role at all. How it is exposed is contingent on the platform/browser and screen reader pairing. More on this later.

If your goal is to create an absolutely consistent disclosure widget behavior across browsers, i.e., ensuring that all <summary>s are exposed as expand/collapse buttons, then you’d be better off creating your own using JavaScript and the necessary ARIA attributes. You’d lose out on the find-in-page functionality that the native disclosure widget provides… but depending on the type of disclosure widget you’re creating, you may actually want that.

For me, VoiceOver Safari still doesn't announce the state change on a vanilla details / summary.

I see this as another case of the "spec" failing to live up to its promises.

Inconsistent browser (and AT) adoption is an ongoing problem. It would be so nice if there was pressure put on user agents to comply with https://www.w3.org/WAI/standards-guidelines/uaag/ so that everyone wasn't just doing their own thing.

I feel that there are times when the right thing to do is to point to the upstream problem and make it very clear where this problem needs to be fixed. I don't think it is a problem with the HTML as such, but the rendering by other software.

Some UK gov links on this topic:

• https://github.com/alphagov/govuk-design-system/issues/4030
• https://github.com/alphagov/reported-bugs/issues/64
• https://github.com/alphagov/reported-bugs/issues/29
• https://github.com/alphagov/govuk-frontend/pull/5089

From one of these threads someone recommended:
https://u-elements.github.io/u-elements/elements/u-details

Which claims:

There is no longer need for 🎉
Please use native as it has sufficient support in major browsers and screen readers,

I guess Safari/VO isn't big enough. They do have a polyfill though.

Other information from A11ysupport.io:
https://a11ysupport.io/tests/tech__html__details-summary

I worry about putting up a non-standard bit of JS that we then need to explain going ahead.

Rebased as it was 700+ commits back. The failure

       ├ Behat\Mink\Exception\ElementHtmlException: The attribute "role" was not found in the element matching xpath "//summary".
       │
       │ /builds/vendor/behat/mink/src/WebAssert.php:927
       │ /builds/vendor/behat/mink/src/WebAssert.php:625
       │ /builds/core/modules/views/tests/src/Functional/Plugin/StyleTableTest.php:54

Seems related to the change.

Looks like we're going with #3571628: The aria-expanded attribute doesn't change once it became true on a node edit forms detail summary element. I'll transfer credit to that issue.

[Edit: Not really a duplicate, but that sounds better than the other options.]