WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 12 days ago

#16206 assigned defect (bug)

Comment text not marked as required

Reported by: infected Owned by: rianrietveld
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.0.4
Component: Comments Keywords: has-patch 2nd-opinion has-screenshots
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 (5)

16206.patch (2.0 KB) - added by solarissmoke 5 years ago.
Mark comment field as required
16206.2.patch (1.6 KB) - added by rianrietveld 3 weeks ago.
adds * to label comment textarea
16206.3.patch (3.2 KB) - added by rianrietveld 2 weeks ago.
adds comment_notes_logged_in_before
16206.4.patch (2.8 KB) - added by rianrietveld 2 weeks ago.
restores required="required"
16206.5.patch (6.6 KB) - added by afercia 2 weeks ago.

Download all attachments as: .zip

Change History (33)

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

Mark comment field as required

#2 @solarissmoke
5 years ago

  • Keywords has-patch added; needs-patch removed

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

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

Ah, comment text already has aria-required.

#5 follow-up: @infected
5 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 5 years ago by infected (previous) (diff)

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

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

#10 @ocean90
2 years ago

#25384 was marked as a duplicate.

#11 @nofearinc
2 years ago

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

#12 @SergeyBiryukov
3 months ago

#34665 was marked as a duplicate.

#13 @SergeyBiryukov
3 months ago

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

#14 @ocean90
3 weeks ago

#35515 was marked as a duplicate.

#15 @rianrietveld
3 weeks 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
3 weeks ago

adds * to label comment textarea

#16 @afercia
3 weeks 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
3 weeks 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
3 weeks 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.


3 weeks ago

#20 follow-up: @rianrietveld
2 weeks 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
2 weeks ago

adds comment_notes_logged_in_before

#21 in reply to: ↑ 20 ; follow-up: @strider72
2 weeks 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
2 weeks 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
2 weeks 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 2 weeks ago by strider72 (previous) (diff)

#24 in reply to: ↑ 23 @strider72
2 weeks 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
2 weeks 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
2 weeks ago

restores required="required"

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

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


2 weeks ago

#28 in reply to: ↑ 26 @rianrietveld
12 days 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.

Note: See TracTickets for help on using tickets.