Duplicate HTML id attribute in Quick/Bulk Edit on pages and post list

[zodiac1978]

In the Chrome dev tools, I stumbled upon an error message in a clean fresh WP 6.5 installation:

Duplicate form field id in the same form

This happens for post and pages:
/wp-admin/edit.php (posts)
/wp-admin/edit.php?post_type=page (pages)

This happens because the markup for the quickedit and the bulk edit share the same ID attribute for the category checkbox(es):

<input value="1" type="checkbox" name="post_category[]" id="in-category-1">

For the pages list it is the select for the parent page (again QuickEdit vs. Bulk Edit):

<select name="post_parent" id="post_parent">

I am not sure if this is an accessibility problem per se, because the QuickEdit is hidden. But in a similar case it was fixed. See: #55575

[joedolson]

Assigning to @rcreators to look at. This probably has no real world impact on accessibility, since we should never be interacting with more than one of these forms at a time, but it's also easily solvable.

[rcreators]

Hey @joedolson It looks like an easy one but it's a complicated issue which needs a core update. Here is my analysis.

The duplicate ID is generated only 2 times on a page
1) First as an inline edit template
2) When you click on quick edit for any item it will take the inline edit template and add it to the view.

So I looked into

$wp_list_table->inline_edit()

This function is loading any hierarchical data with WordPress native

wp_dropdown_pages

I tried to add args id = but that didn't seem to work as wp_dropdown_pages has backward compatibility for id in case of empty.

// Back-compat with old system where both id and name were based on $name argument.
if ( empty( $parsed_args['id'] ) ) {
	$parsed_args['id'] = $parsed_args['name'];
}

I am not sure now if we should remove this code or not but whatever we put in the id or name, it will always get duplicated when anyone clicks on a page.

Now the main point is, do we have to move on this one further as it's not that much a bigger issue and by default or in source code, you won't find duplicate ID. It's only getting added by Javascript when anyone clicks on quick edit.

[rcreators]

This patch is adding 2 lines of code to inline-edit-post.js to remove id attributes for category input for posts and post parent for pages.

[rcreators]

Hey @joedolson Sorry my bad, didn't read through the completed ticket. The above patch will solve the issue and there won't be duplicate ID as per the ticket.

​https://prnt.sc/sCPYY7eWya25 - For posts

​https://prnt.sc/87PcvMoK2Ywl - For pages

[joedolson]

I'm on the fence about whether to do this. The duplicate ID will have no real-world impact, because the two forms are never available at the same time; you can't quick edit and bulk edit simultaneously.

If we were to change the structure of the checkbox down the road such that it used explicit labeling instead of implicit labeling, this change would cause the checkboxes to be improperly labeled - right now, the ID isn't critical, because there's an implicit label. But it could become a problem in the future.

Given that I'd prefer, on the whole, to change the checkboxes to be explicitly labeled instead of implicitly, this might be a candidate for wontfix.

[rcreators]

@joedolson I think you missed the patch and last comment. I was able to fix it with minor 2 line of code.

[joedolson]

No, I didn't miss it - but what that code does is remove the ID attribute. This is fine for now, but could become a problem if we change the output of the function to use explicit labels on the checkbox, which is something that we should do - in general, we try to avoid implicit labeling of input fields.

[webcommsat]

Reviewing where we are with this ticket now that we have had 6.6 beta 1. @rcreators thanks for raising it and for the comments to date too.

The next steps seem to revolve around the possibility or at least hope for checkboxes in the future to be explicity labeled. The patch in this case removes the ID attribute and this therefore would have a negative impact as the checkboxes would be improperly labeled, and the approach is to avoid implicit labeling.

My preference would be to also move to explicit labeling as some assistive tools do not support implicit labeling well, so a change that would make this difficult may not be the correct solution.

@rcreators is there anything you would like to add in terms of other solutions or the added information in @joedolson's review (thanks for the comments Joe) before we look to close this ticket. Also copying in @oglekler @nalininonstopnewsuk and @marybaum before this ticket is marked 'wontfix'.

Thanks everyone for your time and analysis on this issue.

[siliconforks]

Why not just give each element a unique id attribute instead of removing the id attribute?

[rcreators]

@siliconforks as mentioned in comment no 3 by me, the I'd is generated through native code for drop-down. Now if we make changes to it, it will effect everywhere in the core as well as in plugins and themes which uses it. So it's best to not work on it as the issue itself not effecting much while solution might cause more trouble.

[siliconforks]

What I mean is, instead of removing the ID using JavaScript, just use JavaScript to change the ID.

I'll attach a patch to illustrate.

[siliconforks]

This patch changes the "id" attributes using JavaScript.

[siliconforks]

Note that this patch addresses the issue only for Posts, not for Pages. (The issue is somewhat more complicated for Pages.)

[joedolson]

This looks like a practical solution. We'll need to extend it, but it's a good start.

[joedolson]

Changing to needs patch, as the current patch is incomplete.

[siliconforks]

This patch is intended to fix the issue for Pages (not Posts).

[siliconforks]

For bulk editing Pages, it looks like the fix can be made just in HTML code (no JavaScript required).

Removes duplicate IDs generated on post list tables.

• in-{$taxonomy}-{$term_id} elements are modified to use wp_unique_prefixed_id() to ensure the select and LI have different IDs
• JS duplication of quick edit to bulk edit adds a prefix -- props @siliconforks
• #post_parent dropdown uses a different ID for bulk edit -- props @siliconforks

Trac ticket: https://core.trac.wordpress.org/ticket/61014

[peterwilsoncc]

In the ​linked pull request, I have

• modified Walker_Category_Checklist to avoid duplicate IDs on both the li and select elements. This affects both the post lists page and the classic editor meta boxes (cc @azaozz)
• added the prefix to the JavaScript code's duplication of the inline editor to the bulk editor
• used a different ID for the #post_parent dropdown in the bulk editor

The last two items are from @siliconforks patches on this ticket. Thank you :)

I'm not seeing any further warnings of duplicate IDs in Chrome's dev tools.

The autosave function for the classic editor appears to be unaffected by these changes (it searches through taxonomy terms using the existing IDs) as it's saving via the field value.

However, it would be helpful if others could check the combination of all the patches to ensure it's still working as expected.

A question that's occurred to me overnight is whether both the IDs in the walker should be renamed or just one of them?

If a plugin is targeting an ID for either CSS or JS reasons then changing both of them will certainly break the plugin. Changing one will not. My instinct is that renaming the li's ID is best as CSS will still apply to the form element, and JavaScript will still capture events. @SergeyBiryukov, what do you think?

A question that's occurred to me overnight is whether both the IDs in the walker should be renamed or just one of them?

I think you want to rename both the IDs, to handle the case where the walker gets executed twice on the same page?

I think you want to rename both the IDs, to handle the case where the walker gets executed twice on the same page?

Excellent point. There could be alternative ways of managing this but the code would be pretty ugly.

On the trac ticket, Joe Dolson mentioned that this code may possibly need to be extended further so I will wait until I hear back before committing. The ticket is on the 6.7 milestone so your portion of the code (ie, most of it) on this ticket should make it to the next release.

Looks good imho too, and maintains the way HTML IDs work at the moment as far as I see (i.e. when duplicate IDs only the first element is selected/used).

This change wouldn't cause a problem with the hypothetical changes we might want to make in the future (changing the label structure from implicit to explicit associations), so I don't see any barriers to this.

[peterwilsoncc]

In 58894:

Bulk/Quick Edit: Remove duplicate HTML IDs from post list tables.

Removes duplicate IDs on the post list admin pages affecting various list items, selects and checkboxes:

• JavaScript duplication of the inline editing HTML for bulk editing renames various IDs to include the prefix bulk-edit-,
• IDs in the Category Checkbox Walker make use of wp_unique_prefixed_id() to avoid duplicates, resulting in a numeric suffix, and,
• the post parent dropdown for the bulk editor is given a custom ID bulk_edit_post_parent.

Props peterwilsoncc, sergeybiryukov, azaozz, joedolson, siliconforks, zodiac1978, rcreators.
Fixes #61014.

Merged r58894 / ​https://github.com/WordPress/wordpress-develop/commit/2061a52da3d9d6d3994b0a60b2df07037d95e3d8