Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#55664 closed defect (bug) (fixed)

WP post embed: missing labels and duplicate IDs

Reported by: afercia's profile afercia Owned by: joedolson's profile joedolson
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Embeds Keywords: has-screenshots has-patch commit
Focuses: accessibility Cc:

Description (last modified by afercia)

The form fields in the sharing dialog of the WordPress post embed aren't labeled. Also, when there's more than one WordPress post embed on a page, there are duplicate IDs.

To reproduce:

  • Add a WordPress post embed twice. Add a third embed to another post.
  • Click the 'share' icon on both the embeds to reveal the Sharing dialog.
  • Check the URL input field and the HTML textarea aren't labeled: there's no associated <label> element, no aria-label or any other labeling.
  • See the 'WordPress Embed' and 'HTML Embed' tab panels use the same IDs on both embeds.
  • See the input field and textarea descriptions use the same IDs on both embeds.

These IDs are then used for some aria-* attributes e.g. aria-controls, aria-describedby thus referencing different elements with the same IDs.

See attached screenshot for reference.

Attachments (3)

WP post embeds.png (304.8 KB) - added by afercia 3 years ago.
55664.diff (3.5 KB) - added by afercia 3 years ago.
55664.2.diff (3.6 KB) - added by afercia 3 years ago.

Download all attachments as: .zip

Change History (21)

@afercia
3 years ago

#1 follow-up: @afercia
3 years ago

  • Keywords has-patch added; needs-patch removed

55664.diff is a first pass:

  • Uses uniqid() for the IDs. Open to better ways to generate unique IDs.
  • Adds aria-label attributes to label the input field and the textarea.

#2 in reply to: ↑ 1 @ocean90
3 years ago

Replying to afercia:

  • Uses uniqid() for the IDs. Open to better ways to generate unique IDs.

Sounds like a case where wp_unique_id() should be used instead, see #44883 for some background.

#3 @swissspidy
3 years ago

+1 to using wp_unique_id()

#4 @afercia
3 years ago

I did try wp_unique_id() as first thing but it always prints out 1 to me :) I think it's because the particular way print_embed_sharing_dialog is used but I didn't dive deep.

Edit: I guess the wp_unique_id() counter variable is actually processed in the php that's used within the iframe so... it always restarts the count for each iframe.

Last edited 3 years ago by afercia (previous) (diff)

#5 @swissspidy
3 years ago

  • Milestone changed from Awaiting Review to Future Release

Ah right, that makes sense. Hmm.

What about using the post ID then instead of uniqid/wp_unique_id?

Seems unlikely that one would embed the same post twice on a page, or that two embeds from different websites have the same post IDs.

To address these edge cases, the IDs could be updated using JavaScript. For example by using the unique secret/hash passed to each iframe.

#6 @afercia
3 years ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to Awaiting Review

Thanks for the insight @swissspidy. I'd rather keep it simple. After all, the worst (very unlikely) thing that can happen is a missing association between labels and input fields.

To me, something like 'some-prefix' . wp_rand() would be enough. Thoughts?

I see the iframe unique secret is generated with wp_generate_password( 10, false ) but that seems a bit overkill to me. Also, I'd rather avoid using JavaScript. Other approaches used in core, see combinations of various functions e.g. md5( 'some-string' . time() . wp_rand() ) seem definitely overkill.

Please, let's keep the milestone to 'awaiting review' for now, so the accessibility team can review this ticket during their next bug scrub, especially for part related to the missing labels.

#7 @swissspidy
3 years ago

Random numbers have downsides, see #44883

Why does using the post ID for the IDs not work?

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


3 years ago

#9 @joedolson
3 years ago

  • Milestone changed from Awaiting Review to 6.1

#10 @joedolson
3 years ago

  • Owner set to joedolson
  • Status changed from new to accepted

#11 @afercia
3 years ago

Why does using the post ID for the IDs not work?

Seems unlikely that one would embed the same post twice on a page, or that two embeds from different websites have the same post IDs.

It's unlikely but still possible. I'd propose a combination of the post ID + wp_rand().

#12 @aristath
3 years ago

Using the post-ID + wp_rand() sounds like a reasonable and pretty performant solution 👍

@afercia
3 years ago

#13 @afercia
3 years ago

  • Keywords needs-refresh removed

55664.2.diff updates the previous patch and uses post-ID + wp_rand() to generate unique IDs.

#14 @afercia
3 years ago

  • Description modified (diff)

#15 @joedolson
3 years ago

  • Keywords commit added

Tested patch using NVDA & Jaws, aria-label values read as expected; no unlabeled fields. The changes are clear, and nothing is complicated. Marking for commit.

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


2 years ago

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


2 years ago

#18 @joedolson
2 years ago

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

In 53912:

Embeds: Fix missing labels and duplicate IDs.

Add aria-label and aria-describedby to provide embed form fields with accessible names and ensure each embed has a unique ID attribute to prevent duplicate IDs.

Props afercia, swissspidy.
Fixes #55664.

Note: See TracTickets for help on using tickets.