Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Simple copy of $variables['summary_attributes']['aria-expanded'] lead to dupliction of aria-expanded attribute on resulting tag and the loss of the aria-pressed attribute.
These two aria attributes have an important impact on accessibility of collapsible fieldsets like those used everywhere in Drupal.
Steps to reproduce
- run Drupal installer
- go to the 'Database configuration' page
- see the 'Advanced options'
<summary>HTML code (but do not open it or JS is going to hide the issue)
Proposed resolution
From my point of view, this is proper solution:
$variables['summary_attributes']['aria-pressed'] = $variables['summary_attributes']['aria-expanded']->value();
To avoid that kind of issue to be randomly reproduced, fix the Drupal\Core\Template\Attribute::createAttributeValue(). So, if the provided value is already an AttributeValueBase object, we create a new object with the right name and copy the value instead of returning the provided one.
Remaining tasks
| Task | Novice task? | Contributor instructions | Complete? |
|---|---|---|---|
| Create a patch | Instructions | Done | |
| Update the issue summary | Instructions | Done | |
| Update the issue summary noting if allowed during the rc | Template | Done | |
| Add automated tests | Instructions | Done | |
| Manually test the patch | Novice | Instructions | Done |
| Add steps to reproduce the issue | Novice | Instructions | Done |
| Embed before and after screenshots in the issue summary | Novice | Instructions | Done |
| Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions | Done |
User interface changes
None. (Some invisible markup changes)
API changes
None.
Data model changes
None.
Why this issue should be an rc target
This is an accessibility issue, but also just about fixing up some embarrassing & redundant code that is all over the admin side of Drupal 8. Anytime there's a Detail/Summary element there will be some bad markup. The duplicate aria-expanded isn't as critical as the missing aria-pressed attribute. aria-pressed Indicates the current "pressed" state of toggle buttons. aria-expanded indicates whether the element, or another grouping element it controls, is currently expanded or collapsed.
Before patch:
After patch:
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | aria-expanded-aria-pressed.png | 73.66 KB | mgifford |
| #22 | aria-expandedx2.png | 100.55 KB | mgifford |
| #18 | wrong_usage_of-2605420-18-pass.patch | 1.94 KB | joelpittet |
| |||
| #18 | wrong_usage_of-2605420-18-tests-only-fail.patch | 1.01 KB | joelpittet |
| #15 | wrong-attributes-in-preprocess-details-2605420-15.patch | 950 bytes | IRuslan |
Comments
Comment #2
IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commentedPatch attached.
Comment #3
IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commentedComment #5
IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commentedComment #6
cilefen CreditAttribution: cilefen commentedI am tagging this "accessibility" to get it a little more attention.
Comment #7
mgiffordWhat is the best way to see this with/without the patch?
Comment #8
IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commentedHow to check:
During Drupal install go to Database configuration page. Check the page source. You will see summarry HTML tag with 2 same aria-expanded attributes.
Apply patch, reload the page and look into source – you will see 2 different attributes, aria-expanded and aria-pressed. Which is corrrect.
Comment #9
mgiffordMakes sense to me. This is clearly wrong:
<details data-drupal-selector="edit-mysql" id="edit-mysql--2" class="js-form-wrapper form-wrapper"><summary role="button" aria-controls="edit-mysql--2" aria-expanded="false" aria-expanded="false">Advanced options</summary><div class="details-wrapper">This is way better:
<details data-drupal-selector="edit-mysql" id="edit-mysql--2" class="js-form-wrapper form-wrapper"><summary role="button" aria-controls="edit-mysql--2" aria-expanded="false" aria-pressed="false">Advanced options</summary><div class="details-wrapper">Seems like a simple fix.
Comment #10
mgiffordActually, let's test this a bit more. Is this patch going to throw off anything else that uses template_preprocess_details?
Comment #11
joelpittet@mgifford it's not, the RTBC is correct fix, because it's passing the object by reference any changes would affect the other. This will create a new object.
The only thing we could look at is maybe what should be expected? Maybe this should always clone/grab the value?
I'm throwing up a fix that is a bit more generic, if this passes it may need test coverage and another manual test @mgifford if you're up for it?
Comment #12
DuaelFrI just marked #2548309: aria-expanded="true" added twice as duplicate of this one.
Some work started there during a little sprint this summer but we were not able to reproduce.
Comment #13
mgifford@joelpittet with the patch, I still get
aria-expanded="false" aria-expanded="false"Comment #14
IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commentedClone approach will not work in the current design of Attribute class — name also will be to cloned, and there is no way to set up a different name after.
I propose to create a new instance of required class directly with value from object to clone from, but with the proper name.
See patch.
Comment #15
IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commentedCorrected comments to this block a bit.
Comment #17
joelpittet@IRuslan you are totally right, thanks a bunch. @mgifford does that fix it for the manual testing? I'd expect it should have the same value but the name should be the new one.
Comment #18
joelpittetComment #19
joelpittetI don't know if this is totally necessary to get into RC, it could easily go into 8.0.1 or something though. Tag it with 'rc target triage' if you can put some rational behind it's inclusion.
Comment #22
mgiffordThat fixes the problem with the install page. Tested it also in /admin/config/people/accounts
Before patch:
After patch:
I think this is ready to go.
Comment #23
xjmDiscussed with @effulgentsia. The problem described in the summary doesn't seem sufficient to us for this to necessitate a change during RC. If there is a more specific accessibility bug beyond that, such that it actually impaired the functionality, please add that information to the summary and retag the issue. Thanks!
Comment #24
mgiffordOk, I'm just wondering when this simple change could get in then.
Can it come in at a 8.0.1 release? It's an annoying redundant bug that just looks bad and doesn't help.
I can just see changing this later causing a lot more work down the line.
Comment #25
joelpittetYes likely @mgifford.
Though if update the issue summary to the proposed latest patch it may get in in RC target.
As the summary doesn't show the nature of the bug.
Comment #26
mgiffordTaking a suggestion from twitter.
@xjm I hope that this clarification helps.
Comment #27
DuaelFrI'd add that this patch only affects markup and contains tests that illustrate the bug. It's not disruptive at all and has been manually tested a lot.
As Mike said, it's not critical but the more accessible and clean we are at the time of the release, the more exemplary we seem for both the a11y and the front end world.
Comment #28
xjmSo having a duplicated attribute really did not seem important enough to fix during RC, but @effulgentsia walked me through the test coverage and I can see how the broader bug (and the loss of the aria-pressed attribute) make this reasonable as an RC change. It could also go into a patch release safely (i.e. 8.0.1).
Also tagging for issue summary update since some bits of the summary are incorrect, others are unclear, and the full scope of the bug isn't covered. Leaving at RTBC though since @effulgentsia seems to have a handle on this issue now.
Comment #29
DuaelFrComment #30
catchCommitted/pushed to 8.0.x, thanks!
Comment #32
joelpittetSilly testbot, tricks are for kids.
Comment #33
mgiffordSo great that this is fixed! Thanks @joelpittet for the bunny jokes. :)