WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 7 weeks ago

Last modified 4 weeks ago

#39662 closed defect (bug) (fixed)

Color picker accessibility improvements

Reported by: afercia Owned by: afercia
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-screenshots has-patch commit
Focuses: ui, accessibility, javascript Cc:

Description

While investigating #39096, I've noticed the color picker control could be greatly improved and simplified. The main accessibility issue is about the HTML output, which turns out to be confusing for screen readers.

Ideally, a <label> element should contain just the label text (if it's explicitly associated) or the label text and the associated form control (if it's implicitly associated). That is what screen readers (and any other software) expect to find.

In this case instead, the <label> element contains other controls (a link, a button, the notification container, and the whole iris color picker). Not surprisingly, this can make screen readers not able to understand "what" the color picker is and how it is supposed to work.

Testing with Safari 10 + VoiceOver, the confusion is so severe that VoiceOver starts announcing "Safari busy" and then just hangs on a "busy" state:

https://cldup.com/zvk6tk3Bk8.png

Note: I think the message "Safari busy" is unrelated to the real issue. I guess what's happening here is VoiceOver just assumes it has to wait for Safari to correctly expose the information it needs, but that never happens.

Attachments (5)

39662.diff (9.2 KB) - added by afercia 9 months ago.
39662.2.diff (12.3 KB) - added by afercia 9 months ago.
39662.3.diff (12.6 KB) - added by afercia 8 weeks ago.
39662.4.diff (12.7 KB) - added by afercia 7 weeks ago.
39662.5.diff (13.1 KB) - added by afercia 7 weeks ago.

Download all attachments as: .zip

Change History (28)

@afercia
9 months ago

#1 @afercia
9 months ago

  • Keywords has-patch added; needs-patch removed

39662.diff is just a first pass. There's still the need to check a few things, for example the notification container and the hue control. What the proposed patch does so far:

  • the toggle button is now a real button
  • thus, it needs just a click event: no need to handle the Spacebar and Enter keys separately
  • the label element now contains just the label text and the input field
  • refactored the HTML to have a better overall structure
  • simplified the way elements visibility gets toggled
  • added an aria-expanded attribute on the toggle button
  • added aria-label attributes on the "Default/Clear" button
  • CSS clean-up
Last edited 7 months ago by afercia (previous) (diff)

#2 @afercia
9 months ago

  • Owner set to afercia
  • Status changed from new to assigned

#3 @afercia
9 months ago

Forgot to mention it also needs to check what happens when the color picker is used outside of the Customizer.

Last edited 7 months ago by afercia (previous) (diff)

#4 @celloexpressions
9 months ago

  • Milestone changed from Awaiting Review to 4.8

This looks like a good start. It should be safe to make all of these changes but let's double-check touch, mouse, keyboard, and screen reader with the final patch to be sure.

@afercia
9 months ago

#5 @afercia
9 months ago

  • Keywords needs-testing added

Yep, 39662.diff works in the Customizer but doesn't in the standalone version. The new patch takes a different approach and uses a default label to wrap the input field. This way, either in the standalone version or in the Customizer one, there's always a label that wraps the input field and it can be used as "hook" in the DOM to inject all the other elements.

The color picker could probably benefit from some kind of JS templating, and that would be a nice improvement for the future. For back-compat reasons, I guess the safer option for now is to stick to jQuery methods to build all the HTML.

39662.2.diff improves a bit also the responsive view. To test the standalone version, it's possible to directly access the old wp-admin/themes.php?page=custom-header and wp-admin/themes.php?page=custom-background pages, if the active theme supports them (try with Twenty Sixteen).

Note: the patch includes some labels for the Iris color picker meant for future improvements. Iris could be greatly improved for accessibility, however some of the required changes should be implemented upstream. Will open a separate ticket.

Responsive view, no default color:

https://cldup.com/Euj7jm3pvW.png

Responsive view, with default color:

https://cldup.com/Xxrbg6EMgE.png

In other languages, the translated strings may be a bit longer and there's not enough space. The input field and the Clear/Default button drop below the toggle button but that doesn't look too bad:

https://cldup.com/0EETPm2zHC.png

Last edited 9 months ago by afercia (previous) (diff)

#6 @afercia
9 months ago

Couple additional things to mention:

  • the patch removes moving the focus on the input field: for better accessibility, focus should stay on the toggle button to make 'aria-expanded` work properly
  • the preset palette colors on the bottom have now a focus style

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


9 months ago

#8 @afercia
9 months ago

Forgot to mention the patch removes:

  • the title attribute: because it just repeats the initial button text
  • the switching of the button text to "Current Color" since it doesn't add a great value and it's potentially confusing for assistive technologies users

https://cldup.com/2W-XlIlZtq.png

#9 @afercia
6 months ago

  • Milestone changed from 4.8 to Future Release

Punting as we're running out of time for the 4.8 release.

#10 @afercia
6 months ago

  • Milestone changed from Future Release to 4.8.1

#11 @Cheffheid
5 months ago

I'm testing in Firefox, with NVDA, on Windows. Some of these issues are not directly related to your changes, so I'm sorry if this is not the place for them. :)

When I toggle a colour picker field (in the Customizer) it doesn't announce the change in the aria-expanded state. I can tell it's changing, so I'm not sure what's going on there (will investigate further).

Additionally, while we're on the subject of improvements, when you enter an invalid hex code (like, #0000) the field turns red. But no feedback is given by NVDA that the value is invalid. I can still hit the "Save & Publish" (assuming it is active, it doesn't always enable when the value is invalid), but my value is never saved. I only find this out after refreshing.

#12 @westonruter
5 months ago

  • Milestone changed from 4.8.1 to 4.9

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


3 months ago

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


8 weeks ago

@afercia
8 weeks ago

#15 @afercia
8 weeks ago

Refreshed patch.

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


8 weeks ago

#17 @afercia
8 weeks ago

@Cheffheid I've finally found some time to check this. I can confirm Firefox+NVDA don't announce aria-expanded. Crazily enough, it works with IE 11 + NVDA, and even with Chrome + NVDA (which is known to not be the best combination). It works with IE 11 and JAWS and Firefox and JAWS.

After some hours of debugging, I've found that removing the CSS class button (or simply renaming it, say to xxxbutton) fixes it. No idea why, seems not related to CSS to me. Can you please try this, ignoring the temporarily broken style? The CSS class is set in the wp-admin/js/color-picker.js file:
_before = '<button type="button" class="button wp-color-result" aria-expanded ...

@afercia
7 weeks ago

#18 @afercia
7 weeks ago

No idea why, the CSS rule triggering this bug is the transform applied to the buttons when they're in an :active state, i.e. while they're "clicked". See transform: translateY(1px); in buttons.css.
(just a wild guess: Firefox repaints the button and in combination with "something else" this makes NVDA think it's landed on a new element).

The refreshed patch:

  • resets the CSS transform for this button
  • cleans up a CSS rule that as using a no more used selector
  • removes translatable strings that were introduced to test future improvements and not actually used in this patch

aria-expanded is now announced correctly by al the major screen readers.

#19 @afercia
7 weeks ago

  • Keywords commit added; needs-testing removed

Things still to check here were:

  • the hue control (if it's affected in any way)
  • the container for the customizer notifications
  • the input field size in the responsive view

Everything looks good to me. Screenshot:

https://cldup.com/n1MZQwpGjt.png

@afercia
7 weeks ago

#20 @afercia
7 weeks ago

39662.5.diff slightly improves the responsive view. Worth noting is basically impossible to achieve a pixel-perfect layout across browsers and platforms. Since the introduction of system fonts (see #36753), some form elements that base their height on the line-height value get rendered differently depending on the platform font metrics. This is particularly noticeable when buttons use line-height: normal. To improve consistency across platforms and different system fonts, great part of the WordPress CSS should be refactored and this is out of the scope of this ticket.

Worth also noting buttons in the Customizer use an additional media query at 640px, so there's the need of some additional adjustments.

Screenshots to recap:

Closed and open, before and after:

https://cldup.com/Oqj38X9uli.png

Responsive view in macOS Chrome, before and after

https://cldup.com/8MBlwNbsw5.png


#21 @afercia
7 weeks ago

  • Component changed from Customize to Administration
  • Focuses ui added

Changing component to "Administration" since the color picker is not specific to the Customizer.

#22 @afercia
7 weeks ago

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

In 41329:

Accessibility: Improve the color picker UI accessibility, interaction, and generated markup.

  • Refactors the UI controls around the Iris color picker to output valid and semantic markup
  • Simplifies the way elements visibility gets toggled
  • Properly associates the visually hidden label with the color input field
  • Makes the toggle button a real button
  • Adds aria-expanded to the toggle button
  • Keeps focus on the toggle button instead of moving it to the color input field
  • Adds aria-label attributes to give better context to some controls
  • Removes a redundant title attribute
  • Keeps the toggle button text to "Select Color" instead of changing it to "Current Color" when a color is selected
  • Slightly improves the responsive view
  • CSS clean-up

Fixes #39662.

#23 @SergeyBiryukov
4 weeks ago

In 41568:

Admin CSS: Autoprefixer for [41329].

See #39662.

Note: See TracTickets for help on using tickets.