WordPress.org

Make WordPress Core

Opened 21 months ago

Last modified 3 months ago

#33361 accepted defect (bug)

"Edit Comment" metabox header styling is inconsistent with others

Reported by: johnjamesjacoby Owned by: rachelbaker
Milestone: Future Release Priority: normal
Severity: minor Version:
Component: Comments Keywords: has-patch
Focuses: ui, accessibility, administration Cc:

Description

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.

Attachments (6)

Screen Shot 2015-08-12 at 7.27.27 PM.png (90.2 KB) - added by johnjamesjacoby 21 months ago.
Note "Author" is off kilter, and "Status" is underlined
33361.01.patch (978 bytes) - added by johnjamesjacoby 21 months ago.
Screen Shot 2015-08-12 at 7.33.40 PM.png (94.4 KB) - added by johnjamesjacoby 21 months ago.
After
Screen Shot 2015-08-19 at 7.16.28 AM.png (28.2 KB) - added by johnjamesjacoby 21 months ago.
33361.02.patch (1.5 KB) - added by johnjamesjacoby 21 months ago.
Screen Shot 2015-08-19 at 7.42.35 AM.png (89.9 KB) - added by johnjamesjacoby 21 months ago.

Download all attachments as: .zip

Change History (25)

@johnjamesjacoby
21 months ago

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

#1 @johnjamesjacoby
21 months ago

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
Last edited 20 months ago by johnjamesjacoby (previous) (diff)

#2 follow-up: @rachelbaker
21 months ago

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

#3 in reply to: ↑ 2 @johnjamesjacoby
21 months ago

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.

#4 @johnjamesjacoby
21 months ago

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.

#5 @johnjamesjacoby
21 months ago

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.

#6 follow-up: @rachelbaker
21 months ago

  • Milestone changed from Awaiting Review to 4.4
  • Owner set to rachelbaker
  • Status changed from new to accepted

#7 in reply to: ↑ 6 @johnjamesjacoby
21 months ago

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

#8 follow-up: @helen
21 months ago

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.

#9 in reply to: ↑ 8 @johnjamesjacoby
21 months ago

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

#10 @rachelbaker
21 months ago

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

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


20 months ago

#12 @afercia
20 months ago

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

https://cldup.com/UTg2Q3ahyJ.png

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

#13 @afercia
20 months ago

  • Focuses accessibility added

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

#14 @johnjamesjacoby
20 months ago

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?

#15 @johnjamesjacoby
20 months ago

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

#16 follow-up: @afercia
20 months ago

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

https://cldup.com/HdPirfFNql.png

Screen readers read out all this redundant stuff:

https://cldup.com/xnQoDKSPIx.png

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.

#17 @afercia
20 months ago

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

#18 in reply to: ↑ 16 @rachelbaker
20 months ago

  • Milestone changed from 4.4 to Future Release

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.

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


3 months ago

Note: See TracTickets for help on using tickets.