#54394 closed feature request (fixed)
Add functions for required fields indicator and message
Reported by: | sabernhardt | Owned by: | audrasjb |
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch has-dev-note |
Focuses: | accessibility | Cc: |
Description (last modified by )
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)
Change History (71)
#1
@
3 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.
@
3 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
@
3 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
@
3 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
.)
This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.
3 years ago
This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.
3 years ago
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
3 years ago
#9
@
3 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.
#10
@
3 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:
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
3 years ago
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
3 years ago
#14
@
3 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.
3 years ago
#16
@
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.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
2 years ago
#21
@
2 years 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.
2 years ago
This ticket was mentioned in PR #2819 on WordPress/wordpress-develop by audrasjb.
2 years ago
#23
- Keywords needs-refresh removed
Trac ticket: https://core.trac.wordpress.org/ticket/54394
#24
@
2 years ago
In the above PR I refreshed the patch against trunk, I updated @since mentions, I renamed $echo
to $display
.
#25
@
2 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.
2 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
@
2 years ago
Thanks for refreshing the patch and running the tests. The PR is more polished now.
2 years ago
#29
Closed in favor of https://github.com/WordPress/wordpress-develop/pull/2821
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
2 years ago
#31
@
2 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
@
2 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.
2 years ago
#34
@
2 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.
2 years ago
This ticket was mentioned in PR #2982 on WordPress/wordpress-develop by sabernhardt.
2 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
@
2 years ago
- Description modified (diff)
- Keywords early added; dev-feedback removed
In yesterday's Slack discussion, @markjaquith recommended removing the parameters.
#38
@
2 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).
sabernhardt commented on PR #2821:
2 years ago
#40
Closing in favor of #2982
#41
@
2 years ago
@audrasjb PR 2982 simplifies the functions without parameters. Did you have other changes to request?
#42
@
2 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.
2 years ago
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
2 years ago
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
2 years ago
#46
@
2 years ago
- Keywords commit added; changes-requested removed
- Status changed from reviewing to accepted
As per today's bugscrub, markign this for commit
.
2 years ago
#48
Committed in https://core.trac.wordpress.org/changeset/53888
#50
@
2 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.
2 years ago
#51
Introduce 3 filters to the 2 new wp_required_field_*-functions.
https://core.trac.wordpress.org/ticket/54394
#52
@
2 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:
↓ 55
@
2 years 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
@
2 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
@
2 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
@
2 years ago
- Resolution set to fixed
- Status changed from reopened to closed
Closing as #56389 was opened to introduce some hooks.
This ticket was mentioned in Slack in #core by sergey. View the logs.
2 years ago
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
2 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?
2 years ago
#65
Committed in https://core.trac.wordpress.org/changeset/54137
basic concept