Make WordPress Core

Opened 7 years ago

Closed 8 weeks ago

Last modified 5 weeks ago

#42002 closed defect (bug) (fixed)

Improve the accordions accessibility

Reported by: afercia's profile afercia Owned by: joedolson's profile joedolson
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-screenshots has-patch commit
Focuses: ui, accessibility Cc:

Description

Splitting this out from #37013, props to monikarao, xavortm, mihai2u, Kopepasah for the work done there.

In #37013 was noted that the toggle "arrows" of the accordions in the Menus screen don't have the circular shape to indicate keyboard focus. This is inconsistent with other similar controls that do use the circular focus and could be improved.

https://cldup.com/WXR2bcgPQp.png

However, the accordions in the Menus screen are generated with do_accordion_sections(): plugins or themes might use this function for their own purposes and any change here should be carefully considered and well communicated.

In core, as far as I see, the Menus screen is the only place where do_accordion_sections() is used. In other places, for example the Customizer, the accordions markup is output by a custom implementation. The JS part instead, if I'm not wrong, is still shared and uses accordion.js.

This would be also a good opportunity to improve the accordions accessibility in core and standardize all the different implementations.

I'd recommend to follow the example on the ARIA Authoring Practices, see https://www.w3.org/TR/wai-aria-practices/#accordion and see the example on https://www.w3.org/TR/wai-aria-practices/examples/accordion/accordion.html where the accordion "titles" use a button inside a heading (note: the example uses <dt role="heading" aria-level="3"> because it's an ARIA example :) that's equivalent to a heading).

Attachments (10)

42002.patch (1.3 KB) - added by rishishah 7 years ago.
42002.2.patch (1.3 KB) - added by rishishah 7 years ago.
Here is the proper patch from src.
42002.01.02.24.diff (3.2 KB) - added by kushang78 10 months ago.
Added patch for this ticket.
42002.diff (3.3 KB) - added by kushang78 9 months ago.
Added latest patch for this ticket.
42002.jpg (264.2 KB) - added by rcreators 9 months ago.
I manually copied code from the patch was able to apply it. The tab index issue is solved with tab-index -1 for button. So tabbing works fine now. Yet css needs to be updated, so both left and right side have same appearance in each state
42002.2.diff (3.3 KB) - added by kushang78 8 months ago.
Added latest patch for this ticket.
42002.3.diff (3.2 KB) - added by kushang78 8 months ago.
Added latest patch for this ticket.
Customizer screen.jpg (201.7 KB) - added by krupajnanda 6 months ago.
menu.jpg (127.0 KB) - added by krupajnanda 6 months ago.
Site health.jpg (271.8 KB) - added by krupajnanda 6 months ago.

Download all attachments as: .zip

Change History (58)

@rishishah
7 years ago

#1 @rishishah
7 years ago

Hello afercia,

Please review my patch and let me know if anything.

Please see changes here: https://ibb.co/gfniB5

Thanks,

@rishishah
7 years ago

Here is the proper patch from src.

#2 @afercia
7 years ago

@rishishah thanks for the patch! What I meant is a bit more substantial chance though, making the heading contain just a button with some proper text. This would require some more changes. Basically something like:
<h3><button ...>accordion title here<span aria-hidden="true" class="toggle-indicator"></span></button></h3>

The span with icon should go inside the button.

This would match the aria example and allow to remove some of the screen reader text there. Also, it would be nice to remove the word "toggle" sinte it's difficult or impossible to translate in some languages, see #34753

With this changes, also the JS part should be reviewed to ensure it works properly.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

#4 @afercia
7 years ago

  • Milestone changed from Awaiting Review to Future Release

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.


6 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 years ago

#8 @afercia
6 years ago

Note: the new Site Health feature that is going to land in WordPress 5.2 adds some new accordions, see #46573.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

#11 @joedolson
14 months ago

  • Milestone changed from Future Release to 6.5
  • Owner set to joedolson
  • Status changed from new to accepted

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


14 months ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


13 months ago

@kushang78
10 months ago

Added patch for this ticket.

#14 @kushang78
10 months ago

  • Keywords has-patch added

@afercia,

I've worked upon your suggestion as you mentioned above. Here, I've attached the latest patch for that. So, Please check it and drop your feedback on it!

Thanks!

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


10 months ago

#16 @joedolson
10 months ago

  • Keywords needs-testing added

#17 @rcreators
9 months ago

Hello @kushang78,

I checked the patch. There is some tabbing order issue with the accordion. When we change from one item to another with the tab key, there is something hidden area which is getting focused and pressing enter to open/close the accordion as well. Which does not appear to be happening for right-side menu items.

For reference see this video: https://screencast-o-matic.com/watch/cZnO6LVKbhF

Also, the focus border of the icon does not match the right-side menu item. It needs to be the same as well.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


9 months ago

#19 @joedolson
9 months ago

  • Milestone changed from 6.5 to 6.6

With a week until release candidate, this isn't ready yet. Moving to 6.6 so it has a bit more time.

There are some CSS issues with inconsistent focus design in addition to the tabbing issue that needs addressing.

#20 @joedolson
9 months ago

  • Keywords changes-requested added

@kushang78
9 months ago

Added latest patch for this ticket.

#21 @kushang78
9 months ago

Thanks for the feedback @rcreators @joedolson

I’ve attached the latest patch for this.

Thanks!

#22 @rcreators
9 months ago

@kushang78 This patch failed. Can you check on it? The below lines are error codes.

error: patch failed: src/wp-admin/css/common.css:2357
error: src/wp-admin/css/common.css: patch does not apply
error: patch failed: src/wp-admin/includes/template.php:1574
error: src/wp-admin/includes/template.php: patch does not apply

#23 @kushang78
9 months ago

@rcreators ,

In my case, The patch is working fine without any errors using below 2 methods that I've tried!

  • Using Command, git apply 42002.diff
  • Using Git Patch extension in VS Code

Usually, I've applied the patch using Git Patch extension in VS Code.

Thanks!

Version 0, edited 9 months ago by kushang78 (next)

@rcreators
9 months ago

I manually copied code from the patch was able to apply it. The tab index issue is solved with tab-index -1 for button. So tabbing works fine now. Yet css needs to be updated, so both left and right side have same appearance in each state

@kushang78
8 months ago

Added latest patch for this ticket.

@kushang78
8 months ago

Added latest patch for this ticket.

#24 @kushang78
8 months ago

Please consider 42002.3.diff file as latest patch. Thanks!

This ticket was mentioned in Slack in #accessibility by rcreators. View the logs.


8 months ago

#26 @devsahadat
8 months ago

@kushang78 I just checked it and it's working fine. Thanks

This ticket was mentioned in Slack in #accessibility by rcreators. View the logs.


7 months ago

#28 @joedolson
7 months ago

Let's move the event trigger from .accordion-section-title to the button; that will allows us to remove the extraneous tabindex attributes, and will make the keyboard handling more standard.

It should also allow us to remove the keydown event handler from accordion.js, since the button will pass events natively.

Be sure to also check accordions in the Customizer and in the Site Health UI to make sure everything is consistent across those features.

#29 @hmbashar
6 months ago

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: 42002.3

Environment

  • WordPress: 6.6-alpha-57778-src
  • PHP: 8.3.7
  • Server: nginx/1.25.4
  • Database: mysqli (Server: 8.3.0 / Client: mysqlnd 8.3.7)
  • Browser: Firefox 126.0
  • OS: macOS
  • Theme: Twenty Fifteen 3.7
  • MU Plugins: None activated
  • Plugins:
    • FakerPress 0.6.6
    • Test Reports 1.1.0

Actual Results

  1. ✅ Issue resolved with patch.

Screenshots

Before Patch
https://i.ibb.co/84Cmxym/Screenshot-at-Jun-09-6-51-34-PM.png

After Patch
https://i.ibb.co/nQ12rSg/After-Patch.png

This ticket was mentioned in Slack in #accessibility by rcreators. View the logs.


6 months ago

#31 @krupajnanda
6 months ago

Test Report

Patch tested: https://core.trac.wordpress.org/attachment/ticket/42002/42002.3.diff

Environment

  • WordPress: 6.5.4
  • PHP: 8.2.15
  • Server: nginx/1.25.3
  • Database: mysqli (Server: 8.0.36 / Client: mysqlnd 8.2.15)
  • Browser: Chrome 125.0.0.0
  • OS: macOS
  • Theme: Twenty Sixteen 3.2
  • MU Plugins:
    • Safe Autoloaded Options Limit Test (MU Plugin) 1.0
  • Plugins:
    • Test Reports 1.1.0

Additional Notes

  • Tested under Menu, Customiser and Site Health

For Menu and Site Health : I found that the accordion is not high lighted instead it high lights the entire row. Please check the attachment for the same.

For Customiser: I found that accordion is also get high lighted.

I am not sure if this comparison is valid. But I would really appreciate if some can take a look.

Supplemental Artifacts

Add as Attachment

@krupajnanda
6 months ago

#32 @krupajnanda
6 months ago

  • Keywords 2nd-opinion added

This ticket was mentioned in Slack in #core-test by krupajnanda. View the logs.


6 months ago

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


6 months ago

#35 @sabernhardt
6 months ago

  • Milestone changed from 6.6 to 6.7

Let's work on this in the next cycle.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


5 months ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


4 months ago

This ticket was mentioned in Slack in #core by stoyangeorgiev. View the logs.


3 months ago

This ticket was mentioned in Slack in #accessibility by rcreators. View the logs.


3 months ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


2 months ago

This ticket was mentioned in PR #7531 on WordPress/wordpress-develop by @joedolson.


2 months ago
#41

  • Change event handlers to directly address buttons
  • Remove unneeded handling of tabindex attributes
  • Remove unneeded screen reader text now that accordions have appropriate markup

Trac ticket: https://core.trac.wordpress.org/ticket/42002

#42 @joedolson
2 months ago

  • Keywords changes-requested 2nd-opinion removed

Updated this to move the event handler to the button and extend handling to cover the customizer.

The changes in the customizer make use of the :has selector, which is possibly a bit marginal for our browser support rules. The gap is in the UC Browser for Android, which is just above our support guidelines at 1.14%.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


2 months ago

#44 @joedolson
8 weeks ago

  • Keywords commit added; needs-testing removed

In my opinion, the amount of extra code required to handle this CSS in order to cover .14% usage are not justified, given that the result of the lack of support is a minor visual difference.

Based on my own testing, I think this is ready for commit.

Changes:

1) Accordions now contain immediate children button elements with aria-controls attributes pointing to their controlled content.
2) Text provided to explain how to use the accordions in screen readers removed. (No longer required since these are now buttons with correct markup.)
3) Event triggers moved to button elements.
4) Unneeded tabindex="0" removed, as headings no longer need to be made navigable.
5) Tests updated.

Primary visual change is that the nav menu accordions have a visible :focus state, previously missing. It is the standard focus state, identical to the existing state in Site Health.

#45 @joedolson
8 weeks ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 59224:

Administration: A11y: Fix accordion accessibility.

Change accordions in the customizer and the navigation menus to make proper usage of accordion markup patterns. This includes adding missing :focus states, using a button element to control tabbing and interaction, instead of the heading elements, and removing instructional text for screen reader users that was used to compensate for the incorrect markup pattern.

Props afercia, rishishah, kushang78, rcreators, krupajnanda, hmbashar, joedolson.
Fixes #42002.

@joedolson commented on PR #7531:


8 weeks ago
#46

In r59224

#47 @joedolson
8 weeks ago

I did not choose to merge the implementations of Site Health and the Nav Menus accordion sections in this change. I think that's worthwhile, but separate from the accessibility issue, and I'm opening a separate issue for that.

#48 @wildworks
5 weeks ago

The all: unset; defined here also resets box-sizing:border-box, which results in overflowing buttons in the Customizer. Please see #62313.

Note: See TracTickets for help on using tickets.