Make WordPress Core

Opened 7 years ago

Last modified 14 months ago

#39441 new enhancement

Improve the Settings API for accessibility and ease of use.

Reported by: flixos90's profile flixos90 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Plugins Keywords: settings-api has-patch 2nd-opinion
Focuses: accessibility, administration Cc:

Description (last modified by flixos90)

Today a kickoff meeting for the Settings API took place on Slack (Archive: https://wordpress.slack.com/archives/accessibility/p1483376507000492) where we discussed ways to improve it, both in terms of accessibility and ease of use.

After a good discussion we came to the conclusion that we would like to focus on the existing Settings API for now and do what we can to improve it. The Fields API project will eventually make the process of registering settings and having them automatically rendered even easier and complete, but we should not wait until it is ready for a core-merge, as some issues in the existing Settings API should be solved sooner than later.

We figured out two main goals:

  • Add some basic support to automatically render fields so that plugin developers no longer need to write their own callback functions for basic fields.
  • Get rid of the table structure to improve accessibility. Furthermore the accessibility team should also ensure that the markup generated as the core field output is accessible.

For the technical improvements, we would like to do the following:

  • Adjust add_settings_field() to support a predefined set of identifiers for a field type instead of a callback function. In that case a default callback function that we would introduce in core would take care of rendering the field. The requirement to write custom callbacks for even the most basic fields is one major issue with the current Settings API and why most people rely on external libraries for that.
  • Enhance the $args parameter of add_settings_field(). It should become more significant and probably go through some validation, filling it with default values. This is especially important for the new default callbacks.
  • Possibly support providing one $args array as sole parameter to add_settings_field() that contains the other parameters as well. This would of course need to work in a backward-compatible way.

For the accessibility part, we would like to do the following:

  • Scaffold an HTML prototype for what a settings page should look like. This will give a good overview of the accessibility approach we should take without having to deal with the PHP implementation.
  • Get rid of the current table structure. Whatever the above prototype looks like, it will not have tables anymore. We can generally remove the table structure and change it to something else easily, since all the table output is generated by core (in particular by do_settings_sections() and do_settings_fields()). For everyone who uses the API as recommended, this will not bring any BC issues unless they are using specific table-related selectors (like td) in CSS or JS code. It is unclear whether these should be considered edge-cases and whether a dev-note reflecting the changes is sufficient, or whether we should only support the new markup through an additional parameter which would default to the current table way. The latter is backward-compatible, but on the other hand it would decrease the amount of sites that become more accessible out-of-the-box.
  • Do not deal with people who completely write the table markup manually. We simply cannot do this, other than recommending them to switch to using the Settings API or at least changing their markup. The only thing to keep in mind here is that we should never remove any CSS related to these tables, in order to keep their code working.

All of these enhancements would also benefit #38734 as it would become a lot easier to change core's own settings pages to actually use the Settings API.

We will from now on have meetings on Slack to continue discussion in detail every 2 weeks on Monday at 17:00 UTC. However, general opinions and discussion can and should also be placed in this ticket.

Attachments (12)

39441.diff (9.4 KB) - added by flixos90 7 years ago.
39441.2.diff (11.3 KB) - added by flixos90 7 years ago.
39441.3.diff (23.3 KB) - added by flixos90 7 years ago.
39441.css.diff (8.9 KB) - added by flixos90 7 years ago.
39441.implementation.diff (28.7 KB) - added by flixos90 7 years ago.
39441.field-list.md (1.7 KB) - added by flixos90 7 years ago.
39441.4.diff (23.5 KB) - added by flixos90 7 years ago.
39441.5.diff (23.5 KB) - added by flixos90 7 years ago.
39441.6.diff (23.5 KB) - added by flixos90 7 years ago.
39441.css.2.diff (8.9 KB) - added by flixos90 7 years ago.
39441.implementation.2.diff (31.0 KB) - added by flixos90 7 years ago.
39441.7.diff (23.2 KB) - added by flixos90 7 years ago.

Download all attachments as: .zip

Change History (48)

@flixos90
7 years ago

#1 @flixos90
7 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

39441.diff is an initial take on the technical part of the improvements from the ticket description (except implementing the single array parameter support of add_settings_field()). What the patch does:

  • Support passing one out of a few predefined strings as the $callback parameter in add_settings_field(), which then refer to core callback functions (happens in do_settings_fields()): What these strings and core-supported field types are needs to be discussed, I just added a few very basic ones that I think we should support. Four of the five callbacks are actually not yet implemented and only exist for demonstration purposes.
  • Enhance the $args parameter in add_settings_field(). It now supports more arguments and does some sanitization by filling defaults in. The only complex new argument is probably the value_callback one: It will be used to fetch the current value for the field. It usually does not need to be used, as most developers are probably using a regular get_option() call here. The default value for this argument is a new function get_settings_field_option() which retrieves the option based on the field's input_name argument. The function also supports fetching option values for multidimensional names, similar how it happens currently in the Customizer.

I also created a small plugin settings-api-tests.php that can be used to test this functionality. Note that some fields there won't have any output yet, simply because I didn't implement their functions yet, as I mentioned above.

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


7 years ago

#3 @flixos90
7 years ago

  • Description modified (diff)

Cross-reference to the ticket was wrong in the description. It is supposed to be #38734.

@flixos90
7 years ago

#4 @flixos90
7 years ago

39441.2.diff is a minor update over the first iteration.

  • adds support for a new input_class key in the $args array of add_settings_field()
  • properly fills the label_for key with the value of the input_id key
  • automatically adds an id attribute to the field description and the same value to the field's aria-describedby
  • implements render_settings_field_select() (just another test implementation)
  • moves settings field callback switch clause into add_settings_field() as it should be part of the argument parsing

The small test plugin settings-api-tests.php is no longer available on the ticket. It is now available on https://github.com/felixarntz/settings-api-tests for collaboration.

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


7 years ago

#6 @swissspidy
7 years ago

See also discussion on #21488.

#7 @swissspidy
7 years ago

#21488 was marked as a duplicate.

@flixos90
7 years ago

@flixos90
7 years ago

#8 @flixos90
7 years ago

As discussed with @afercia during the last Settings API meeting, I looked at the HTML prototypes he created at https://wpaccessibility.github.io/test-forms/ (see "General Settings", "Discussion Settings" and "Network Settings") and improved the existing patch to be suitable for an implementation of these fields. I uploaded the following new files:

  • 39441.3.diff is the updated patch for the new Settings API behavior. It now has all the default supported field types implemented, uses div elements with specific classes instead of tables and also supports automatic fieldset handling for the default field types. Several minor enhancements over the previous patch have been added.
  • 39441.css.diff acts closely together with 39441.3.diff as it provides the CSS required for the table replacement markup to display properly. I didn't really dive into it deeply, but rather duplicated several styles, just to make it work. This would probably need to be redone in a better way before a possible core merge.
  • 39441.implementation.diff is an implementation of the settings fields in "General Settings", using the new Settings API behavior. For easier readability of the patch, I put all the code at the top of wp-admin/options-general.php. There are four custom render callbacks, which are needed for a few fields which require very custom code. A tiny bit of JavaScript was changed in wp-admin/includes/options.php, just to apply to some adjusted markup correctly.
  • 39441.field-list.md is the list of all fields in @afercia's prototypes and which of the default callbacks they could use. Only very few will need custom callbacks (most of them in "General Settings"), so this was a positive thing to find out.

A thorough review on all of this needs to be done, and tomorrow's meeting can be the starting point for that. We must also figure out further ways to improve accessibility on the fields and, if necessary, update the patches accordingly. The last step will probably be to write further implementations for the other settings pages (see #38734 as well).

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


7 years ago

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


7 years ago

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


7 years ago

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


7 years ago

#13 follow-up: @flixos90
7 years ago

Replying to @afercia (see https://wordpress.slack.com/archives/accessibility/p1487614539002584):

  1. labels must be required: no label, no field output
  2. we do only explicitly associated label ("for" attribute)
  3. we don't output stray labels e.g. echo ' <label>' . $field_args['label'] . '</label>';
  4. groups shouldn't have a label for the... group
  5. we need CSS classes for the labels (or, better, an option to print a screen-reader-text class?)
  1. Are you referring to the $title parameter of add_settings_field() or to the input_id / label_for arguments of $args? Currently the $title parameter of add_settings_field() is already required (which makes sense!), and that one is used as label. The id attribute is automatically populated with the $id parameter as well as the label for attribute is. One can override that behavior by specifying these in $args and could theoretically remove one of the attributes, but I think this possibility should be left in there for some edge-cases - think someone wants to use a field callback, but then output the label somewhere else: They could currently do so by overriding label_for to an empty string, but not any more if we disallow empty label_for. I think providing sane defaults here should be sufficient.
  2. Not sure I understand what you mean here.
  3. I agree. In 39441.3.diff that is only the case in the checkbox callback, so I'll adjust it there.
  4. I think the code in 39441.3.diff does not put a label on a group. While in the code it is handled the same way as a regular label, it is actually output as a legend of a fieldset in these group cases (which I thought is okay). Correct me if I'm wrong. :)
  5. I'll put that in the next patch as well.

Just for reference, I'll leave https://wordpress.slack.com/archives/accessibility/p1487614972002599 here: We need to figure out whether before and after should be printed inside or outside of fieldsets (only applies to fields that have fieldset set to true).

For anyone who would like to participate, join today's meeting at 17:00 UTC in #core.

@flixos90
7 years ago

#14 @flixos90
7 years ago

39441.4.diff makes the above improvements:

  • Do not output a <label> without a for attribute.
  • Support a label_class argument. This class is applied to the visible element that outputs the field's $title.
  • Fix a bug with before and after: Previously a callback string would be printed out instead of calling the callback.

#15 in reply to: ↑ 13 @afercia
7 years ago

Replying to flixos90:
@flixos90 thanks very much for the updated patch!

1
The $title parameter of add_settings_field(): passing a falsy value (empty string, empty array, false, '0', 0, ...) will output a field without a label. This shouldn't be allowed. Labels should be always required. Also, wondering if enforcing a type check would make sense since having the ability to pass something other than a string doesn't look right to me.

2
There are different ways to associate a label to a field:
Explicit association: the label doesn't wrap the field and uses a for attribute

<label for="something" ... >Label text</label>
<input id="something" ... / >

Implicit association: the label wraps the field and doesn't use a `for attribute:

<label ... >Label text
<input id="something" ... / >
</label>

A mix of the twos:

<label for="something" ... >Label text
<input id="something" ... / >
</label>

We should only use Explicit association.

4
For example, the Date and Time format groups of radio buttons have a label that shouldn't be a label. Radio buttons already have their label. Instead, "Date Format" and "Time Format" should be legend elements of a fieldset grouping all the radio buttons.

https://cldup.com/2QPysvvMWC.png

#16 follow-up: @flixos90
7 years ago

@afercia

  1. I generally agree that an empty $title should be disallowed, on the other hand I'm not sure how to handle this in terms of BC. Also I think we should try to make all the default form controls accessible by default, but if the user explicitly wants to have it in a different way, I think it should be possible. For example, even with a title it is still possible to not print a label (only if you really do not want it) by explicitly setting the label_for argument to an empty string. I think that should be acceptable since it would otherwise restrict developers too much (if they would like to display the label manually for example, let's say in before or something like that).
  2. Was that incorrect somewhere in the patch? I thought it uses explicit association all over.
  3. Right, that is a bug in 39441.implementation.diff specific to these fields. I'm generally aware of this and considered it in the main patches. When using the default radio group callback, that text "Date Format" would automatically be printed as a fieldset. Since I use a custom callback for these fields though, I need to add the fieldset => true argument manually, which I overlooked. So I'll update that later as well.

#17 in reply to: ↑ 16 @afercia
7 years ago

Replying to flixos90:

  1. Was that incorrect somewhere in the patch? I thought it uses explicit association all over.

Hm, yep can't find the reason why I've written that any longer. Maybe I was looking at something else in template.php 😬

I'd propose to discuss 1. later today during the meeting, together with the before and after thing.

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


7 years ago

#19 follow-up: @flixos90
7 years ago

Quick summary on today's meeting:

  • For single checkboxes, the $title should be used as the checkbox label, and nothing should be displayed in the left column. The checkbox label should be printed in bold.
  • The $title is currently printed out twice when fieldset is true. That is because the two column layout restricts us in a way that the fieldset can only be part of the right column, not actually wrap both columns. Therefore the $title is printed as <span aria-hidden="true"> in the left column and as <fieldset><legend class="screen-reader-text"> in the right column. It would be much better if the <legend> could be used as visual element as well instead of having two elements. This would require us to get rid of the two column layout though.
  • Moving away from the two column layout would be drastic change to the UI. We need opinions from more core developers and also discuss this with the design team. Is that change acceptable, and if so, what could the new layout look like?
  • The $title is a required argument, but it's possible to specify it as empty. There was no concensus yet whether an empty title should be ignored or whether a notice should be triggered. Generally it's wrong to pass an empty title, but there might be edge-cases where developers want to print the label themselves, and this would not be possible if we enforced the $title. We probably leave it as is, but this still needs more discussion.

@flixos90
7 years ago

#20 @flixos90
7 years ago

39441.5.diff is an updated patch which changes the following over the previous one:

  • For a single checkbox, the $title is used as the checkbox label. A new skip_title argument is used to tell do_settings_fields() to not print the title in there.
  • This new skip_title argument is generally used instead of simply checking if the title is empty. This might be a good idea for our ongoing discussion with requiring the title. At least in the code itself it now appears to be required, and one needs to explicitly set skip_title to set it as not required. This allows us to actually throw a notice if the $title is empty, and at the same time allow devs to bypass printing it if they really do not want that.

@flixos90
7 years ago

@flixos90
7 years ago

#21 @flixos90
7 years ago

A few small updates:

  • 39441.6.diff and 39441.css.2.diff introduce a new .title-label class that is used to make the checkbox label appear bold (see above).
  • 39441.implementation.2.diff is an updated patch for the implementation of General Settings. The issue that no fieldsets were used on the Date and Time radio groups (see further above) was fixed, as well as the $title for users_can_register has been updated to be the label attribute (as we don't need the "Membership" text any longer). The code to add the settings fields has been moved to a separate file.

#22 in reply to: ↑ 19 @afercia
7 years ago

Replying to flixos90:

  • The $title is currently printed out twice when fieldset is true. That is because the two column layout restricts us in a way that the fieldset can only be part of the right column, not actually wrap both columns. Therefore the $title is printed as <span aria-hidden="true"> in the left column and as <fieldset><legend class="screen-reader-text"> in the right column. It would be much better if the <legend> could be used as visual element as well instead of having two elements. This would require us to get rid of the two column layout though.

About this, there's also another option. In the current patch, it is not possible to make the fieldset wrap both columns because <div> elements are used to mimic the two columns layout. However, these <div>s are not actually required and there are other ways to have a two columns layout via CSS. Without <div>s, the fieldset legend could be actually, and properly, used as a visual element avoiding aria-hidden and screen-reader-text all together.
Not to mention the markup would be a lot cleaner.

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


7 years ago

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


7 years ago

#25 @rianrietveld
7 years ago

During the contributors day at WordCamp London we discussed how to proceed with the Settings API changes together with
@afercia @karmatosed @flixos90 @samikeijonen @rianrietveld @hedgefield.

Result discussion
We are going for the one column approach, so the labels will be above the input fields.
Advantages:

  • more fresh and modern look
  • easier to design responsive
  • we can implement proper fieldsets and have better controle over the HTML to make it WCAG 2 AA accessible
  • people who only see a small part of the screen or magnify the screen still have a better understanding of the form content

Design ideas

  • ask @michael-arestad for help, Tammie can join in if Michael has no time
  • use space/padding between the fieldsets to make them stand out
  • don't be afraid to use rules for a cleared layout
  • look at the settings page at WordPres.com with Calypso for design inspiration (but stick to WCAG rules for colours and HTML)
  • maybe style fieldsets
  • maybe bigger font

Plan of action

  • start with the settings page, in a mock up page, first get the HTML/CSS sorted out.
  • start with a featured plugin on GitHub and ask for feedback and design help. @flixos90 will start with that and will get help from the design team
  • when approved and optimised we will also redesign the other pages that include tables, (e.g. all cases that use the class form-table).

Time
Get the settings page plus design plugin ready for WordCamp Europe, for review.

Last edited 7 years ago by rianrietveld (previous) (diff)

@flixos90
7 years ago

#26 @flixos90
7 years ago

Related to the above discussion, 39441.7.diff is a minor update that removes the far-from-optimal way of printing the field title twice and instead produces clean markup.

With this patch, the CSS styles will be visually messed up a bit, but it shouldn't matter now since we are going for a one-column layout anyway. The markup in the patch is probably closer to what we are going to end up with though.

Last edited 7 years ago by flixos90 (previous) (diff)

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


7 years ago

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


7 years ago

#29 @flixos90
7 years ago

I just committed an update to https://github.com/wpaccessibility/settings-api-enhanced:

  • The options-general.php page now uses separate settings sections instead of just one 'default'.
  • The margin between field groups is now handled through those settings sections.
  • None of the settings sections currently have headings (in order to appear as before, but we could add some if necessary).
  • Which fields are grouped together under which name can of course be adjusted, for now I put it in a way I thought would make sense.

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


7 years ago

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


7 years ago

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


6 years ago

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


5 years ago

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


5 years ago

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


5 years ago

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


14 months ago

Note: See TracTickets for help on using tickets.