"Edit Comment" metabox header styling is inconsistent with others

[johnjamesjacoby]

Screen shot imminent to better describe the issue.

Basically, the h3 tags in comments.php?action=editcomment have classes and markup that differs from other classes and markup in similar areas of wp-admin with similar intentions.

I'm guessing it's been a while since these were updated to match other areas, though r32796 did touch one of the lines that I'm about to patch.

Marking as a defect, since I don't believe this is intentionally this way.

[johnjamesjacoby]

Note "Author" is off kilter, and "Status" is underlined

[johnjamesjacoby]

After

[johnjamesjacoby]

33361.01.patch suggests the following changes:

• Add a proper h3 for the "Author" metabox
• Add screen-reader-text class to legend tag in "Author" metabox to hide it. This is a compromise to avoid rewriting this metabox or introducing new CSS for hiding fieldset legends.
• Moves the hndle class off of the span in the "Status" metabox and on to the h3
• The second screenshot shows off the results, which looks more inline with other metabox header areas IMO

[rachelbaker]

@johnjamesjacoby Apologies for my ignorance, but which metabox header and alignment are you using as a reference to match? I can see that the Edit Comment heading should be an h3 but I prefer the heading and form labels being aligned.

[johnjamesjacoby]

Replying to rachelbaker:

@johnjamesjacoby Apologies for my ignorance, but which metabox header and alignment are you using as a reference to match? I can see that the Edit Comment heading should be an h3 but I prefer the heading and form labels being aligned.

Basically, all other metaboxes. And I agree that having the h3 (or any title) left-align with it's content is pleasing (and I can update the patch to include padding adjustments to make that happen) but I don't think having unique mark-up for 1 box vs. what the API would generate for us (if we used it in this instance) is the solution to left-alignment bliss.

(Note also that the "Edit Comment" metabox content is unique in that it uses a fieldset and 'legend' combo inside a table, which is normally reserved for the settings API which does not use metaboxes.)

Updated patch imminent.

[johnjamesjacoby]

33361.02.patch is the same as 01, plus the following:

• .form-table td padding is tweaked in forms.css.
• #namediv td.first left padding is set to 0 (note that #namediv is still used by the retired "Links" feature)

Attached screenshot shows everything left-aligned, but to my eyes I don't consider this an improvement; the field labels feel too close to the edge. So, I prefer the results of the 01 patch, but think either is better than what we have today.

[johnjamesjacoby]

FWIW, the metaboxes we are patching here are likely a few of the last remaining that haven't seen a refresh in the past 5 years or so. All of comment.php and it's relative parts should be totally brought up to speed with the rest of WordPress eventually. These patches are purposely minimally evasive.

[johnjamesjacoby]

Replying to rachelbaker:
Awesome. Ping if you want to hash this out more.

[helen]

Why do we need a redundant h3? There's no collapse or drag for this not-really-a-post-box, so it seems unnecessary, and will cause a double read out for screen readers and a second navigation landmark. I agree that the spacing is still off, but I don't see any need for more markup changes here. The changeset you referenced was for #31326, which was markup changes for accessibility concerns.

[johnjamesjacoby]

Replying to helen:

Why do we need a redundant h3? There's no collapse or drag for this not-really-a-post-box, so it seems unnecessary...

Because that's the tag all metaboxes use, draggable or not.

and will cause a double read out for screen readers and a second navigation landmark.

I agree this is not ideal. Because fieldset elements cannot omit the legend element, barring a rewrite of the fieldset contents, this was my compromise.

I agree that the spacing is still off, but I don't see any need for more markup changes here.

Normalizing metabox header markup removes this 1 hidden gotcha (rather than introducing more hidden gotchas to support the unique markup with unique CSS) while also bringing it up to par visually with the rest of WordPress. This entire screen is a bit of a sore thumb; my suggestion is to patch it, not cut it off, replace it, or leave it more sore than I found it.

The changeset you referenced was for #31326, which was markup changes for accessibility concerns.

I understand; we all have the same annotations. I was just noting the most recent activity to that line to make the jaunt down memory lane a few clicks easier.

I'm fuzzy on what you're suggesting happens next. The attached patches are a visual improvement that negatively impact accessibility in a relatively minor way. The way I see it, our options are to:

• Commit the patch and iterate on the a11y later
• Iterate on the patch and spend a bunch more time on a 3 line change
• Do nothing until these boxes get a full a11y treatment
• Turn comments off everywhere by default forever and declare success over the internet

[rachelbaker]

@johnjamesjacoby,

We agree the Edit Comment screen looks "off", needs to be fully reworked, and that is going to take some time.

Initially, I rushed to the assumption we could find a minimally invasive patch. From the feedback and discussions here I don't see a hotfix patch that will do more good than harm.

I believe the best solution here is to hold-off on spending the energy on a hotfix patch, in favor of a full visual reworking of the Edit Comment screen. I am happy to drive the effort to update the UX of the Edit Comment screen, and will create a canonical ticket for the project.

[afercia]

In #31326 we started from the consideration that this box content is a group of logically related form fields, so we added a fieldset to group them and made "Author" a fieldset legend.

I agree with Helen this looks like a "not-really-a-post-box" and probably the same CSS classes were used just to make it look like a post box. I don't see any reason to add a redundant heading that would just repeat "Author" or something similar and I would recommend to don't do that. Wondering if it really should be a post box in the first place :) Since it's not draggable, maybe it shouldn't look like a postbox.

Please have a look at this screen with CSS turned off and notice how content gets read out by a screen reader (NVDA in this example):

You will notice that semantics is not just an abstract concept but directly affects the way software gets and "understands" content. And thus affects people who use that software.

The practical consequence of adding a fieldset with a legend is that NVDA announces this content block as "grouping". And then it also counts the form elements, announcing "1 of 3", "2 of 3" etc. All this would be lost without a fieldset + legend.

I'm all for any CSS improvements :)

[afercia]

Please always add the "accessibility" focus to any ticket related to content structure, headings, semantics, etc. Thanks :)

[johnjamesjacoby]

Hi everyone. Thanks for the attention, and thanks @afercia for the in-depth refresher.

I think that, so far, all of us involved in this ticket are really narrowing in on our specialties instead of identifying (hence agreeing) that there is actually something wrong with the markup or styling of this specific page. If y'all feel there isn't anything wrong, then I'd vote we close this ticket and move on. I don't think this is the case, though, and I think we've lost sight of the problem by choosing to focus on how poor my solution apparently was instead of iterating and suggesting more acceptable improvements.

It may make sense to re-outline the details, so we can work towards and propose agreeable solutions. Here's my take:

• Visually, the elements used to represent header text in comments.php are not in alignment with the rest of wp-admin
• Mark-up wise, the elements used to represent header text in comments.php are out-of-date with current trends in wp-admin standards
• Accessibility wise, the structure on this page is unique compared to other similar metabox insides
• CSS wise, styling is being applied in unintended ways because of the aforementioned legacy out-of-date mark-up

A few other tidbits:

• My (vetoed) suggestion of adding the screen-reader-text class to the legend tag is not unheard of. It's used no less than 40 times in wp-admin already for settings (though not paired with header tags, they are paired with th's)
• The link_target_meta_box() function is where I got this idea from, meaning we should include whatever conclusions we come to here, there also, since it's already in core with all the bad qualities noted above.
• Whether or not these should be collapsable metaboxes is mostly irrelevant to the interests of the visual inconsistency I've identified. They could be, and maybe they should be, but they aren't and never have been and no one seems to have complained but me until now. :)

@afercia - have you applied either of my attached patches to assess the assumed negative impact of them? Can you explain a bit about why this does not negatively impact other hidden fieldset legends in the same way, or why those are acceptable and this is not?

[johnjamesjacoby]

Tangentially, these look to me to be the last remaining metaboxes that are not collapsable on a new installation of WordPress. On upgrades to older ones, the old "Links" feature has the same problems that comments does here.

I've been trying to put together screenshots to zoom in deeply & compare pixel-by-pixel what the visual differences are, but not having another non-collapsable metabox to compare to makes that pretty difficult to do. :)

[afercia]

@johnjamesjacoby thanks for your in-depth analysis. That made me notice additional issues and inconsistencies :) For example, wondering why the first thing in the content is the "Author" box (maybe just for the initial focus?) while in all the other admin screens the Editor box is the first one, followed by additional meta boxes like Author, Publish, etc. By the way this issue is out of the scope of this ticket, should go in a separate one, but maybe we should look at the structure of this screen as a whole and see how the users flow can be improved.

My (vetoed) suggestion of adding the screen-reader-text class to the legend tag is not unheard of. It's used no less than 40 times in wp-admin already for settings (though not paired with header tags, they are paired with th's)

I think there's some consensus the Settings screens should be reviewed, they're not the best example in terms of semantics and accessibility.

Can you explain a bit about why this does not negatively impact other hidden fieldset legends in the same way, or why those are acceptable and this is not?

It does impact other screens too :) We discussed this a bit on Slack a while ago, haven't made a plan though. Consider for example the screenshot below, with all styles disabled it's very evident all the duplicated and redundant information there. These are the legends paired with th's you mentioned:

Screen readers read out all this redundant stuff:

The same redundancy happens when <label>'s are used inside table headers. Screen readers read them twice: 1. because of the th > td relationship 2. because of the label > form field association.

I've applied your patch .02 and "Author" will be read out twice. I agree it's a minor thing but since we're on it why should we intentionally double this info and add noise for users? :)

About the CSS part, I don't get what class="hnld" is meant for but maybe I'm missing something. Also, maybe we could remove the <span>'s? Finally, since now the main heading is a h1, the Status heading should be a h2. See all the ongoing work related to the #a11y-headings tickets.

[afercia]

P.S. Any example of plugins that add custom meta boxes in this screen?

[rachelbaker]

Replying to afercia:

...maybe we should look at the structure of this screen as a whole and see how the users flow can be improved.

I completely agree, and this was my intended position above.

I am moving this ticket out of the 4.4 milestone, while a direction continues to be hashed out here.

[afercia]

Discussed during today's accessibility bug scrub. Many things have changed in this page. Also, regarding collapsible meta boxes, a similar proposal was made for the Edit terms page and the design team decided against it, see #36574

@johnjamesjacoby what's left here? :)

[johnjamesjacoby]

From trunk (WordPress 5.1)

[johnjamesjacoby]

Many things have changed in this page.

I see that. Progress!

Also, regarding collapsible meta boxes, a similar proposal was made for the Edit terms page and the design team decided against it

My final suggestion would be to make the h2's on this page consistent. Either remove the bottom border on "Author" (.edit-comment-author) or add one to "Status" in the other .stuffbox wrapper.

Frankly, why one h2 gained an underline during 5.1 and not the other one, seems strange. In addition to this issue not being mentioned while those changes were happening, I don't really have any context.

How come this ticket wasn't referenced or used for those changes? (I see Slack mentions in the logs from you 2 years and 2 months ago, respectively, but nothing about the work that was done towards it.)

[afercia]

How come this ticket wasn't referenced or used for those changes?

Trac is hard. I forgot this ticket.

The h2 bottom border was added in [44466] / #43586 together with other adjustments. Probably #43586 should have been closed as duplicate of this ticket.

More importantly, I see [44466] removed the legend from the fieldset element. That change introduces a regression in the accessibility level of this page so I'm going to reopen #43586.

Re: the "Status" box, I see it has been split out from #43586 see #43587.

[johnjamesjacoby]

Trac is hard

I can hear you in a joking way, but Trac isn’t any harder than any other issue tracker. Saying so kinda only perpetuates the myth? Please don’t blame the tools for the process problem of not a single person searching for an existing issue, the result of which is everyone gets a new tool that nobody will use correctly again.

I forgot this ticket

You and everyone else, it looks like.

The most frustrating this as a contributor, is seeing leaders shut this down hard here, and then seeing the same exact ideas and work being iterated on elsewhere by other people, outside of this issue.

Frankly, I don’t blame people one bit for doing it that way. Even if they had found this ticket, they’d have read the back & forth over an incredibly small improvement, and avoided all of what’s here just to get the job done and totally avoid the up-front human obstruction.

This is why it’s so important to be open to all new ideas and changes, and to stop closing tickets and shutting people down. Most of the work is going to get done one way or the other, with or without the people that stand in the way. The only thing that comes from intentionally making things harder for each other, is failure.

See also: ​https://github.com/tootsuite/mastodon/pull/9898 for another good example outside of WordPress where similar folly is taking place.

Thanks a bunch @afercia for linking those tickets to this one. It’ll help everyone else out to see how they’re related. I appreciate you!

[afercia]

I can hear you. Re: the mastodon pull request, I've followed a bit as aardrian, the PR author, is a fellow accessibility practitioner :)

[joedolson]

Since #43587 has been closed, as far as I can tell everything in this ticket is now resolved - and it apparently just got forgotten a second time.

Closing it, but if there is something still left from this ticket, feel free to reopen!