WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 9 months ago

Last modified 7 months 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 16 months ago.
39662.2.diff (12.3 KB) - added by afercia 16 months ago.
39662.3.diff (12.6 KB) - added by afercia 9 months ago.
39662.4.diff (12.7 KB) - added by afercia 9 months ago.
39662.5.diff (13.1 KB) - added by afercia 9 months ago.

Download all attachments as: .zip

Change History (31)

@afercia
16 months ago

#1 @afercia
16 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 14 months ago by afercia (previous) (diff)

#2 @afercia
16 months ago

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

#3 @afercia
16 months ago

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

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

#4 @celloexpressions
16 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
16 months ago

#5 @afercia
16 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 16 months ago by afercia (previous) (diff)

#6 @afercia
16 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.


16 months ago

#8 @afercia
16 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
13 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
13 months ago

  • Milestone changed from Future Release to 4.8.1

#11 @Cheffheid
13 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
12 months ago

  • Milestone changed from 4.8.1 to 4.9

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


10 months ago

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


9 months ago

@afercia
9 months ago

#15 @afercia
9 months ago

Refreshed patch.

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


9 months ago

#17 @afercia
9 months 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
9 months ago

#18 @afercia
9 months 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
9 months 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
9 months ago

#20 @afercia
9 months 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
9 months 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
9 months 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
8 months ago

In 41568:

Admin CSS: Autoprefixer for [41329].

See #39662.

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


7 months ago

#25 @aristath
7 months ago

@afercia Just a note on this one:

A lot of plugins and themes use 3rd-party scripts to modify the default behaviour of colorpickers and add an alpha channel to them. The most widely used script to accomplish that is https://github.com/kallookoo/wp-color-picker-alpha The changes in this ticket are great, the only downside is that most 3rd-party scripts (including the wp-color-picker-alpha mentioned above) will now fail because of the html changes in the colorpickers. Ideally rgba would be possible without using any hacky scripts (see https://core.trac.wordpress.org/ticket/39681) but for the time being Plugin & Theme authors should be very careful and release updates before WP 4.9 is released. Some support tickets will be inevitable though until everyone is up to speed and everything gets updated.

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

#26 @afercia
7 months ago

@aristath thanks. Seems to me they're aware of this change, see https://github.com/kallookoo/wp-color-picker-alpha/issues/10

Worth noting the markup to build controls around the Iris color picker is not part of any standardized UI. Plugins or themes that extend this part are actually hacking around it. No objections to that, but they should be aware that this code is not guaranteed to be immutable and they should follow closely core development to update timely.

Note: See TracTickets for help on using tickets.