WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#33646 closed defect (bug) (fixed)

#a11y-headings - the Screen Options h5

Reported by: afercia Owned by: joedolson
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.3
Component: Administration Keywords: has-patch commit
Focuses: ui, accessibility Cc:

Description

Several admin screens have a "Screen Options" panel which uses <h5> headings as titles. See the screenshot below which also shows the current document outline (just an example, each screen is different):

https://cldup.com/TDDkPGi4WV.png

The Customizer uses Screen Options too, the html output comes from the same bunch of code in WP_Screen::render_screen_options() in /wp-admin/includes/screen.php.

https://cldup.com/EXmGr59w4d.png

Discussed a bit in the last a11y Slack meeting and we agreed those h5 shouldn't be headings in the first place. Since we're inside forms, they're sort of "titles" that identify form sections with logically related form controls. We should probably use <fieldset> elements for each group of related form controls and <legend> elements as titles. Any thoughts welcome.

#a11y-headings

Attachments (6)

33646.patch (2.9 KB) - added by joedolson 5 years ago.
Use fieldset/legend for screen options
33646.2.patch (2.9 KB) - added by joedolson 5 years ago.
Fix logic error
33646.3.patch (3.5 KB) - added by afercia 5 years ago.
33646.4.patch (4.0 KB) - added by afercia 5 years ago.
33646.5.patch (11.1 KB) - added by afercia 5 years ago.
33646.6.patch (11.1 KB) - added by afercia 5 years ago.

Download all attachments as: .zip

Change History (30)

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


5 years ago

@joedolson
5 years ago

Use fieldset/legend for screen options

#2 @joedolson
5 years ago

  • Owner set to joedolson
  • Status changed from new to accepted

Patch switches H5 in screen options to fieldset/legend combination. For 'Screen Layout', I elected to use aria-describedby for the 'Number of Columns' text rather than use two nested fieldsets.

I noticed the h5 referenced in CSS for the Help panel, as well, but couldn't find an example of that in use; I think it's now obsolete. If somebody can confirm that, I'll remove that from the CSS, as well.

#3 @joedolson
5 years ago

  • Keywords has-patch needs-testing added

#4 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 4.4

#5 @afercia
5 years ago

It's a bit complicated to keep track of all the conditionals and different options available but looks like checking for if ( ! empty( $columns['_title'] ) ) is not appropriate. To my understanding, columns may or may not have a title.

https://cldup.com/CeQRzSMWAl.png

#6 @joedolson
5 years ago

I didn't really look at the conditionals; just re-used what was there, assuming that they were already written to consider that aspect. I'll take a closer look at those tomorrow.

#7 @joedolson
5 years ago

So, I think that checking for if ( ! empty( $columns['_title'] ) ) is actually appropriate. If there's no column title, then we have no text to use as a legend, so we shouldn't group these options in a fieldset. There's no point in a fieldset without a legend.

There was a logic error in how the fieldsets were set up; this required me to use nested fieldsets, which I don't really like, but it works great with NVDA in my testing.

Could use JAWS and VoiceOver testing for the nested fieldsets.

@joedolson
5 years ago

Fix logic error

@afercia
5 years ago

#8 @afercia
5 years ago

Refreshed patch after [34169]. Removed a couple of <br class="clear" />. Added a couple of CSS adjustments.

Could use JAWS and VoiceOver testing for the nested fieldsets.

Yup :) Should be quickly checked in the Customizer too (e.g. Menu Customizer Screen Options). cc @ocean90

Last edited 5 years ago by afercia (previous) (diff)

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


5 years ago

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


5 years ago

#11 @westonruter
5 years ago

Instead of duplicating the conditional logic, having two instances each of:

if ( isset( $wp_meta_boxes[ $this->id ] ) || $this->get_option( 'per_page' ) || ( $columns && empty( $columns['_title'] ) ) ) :

and

if ( ! empty( $columns['_title'] ) ) :

I suggest storing the value for the conditional in a variable:

$do_outer_fieldset_wrap = ( 
    isset( $wp_meta_boxes[ $this->id ] ) 
    || 
    $this->get_option( 'per_page' ) 
    || 
    ( $columns && empty( $columns['_title'] ) ) 
);
$do_inner_fieldset_wrap = ( ! empty( $columns['_title'] ) );

And then use these bare variables in the if conditions.

This will keep the code DRYer and less prone to error when/if the conditions change.

@afercia
5 years ago

#12 @afercia
5 years ago

Refreshed patch as per @westonruter suggestion. I'm not sure about a couple of things though. Before, the check for

isset( $wp_meta_boxes[ $this->id ] ) || $this->get_option( 'per_page' ) || ( $columns && empty( $columns['_title'] ) )

was used just for printing out (or not printing out) the heading 'Show on screen'. Now, it's used to conditionally print out the main fieldset + its legend but:

  • we're checking for the "per page" option but then the per page field is outside the fieldset... maybe we should remove the check or move the per page field inside the fieldset
  • if I'm not mistaken, in a hypothetical screen with meta boxes + columns with no title, the second group of checkboxes would be printed out without a "title"

Any thoughts?

Side note: there are outstanding accessibility issues in this form but they should go in separate tickets.

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


5 years ago

#14 @afercia
5 years ago

  • Focuses ui added
  • Keywords needs-testing removed

A couple of weeks ago we discussed a bit this ticket in the a11y meeting on Slack, agreed to try to find a new approach, avoid nested fieldsets and have cleaner markup. I'm a bit late but here's a first refreshed patch which tries to do that :)
Thinking at what's displayed inside the "Screen Options" tab, there are different groups of things, they're logically different and have different goals. Nonetheless, there's usually just one "title" to identify all of them and it's "Show on screen". Maybe, a better approach would be wrapping each group in a <fieldset> and give each group a unique <legend> as "title".
As far as I can see, there are 5 groups:

- meta boxes
- list table columns
- layout columns
- per page setting
- screen settings (e.g. expand editor, widgets accessibility mode. This is the place where plugins usually add their own stuff)

These groups are displayed conditionally in the admin screens and witht he patch applied, each group will have its own proper semantic. We shouldn't worry anymore about a "wrapper fieldset" or mess with conditionals.

Special care should be taken in choosing a proper label text, would need some native English speaker help for this :) The temporary legend text are:

- Boxes to show on screen
- Display columns
- Layout columns
- Pagination settings
- Additional settings

Also, the patch avoids to output the "Enable accessibility mode" links inside a form.

Example with the patch applied in the Edit Post screen:

https://cldup.com/1dJETroqVI.png

Example with additional settings appended by plugins, some of them provide their own "titles", some not:

https://cldup.com/invAGz8xoU.png

@afercia
5 years ago

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


5 years ago

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


5 years ago

#17 @afercia
5 years ago

Related: #22222

@afercia
5 years ago

#18 @afercia
5 years ago

Refreshed patch. As per conversation on Slack with @helen we'd like to use very simple "titles":

- Boxes
- Columns
- Layout
- Pagination
- Additional settings

This ticket should be now handled in conjunction with #22222 which aims to move the "List/Excerpt mode" setting within the Screen Options and make the "Apply" button a primary button.
When plugins append their settings, we should also try to have the Apply button as last thing at the bottom, for example:

https://cldup.com/Wgk7l-4-2G.png

#19 @afercia
5 years ago

  • Keywords commit added

Agreed with @helen to commit the latest patch here and doing the other stuff separately.

#20 @afercia
5 years ago

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

In 34991:

Administration: Convert H5 usage in Screen Options to use fieldsets and legends.

These H5 (heading level 5) don't allow for a good headings hierarchy and shouldn't be headings in the first place. Each group of options has now its own fieldset and legend.
In render_screen_options(), introduce two new "render" methods: render_meta_boxes_preferences() and render_list_table_columns_preferences() for consistency with already existing render methods and cleaner code.

Props joedolson, afercia.
Fixes #33646.

#21 @wonderboymusic
5 years ago

In 35010:

WP Screen: after [34991], avoid unnecessary nesting levels and remove unused global import.

See #33646.

#22 follow-up: @GaryJ
5 years ago

<legend><?php echo $legend; ?></legend>

That legend can be populated from column titles - so should it be escaped?

#23 in reply to: ↑ 22 @afercia
5 years ago

Replying to GaryJ:

That legend can be populated from column titles - so should it be escaped?

Good point. Not sure what to do here, looking at this kind of stuff (e.g. Post or Taxonomy labels) I see sometimes they're escaped, sometimes not. Needs some feedback from leads.

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


5 years ago

Note: See TracTickets for help on using tickets.