WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 3 months ago

#46964 accepted defect (bug)

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

Reported by: jankimoradiya Owned by: audrasjb
Milestone: 5.4 Priority: normal
Severity: normal Version: 5.0
Component: Editor Keywords: has-screenshots has-patch dev-feedback 2nd-opinion early
Focuses: ui, accessibility Cc:
PR Number:

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 (2)

Screenshot.png (497.7 KB) - added by jankimoradiya 8 months ago.
46964.diff (10.6 KB) - added by donmhico 4 months ago.
Convert #poststuff and #postbox-container-2 to .poststuff and .postbox-container-2.

Download all attachments as: .zip

Change History (22)

#1 @SergeyBiryukov
8 months ago

  • Focuses performance removed

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


8 months ago

#3 @afercia
8 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.


8 months ago

#5 @afercia
8 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
7 months ago

  • Focuses coding-standards removed

#7 @afercia
7 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
6 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
4 months ago

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

#9 @donmhico
4 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.


4 months ago

#11 @audrasjb
4 months ago

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

#12 @audrasjb
4 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
4 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
4 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
4 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.


3 months ago

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


3 months ago

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


3 months ago

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


3 months ago

#20 @audrasjb
3 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).

Note: See TracTickets for help on using tickets.