WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 6 months ago

#46964 assigned defect (bug)

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

Reported by: jankimoradiya Owned by:
Milestone: Future Release Priority: high
Severity: normal Version: 5.0
Component: Editor Keywords: has-screenshots needs-patch
Focuses: ui, accessibility Cc:

Description

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

Attachments (6)

Screenshot.png (497.7 KB) - added by jankimoradiya 18 months ago.
46964.diff (10.6 KB) - added by donmhico 15 months ago.
Convert #poststuff and #postbox-container-2 to .poststuff and .postbox-container-2.
46964.1.diff (10.6 KB) - added by audrasjb 9 months ago.
Patch refreshed
46964.2.diff (9.5 KB) - added by SergeyBiryukov 9 months ago.
46964.3.diff (17.4 KB) - added by joedolson 8 months ago.
Preserves #poststuff and #postbox-container in CSS for backcompat
46964.4.diff (11.0 KB) - added by afercia 8 months ago.

Download all attachments as: .zip

Change History (62)

#1 @SergeyBiryukov
18 months ago

  • Focuses performance removed

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


18 months ago

#3 @afercia
18 months ago

  • Component changed from Administration to Editor
  • Version changed from trunk to 5.0

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


18 months ago

#5 @afercia
18 months ago

  • Milestone changed from Awaiting Review to 5.3

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

#6 @desrosj
17 months ago

  • Focuses coding-standards removed

#7 @afercia
17 months ago

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

#8 @afercia
16 months ago

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
15 months ago

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

#9 @donmhico
15 months ago

  • Keywords has-patch added; needs-patch removed

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.

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


14 months ago

#11 @audrasjb
14 months ago

  • Keywords needs-testing added
  • Owner set to audrasjb
  • Status changed from new to accepted

#12 @audrasjb
14 months ago

  • Keywords dev-feedback needs-testing removed

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.

#13 @afercia
14 months ago

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.

#14 @donmhico
14 months ago

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.

#15 @afercia
14 months ago

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.

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


14 months ago

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


14 months ago

This ticket was mentioned in Slack in #core-editor by afercia. View the logs.


13 months ago

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


13 months ago

#20 @audrasjb
13 months ago

  • Keywords dev-feedback 2nd-opinion early added
  • Milestone changed from 5.3 to 5.4

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

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


9 months ago

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


9 months ago

@audrasjb
9 months ago

Patch refreshed

#23 @audrasjb
9 months ago

  • Keywords commit added; dev-feedback 2nd-opinion removed

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.

#24 @audrasjb
9 months ago

  • Keywords needs-dev-note added

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


9 months ago

#26 @sageshilling
9 months ago

@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

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


9 months ago

#28 @joedolson
9 months ago

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.

#29 @joedolson
9 months ago

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

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.

#30 @SergeyBiryukov
9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • 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">
    

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.

Last edited 9 months ago by SergeyBiryukov (previous) (diff)

#31 @joedolson
9 months ago

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.

#32 @joedolson
9 months ago

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
8 months ago

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

#33 @joedolson
8 months ago

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

#34 @SergeyBiryukov
8 months ago

#49424 was marked as a duplicate.

#35 @elliotcondon
8 months ago

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

#36 @joedolson
8 months ago

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

#37 @elliotcondon
8 months ago

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

#38 @audrasjb
8 months ago

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.

Last edited 8 months ago by audrasjb (previous) (diff)

#39 follow-up: @azaozz
8 months ago

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.

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


8 months ago

This ticket was mentioned in Slack in #core-multisite by jjj. View the logs.


8 months ago

#42 @johnjamesjacoby
8 months ago

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!

#43 @audrasjb
8 months ago

  • Keywords commit removed

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 ♥️

#44 @jadpm
8 months ago

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
8 months ago

#45 @afercia
8 months ago

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)

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


8 months ago

#47 @azaozz
8 months ago

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

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


8 months ago

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


8 months ago

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


8 months ago

#51 @johnbillion
8 months ago

  • Keywords revert added; early needs-dev-note removed
  • Owner changed from audrasjb to johnbillion
  • Priority changed from normal to high
  • Status changed from reopened to accepted

Needs a revert for 5.4 then punting.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


8 months ago

#53 @johnbillion
8 months ago

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

#54 @johnbillion
8 months ago

  • Keywords needs-patch added; has-patch revert removed
  • Milestone changed from 5.4 to Future Release

#55 in reply to: ↑ 39 @SergeyBiryukov
8 months ago

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.

#56 @johnbillion
6 months ago

  • Owner johnbillion deleted
  • Status changed from accepted to assigned
Note: See TracTickets for help on using tickets.