WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 3 months ago

#33085 reviewing defect (bug)

Customizer: controls description inside labels are not real labels nor descriptions

Reported by: afercia Owned by:
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.2
Component: Customize Keywords: needs-patch
Focuses: ui, accessibility Cc:

Description

Splitting this out from #32493 (which changed its scope)

I'd propose to investigate about the Customizer controls descriptions. Currently, they're inside the label element. The result is they're not a proper form label, since labels should be short. Longer descriptions should be associated with form controls using aria-describedby.

As consequence for accessibility: as soon as a form control is focused, its label gets read out. In the following example from the new "Site Icon" functionality in 4.3, all the text highlighted in the blue box will be read out but the actual button text "Select File" won't. Buttons can have labels, that's correct HTML, but of course the label text will override the button text.

Not to mention all the label text is clickable and will activate the button when clicked.

https://cldup.com/YrBf73CbMo.png

Same for input fields, the control description used as label will be read out as... a label.

https://cldup.com/cgetk_WbNT.png

Related: #31540

Attachments (2)

33085.diff (6.4 KB) - added by valendesigns 18 months ago.
33085.2.diff (12.8 KB) - added by valendesigns 18 months ago.

Download all attachments as: .zip

Change History (32)

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


20 months ago

#2 @westonruter
20 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.4

#3 @valendesigns
20 months ago

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

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


19 months ago

#5 @celloexpressions
19 months ago

For reference, this pattern is used by pretty much all of the core Customizer controls currently. Probably shouldn't have been done that way in #27981, but it was.

@valendesigns
18 months ago

#6 @valendesigns
18 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

@afercia Could you please test out 33085.diff and see if the approach works for screen readers. I would still need to update custom controls, this patch only updates the controls in WP_Customize_Control::render_content. However, I want to verify this is the correct direction before tackling the custom controls. Which are not as easy and straight forward as these default ones were to change, mainly because most of them are JS templates.

#7 @afercia
18 months ago

@valendesigns Coulnd't find any of the "default" controls from WP_Customize_Control::render_content with a description so I've manually added a couple of them, see in the screenshot below how they're announced using Firefox + NVDA: works out the way it's supposed to :)

https://cldup.com/XiW379Vj6i.png

Having the labels associated with the for attribute (i.e. no more "wrapping" the control) is a nice improvement too especially thinking at a way to provide the ability to hide labels, see #33064.

#8 @valendesigns
18 months ago

@afercia Awesome! I'll roll the changes out to the other controls and create a new patch. Thanks for verifying.

#9 @wonderboymusic
18 months ago

soon please

#10 @westonruter
18 months ago

  • Keywords needs-patch added; has-patch removed

@valendesigns Will you be able to update the patch today? This needs to be committed today ideally so we don't cut it too close to RC1.

#11 @valendesigns
18 months ago

  • Keywords needs-testing removed

Sure, I'll take another crack at it when I wake up. Right now I'm going to sleep.

#12 @valendesigns
18 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

@westonruter As promised, the patch has been updated.

@afercia Could you please test the markup changes if you have time?

#13 @westonruter
18 months ago

  • Owner changed from valendesigns to afercia
  • Status changed from assigned to reviewing

#14 @afercia
18 months ago

@valendesigns tried to do my best, please consider I'm not so familiar with the Customizer. For example, about the Media control I've checked just the Site Icon not sure if all the other controls that inherit from Media control use the same template. I guess they should. The controls I've seen look OK to me, the descriptions are moved out from the labels and the aria-describedby attributes just work.

The color picker is a bit weird since the description gets read out only in a second moment, when the control is expanded and the real input is focused. I guess there's not much we can do here for now.

https://cldup.com/Kev0UoD85C.png

From a UI perspective, I'm not sure the descriptions should be printed out before their controls, maybe they should be moved after, probably something to consider for the next release :)

All in all I think this is a very nice improvement.

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


18 months ago

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


18 months ago

#17 @westonruter
18 months ago

  • Milestone changed from 4.4 to Future Release

Punting due to the size of the patch in relation to the start of RC. Let's commit at the beginning of 4.5.

#19 @westonruter
8 months ago

  • Milestone changed from Future Release to 4.7

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


8 months ago

#21 @westonruter
8 months ago

  • Keywords needs-patch added; has-patch removed

The label needs to go _inside_ the customize-control-title not wrapping around it. This is because notifications container gets injected after the title and we don't want notifications to appear inside the label.

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


7 months ago

#23 @afercia
7 months ago

  • Owner changed from afercia to westonruter

@westonruter assigning to you, since you set the milestone and I can't be of any help here :)

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


7 months ago

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


6 months ago

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


6 months ago

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


6 months ago

#28 @celloexpressions
6 months ago

  • Keywords needs-testing removed
  • Milestone changed from 4.7 to Future Release
  • Owner westonruter deleted

This is fixed by #30738, so we'll wait until we can address both in the same release.

#29 @westonruter
3 months ago

  • Milestone changed from Future Release to 4.8

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


3 months ago

Note: See TracTickets for help on using tickets.