Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#54394 closed feature request (fixed)

Add functions for required fields indicator and message

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

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 5 years ago.
basic concept
53494.2.patch​ (4.3 KB ) - added by sabernhardt 5 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 5 years ago.
updating comments form example after r52200
53494.4.patch​ (9.2 KB ) - added by sabernhardt 4 years ago.
using functions in comments template, the New Site form, and 3 media functions

Download all attachments as: .zip

Change History (71)

@sabernhardt
5 years ago

basic concept

#1 @sabernhardt
5 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
5 years ago

  • Keywords has-patch required-fields added

@sabernhardt
5 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
5 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
5 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 5 years ago by sabernhardt (previous) (diff)

#5 @sabernhardt
5 years ago

  • Description modified (diff)

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


5 years ago

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


5 years ago

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


5 years ago

#9 @ryokuhi
5 years ago

  • Milestone Awaiting Review → 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
5 years ago

updating comments form example after r52200

#10 @sabernhardt
5 years ago

  • Description modified (diff)

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

Version 0, edited 5 years ago by sabernhardt (next)

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


5 years ago

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


5 years ago

#13 @sabernhardt
5 years ago

  • Keywords dev-feedback added

#14 @joedolson
5 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.


5 years ago

@sabernhardt
4 years ago

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

#16 @sabernhardt
4 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
4 years ago

  • Keywords needs-testing added

#18 @sabernhardt
4 years ago

  • Milestone 6.0 → 6.1

#19 @sabernhardt
4 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.


4 years ago

#21 @audrasjb
4 years ago

  • Owner set to audrasjb
  • Status new → reviewing

Self assigning for review and, hopefully, early commit.

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


4 years ago

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


4 years ago
#23

  • Keywords needs-refresh removed

#24 @audrasjb
4 years ago

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

#25 @audrasjb
4 years 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.


4 years 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
4 years ago

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

#28 @audrasjb
4 years 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.


4 years ago

#31 @sabernhardt
4 years 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
4 years 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.


4 years ago

#34 @SergeyBiryukov
4 years 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.


4 years ago

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


4 years 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
4 years ago

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

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

#38 @audrasjb
4 years 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
4 years ago

  • Keywords changes-requested added

​sabernhardt commented on ​PR #2821:


4 years ago
#40

Closing in favor of #2982

#41 @sabernhardt
4 years ago

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

#42 @audrasjb
4 years 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.


4 years ago

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


4 years ago

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


4 years ago

#46 @audrasjb
4 years ago

  • Keywords commit added; changes-requested removed
  • Status reviewing → accepted

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

#47 @audrasjb
4 years ago

  • Resolution → fixed
  • Status accepted → 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
4 years ago

  • Keywords early commit removed

#50 @audrasjb
4 years 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.


4 years ago
#51

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

#52 @kebbet
4 years 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
4 years ago

  • Keywords required-fields removed
  • Resolution fixed
  • Status closed → reopened

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

#54 @audrasjb
4 years 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
4 years 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
4 years ago

  • Resolution → fixed
  • Status reopened → closed

Closing as #56389 was opened to introduce some hooks.

​costdev commented on ​PR #3088:


4 years ago
#57

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

​audrasjb commented on ​PR #3088:


4 years ago
#58

Ah yes 👍

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


4 years ago

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


4 years ago

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


4 years ago

#62 @sabernhardt
4 years ago

  • Keywords needs-dev-note added

​costdev commented on ​PR #3088:


4 years 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
4 years 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.


4 years ago

Note: See TracTickets for help on using tickets.