Make WordPress Core

Opened 10 years ago

Closed 2 months ago

#31354 closed defect (bug) (fixed)

Compound settings confusing with screen readers (checkboxes)

Reported by: cheffheid's profile Cheffheid Owned by: joedolson's profile joedolson
Milestone: 6.7 Priority: normal
Severity: normal Version: 4.1
Component: Administration Keywords: settings-api core-fields has-patch commit
Focuses: accessibility Cc:

Description

There are a number of settings in the admin that are made out of multiple variables. That is, they combine a checkbox/radio button toggle and a value.

When you can see it, it makes sense because it forms a logical sentence and everything works out great.

When you rely on a screen reader, it doesn't really come across as such and there's no link between the different inputs in the setting.

For example:

http://i.imgur.com/0r0lDSn.png

As you tab through the different fields, NVDA (with default settings) will announce:

  • Break comments into page with - checkbox (not) checked.
  • top level comments per page and the - edit fifty.
  • page displayed by default - combo box last collapsed.

We need to come up with a pattern that can be applied to split up these toggle/value inputs and clear up confusion this may create.

It would be nice if the inputs for the actual values stay disabled until the appropriate toggle is enabled as well (similar to how the home/blog static page selectors work).

This issue concerns the following fields:

On /wp-admin/options-discussion.php:

  • Automatically close comments on articles older than X days
  • Enable threaded (nested) comments X levels deep
  • Break comments into pages with X top level comments per page and the X page displayed by default.
  • Comments should be displayed with the X comments at the top of each page

On /wp-admin/network/settings.php

  • Limit total size of files uploaded to X MB

Could use input from accessibility and UI teams on how to best tackle this. :)

Attachments (11)

31354.patch (7.7 KB) - added by anthakkar08 10 years ago.
Patch for the issue
31354.2.patch (4.8 KB) - added by anthakkar08 10 years ago.
patch based on https://core.svn.wordpress.org/trunk/ @ 31465
31354.3.patch (4.8 KB) - added by DrewAPicture 10 years ago.
regenerated from root
WP31354_ChangedSettingsOptions001.png (70.4 KB) - added by jwgoedert 7 months ago.
Image of decoupled discussion options fields, need feedback (indents? hidden/disabled state from parent?)
31354_hide.patch (9.2 KB) - added by jwgoedert 7 months ago.
31354 show/hide children patch
31354-show_hide.mp4 (1.7 MB) - added by jwgoedert 7 months ago.
Quick video demo with voiceover audio to convey aria-live state changes
31354_disable.patch (10.4 KB) - added by jwgoedert 6 months ago.
Disable dependent input fields on parent toggle patch-need feedback on accessibility and structure
31354.5.patch (11.4 KB) - added by joedolson 6 months ago.
Updated patch
31354.6.patch (8.9 KB) - added by jwgoedert 6 months ago.
Adjusted previous patch to remove js and php toggle disable setting to focus on structural changes only(bug related)
31354.6.2.patch (9.0 KB) - added by jwgoedert 6 months ago.
Patch for structural changes only, no toggles
31354.7.patch (7.6 KB) - added by joedolson 2 months ago.
Updated patch; language changes

Download all attachments as: .zip

Change History (71)

@anthakkar08
10 years ago

Patch for the issue

#1 @anthakkar08
10 years ago

  • Keywords has-patch needs-testing added

Created a patch for this this requires to be tested with both Normal and Multisite installation so please check this out and do share the feedback

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


10 years ago

#3 @anthakkar08
10 years ago

Previous Patch was creating Some conflict so have updated the Patch with the Latest Trunk copy of https://core.svn.wordpress.org/trunk/ @ 31465

#4 follow-up: @Cheffheid
10 years ago

@anthakkar08 Thank you sir! Could you create the patch from the root folder though (Where the src folder and wp-config.php file resides as well)? I'm having some issues applying it on my install.

@DrewAPicture
10 years ago

regenerated from root

#5 in reply to: ↑ 4 @DrewAPicture
10 years ago

Replying to Cheffheid:

@anthakkar08 Thank you sir! Could you create the patch from the root folder though (Where the src folder and wp-config.php file resides as well)? I'm having some issues applying it on my install.

31354.3.patch is .2 regenerated from the project root.

If you can't get a patch to apply via the grunt patch command, you can always attempt to do it manually by downloading the patch to the sub-directory in question, in this case /wp-admin, and applying it there with the patch command e.g.

$ patch -p0 < 31354.2.patch

#6 @Cheffheid
10 years ago

Thank you sir! I'm one of those noobs that uses TortoiseSVN instead of grunt to apply patches, because Windows, and it kept suggesting me the parent folder of where-ever I was trying to apply it. And then failed to apply it, even on /wp-admin.

In any case, the patch is a good start and the subfields seem to enable/disable appropriately based on the checkbox state.

Would still like a good solution for the labels though. @afercia had already suggested splitting them up so the labels can make sense by themselves. Something like this (rough example):

http://i.imgur.com/VB8JmkB.png

I'll solicit for some more feedback from the a11y peoples too. :)

#7 @afercia
10 years ago

I'm not sure playing with JavaScript can really help here, I'd say it would be a possible enhancement to evaluate after the main issue is solved. IMHO the first thing to do here should be using proper labels that make sense also when read out of context and probably restructure a bit the form. Just use Plain Old Semantic HTML (POSH), nothing fancy, consider HTML give us fieldset and legend elements for free and use them when appropriate.

About the first example:

http://i.imgur.com/0r0lDSn.png

  • the checkbox is meant to enable/disable the setting, it's a general "switch" and this should be clear even when the label is read out of context
  • the other two, are sort of "sub settings", it should be clear they're discarded if the main checkbox is not checked

What I would suggest:

fieldset
legend:                 Comments pagination
checkbox + label:       Break comments into pages
label + input field:    Top level comments per page
label + select:         Comments page to display by default: last page|first page
(added) label + select: Comments to display at the top of each page: older comments|newer comments

Consider this other example:
https://cldup.com/doimEsOQKY.png

Here, the labels are:

"Automatically close comments on articles older than"
"days"

The first one doesn't tell me it's an on/off switch, but says something about "older than". Confusing. The second one says just "days", I don't have a clue what it refers to, when read out of context. This second example is a bit more difficult to handle, still thinking about a proper solution :)

I understand this would require some minor UI changes, so it needs UI feedback and I'm pretty confident we could find a good balance between functionality and visual. Any thougths more than welcome.

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


9 years ago

#9 @rianrietveld
9 years ago

  • Milestone changed from Awaiting Review to Future Release

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


8 years ago

#11 @afercia
8 years ago

  • Keywords settings-api added

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


8 years ago

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


4 years ago

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


2 years ago

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


21 months ago

#16 @joedolson
21 months ago

  • Keywords core-fields added
  • Owner set to joedolson
  • Status changed from new to accepted

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


13 months ago

#18 @joedolson
13 months ago

  • Milestone changed from Future Release to 6.5

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


10 months ago

#20 @joedolson
10 months ago

  • Keywords needs-refresh changes-requested added; has-patch needs-testing removed

#21 @joedolson
10 months ago

  • Milestone changed from 6.5 to Future Release

@jwgoedert
7 months ago

Image of decoupled discussion options fields, need feedback (indents? hidden/disabled state from parent?)

#22 @jwgoedert
7 months ago

Working on this locally. I decoupled the options so there would be explicit check boxes followed by additional related inputs. https://core.trac.wordpress.org/raw-attachment/ticket/31354/WP31354_ChangedSettingsOptions001.png

I am still uncertain the best way to convey the relation of the 'parent' check box element to the affected sub-selections and whether they should be visible but disabled or hidden.

example:'Automatically close comments for old posts' affects whether 'Number of days to keep old comments' is used, but doesn't necessarily have to affect if it is set.

May pursue the way this is handled on 'Reading Settings' with static page selection using a UL for options and disabling selectors.

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


7 months ago

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


7 months ago
#24

  • Keywords has-patch added; needs-refresh removed

This patch addresses the difficulty with using input fields inline with text for screen readers.

For example: 'Breaking comments into pages' with inputs on a single line-
https://github.com/WordPress/wordpress-develop/assets/6878159/0d89dae0-af2b-45b7-812f-80b2bf0a2e31
Becomes: 'Breaking comments into pages' with a parent checkbox that controls styles(visibility in this case-with a disabled version to come) and displays the options one after the other for better screen reader and tab accessibility.
https://github.com/WordPress/wordpress-develop/assets/6878159/73193889-ac7d-46f7-979c-102b546f4b2f

An 'aria-live' div has been added to announce the state of the checkbox and display or removal of dependent elements.

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

@jwgoedert
7 months ago

31354 show/hide children patch

@jwgoedert
7 months ago

Quick video demo with voiceover audio to convey aria-live state changes

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


7 months ago

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


6 months ago
#26

Trac ticket:

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


6 months ago

@jwgoedert
6 months ago

Disable dependent input fields on parent toggle patch-need feedback on accessibility and structure

#28 @joedolson
6 months ago

  • Milestone changed from Future Release to 6.6

Adding to 6.6 milestone; this is looking promising. In accessibility bug scrub, we discussed this and prefer the disabled model of the hidden model, as it then uses the same pattern as on the Reading settings.

#29 @oglekler
6 months ago

  • Keywords needs-testing added; changes-requested removed
  • Milestone changed from 6.6 to 6.7

We have 2 days before Beta 1 and, objectively, no time to test, so I am moving it into the next milestone.

#30 @joedolson
6 months ago

  • Milestone changed from 6.7 to 6.6

Moving back to 6.6. This is on my schedule for today and tomorrow.

@joedolson
6 months ago

Updated patch

#31 @joedolson
6 months ago

This updated patch removes the ARIA live announcement and the aria-expanded attributes. The aria-expanded attribute is not required since the checkboxes do not open and close anything, and their current state is conveyed using the natural semantics of the checkbox input.

The aria-live announcement isn't needed, in my opinion; it would just make things more verbose. If the change was made to something that preceded the current position in the DOM, it might be necessary, but since the change is to items that follow in the DOM, I think it's fine as is.

Edit: and I also added the disabled attribute in PHP, so it would apply without JS.
Also changed the CSS & HTML so to reuse the styles from the Reading options screen, updated the function comments to better describe what the added JS now does, and removed the styles that impacted the label design. It's generally considered acceptable for a disabled input to be below color contrast guidelines, but the label still needs to be visible.

Version 1, edited 6 months ago by joedolson (previous) (next) (diff)

#32 @joedolson
6 months ago

  • Milestone changed from 6.6 to 6.7

While I think this is a highly worthwhile change, I also think it has not had sufficient feedback to go into core this late as an enhancement.

It may be reasonable to re-characterize this as a bug - the existing labeling is a poor experience, but is borderline as a bug. WCAG 3.3.2 requires that fields have labels or instructions that describe the input; and whether that's true or not on these is open to discussion.

I'll raise this issue in a bug scrub to determine whether it's viable to change to a bug for 6.6, but in the meanwhile I'm moving to 6.7.

#33 @jwgoedert
6 months ago

I would have to agree from a screenreader perspective that this falls more under bug territory as the content and quantities to not line up in a discernible way.

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


6 months ago

#35 @joedolson
6 months ago

  • Milestone changed from 6.7 to 6.6
  • Type changed from enhancement to defect (bug)

#36 @joedolson
6 months ago

Per discussion in the accessibility bug scrub, transitioning this to a bug.

@jwgoedert
6 months ago

Adjusted previous patch to remove js and php toggle disable setting to focus on structural changes only(bug related)

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


6 months ago
#37

Nested child elements in list items to create visual hierarchy, moved input fields to end of labels for screen reader usability
Trac ticket:
https://core.trac.wordpress.org/attachment/ticket/31354/

@jwgoedert
6 months ago

Patch for structural changes only, no toggles

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


6 months ago

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


6 months ago

#40 @sabernhardt
6 months ago

  • Milestone changed from 6.6 to 6.7

The new patch does not have testing yet, and it contains new text strings. I'll move the ticket to 6.7 (but it still could move back in the next week if a committer is confident).

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


4 months ago

This ticket was mentioned in Slack in #core-test by ankit-k-gupta. View the logs.


4 months ago

#43 @sannevndrmeulen
4 months ago

Test Report

Patch tested: https://core.trac.wordpress.org/attachment/ticket/31354/31354.6.2.patch

Environment

  • OS: macOS Sonoma 14.5
  • WordPress: 6.7-alpha-58576-src
  • Browser: Google Chrome Version 126.0.6478.114

Actual Results

  • After applying the patch I see that certain discussion settings are displayed one after the other for better screen reader and tab accessibility (see attachment).

Additional Notes

  • I have only tested Discussion Settings.
  • I noticed that when I change the 14 for Number of days to keep old comments to 13 the screen reader says selection replaced 3, somehow I was expecting it to say 13?

Supplemental Artifacts

https://imgur.com/ORzLkxc

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


3 months ago

#45 @sabernhardt
3 months ago

#61976 was marked as a duplicate.

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


3 months ago

#47 @sudipatel007
2 months ago

Test Report

Tested Patch: https://core.trac.wordpress.org/attachment/ticket/31354/31354.6.2.patch

Environment

  • WordPress: 6.7-alpha-59086
  • PHP: 8.2.23
  • Server: Apache/2.4.62 (Debian)
  • Database: mysqli (Server: 11.5.2-MariaDB-ubu2404 / Client: mysqlnd 8.2.23)
  • Browser: Chrome 129.0.0.0 (macOS)
  • Theme: Twenty Twenty-Four 1.2
  • MU-Plugins: None activated
  • Plugins:
    • WordPress Beta Tester 3.5.6

Results
I noticed that certain discussion settings are now displayed sequentially to enhance compatibility with screen readers after the patch was applied.

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


2 months ago

#49 @joedolson
2 months ago

  • Keywords needs-testing removed

@joedolson
2 months ago

Updated patch; language changes

#50 @joedolson
2 months ago

Updated patch makes a couple of text changes; one because the text used for the age of post to automatically close comments was labeled "Number of days to keep old comments", which is not accurate.

Other changes are minor wording clarifications.

#51 @joedolson
2 months ago

  • Keywords commit added

@sannevndrmeulen The screen reader says "selection replaced" or "selection removed" because the value of the value is selected when you focus into the input field. When you type or change the value using arrow keys, the selection highlight is removed, and that's announced to the user. It's as expected!

#52 @tirth03
2 months ago

Test Report

Patch tested: https://core.trac.wordpress.org/attachment/ticket/31354/31354.7.patch

Environment

  • OS: Ubuntu 22.04.4 LTS
  • WordPress: 6.7-alpha-58576-src
  • Browser: Chrome (127.0.6533.119)
  • Theme: Twenty Twenty-One
  • Active Plugins:
    • None

Actual Results

  • ✅ Issue resolved with patch.

Additional Notes

  • If I don't select the check box, Shouldn't it supposed go to the next option? Right now it is going inside of that option, if it's not selected

Supplemental Artifacts

https://i.postimg.cc/HsWRXT3t/Screenshot-from-2024-10-02-14-11-00.png

#53 @joedolson
2 months ago

@tirth03 If I understand your question correctly, you're expecting that if one of the parent options, e.g. "Break comments into pages" is not selected, then the next tab action should move past the related options.

We did consider making those options dynamically enabled, but decided that it was not necessary to change when the settings were available. It is entirely feasible that somebody might want to change those settings without enabling the parent option.

Thanks for testing!

#55 @joedolson
2 months ago

In 59160:

Administration: A11y: Clarify settings in discussion options.

Fix several settings groups in the discussion options that were written in a compound/sentence structure format. These formats are difficult to parse for screen reader users and have significant layout problems in mobile viewports.

Change settings to use independent labeling.

Props Cheffheid, anthakkar08, DrewAPicture, afercia, jwgoedert, sannevndrmeulen, sudipatel007, tirth03, joedolson.
See #31354.

@joedolson commented on PR #6595:


2 months ago
#57

Closed in favor of later PR

@joedolson commented on PR #6659:


2 months ago
#58

Closed in favor of later PR

#60 @joedolson
2 months ago

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

Accidentally marked this with a see reference in the commit message instead of a fixes reference. Closing as fixed.

Note: See TracTickets for help on using tickets.