Make WordPress Core

#55575 closed defect (bug) (fixed)

6.0 Regression: Duplicate HTML id attribute in Quick/Bulk Edit

Reported by: greglone's profile GregLone Owned by: joedolson's profile joedolson
Milestone: 6.0 Priority: normal
Severity: normal Version: 6.0
Component: Quick/Bulk Edit Keywords: has-patch has-testing-info commit dev-reviewed fixed-major
Focuses: accessibility Cc:

Description

This commit introduced a "static" HTML id attribute inline-edit-legend in Quick/Bulk Edit forms.
However, when these forms are both printed in the page, this id then duplicated.

<div class="inline-edit-wrapper" role="region" aria-labelledby="inline-edit-legend">
	<fieldset class="inline-edit-col-left">
		<legend class="inline-edit-legend" id="inline-edit-legend">Quick Edit</legend>
...
<div class="inline-edit-wrapper" role="region" aria-labelledby="inline-edit-legend">
	<fieldset class="inline-edit-col-left">
		<legend class="inline-edit-legend" id="inline-edit-legend">Bulk Edit</legend>

One solution would be to do something like this:

<div class="inline-edit-wrapper" role="region" aria-labelledby="<?php echo $bulk ? 'bulk' : 'quick'; ?>-edit-legend">
	<fieldset class="inline-edit-col-left">
		<legend class="inline-edit-legend" id="<?php echo $bulk ? 'bulk' : 'quick'; ?>-edit-legend"><?php echo $bulk ? __( 'Bulk Edit' ) : __( 'Quick Edit' ); ?></legend>

But by doing this only, the issue is not solved entirely: when using Bulk Edit or Quick Edit, the related form is copied into the list with JavaScript, leading to duplicating the id attribute again (the original form is left where it was). So I think it would require some more JavaScript to modify the id and aria-labelledby attributes before inserting the form into the list.

Attachments (3)

55575.diff (1.1 KB) - added by joedolson 22 months ago.
Fix duplicate ID
55575.1.diff (3.1 KB) - added by sabernhardt 22 months ago.
removing ID from extra quick edit elements
55575.2.diff (1.9 KB) - added by joedolson 22 months ago.
Add comment, remove extraneous code

Download all attachments as: .zip

Change History (32)

#1 @costdev
23 months ago

  • Milestone changed from Awaiting Review to 6.0

Moving to the 6.0 milestone for visibility as this was introduced during the current cycle.

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


23 months ago

#3 @joedolson
23 months ago

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

@joedolson
22 months ago

Fix duplicate ID

#4 @joedolson
22 months ago

  • Keywords has-patch added

Patch to fix duplicate ID, follows suggestion by @GregLone, which is consistent with the pattern used nearby to differentiate the tr containing the wrapper.

#5 @joedolson
22 months ago

  • Keywords commit added

This is a simple change; works as expected. Marking for commit.

#6 @joedolson
22 months ago

  • Keywords commit removed

Never mind; I missed the second part of this issue.

#7 @joedolson
22 months ago

Looks to me like the copying problem is only an issue on quick edit; the form does not appear to be duplicated for bulk edit. There's also a problem with IDs for any field that's repeated between multiple forms, and that does cover both quick and bulk edit.

#8 @joedolson
22 months ago

Appending context to the form at the point of display, e.g. adapting aria-labelledby, for, and id attributes to something like current-quick-edit-... is probably worth exploring.

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


22 months ago

#10 @sabernhardt
22 months ago

  • Keywords needs-patch added; has-patch removed

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


22 months ago

@sabernhardt
22 months ago

removing ID from extra quick edit elements

#12 @sabernhardt
22 months ago

If we can rely on always retaining the original (hidden) quick edit elements, the id could be removed from the script-added quick edit rows. Then the aria-labelledby and aria-describedby would refer to a hidden element.

There is still at least one flaw with the patch: I don't know why the bulk edit row prints quick-edit-[post_tag]-desc for the taxonomy field descriptions when I used the same condition as in the legend.

#13 @joedolson
22 months ago

@sabernhardt $bulk is always going to be false within that loop - see line 1831. The taxonomy textarea is cloned in the JS on line 134.

Last edited 22 months ago by joedolson (previous) (diff)

#14 @costdev
22 months ago

  • Summary changed from Duplicate HTML id attribute in Quick/Bulk Edit to 6.0 Regression: Duplicate HTML id attribute in Quick/Bulk Edit

Updating the title to indicate that work on this can continue beyond RC1 and hard string freeze.

#15 @hellofromTonya
22 months ago

6.0 RC1 is happening within a few minutes. The issue reported here is a regression introduced in the 6.0 cycle. Unfortunately, there's not a fix yet. Leaving this one in the milestone to go past RC1 point in the hopes that a resolution can be found in time for RC2.

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


22 months ago

#17 @joedolson
22 months ago

I do want to note that the extremely minor regression represented here is considerably less significant than the fix it's related to. The only regression is that instead of having n duplicate IDs in quick/bulk edit, it's now n+2 (where n = the number of categories). The improvements, however, are the difference between 'usable' and 'unusable' for many users of assistive technology.

I can spend some time looking at the issue this week, but I haven't considered it a high priority.

@joedolson
22 months ago

Add comment, remove extraneous code

#18 @joedolson
22 months ago

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

As far as I'm seeing (and I may be overlooking something), @sabernhardt's patch fulfills the requirements. Updated patch removes the extraneous code inside the taxonomy loop (which would always resolve the same way, so is not needed), and adds a comment to the JS stripping off the ID attributes to explain that purpose.

Testing: apply, verify the quick and bulk edit legends don't trigger duplicate IDs.

#19 @ironprogrammer
22 months ago

Thank you @sabernhardt and @joedolson for the patch and refresh!

Test Report

Patch 55575.2 refresh via @joedolson

Env

  • WordPress 6.0-RC1-53341-src
  • Chrome 101.0.4951.54
  • macOS 12.3.1 (Monterey)
  • Theme: Twenty Twenty-One

Steps to Reproduce

  1. Navigate to the main Posts page, Posts > All Posts.
  2. Verify there is at least one post displayed in the table. Create a sample post if necessary. This is required to verify Steps 5-7 below.
  3. Open DevTools (or your browser's equivalent), and in the JavaScript "Console" run/evaluate the following expression: document.querySelectorAll('#inline-edit-legend') (or use the $$(...) function shorthand).
  4. 🐞 Observe that 2 nodes are returned in the console, indicating that the element ID inline-edit-legend exists more than once in the DOM (for both the "quick" and "bulk" -edit legend elements).
  5. Hover over a post in the list, activating the contextual row actions, and click the "Quick Edit" link to expand the inline post editor.
  6. Re-run the expression from Step 3.
  7. 🐞 Observe that now 3 nodes are returned, as the existing element ID inline-edit-legend has been cloned into the row.

Steps to Test Patch

  1. After applying patch 🛠, run npm run build:dev to recompile JS, and then refresh the Posts page.
  2. In the console, re-run the expression: document.querySelectorAll('#inline-edit-legend').
  3. ✅ Observe that 0 nodes are returned, confirming that the previously duplicated ID (inline-edit-legend) is no longer present in the DOM.
  4. In the console, run the expression: document.querySelectorAll('#quick-edit-legend').
  5. ✅ Observe that 1 node is returned, and the ID quick-edit-legend is not used elsewhere.
  6. In the console, run the expression: document.querySelectorAll('#bulk-edit-legend').
  7. ✅ Observe that 1 node is returned, and the ID bulk-edit-legend is not used elsewhere.
  8. Hover over a post in the list, activating the contextual row actions, and click the "Quick Edit" link to expand the inline post editor.
  9. Re-run the expression from Step 4.
  10. ✅ Observe that 1 node is returned, and the ID quick-edit-legend is not used elsewhere (the ID is removed via script after being cloned into the row).

Expected Results

  • The issue described by the reporter was reproduced successfully. ✅
  • The patch addressed the reported issue. ✅

Additional Notes

  • If in Step 10 above you see 2 nodes returned, make sure that the inline-edit-post.js file was compiled and deployed correctly to your environment.
Last edited 22 months ago by ironprogrammer (previous) (diff)

#20 follow-up: @peterwilsoncc
22 months ago

Reviewing the patch, the duplicate IDs are gone per Brian's notes in the comment above.

With the patch applied, I notice the inline edit form's markup uses aria-labelledby="quick-edit-legend". Similar to https://jsbin.com/dunusoy/edit?html

#quick-edit-legend is the descendant of an element hidden with display:none, will that affect the accessibility improvement the original commit was making?

#21 @joedolson
22 months ago

@peterwilsoncc No, this is not a problem for accessibility. aria-labelledby is designed with the idea in mind that the referenced content might not be visible.

#22 @hellofromTonya
22 months ago

  • Keywords has-testing-info commit added; needs-testing removed

Given 2 testing reports that 55575.2.diff resolves the issue and confirmation of no accessibility impacts, marking the patch for commit and updating the keywords.

#23 in reply to: ↑ 20 @azaozz
22 months ago

Replying to peterwilsoncc:

With the patch applied, I notice the inline edit form's markup uses aria-labelledby="quick-edit-legend".

This is some extremely old code, 14-15 years I think. There is something like a template with display:none at the bottom of the page and some js to copy (clone) half of it, either bulk or quick edit, and insert it as a new row in the main list table. Then the data is added in that new row, including classes/IDs.

Originally there were no HTML IDs in that template node, all were added after cloning the node. As aria-labelledby requires IDs, it would be proper to add them (or replace/fix them) after the cloning, as done in this patch. The extra aria-labelledby are in the template node and not used/not reachable.

#24 @peterwilsoncc
22 months ago

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

In 53352:

Quick/Bulk Edit: Remove duplicate HTML IDs.

Rename #inline-edit-legend to avoid duplicate HTML IDs. These have been renamed #quick-edit-legend and #bulk-edit-legend for the quick and bulk editors respectively.

This HTML ID is not required by the quick editor duplicated via JavaScript so is removed as part of the duplication process.

Follow up to [53096].

Props azaozz, costdev, greglone, hellofromtonya, ironprogrammer, joedolson, sabernhardt.
Fixes #55575.
See #35483.

#25 @peterwilsoncc
22 months ago

  • Keywords dev-reviewed fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to 6.0

Could I get sign off from another committer? Probably not Joe as it was Joe's patch that got committed.

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


22 months ago

#27 @SergeyBiryukov
22 months ago

[53352] looks good to backport.

#28 @audrasjb
22 months ago

The change looks good to me as well, feel free tom backport to branch 6.0 👍

#29 @peterwilsoncc
22 months ago

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

In 53361:

Quick/Bulk Edit: Remove duplicate HTML IDs.

Rename #inline-edit-legend to avoid duplicate HTML IDs. These have been renamed #quick-edit-legend and #bulk-edit-legend for the quick and bulk editors respectively.

This HTML ID is not required by the quick editor duplicated via JavaScript so is removed as part of the duplication process.

Follow up to [53096].

Props azaozz, costdev, greglone, hellofromtonya, ironprogrammer, joedolson, sabernhardt, SergeyBiryukov, audrasjb.
Merges [53352] to the 6.0 branch.
Fixes #55575.
See #35483.

Note: See TracTickets for help on using tickets.