WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 3 weeks ago

#33085 reviewing defect (bug)

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

Reported by: afercia Owned by:
Milestone: 4.9.1 Priority: high
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 (3)

33085.diff (6.4 KB) - added by valendesigns 2 years ago.
33085.2.diff (12.8 KB) - added by valendesigns 2 years ago.
33085.base-control.3.diff (10.6 KB) - added by westonruter 6 weeks ago.

Download all attachments as: .zip

Change History (51)

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


2 years ago

#2 @westonruter
2 years ago

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

#3 @valendesigns
2 years ago

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

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


2 years ago

#5 @celloexpressions
2 years 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
2 years ago

#6 @valendesigns
2 years 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
2 years 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
2 years ago

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

#9 @wonderboymusic
2 years ago

soon please

#10 @westonruter
2 years 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
2 years ago

  • Keywords needs-testing removed

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

@valendesigns
2 years ago

#12 @valendesigns
2 years 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
2 years ago

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

#14 @afercia
2 years 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.


2 years ago

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


2 years ago

#17 @westonruter
2 years 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
14 months ago

  • Milestone changed from Future Release to 4.7

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


14 months ago

#21 @westonruter
14 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.


14 months ago

#23 @afercia
14 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.


13 months ago

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


13 months ago

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


13 months ago

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


13 months ago

#28 @celloexpressions
13 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
10 months ago

  • Milestone changed from Future Release to 4.8

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


10 months ago

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


6 months ago

#32 @afercia
6 months ago

  • Owner set to westonruter

Needs a decision for the 4.8 release or punt? /cc @westonruter

#33 @westonruter
6 months ago

  • Milestone changed from 4.8 to 4.8.1

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


6 months ago

#35 @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.


4 months ago

#37 @westonruter
2 months ago

  • Priority changed from normal to high

Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.

#38 @westonruter
7 weeks ago

#42079 was marked as a duplicate.

#39 @westonruter
6 weeks ago

In 41740:

Customize: Improve accessibility of markup for base WP_Customize_Control and WP_Customize_Nav_Menu_Control with proper use of label elements and inclusion of aria-describedby.

See #33085.
Props valendesigns, afercia, westonruter.

#40 @westonruter
6 weeks ago

  • Owner changed from westonruter to afercia

I'll leave the other controls to your capable hands.

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


6 weeks ago

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


6 weeks ago

#43 @westonruter
6 weeks ago

In 41817:

Customize: Fix vertical alignment of radio and checkbox inputs after [41740].

Props Shital Patel, subrataemfluence, sayedwp.
Amends [41740].
See #33085.
Fixes #42157.

#44 @westonruter
6 weeks ago

In 41822:

Customize: Prevent outputting value attribute if already supplied among input_attrs.

This allows for input[type=button] controls to be added without producing illegal HTML.

Amends [41740].
See #30738, #33085.

#45 @afercia
5 weeks ago

  • Owner afercia deleted

I'm sorry but realistically I don't have time to handle the other controls in the next week, nor I've ever asked to own this ticket. This doesn't need just a review, it needs actual work to complete the unfinished job. On a side note, I think assigning to someone a ticket that still needs some work without asking before if he/she has time to dedicate to the ticket is not a good approach.

#46 @westonruter
5 weeks ago

  • Milestone changed from 4.9 to 4.9.1

Punting.

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


5 weeks ago

#48 @westonruter
3 weeks ago

In 42031:

Customize: Improve Media control accessibility and compatibility for settings passed as arrays or as solitary setting.

  • Eliminate Media control template from having dependency on params.settings.default for element ID, to fix compat with params.settings array or single params.setting. See #36167.
  • Move description out of label and add aria-describedby to Media control's Select button. See #30738, #33085.
  • Obtain notification container whenever content is (re-)rendered (such as for Media control). See #38794.
  • Re-render notifications after control content is re-rendered, if control is in expanded section. See #38794.

Amends [41390].
See #36167, #38794, #33085, #30738.

Note: See TracTickets for help on using tickets.