#16206 closed defect (bug) (fixed)
Comment text not marked as required
Reported by: | infected | Owned by: | joedolson |
---|---|---|---|
Milestone: | 5.9 | Priority: | normal |
Severity: | normal | Version: | 3.0.4 |
Component: | Comments | Keywords: | has-patch has-screenshots required-fields has-dev-note |
Focuses: | ui, accessibility | Cc: |
Description
I thought i already opened a ticket to this but i cant find it... Maybe i didnt hit the create ticket button?!
When using comment_form() the name and email fields marked as required while the textarea for commenttext is not. If i send my comment without text i'll get this notice
Error: please type a comment.
So it also should be marked as required field.
Attachments (11)
Change History (96)
#1
@
14 years ago
- Keywords 3.2-early needs-patch added; comment commenttext required removed
- Milestone changed from Awaiting Review to Future Release
#3
@
14 years ago
Looks like maybe my comment either got stopped by dd32's, or I was commenting to the other phantom ticket. Looks like the former.
Anyway, I don't mind adding aria-required, but the * seems just redundant and silly. Mentioned it to MarkJaquith and a few others and I seemed to recall them nodding in agreement on that.
#4
@
14 years ago
- Keywords 2nd-opinion added; 3.2-early removed
Ah, comment text already has aria-required.
#5
follow-up:
↓ 6
@
14 years ago
My opinion: Either the field is required (the error notice is saying that) then the * isn´t redundant, otherwise it isn´t a required field. Maybe the code already has an aria-required but visitors don´t look into the code for required fields. ;-)
#6
in reply to:
↑ 5
;
follow-ups:
↓ 7
↓ 8
@
14 years ago
Replying to infected:
Maybe the code already has an aria-required but visitors don´t look into the code for required fields. ;-)
I guess in the case of a comment text that's just implied by common sense.
#7
in reply to:
↑ 6
@
14 years ago
Replying to SergeyBiryukov:
I guess in the case of a comment text that's just implied by common sense.
From this point of view, yes. It's just a cosmetic thing...
#8
in reply to:
↑ 6
@
12 years ago
- Cc mdhansen@… added
- Keywords close added
Replying to SergeyBiryukov:
Replying to infected:
Maybe the code already has an aria-required but visitors don´t look into the code for required fields. ;-)
I guess in the case of a comment text that's just implied by common sense.
I would agree that the comment field is common sense. Proposing close
#9
@
12 years ago
- Keywords close removed
- Milestone Future Release deleted
- Resolution set to wontfix
- Status changed from new to closed
#11
@
11 years ago
Based on reports from confused newbie clients, I'd also vote +1 for adding the asterisk for the comment field.
#13
@
9 years ago
- Summary changed from Commenttext not marked as required to Comment text not marked as required
#15
@
9 years ago
- Resolution wontfix deleted
- Status changed from closed to reopened
I'm reopening this ticket to discuss this further from an accessibility point of view.
Required fields need to be indicated, also visually.
I agree that it's obvious, but if you use an asterisk do it consistent.
#16
@
9 years ago
- Focuses ui accessibility added
- Milestone set to 4.5
- Owner set to rianrietveld
- Status changed from reopened to assigned
I'd agree, since it uses also aria-required="true" required="required"
then for consistency should use an asterisk too. By the way the sentence "Required fields are marked *" appears only when users are logged out. When logged in there's just the textarea. I guess this is OK, I'd just like to discuss it a bit in the next accessibility weekly meeting.
Worth noting required
is a boolean attribute and doesn't need a value.
#17
@
9 years ago
By the way the sentence "Required fields are marked *" appears only when users are logged out. When logged in there's just the textarea. I guess this is OK
I thought we're adding the asterisk for consistency? Showing no help text feels inconsistent, especially for sites where only logged in users can comment.
#18
@
9 years ago
Hm, maybe :) Was thinking when logged in it's the only field available so it's a "use it or don't". Saying "this unique field is required" would sound a little pedantic :) Then realized plugins and themes can add additional fields so maybe better always use an asterisk.
This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.
9 years ago
#20
follow-up:
↓ 21
@
9 years ago
Updated the patch after discussion with @joedolson and @afercia in Slack:
Added to the defaults array:
'comment_notes_logged_in_before' => '<p class="comment-notes">'. $required_text . '</p>',
Added
echo $args['comment_notes_logged_in_before'];
to the form if is_user_logged_in()
If a user is logged in the message shows: Required fields are marked *
If a user is not logged in the message shows: Your email address will not be published. Required fields are marked *
Also changed required="required" into required (http://www.w3.org/WAI/GL/wiki/Techniques/HTML5/Using_the_required_attribute_to_indicate_a_required_input_field)
#21
in reply to:
↑ 20
;
follow-up:
↓ 22
@
9 years ago
Replying to rianrietveld:
Also changed required="required" into required (http://www.w3.org/WAI/GL/wiki/Techniques/HTML5/Using_the_required_attribute_to_indicate_a_required_input_field)
I know it's all HTML5 technically, but aren't we generally using the XHTML standard for attributes?
#22
in reply to:
↑ 21
;
follow-up:
↓ 23
@
9 years ago
Replying to strider72:
I know it's all HTML5 technically, but aren't we generally using the XHTML standard for attributes?
Hm, not sure but interesting. Do you have any reference or argumentation to stick with the old syntax?
#23
in reply to:
↑ 22
;
follow-up:
↓ 24
@
9 years ago
Replying to afercia:
Replying to strider72:
I know it's all HTML5 technically, but aren't we generally using the XHTML standard for attributes?
Hm, not sure but interesting. Do you have any reference or argumentation to stick with the old syntax?
Yes. The WordPress Core Handbook section on HTML Coding Standards specifically references XHTML standards:
https://make.wordpress.org/core/handbook/best-practices/coding-standards/html/
Also, this is the way we've actually been doing it.
#24
in reply to:
↑ 23
@
9 years ago
Replying to strider72:
Replying to afercia:
Replying to strider72:
I know it's all HTML5 technically, but aren't we generally using the XHTML standard for attributes?
Hm, not sure but interesting. Do you have any reference or argumentation to stick with the old syntax?
Yes. The WordPress Core Handbook section on HTML Coding Standards specifically references XHTML standards:
https://make.wordpress.org/core/handbook/best-practices/coding-standards/html/
Or if you mean other than "That's just the way we do it"... well, I do think code consistency is a good thing, but I also think XHTML syntax is much cleaner and easier to read. The same with the closing slash on IMG tags — not technically necessary on HTML5, but consistently used it makes the code just a bit easier to read (and easier for more parsers to parse). There are a number of long-established coding standards within WordPress that are not technically necessary, such as spaces in parentheses in PHP, e.g.: ( $this ) not ($this)
#25
@
9 years ago
Oh well, sorry I was focused only on the admin screens (which are HTML5). Since this is something output also on the front end for themes yep we shouldn't assume HTML5.
#26
follow-up:
↓ 28
@
9 years ago
- Keywords has-screenshots added
Thinking maybe it is possible to simplify a bit. Since now the Comment textarea is always indicated as required, the "required text" should always be printed out regardless of the require_name_email
option (in the backend is "Comment author must fill out name and email "). So when logged in maybe just append the required text to the logged_in_as
string? See screenshot below:
Additionally, the required
attribute can use the HTML5 syntax depending on the support for HTML5.
Lastly, I'd propose to change the error message for missing required fields:
since we shouldn't assume they're just "name" and "email".
Thoughts?
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
9 years ago
#28
in reply to:
↑ 26
@
9 years ago
Replying to afercia:
Thinking maybe it is possible to simplify a bit. Since now the Comment textarea is always indicated as required, the "required text" should always be printed out regardless of the
require_name_email
option (in the backend is "Comment author must fill out name and email "). So when logged in maybe just append the required text to thelogged_in_as
string?
Is adding the "required text" default not conflicting with already made changes in the filters by theme developers?
Additionally, the required attribute can use the HTML5 syntax depending on the support for HTML5.
Is this an HTML5/XHTML/HTML4 compliance issue or a code standards compliance issue?
If it's a code standards compliance issue we should keep required="required" in whatever the theme is build in.
Lastly, I'd propose to change the error message for missing required fields:
since we shouldn't assume they're just "name" and "email".
Agree on that and think we should address this in a separate ticket.
#29
@
9 years ago
@rianrietveld @afercia is there an attainable path forward here for 4.5? Or should this ticket be punted?
This ticket was mentioned in Slack in #accessibility by rachelbaker. View the logs.
9 years ago
#32
@
9 years ago
This started easy and got more and more complicated, I’m for punting it and look better at it in the next release.
This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.
9 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
8 years ago
#36
@
8 years ago
- Milestone changed from Future Release to 4.7
Discussed this in the wpa11y bug scrub:
We'll have a go infixing this for 4.7. together with #37331
This ticket was mentioned in Slack in #core by stevenkword. View the logs.
8 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#43
@
8 years ago
- Milestone changed from 4.7 to Future Release
Not enough time to discuss and establish a convention for all the required form fields in core, punting for now.
#44
@
8 years ago
Just tested a bit using:
Safari 10, VoiceOver 7, on macOS Sierra
Firefox 50, NVDA 2016.3, on Windows 10
IE 11, JAWS 17, on Windows 10
and the "asterisk" is always announced as star
. Also, worth noting since this character is actually part of the label, it will be announced both when reading out the label and when focusing the related field. For example, when you tab and focus the Email field, screen readers will announce Email star ...
which is a bit annoying:
This ticket was mentioned in Slack in #accessibility by cheffheid. View the logs.
8 years ago
#46
@
8 years ago
It's worth noting that if the fields are all marked as aria-required=true, then the asterisk is only a visual indicator, and could be marked as aria-hidden.
The problem with that, obviously, is that then the sentence stating that required fields are marked with an asterisk will be confusing to screen reader users, since there would be no evidence of that asterisk in the required fields.
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.
6 years ago
#49
@
5 years ago
Has there been any movement on this? From a usability standpoint it seems odd to say "Required fields are marked *", but then leave the asterisk off of the required "Comment" field.
#51
@
4 years ago
If the star/asterisk is hidden for screen readers, then I'd vote for also hiding the text "Required fields are marked...". I mean if those fields anyway are marked as mandatory via aria parameters...
This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.
4 years ago
#53
@
4 years ago
- Keywords needs-refresh added
- Owner set to sabernhardt
- Status changed from assigned to accepted
#54
@
3 years ago
- Keywords needs-testing added; 2nd-opinion needs-refresh removed
- Milestone changed from Future Release to 5.9
16206.6.patch hides the required field message from screen readers with aria-hidden
.
The patch also uses a $required_glyph
variable for easier customization in #23870. This results in an aria-hidden
span inside the hidden field message span, but the redundancy is probably better than trying to make a special case.
#55
@
3 years ago
The $html_req
variable can be an empty string (in the latest patches), so it is not appropriate in the comments textarea.
@
3 years ago
editing variable names, defining HTML5 attributes together (both checked and required)
#56
@
3 years ago
I polished the patch a little:
- Changing variable names (for one,
$required_indicator
includes more than the asterisk glyph itself) - Defining HTML5 attributes together, both checked and required, and then verifying within the
sprintf
function whether therequired
attribute belongs on that form field - Paying more attention to spaces, using the
trim
function to avoid two spaces in the required note
This ticket was mentioned in Slack in #core-test by mai21. View the logs.
3 years ago
#58
@
3 years ago
- Keywords fixed added; needs-testing removed
Test Report
Env
- WordPress (5.9-alpha-51877) (could reproduce it and latest patch https://core.trac.wordpress.org/attachment/ticket/16206/16206.8.patch fixed it )
- Chrome 94.0.4606.54
- ubuntu 18.04.6
- Theme: Twenty Twenty One
- Plugin: WordPress Beta Tester
Steps to test
- Open post incognito
- Fill mandatory fields and submit
Actual:
An error displayed for comment txt field also it wasn't having *
Expected:
Make Comment Field mandatory in UI (having *)
Screencast after fix:
#61
follow-up:
↓ 62
@
3 years ago
@mai21 Can you elaborate on the automated tests needed on the 16206.8.patch?
#62
in reply to:
↑ 61
@
3 years ago
Replying to rachelbaker:
@mai21 Can you elaborate on the automated tests needed on the 16206.8.patch?
Hello @rachelbaker, we can add in e2e tests that the * is displayed in UI for this field.
what do you think?
#63
@
3 years ago
[silent 5.9 bugscrub]
Looking at the last patch, I'm not sure this is something where we really need an automated test. It wouldn't hurt of course (if we can define which specific tests are needed), but it would be worst if the patch can't ship at all :)
Anyway, I tested 16206.8.patch
: it still applies cleanly on my side.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 years ago
#66
@
3 years ago
- Keywords commit added
As per today's bug scrub, the last patch looks ready to go.
Marking for commit
.
#68
@
3 years ago
The dev note for this can be included within a larger dev note for themes. Some themes might have a special style for the asterisks (Twenty Eleven is one of them).
#69
@
3 years ago
- Keywords needs-refresh added; commit removed
required note is missing when logged in (while label is marked with asterisk)
@
3 years ago
alternative option: fix discrepancy with Comment label when name and email are required
#70
@
3 years ago
- Keywords needs-refresh removed
16206.9.patch adds the required fields message that was missing for logged-in users with most themes in 16206.8.patch.
At least 437 themes—including Twenty Nineteen and Twenty Twenty-One—would still have the star inside the Comment label without the explanatory text because they edit/remove the 'logged_in_as' argument. And other themes edit the 'comment_notes_before' text.
So I also uploaded a simpler option 16206.alternative.patch to focus on the form when users are logged out and are required to fill in their name and email. If we go in this direction for now, we still could consider a method to show the symbol and required message for everyone later.
Twenty Seventeen and other themes, before the patch
Logged out, name & email are not required:
- default 'comment_notes_before' text:
Your email address will not be published.
- No fields have * symbol
Logged out, name & email required:
- default 'comment_notes_before' text:
Your email address will not be published. Required fields are marked *
- Only Name and Email have * symbol, Comment and URL are available
Logged in (whether or not name & email are required):
- default 'logged_in_as' text:
Logged in as test. Log out?
- No fields have * symbol (only Comment is available)
With 16206.alternative.patch
This patch adds the symbol to the Comment label when user is not logged in and their name and email are required.
Logged out, name & email are not required:
- default 'comment_notes_before' text:
Your email address will not be published.
- No fields have * symbol
Logged out, name & email required:
- default 'comment_notes_before' text:
Your email address will not be published. Required fields are marked *
- Comment, Name and Email have * symbol
Logged in (whether or not name & email are required):
- default 'logged_in_as' text:
Logged in as test. Log out?
- No fields have * symbol (only Comment is available)
With 16206.9.patch
The Comments label has symbol, regardless of require_name_email
setting or user login status. The default 'comment_notes_before' and 'logged_in_as' text arguments include the required message.
Logged out, name & email are not required:
- default 'comment_notes_before' text:
Your email address will not be published. Required fields are marked *
- Comment is the only label with * symbol
Logged out, name & email required:
- default 'comment_notes_before' text:
Your email address will not be published. Required fields are marked *
- Comment, Name and Email have * symbol
Logged in (whether or not name & email are required):
- default 'logged_in_as' text:
Logged in as test. Log out? Required fields are marked *
- Comment label has * symbol, and that is the only default field.
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 sabernhardt. View the logs.
3 years ago
#74
@
3 years ago
- Keywords commit added
Following review, I think that 16206.9.patch is the better choice. While it's not perfect, since the asterisk on Twenty Twenty One or in similar cases is not explained in visible text, it's better than it currently is - where the field is marked required in code, but excluded in the error message and has no marking of any kind that's visible.
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
#80
@
3 years ago
- Keywords commit removed
Reopening to consider changing the new comment-required-message
class for better reusability (#54394):
A required-field-message
class seems safe. The three plugin directory examples for 'required-field-message' use it for input field names, not as a class. And the directory search had no theme results.
#82
@
3 years ago
- Keywords commit added
For future planning, I think this is a good idea. It's harmless if we never implement a message function, but not doing it would make that implementation significantly harder.
#85
@
3 years ago
- Keywords commit removed
Additional note for changes to bundled themes:
https://make.wordpress.org/core/2022/01/05/bundled-theme-updates-with-wordpress-5-9/
Sounds like a good idea, Kind of should be consistent in the forms :)