ID attribute value are used multiple times in "Custom Field" form

[jankimoradiya]

I have found that, ID attribute value like "poststuff" and "postbox-container-2" are used in multiple times in form of custom filed HTML.

Ex. id="poststuff" and id="postbox-container-2".

I thought that, It will need to update for better coding standard & improve acessibility.

Any suggestions are more welcomes...!!!

[afercia]

Discussed during today's accessibility bug scrub and agreed to try this for the next 5.3 release.

[afercia]

For reference for future contributors and history: WordPress does have HTML Coding Standards

​https://make.wordpress.org/core/handbook/best-practices/coding-standards/html/#validation

[afercia]

Seems to me this is related to what the_block_editor_meta_boxes() does. It loops through the meta boxes locations and then renders always the same IDs. I'm not sure these IDs are really necessary to start with, maybe they could be removed?

[donmhico]

Convert #poststuff and #postbox-container-2 to .poststuff and .postbox-container-2.

[donmhico]

The main culprit for this, like @afercia mentioned, is the function the_block_editor_meta_boxes(). Specifically this block of code.

<?php foreach ( $locations as $location ) : ?>
    <form class="metabox-location-<?php echo esc_attr( $location ); ?>" onsubmit="return false;">
        <div id="poststuff" class="sidebar-open">
            <div id="postbox-container-2" class="postbox-container">
                <?php
                    do_meta_boxes(
                        $current_screen,
                        $location,
                        $post
                    );
                ?>
            </div>
        </div>
    </form>
<?php endforeach; ?>

I think changing them to classes would be better than removing them. I also checked and only #postbox-container-2is inside a loop. The other ones #postbox-container-1, #postbox-container-3, and #postbox-container-4 are not.

[audrasjb]

Hi @donmhico

I tested your patch and all looks good on my side.
There is still a duplicate ID on my test install, on a _wpnonce ID attribute. But that would be better to separate that in another patch so this one could land in 5.3.

[afercia]

I'm not sure we can just change the #poststuff ID to a .poststuff class attribute. For example, Gutenberg targets #poststuff in its stylesheet. Other plugins may target the current IDs for any other reason, also for JavaScript stuff.

[donmhico]

Thanks for the feedback @afercia. If that's the case how do you think we should address this? IMHO CSS IDs should be unique in the page as having multiple ones may lead to unexpected errors. We can inform the Gutenberg team regarding this. Maybe ask what specific context of #poststuff are they targeting then we can leave that context intact and prevent other instances of #poststuff to be created. For the other plugins, at best we can inform them regarding this change.

Any other approach is welcome.

[afercia]

After some software archeology, looks like there was an attempt to fix this issue in Gutenberg, see:

​https://github.com/WordPress/gutenberg/pull/4697

that change was then reverted (February 2018)
​https://github.com/WordPress/gutenberg/pull/4971

Still, there's the need to come up with the best option to use unique IDs.

the_block_editor_meta_boxes() prints out a form for three locations: 'side', 'normal', 'advanced'. Not sure about the internal logic for meta boxes backwards compatibility but seems to me only the first #poststuff is visible, while the other two ones are always hidden within a #metaboxes div and maybe don't need IDs? /Cc @youknowriad any feedback would be greatly appreciated.

[audrasjb]

This ticket still needs some work and improvements. Moving it to 5.4 with early keyword, waiting for thoughts from Gutenberg team (cc @youknowriad).

[audrasjb]

Patch refreshed

[audrasjb]

I refreshed the patch as it didn't applies anymore.

this ticket was discussed during today's bug scrub, between WP Accessibility team and Editor Team.

Let's commit this one as soon as possible in the release cycle so it can be tested in various use cases during the beta cycle.
Adding commit keyword.

[sageshilling]

@donmhico mentioned
CSS IDs should be unique in the page as having multiple ones may lead to unexpected errors.
_
Based on above, let's add dynamic id, so they are not the the same, counted by the content loop.

main file that needs updating, according to the report is the post.php
wordpress\wp-admin\includes\post.php
2 line change from this:

<div id="poststuff" class="sidebar-open">

<div id="postbox-container-2" class="postbox-container">

to this:
<div id="poststuff_<?php echo esc_attr( $uniq_id ); ?>" class="sidebar-open">

<div id="postbox-container_<?php echo esc_attr( $uniq_id ); ?>" class="postbox-container">

*

add dynamic derived $uniq_id, from count relative to the content loop
to content loop
in the beginning $uniq_id
set to 0
count function

end of loop $uniq_id++

with plugins, I use the filter 'the_content',
with possible logic to filter on pages needed

css (believe the css sheets would need to be updated- edit.css, ie.css)
THIs I haven't tried, yet should work, advisement welcomed
CSS [attribute&circ="value"] Selector
(&circ is the carrot entity)
The [attribute&circ="value"] selector is used to select elements whose attribute value begins with a specified value.
The following example selects all elements with a id attribute value that begins with "poststuff_" and "postbox-container_":
Note: The value does not have to be a whole word!
​https://www.w3schools.com/css/css_attribute_selectors.asp
from this:
#poststuff_
#postbox-container-2_

to this:
[id&circ;="poststuff_"] {

sample: yellow;

}

[id&circ;="postbox-container_"] {

sample: yellow;

}

*
propose upping variable to prefix core:
from this:
#poststuff_
#postbox-container-2_
to this something like this (definitely to discuss):
#core_poststuff_
#core_postbox-container-2_

research
found on

wordpress\wp-admin\includes\dashboard.php
doesn't appear to need update, according to notes above and see below that the ids are different

<div id="postbox-container-1" class="postbox-container">

<div id="postbox-container-2" class="postbox-container">

<div id="postbox-container-3" class="postbox-container">

<div id="postbox-container-4" class="postbox-container">

need to look at more:
edit-form-advanced.php
edit-form-comment.php
edit-link-form.php
postbox.js
edit.css
ie.css

[joedolson]

I don't think it's necessary to add a loop to have dynamic IDs on these boxes. If anybody has been doing anything that depends on these ID attributes, it'll either be easily correctable CSS or JS that has been depending on a bug. We can consider adding dynamic IDs as a separate task, but it's not necessary to resolve this ticket.

[joedolson]

In 47222:

Editor: Fix incorrect usage of ID attributes on custom fields.

Repeated containers used for custom fields have duplicate ID attributes. Duplicate IDs are incorrect HTML, and will also cause unexpected results when trying to manipulate using JS. Duplicate IDs are changed to matching classes; CSS & JS updated to match.

Props jankimoradiya, audrasjb, donmhico, afercia.
Fixes #46964.

[SergeyBiryukov]

• If we're changing #postbox-container-2 to a class, then other #postbox-container-* instances should be changed too for consistency, otherwise this just looks weird​ for no particular reason without any context:

  <div id="postbox-container-1" class="postbox-container">
  <div class="postbox-container-2 postbox-container">
  <div id="postbox-container-3" class="postbox-container">
  <div id="postbox-container-4" class="postbox-container">
  

• With ​5200+ plugins using #poststuff or #postbox-container-2, changing these IDs to classes seems like a large back-compat issue. Not all of them would be affected, but this is still a huge number.

If the goal is to avoid duplicate IDs in the_block_editor_meta_boxes(), then let's do just that.

For some context, in the classic editor, #poststuff was used to wrap almost the entire edit form​, not just the meta boxes. Ideally, just moving #poststuff outside of the loop would work for our purposes:

<div id="poststuff" class="sidebar-open">
	<div id="postbox-container-2" class="postbox-container">
		<?php foreach ( $locations as $location ) : ?>
			<form class="metabox-location-<?php echo esc_attr( $location ); ?>" onsubmit="return false;">
				<?php
				do_meta_boxes(
					$current_screen,
					$location,
					$post
				);
				?>
			</form>
		<?php endforeach; ?>
	</div>
</div>

Unfortunately, the block editor moves these boxes around in the DOM, and while the location forms are visible, #poststuff ends up in a hidden area, and this change alone is not enough for this approach to work as expected.

Related notes:

• The .sidebar-open class is apparently not used anywhere and can be removed.
• The #postbox-container-2 ID doesn't provide any difference in the_block_editor_meta_boxes() and can also be removed (as mentioned in ​PR4697). Its usage here is also not quite correct, historically it's only used for normal and advanced meta boxes, not for side meta boxes, which used postbox-container-1 instead as a wrapper.

I think the best solution here would be to reconsider ​PR4697 on the Gutenberg side, which tried to address the issue, but gave #poststuff too broad a scope, resulting in ​CSS bleed and eventual revert in ​PR4971. If #poststuff is just moved outside of the loop and #postbox-container-2 is removed from the_block_editor_meta_boxes(), that would resolve duplicate IDs without any back-compat issues.

If we're fine with a potentially breaking change for 5200+ plugins, I suggest we go the distance and convert the remaining #postbox-container-* IDs to classes to avoid issues like this in the future. See 46964.2.diff​.

[joedolson]

I think the whole idea here is to get this in and be prepared to revert if it's too big of a problem. It's a lot of plugins, but the actual resolutions are very easy in pretty much all cases; that's why this is flagged for a dev note.

I'm fine with breaking back compat for this; I don't think there's a way to fix this without breaking back compat, so better sooner than later.

[joedolson]

Looking at the plugins using this, the vast majority of what implementations are custom plugin admin screens using those styles. If we preserve #postbox-container-2 and #poststuff in the CSS, alongside the new selectors, we can retain most back compat. The accessibility issue only pertains to what appears in the HTML; the CSS is not a problem.

[joedolson]

Preserves #poststuff and #postbox-container in CSS for backcompat

[joedolson]

New patch preserves old ID selectors in CSS for back-compat.

[SergeyBiryukov]

#49424 was marked as a duplicate.

[elliotcondon]

Hi all,

Elliot here from ACF expressing my concern over this issue.

To recap, the element used to wrap admin page HTML has changed in WP 5.4 from <div id="poststuff"> to <div class="poststuff">.

This change will cause major issues for plugins and themes that include css/js relying on that well-established wrapping element to exist.

Q1. Is it be possible to revert this change from class back to id?
Q2. If the change must go ahead, can we retain backwards compatibility by also adding in the additional id attribute?

Thanks,
Elliot

[joedolson]

@elliotcondon Thanks for chiming in!

Can you give me an example of a JS dependency on this? The CSS dependencies are easily handled, but an example of a plugin that depends on this ID for JS would be helpful.

We can't just keep the ID, per se - we can conceivably try and keep the first occurrence of the ID only, but all others would have to change if we're going to solve the problem of duplicate IDs. The poststuff ID should have always been unique, but unfortunately it isn't.

It's possible that this will be reverted, but only to give more time to explore other solutions later.

[elliotcondon]

@joedolson Thanks for the reply.

While I agree with you that the CSS dependencies are easily handled in core, it may be a different story for plugins and themes that use the #poststuff element for inheriting CSS styling for metaboxes.

For example, this popular plugin "Popup Maker" (​https://wordpress.org/plugins/popup-maker/) is using the <div id="poststuff"> element within custom admin pages to inherit WP styling for metaboxes.

Advanced Custom Fields PRO takes a similar approach inheriting metabox/layout styles for our Options Page feature. Also, a quick Google reveals many threads discussing CSS solutions that involve the #poststuff element.

Personally, I'm not aware of any plugins relying on the id #poststuff for jQuery selectors, but can ask around if that would be helpful, although this is not my primary concern.

Moving CSS selectors away from id towards class is definitely in the best interest of WordPress, but I would like to draw attention to the potential compatibility impact this could have on the third-party community.

[audrasjb]

Given we are now approaching Beta 3, I'd say the safer option for us is to revert this change, and to milestone it to 5.5 early, and to communicate it as soon as we can.

[azaozz]

Agree with @elliotcondon and @audrasjb that this should be reverted for now. These HTML IDs #poststuff, #postbox-container*, etc. were used to signify that only one element can be present on the page. That had (as far as I remember) something to do with them being "dynamic" draggable containers and IE6 (yeah, they are that old!).

The bug here is that they are now used multiple times. Changing them to classes is not backwards compatible.

[johnjamesjacoby]

Hey friends. I found 2 plugins of mine that have broken styling due to this change.

​WP Multi Network
​WP User Profiles

For context, these plugins both implement their own versions of the "Add New" interface commonly associated with the Classic Editor experience. As such, they've manually implemented the DOM hierarchy from it, starting with #poststuff and finishing with #postbox-container-1 and #postbox-container-2 both of which contain their own registered meta-boxes for their own screens.

I don't mind changing the ID to a class in my screens. I understand there are no guarantees that the admin-area DOM or accompanying styling will never change, so by copying it I'm assuming the responsibility to maintain my own copy.

Since it is highly unlikely that my own screens will suffer the problem this ticket aims to solve, I may use both the ID and the class in my plugins to be prepared either way.

Good luck with this one, everyone!

[audrasjb]

Hi @johnjamesjacoby thanks for the use case!

I can confirm this is going to be reverted before RC1 next week :-)

Thanks all for sharing your considerations ♥️

[jadpm]

Hi there

I am the team leader for Toolset, and we are also affected by the change here, just adding our own use case here if someone is interested.

We do use the #postbox-container-2 ID for several items. We have some objects that we edit in the post legacy editor, but that require some wizard steps when created, and we manpulate the edit page look&feel by hooking into existing HTML structures. That I is one that we do use and rely on. We can change it to be a classname, no problem there, but I tend to believe that we are aproachign the problem wrong.

My 2c on the issue: we have several #postbox-container-X IDs for some containers. Because, as stated above, they used to idenitfy draggable elements. If we have a repetition on IDs then we should probably target that repetition, check why we have it, and eventually solve it by avoiding the repeated items (something like a #postbox-container-2-X ID for recurring items?).

[afercia]

46964.4.diff​ reverts [47222]. A couple things to note:

• it solves a conflict from [47252]
• it removes a few trailing spaces (because my editor does that automatically)

[azaozz]

46964.4.diff looks good here.

A better way to fix this would be to change the_block_editor_meta_boxes() to more closely resemble the HTML outputted from edit-form-advanced.php. Looking there, the HTML elements with matching IDs are outputted in a loop...

A fix would probably look like:

<div id="poststuff" class="sidebar-open">
<?php foreach ( $locations as $i => $location ) : ?>
	<form class="metabox-location-<?php echo esc_attr( $location ); ?>" onsubmit="return false;">
		<div id="postbox-container-<?php echo ( 1 + $i ); ?>" class="postbox-container">
			<?php
			do_meta_boxes(
				$current_screen,
				$location,
				$post
			);
			?>
		</div>
	</form>
<?php endforeach; ?>
</div>

(move the poststuff div outside of the loop and make the postbox-container-* IDs dynamic to match edit-form-advanced.php).

[johnbillion]

Needs a revert for 5.4 then punting.

[johnbillion]

In 47410:

Editor: Revert a fix for incorrect usage of ID attributes on custom fields.

This reverts [47222] due to compatibility issues with plugins which are using the #poststuff selector.

See #46964

[SergeyBiryukov]

To summarize a bit for future reconsideration:

• #poststuff and #postbox-container-* were always designed to have a single instance on the page. It's just that the block editor is incorrectly using them as a wrapper for individual meta box groups instead of the whole meta box area. Changing these to classes is neither feasible due to back compat issues and technical debt (plugins would have to support both the IDs and classes for a while), nor it is necessary to fix this particular issue.
• Since it's a block editor issue, it should be fixed there, by allowing for the IDs to be moved out of the loop, as suggested in comment:30.

[sabernhardt]

removing id attributes from the_block_editor_meta_boxes

[sabernhardt]

Using a combination of core and plugin files, I think we could maintain the ID on a single container around the 'normal' and 'advanced' forms. Then the 'side' meta box would rely on different (custom) styling instead of the ID.

46964.5.diff​ removes the IDs from the loop, adds a poststuff class, and adds selectors with the class (for use in sidebar meta box).

Still to do in a Gutenberg PR:

• ​Adding `poststuff` ID to `edit-post-layout__metaboxes` div
• ​Adding styles for sidebar meta box

  	#poststuff h2.hndle,
  	.poststuff h2.hndle { /* WordPress selectors yolo */
  		box-sizing: border-box;
  		color: inherit;
  		font-size: 14px;
  		line-height: 1.4;
  		font-weight: 600;
  		outline: none;
  		margin: 0;
  		padding: 16px;
  		position: relative;
  		width: 100%;
  	}