WordPress.org

Make WordPress Core

#38023 closed defect (bug) (fixed)

Menus screen: simplify the Menu Settings controls

Reported by: afercia Owned by: rianrietveld
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-screenshots has-patch
Focuses: ui, accessibility Cc:

Description

On the menus screen, each menus form has a "Menu Settings" section at the bottom with a couple settings. The markup uses two definition lists <dl>:

https://cldup.com/tYqE4OfGz2.png

While these controls are perfectly operable and have correctly associated labels, the definition list conveys inappropriate information to screen reader users and its semantically not correct. It would be great to use proper form elements instead (fieldset, legend) and maybe remove the "Auto add pages" text for the single checkbox.

https://cldup.com/V8e_PtcNDg.png

Attachments (3)

38023.diff (3.5 KB) - added by xavortm 11 months ago.
38023.2.diff (3.6 KB) - added by xavortm 11 months ago.
38023.3.diff (3.8 KB) - added by afercia 11 months ago.

Download all attachments as: .zip

Change History (21)

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


13 months ago

#2 @rianrietveld
13 months ago

  • Milestone changed from Awaiting Review to 4.7

Discussed this in the wpa11y bug scrub:
We agree on:
It would be great to use proper form elements instead (fieldset, legend)
We have similar constructions on other pages, maybe find a consistent way to do them
Needs research.

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


12 months ago

#4 @afercia
12 months ago

  • Owner set to rianrietveld
  • Status changed from new to assigned

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


11 months ago

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


11 months ago

@xavortm
11 months ago

#7 @xavortm
11 months ago

  • Keywords has-patch added

Screenshot of the end result:
http://i.imgur.com/OwvLyzg.png

Visually there is no change, it's just in the markup. I changed the wrapper with fieldset and the label with legend. The only thing I'm uncertain of currently is the div selector in the CSS

Two things improved only on CSS - more padding on smaller screens, now the text is not touching the checkbox and usage of full percentages instead of weird fractions. I couldn't see this breaking something.

Last edited 11 months ago by xavortm (previous) (diff)

#8 @GaryJ
11 months ago

Instead of targeting the CSS via the element, could you add a common class to both fieldsets (in addition to their unique ones), and target that in the CSS selector as well?

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


11 months ago

#10 follow-up: @rianrietveld
11 months ago

Tested the patch, it looks fine to me and works perfect.
Nice job @xavortm.

@GaryJ can you please specify more what you mean?
The CSS/classes are quite similar of what they where.

#11 follow-up: @xavortm
11 months ago

Thanks, @rianrietveld

I think the problem is that I am selecting by HTML element, not a class, which I understand, though I wasn't sure if I had to introduce another classes or if we're using similar ones somewhere else. Also it would be nice to know if such a component is being used elsewhere before rewriting the wrappers.

Having classes for selecting this component is a plus for sure in my opinion. Also another solution would be to change a bit the markup ( I only changed the html elements ) by wrapping the left and right part in divs and setting display:table, just like we have everywhere else around the dashboard, just without the table elements.

I hope I'm not overcomplicating things here If it works as it is, then I'm happy

#12 in reply to: ↑ 11 @afercia
11 months ago

Replying to xavortm:

I hope I'm not overcomplicating things here If it works as it is, then I'm happy

I'd lean towards keeping it simple :) Thanks @xavortm !

#13 in reply to: ↑ 10 @GaryJ
11 months ago

Replying to rianrietveld:

@GaryJ can you please specify more what you mean?
The CSS/classes are quite similar of what they where.

That doesn't mean they were right first time :-)

The patch here is an example of why using element selectors is not great - a change in the choice of element should not necessarily mean a change in the CSS. If CSS classes are used, these coupled changes are not needed (as much).

If the original dl had got a class of something like menu-settings-item, then the dl could have changed to fieldset without any corresponding CSS change (the extra styles for legend would have been needed).

My suggestion is to add a class like that in now, and attach the CSS to it, so that any future markup changes don't necessarily need to change the CSS as well.

Being able to use a class selector, not only decouples the elements from the CSS, it also means the CSS can have reduced specificity, as .menu-settings-item {} could probably be used, instead of .menu-settings fieldset {}.

My suggestion isn't a critical blocker, and the current patch would work as is, but if there's an opportunity to use better practices, and leave the code slightly cleaner than before the patch, I'd personally rather see it done.

#14 @afercia
11 months ago

Yeah @GaryJ totally agree. It's a general issue in core and it's true also for jQuery selectors: they shouldn't ever use elements as selectors (or part of it) because then changing the markup becomes harder. Performance also could be a concern (at least it was in the past). Also lowering specificity is a very good point. I'd like to see these points raised up looking at the big picture, and I'd love to see some work on the CSS roadmap having a start. There are also other considerations, for example in my view:

  • all the core CSS selectors should use some prefix, e.g. wp-
  • they should use some sort of naming convention (not necessarily BEM)
  • CSS selectors used for JavaScript shouldn't be used for styling

That said, this specific case is a very minor one but it's a good opportunity to show some best practices so I'm in favor of using some new classes.

#15 @afercia
11 months ago

See #26259 for some history.

@xavortm
11 months ago

#16 @xavortm
11 months ago

About the patch:
Updated the selectors with generic enough names so it's not limited to the menu item. It's only the wrapper element that says its a menu. Having classes that don't specify context will allow for better reusability.

About the last comment: #14
On class names :
.menu-item -> always a menu
.list-item -> can be anything (same styling of course), though with a different modifier it can be altered. Only the wrapper specifies the type of the component.

I have written an article on that topic, I can share in slack maybe.. In short - in my eyes it's best if the JS selectors are prefixed with .js-* and never targeted with css. State classes are prefixed with .is-* . Then there are utilities and modifiers that can be or not prefixed, depending on the project. Modules (components) have the name of the component on the wrapper element and all inner elements have class names of .<component>-element. Using Sass that can create one level stylings for most of the application. All HTML elements without most inline elements must have classes and no IDs.

Again, I am diverging a lot here from the final goal, sorry about that...

Last edited 11 months ago by xavortm (previous) (diff)

@afercia
11 months ago

#17 @afercia
11 months ago

Refreshed patch to take care of the responsive view. Also, I'd rather reduce specificity than having perfectly reusable classes. I dream of a day when reducing CSS selectors specificity will be a required task for any CSS change. 🙂 Also, keeps classes separated based on their intended usage and adds a comment to clarify the math used for the negative margin calculation. I'm fine with the usage of :last-of-type, the worst that can happen is a slightly different bottom margin.

#18 @afercia
11 months ago

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

In 38912:

Menus: Improve the HTML semantics of the "Menu Settings" section.

  • removes the previous markup based on a definition list
  • groups checkboxes in fieldset elements with a legend
  • simplifies the CSS lowering selectors specificity

Props xavortm.

Fixes #38023.

Note: See TracTickets for help on using tickets.