Make WordPress Core

Opened 14 years ago

Closed 3 years ago

Last modified 3 years ago

#16206 closed defect (bug) (fixed)

Comment text not marked as required

Reported by: infected's profile infected Owned by: joedolson's profile 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)

16206.patch (2.0 KB) - added by solarissmoke 14 years ago.
Mark comment field as required
16206.2.patch (1.6 KB) - added by rianrietveld 9 years ago.
adds * to label comment textarea
16206.3.patch (3.2 KB) - added by rianrietveld 9 years ago.
adds comment_notes_logged_in_before
16206.4.patch (2.8 KB) - added by rianrietveld 9 years ago.
restores required="required"
16206.5.patch (6.6 KB) - added by afercia 9 years ago.
16206.6.patch (3.4 KB) - added by sabernhardt 3 years ago.
16206.7.patch (3.2 KB) - added by sabernhardt 3 years ago.
correction: comments field needs to be required for everyone
16206.8.patch (4.0 KB) - added by sabernhardt 3 years ago.
editing variable names, defining HTML5 attributes together (both checked and required)
16206.9.patch (5.0 KB) - added by sabernhardt 3 years ago.
fix missing message in default 'logged_in_as' argument
16206.alternative.patch (4.4 KB) - added by sabernhardt 3 years ago.
alternative option: fix discrepancy with Comment label when name and email are required
16206.10.patch (602 bytes) - added by sabernhardt 3 years ago.
class name change

Download all attachments as: .zip

Change History (96)

#1 @dd32
14 years ago

  • Keywords 3.2-early needs-patch added; comment commenttext required removed
  • Milestone changed from Awaiting Review to Future Release

Sounds like a good idea, Kind of should be consistent in the forms :)

@solarissmoke
14 years ago

Mark comment field as required

#2 @solarissmoke
14 years ago

  • Keywords has-patch added; needs-patch removed

#3 @nacin
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 @nacin
14 years ago

  • Keywords 2nd-opinion added; 3.2-early removed

Ah, comment text already has aria-required.

#5 follow-up: @infected
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. ;-)

Last edited 14 years ago by infected (previous) (diff)

#6 in reply to: ↑ 5 ; follow-ups: @SergeyBiryukov
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 @infected
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 @MikeHansenMe
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 @SergeyBiryukov
12 years ago

  • Keywords close removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

#10 @ocean90
11 years ago

#25384 was marked as a duplicate.

#11 @nofearinc
11 years ago

Based on reports from confused newbie clients, I'd also vote +1 for adding the asterisk for the comment field.

#12 @SergeyBiryukov
9 years ago

#34665 was marked as a duplicate.

#13 @SergeyBiryukov
9 years ago

  • Summary changed from Commenttext not marked as required to Comment text not marked as required

#14 @ocean90
9 years ago

#35515 was marked as a duplicate.

#15 @rianrietveld
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.

@rianrietveld
9 years ago

adds * to label comment textarea

#16 @afercia
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 @ocean90
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 @afercia
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: @rianrietveld
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)

@rianrietveld
9 years ago

adds comment_notes_logged_in_before

#21 in reply to: ↑ 20 ; follow-up: @strider72
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: @afercia
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: @strider72
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.

Last edited 9 years ago by strider72 (previous) (diff)

#24 in reply to: ↑ 23 @strider72
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 @afercia
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.

@rianrietveld
9 years ago

restores required="required"

#26 follow-up: @afercia
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:

https://cldup.com/blLCdABxo0.png

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:

https://cldup.com/TrUfPcoM-g.png

since we shouldn't assume they're just "name" and "email".

Thoughts?

@afercia
9 years ago

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


9 years ago

#28 in reply to: ↑ 26 @rianrietveld
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 the logged_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 @rachelbaker
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

#31 @rachelbaker
9 years ago

  • Milestone changed from 4.5 to Future Release

#32 @rianrietveld
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.

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

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

#35 @rianrietveld
8 years ago

related: #37331

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

#36 @rianrietveld
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

#40 @afercia
8 years ago

  • Keywords required-fields added

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 @afercia
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 @afercia
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:

https://cldup.com/9w3AG4ee_N.png

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


8 years ago

#46 @joedolson
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 @JG Visual
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.

#50 @rianrietveld
5 years ago

  • Owner rianrietveld deleted

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

  • Keywords needs-refresh added
  • Owner set to sabernhardt
  • Status changed from assigned to accepted

@sabernhardt
3 years ago

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

@sabernhardt
3 years ago

correction: comments field needs to be required for everyone

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

@sabernhardt
3 years ago

editing variable names, defining HTML5 attributes together (both checked and required)

#56 @sabernhardt
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 the required 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 @mai21
3 years ago

  • Keywords fixed added; needs-testing removed

Test Report

Env

Steps to test

  1. Open post incognito
  2. 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:

https://jmp.sh/we7sbGT

Last edited 3 years ago by mai21 (previous) (diff)

#59 @mai21
3 years ago

  • Keywords needs-testing added

Added needs-testing for automation tests

#60 @rachelbaker
3 years ago

  • Keywords fixed removed

#61 follow-up: @rachelbaker
3 years ago

@mai21 Can you elaborate on the automated tests needed on the 16206.8.patch?

#62 in reply to: ↑ 61 @mai21
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 @audrasjb
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

#65 @audrasjb
3 years ago

  • Keywords needs-testing removed

#66 @audrasjb
3 years ago

  • Keywords commit added

As per today's bug scrub, the last patch looks ready to go.
Marking for commit.

#67 @audrasjb
3 years ago

  • Keywords needs-dev-note added

#68 @sabernhardt
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).

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

#69 @sabernhardt
3 years ago

  • Keywords needs-refresh added; commit removed

required note is missing when logged in (while label is marked with asterisk)

@sabernhardt
3 years ago

fix missing message in default 'logged_in_as' argument

@sabernhardt
3 years ago

alternative option: fix discrepancy with Comment label when name and email are required

#70 @sabernhardt
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.
Last edited 3 years ago by sabernhardt (previous) (diff)

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


3 years ago

#72 @joedolson
3 years ago

  • Owner changed from sabernhardt to joedolson

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


3 years ago

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

#75 @joedolson
3 years ago

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

In 52029:

Comments: Mark comment text field as required.

Add required asterisk to the comment text field. Historically, the name and email fields are marked as required, but the comment text field is not, though it is actually a required field.

Props infected, solarissmoke, rianrietveld, afercia, sabernhardt, strider72, mai21, audrasjb.
Fixes #16206.

#76 @sabernhardt
3 years ago

Follow-up tickets:
#54392 - logged_in_as message for Twenty Nineteen and Twenty Twenty-One
#54408 - styling changes for Twenty Eleven
#54394 - add functions to re-use the indicator and message in other situations

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

#79 @joedolson
3 years ago

In 52149:

Bundled Theme: Display required text field in core themes.

Fix Twenty Nineteen and Twenty Twenty-One setting the logged_in_as parameter to null, which also eliminated the required field text. See #16206.

Props sabernhardt, poena.
Fixes #54392.

@sabernhardt
3 years ago

class name change

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

#81 @sabernhardt
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#83 @joedolson
3 years ago

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

In 52200:

Comments: Change new comment required text class.

Change the new class on comment required text from comment-required-message to required-field-message for better future-compatibility, enabling better reuse.

Props sabernhardt.
Fixes #16206.

#84 @audrasjb
3 years ago

  • Keywords has-dev-note added; needs-dev-note removed

#85 @sabernhardt
3 years ago

  • Keywords commit removed
Note: See TracTickets for help on using tickets.