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

[GregLone]

​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.

[costdev]

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

[joedolson]

Fix duplicate ID

[joedolson]

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

[joedolson]

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

[joedolson]

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

[joedolson]

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.

[joedolson]

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.

[sabernhardt]

removing ID from extra quick edit elements

[sabernhardt]

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.

[joedolson]

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

[costdev]

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

[hellofromTonya]

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.

[joedolson]

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]

Add comment, remove extraneous code

[joedolson]

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.

[ironprogrammer]

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.

[peterwilsoncc]

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?

[joedolson]

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

[hellofromTonya]

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.

[azaozz]

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.

[peterwilsoncc]

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.

[peterwilsoncc]

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.

[SergeyBiryukov]

[53352] looks good to backport.

[audrasjb]

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

[peterwilsoncc]

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.