Make WordPress Core

Opened 2 years ago

Closed 21 months ago

Last modified 19 months ago

#54394 closed feature request (fixed)

Add functions for required fields indicator and message

Reported by: sabernhardt's profile sabernhardt Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-dev-note
Focuses: accessibility Cc:

Description (last modified by sabernhardt)

Required fields should include a visual indicator, plus a legend when using a star or similar character for multiple fields.

Having a set of return and echo functions to create strings for both the indicator and a default explanation could help in some core implementations such as #38460, as well as with plugins and themes (if their minimum WordPress version includes these functions).

Attachments (4)

54394.patch (2.3 KB) - added by sabernhardt 2 years ago.
basic concept
53494.2.patch (4.3 KB) - added by sabernhardt 2 years ago.
using a second argument to determine whether to echo each function instead of creating two more functions, plus including adjustments to comment template file
53494.3.patch (2.9 KB) - added by sabernhardt 2 years ago.
updating comments form example after r52200
53494.4.patch (9.2 KB) - added by sabernhardt 2 years ago.
using functions in comments template, the New Site form, and 3 media functions

Download all attachments as: .zip

Change History (71)

@sabernhardt
2 years ago

basic concept

#1 @sabernhardt
2 years ago

The names of the functions could change; these are what I have for now:

get_required_field_indicator
the_required_field_indicator

get_required_field_message
the_required_field_message

Each of these functions has a $space_before argument to define whether the tag should have a space before it and which kind of space (someone may want non-breaking or similar).

I may have overused the escape functions, but the first patch is mainly for the concept.

#2 @sabernhardt
2 years ago

  • Keywords has-patch required-fields added

@sabernhardt
2 years ago

using a second argument to determine whether to echo each function instead of creating two more functions, plus including adjustments to comment template file

#3 @sabernhardt
2 years ago

Or these could be two functions, with the echo selection as a second parameter:

wp_required_field_indicator( $space_before = ' ', $echo = false )
wp_required_field_message( $space_before = ' ', $echo = false )

#4 @sabernhardt
2 years ago

The required-field-message class in these patches would be more generic than the comment-required-message class that's now in the comment template. (Or perhaps it could be wp-required-field-message if the function name is wp_required_field_message.)

Last edited 2 years ago by sabernhardt (previous) (diff)

#5 @sabernhardt
2 years ago

  • Description modified (diff)

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


2 years ago

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


2 years ago

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


2 years ago

#9 @ryokuhi
2 years ago

  • Milestone changed from Awaiting Review to 6.0

This ticket was reviewed today during the accessibility team's bug-scrub.
This ticket already has a patch, so we're milestoning it for 6.0.
The functions introduced with this ticket will probably be useful to solve other open tickets and in other parts of WordPress core: a starting point could be reviewing which required fields can benefit from these functions.
It still has to be decided if such a review should be done here or in a new ticket.

@sabernhardt
2 years ago

updating comments form example after r52200

#10 @sabernhardt
2 years ago

  • Description modified (diff)

If editing the front-end Comments form template is appropriate, some of these other existing cases in the admin might be good to update as well:

Last edited 2 years ago by sabernhardt (previous) (diff)

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


2 years ago

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


2 years ago

#13 @sabernhardt
2 years ago

  • Keywords dev-feedback added

#14 @joedolson
2 years ago

I think it absolutely makes sense to propagate these changes out right away, in every place possible.

I don't see any reason not to implement those changes directly in this patch, and propagate this right away. If there are examples of required indicators in the admin that use a different HTML/class pattern, then those probably need to be documented and handled as individual patches, but the ones that are exact matches are an easy replacement.

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


2 years ago

@sabernhardt
2 years ago

using functions in comments template, the New Site form, and 3 media functions

#16 @sabernhardt
2 years ago

The spacing in 53494.4.patch should be identical to the existing markup in these functions, but each message has the wrapper span and each indicator has aria-hidden.

In wp_media_insert_url_form, the Alternative Text field has had a required (or aria-required) attribute since r8313, and the patch gives that label its missing indicator.

I made the changes to get_compat_media_markup, though I do not know to test that function.

#17 @sabernhardt
2 years ago

  • Keywords needs-testing added

#18 @sabernhardt
2 years ago

  • Milestone changed from 6.0 to 6.1

#19 @sabernhardt
2 years ago

  • Keywords needs-refresh added; needs-testing removed

See #55717 about removing the aria-hidden attributes.

Also, should the $echo parameter name change to $display_message or $display (r53203, r53207)?

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


23 months ago

#21 @audrasjb
23 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Self assigning for review and, hopefully, early commit.

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


23 months ago

This ticket was mentioned in PR #2819 on WordPress/wordpress-develop by audrasjb.


23 months ago
#23

  • Keywords needs-refresh removed

#24 @audrasjb
23 months ago

In the above PR I refreshed the patch against trunk, I updated @since mentions, I renamed $echo to $display.

#25 @audrasjb
23 months ago

@sabernhardt looks like unit test are not passing with the proposed patch :) See PR above.

This ticket was mentioned in PR #2821 on WordPress/wordpress-develop by sabernhardt.


23 months ago
#26

Fixing WPCS and PHP compatibility errors found in #2819, plus correcting the $message variable name

Trac ticket: https://core.trac.wordpress.org/ticket/54394

#27 @sabernhardt
23 months ago

Thanks for refreshing the patch and running the tests. The PR is more polished now.

#28 @audrasjb
23 months ago

Ah thank you for the new PR, looks even better than mine :-)

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


22 months ago

#31 @sabernhardt
22 months ago

The current PR contains the translatable string proposed in #23870. That still might be filterable, yet these functions would not work well with the argument concept suggested there. Perhaps switching to an $args array in these functions would be more flexible.

function wp_required_field_indicator( $args = array() ) {
	$defaults = array(
		'space_before' => ' ',
		/* translators: Character to identify required form fields. */
		'glyph'        => __( '*' ),
		'display'      => false,
	);

	$args = wp_parse_args( $args, $defaults );

#32 @joedolson
22 months ago

Switching to an array argument would make it easier to add arguments in the future, should we need to, although I don't currently see a need for that.

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


22 months ago

#34 @SergeyBiryukov
22 months ago

I think a better location for these new functions would be in wp-includes/general-template.php, next to the checked(), selected(), disabled() and wp_readonly() helpers.

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


22 months ago

This ticket was mentioned in PR #2982 on WordPress/wordpress-develop by sabernhardt.


22 months ago
#36

In this PR, the helper functions of PR 2821 are simplified and moved to general-template.php.

This also adds a space before the indicator glyph in get_media_item() and get_compat_media_markup() that was not there before, for consistency with the other functions.

Trac ticket: https://core.trac.wordpress.org/ticket/54394

#37 @sabernhardt
22 months ago

  • Description modified (diff)
  • Keywords early added; dev-feedback removed

In yesterday's Slack discussion, @markjaquith recommended removing the parameters.

#38 @audrasjb
22 months ago

Yes, the idea is to simplify the usage of this function by moving echoing management (add a space before or after the message, decide on whether to return or echo the message) to the place where the function is called (= in the template).

#39 @audrasjb
22 months ago

  • Keywords changes-requested added

sabernhardt commented on PR #2821:


22 months ago
#40

Closing in favor of #2982

#41 @sabernhardt
22 months ago

@audrasjb PR 2982 simplifies the functions without parameters. Did you have other changes to request?

#42 @audrasjb
22 months ago

@sabernhardt I added one last comment… what do you think?
I think we're pretty close to get this patch committed :)

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


21 months ago

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


21 months ago

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


21 months ago

#46 @audrasjb
21 months ago

  • Keywords commit added; changes-requested removed
  • Status changed from reviewing to accepted

As per today's bugscrub, markign this for commit.

#47 @audrasjb
21 months ago

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

In 53888:

General: Add required fields helper functions for better reusability.

This changeset introduces new wp_required_field_indicator() and wp_required_field_message() helper functions to generate reusable and consistent required field indicator and message. It also implements these functions in various admin screens.

Props sabernhardt, ryokuhi, joedolson, audrasjb, SergeyBiryukov.
Fixes #54394.

#49 @audrasjb
21 months ago

  • Keywords early commit removed

#50 @audrasjb
21 months ago

@sabernhardt now that it is committed, shouldn't we open a ticket to make $indicator and $message filterable?

This ticket was mentioned in PR #3088 on WordPress/wordpress-develop by kebbet.


21 months ago
#51

Introduce 3 filters to the 2 new wp_required_field_*-functions.
https://core.trac.wordpress.org/ticket/54394

#52 @kebbet
21 months ago

PR 3088 adds three filters, two for the markup and one for the indicator glyph, don't know if that actually is needed. Any thoughts?

#53 follow-up: @desrosj
21 months ago

  • Keywords required-fields removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to make sure that ticket:54394#comment:52 is addressed. Though a new ticket is probably preferred.

#54 @audrasjb
21 months ago

Thank you for the patch @kebbet!
I think we can probably just provide one hook to filter the HTML content. In my opinion, wp_required_field_indicator and wp_required_field_message should be more than enough since wp_required_field_indicator allows developer to change the glyph.

Also, there's a typo in one filter name, I commented in the PR :)

#55 in reply to: ↑ 53 @kebbet
21 months ago

Replying to desrosj:

Reopening to make sure that ticket:54394#comment:52 is addressed. Though a new ticket is probably preferred.

Please close this ticket in favor for #56389.

#56 @audrasjb
21 months ago

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

Closing as #56389 was opened to introduce some hooks.

costdev commented on PR #3088:


21 months ago
#57

Should we have unit tests to ensure these filters are always fired? @audrasjb

audrasjb commented on PR #3088:


21 months ago
#58

Ah yes 👍

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


21 months ago

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


20 months ago

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


20 months ago

#62 @sabernhardt
20 months ago

  • Keywords needs-dev-note added

costdev commented on PR #3088:


20 months ago
#63

@SergeyBiryukov In 80ffb13, the test class name was changed from TitleCase to camelCase. In Writing PHPUnit Tests - Test Classes, it says:

Test class names should reflect the filepath, with underscores replacing directory separators and TitleCase replacing camelCase. Thus, the class Tests_Comment_GetCommentClass(), which contains tests for the get_comment_class() function, is located in tests/comment/getCommentClass.php.

I understand this commit was made for consistency. Should the handbook, or the test suite, be updated?

#64 @audrasjb
20 months ago

In 54137:

Comments: Make wp_required_field_indicator() and wp_required_field_message() output filterable.

This changeset introduces two new hooks:

  • wp_required_field_indicator allows developers to filter the HTML output of the wp_required_field_indicator() function.
  • wp_required_field_message does the same for the wp_required_field_message() function.

The changeset also adds new phpunit tests for these filters.

Follow-up to [53888], [54136].

Props kebbet, audrasjb, sabernhardt, costdev, mukesh27.
Fixes #56389.
See #54394.

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


19 months ago

Note: See TracTickets for help on using tickets.