WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 months ago

Last modified 2 months ago

#18650 closed enhancement (fixed)

Make archives and categories widgets dropdown ada compliant

Reported by: jlevandowski Owned by: joedolson
Milestone: 4.2 Priority: normal
Severity: major Version: 3.2.1
Component: Widgets Keywords: has-patch commit
Focuses: accessibility Cc:

Description

Conditionally add the <label> tag for the archives and categories widgets so they are ada compliant.

http://webaim.org/techniques/forms/controls

Attachments (11)

widgets-patch.diff (1.8 KB) - added by jlevandowski 4 years ago.
18650-patch.diff (2.5 KB) - added by jlevandowski 4 years ago.
18650.2.patch (3.0 KB) - added by SergeyBiryukov 2 years ago.
18650.patch (4.1 KB) - added by DrewAPicture 2 years ago.
esc_attr_e
18650.diff (4.1 KB) - added by nacin 15 months ago.
18650.6.patch (2.4 KB) - added by joedolson 4 months ago.
Refresh of patch.
18650.3.patch (2.3 KB) - added by DrewAPicture 3 months ago.
18650.6.png (3.3 KB) - added by SergeyBiryukov 3 months ago.
18650.7.patch (3.0 KB) - added by SergeyBiryukov 3 months ago.
18650.8.patch (2.9 KB) - added by SergeyBiryukov 3 months ago.
18650.9.patch (2.1 KB) - added by SergeyBiryukov 3 months ago.

Download all attachments as: .zip

Change History (42)

@jlevandowski4 years ago

comment:2 @GaryJ4 years ago

I'd prefer to see the <select> element get an id attribute that had the widget ID as a suffix - that way, more than one archive dropdown widget can be used on a page with valid unique ids.

@jlevandowski4 years ago

comment:3 @jlevandowski4 years ago

Added patch that adds widget id so that more than one widget of the same kind can be used on a page.

comment:4 @jlevandowski3 years ago

  • Component changed from Widgets to Accessibility
  • Keywords dev-feedback added

comment:5 @webstoc2 years ago

  • Severity changed from normal to major

We verified that the patch fixes red flags in the WAVE toolbar. Curious what the hold up is on reviewing the patch by jlevandowski and getting it into the widgets? There are dozens (if not hundreds) of .gov sites using WordPress that need these fixes. As an "accessibility" issue, this more than of a "major" than "normal" severity.

Last edited 2 years ago by webstoc (previous) (diff)

comment:6 @webstoc2 years ago

  • Cc webstoc added

@SergeyBiryukov2 years ago

comment:7 follow-up: @SergeyBiryukov2 years ago

  • Milestone changed from Awaiting Review to 3.6

Refreshed the patch.

Not sure if changing the existing id attribute for Categories widget is a good idea though. I guess it may lead to unexpected results in some themes.

comment:8 @SergeyBiryukov2 years ago

#16527 was marked as a duplicate.

@DrewAPicture2 years ago

esc_attr_e

comment:9 in reply to: ↑ 7 @DrewAPicture2 years ago

Replying to SergeyBiryukov:

Not sure if changing the existing id attribute for Categories widget is a good idea though. I guess it may lead to unexpected results in some themes.

I see where you're coming from on this. And I can see why the inner select id was left alone -- the main widget id is unique for each instance just as with the other core widgets. On that same token, themes shouldn't have been targeting ids since they are by nature meant to be unique anyway.

18650.patch builds off 18650.2.patch and swaps out echo esc_attr( __( 'String' ) ) for esc_attr_e( 'String' )

comment:10 @ocean902 years ago

  • Milestone changed from 3.6 to Future Release

comment:11 @grahamarmfield18 months ago

  • Cc graham.armfield@… added

comment:12 @nacin16 months ago

  • Component changed from Accessibility to Widgets
  • Focuses accessibility added

comment:13 @joedolson16 months ago

Is there something in particular that this ticket is waiting on?

comment:14 @nacin16 months ago

  • Keywords dev-feedback removed
  • Milestone changed from Future Release to 3.9

Changing the IDs for these are precarious as themes may be using them for styling (the first instance, anyway). I could go for trying this out, though I wouldn't mind seeing if we could tweak it.

There is a lot of repetitive string building in this patch. A property or method specific to that widget would likely help clean that up.

comment:15 @GaryJ16 months ago

Worth adding a class to the select elements, so that themes can start targeting that instead?

@nacin15 months ago

comment:16 @nacin15 months ago

  • Milestone changed from 3.9 to Future Release

So basically:

  • The first instance of the category widget widget should use the current IDs.
  • Subsequent instances should use incremented IDs.

The archives widget doesn't currently have an ID on the select so it doesn't require this juggling.

I've attached 18650.diff to do this. The problem is, I think the labels wrapping the title have the potential to break a theme's styling of the header. (Imagine <h3><label>Title</label></h3> with label styling overriding h3 styling. An alternative would probably be screen reader text, which we actually don't necessarily have the classes for on the frontend.

As it is, these are not accessible due to the lack of a submit button, while at the very least the first options are "Select Month" and "Select Category", which provide a clue it's a JS jump menu. I'm not inclined to touch this without further consideration.

comment:17 @GaryJ8 months ago

Putting this out there, because.

Since the current widgets have some problems:

  • No submit button
  • No non-title label
  • Missing or non-unique IDs
  • Obtrusive JS (onchange)
  • Whatever else other open tickets might say is wrong

...that make front-end changes, is there potential for creating new widgets (different $id_base) that address all of these problems, only making these available to new installs, and hiding the old versions from new installs?

comment:18 @joedolson8 months ago

I'm open to this; this would open the path for eventual deprecation of the old widget.

comment:19 @ctalkington8 months ago

new widgets that are a bit smarter in regards to accessibility and modern approaches while letting the old widgets remain in a soft-deprecate stage for back-compat is an intriguing thought. i think this approach has been used for different things before, just not widgets?

it seems like doing theme support could potentially work here but would add a bit of overhead based on the level of adjustment that is needed.

comment:20 @slackbot7 months ago

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

comment:21 @joedolson4 months ago

  • Milestone changed from Future Release to 4.2
  • Owner set to joedolson
  • Priority changed from normal to high
  • Status changed from new to assigned

comment:22 follow-up: @joedolson4 months ago

Attached patch is a refresh of Nacin's patch, but uses .screen-reader-text and inserts labels separately from the title content.

Also defines the static variable $first only if the widget is a dropdown, so that non-dropdown versions of the widget don't alter the value.

This doesn't handle the question of adding a submit button. If we add a submit button, we need to also remove the onchange handling.

@joedolson4 months ago

Refresh of patch.

comment:23 @SergeyBiryukov4 months ago

In 18650.6.patch, echo esc_attr( $label ) should not be replaced with esc_attr_e( $label ).

$label is a variable, not a translatable string, so echo esc_attr() is correct there.

comment:24 @DrewAPicture3 months ago

  • Status changed from assigned to reviewing

18650.3.patch addresses feedback from @SergeyBiryukov in comment:23 and also escapes a couple of output attributes.

@joedolson: Is anything else here outstanding?

Last edited 3 months ago by DrewAPicture (previous) (diff)

@DrewAPicture3 months ago

comment:25 @joedolson3 months ago

That does it as far as I can see.

comment:26 @DrewAPicture3 months ago

  • Keywords commit added

@SergeyBiryukov3 months ago

@SergeyBiryukov3 months ago

@SergeyBiryukov3 months ago

comment:27 in reply to: ↑ 22 @SergeyBiryukov3 months ago

Replying to joedolson:

Attached patch is a refresh of Nacin's patch, but uses .screen-reader-text and inserts labels separately from the title content.

That would result in duplicate widget title and label if the theme doesn't support .screen-reader-text, see 18650.6.png.

The alternative is to turn widget titles into labels, but that might result in label styling overriding the header styling, as noted in comment:16.

I've attached two patches which rename some variables for clarity, and only add labels if $title is not empty, to avoid printing an empty label if the title was filtered out by the theme.

Which approach are we more comfortable with?

Last edited 3 months ago by SergeyBiryukov (previous) (diff)

comment:28 follow-up: @joedolson3 months ago

It was actually entirely intentional that the screen reader text should be added. The text should be present as both a widget title and as a field label. The presence of the widget title doesn't matter significantly for accessibility, but the field label does.

Only adding labels if $title is not empty means that the form field has no label, which is not an acceptable solution.

comment:29 in reply to: ↑ 28 @SergeyBiryukov3 months ago

Replying to joedolson:

It was actually entirely intentional that the screen reader text should be added. The text should be present as both a widget title and as a field label.

Thanks for the clarification, 18650.9.patch should be good to go then.

@SergeyBiryukov3 months ago

comment:30 @SergeyBiryukov3 months ago

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

In 31520:

Add missing labels to Archives and Categories dropdown widgets.

props joedolson, jlevandowski, DrewAPicture, SergeyBiryukov.
fixes #18650.

comment:31 @DrewAPicture2 months ago

  • Priority changed from high to normal
Note: See TracTickets for help on using tickets.