Skip to content

Omit novalidate from comment form by default but introduce novalidate arg to override #8858

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
+38 −1
Closed

Omit novalidate from comment form by default but introduce novalidate arg to override #8858

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2754f67
Option 1: Add a novalidate_form argument to allow the novalidate attr…
westonruter May 31, 2025
cbd92d2
Omit the novalidate attribute from the comment form by default
westonruter May 31, 2025
c5f8820
Default to omitting novalidate from comment form and add tests
westonruter May 31, 2025
04b4343
Fix typo
westonruter May 31, 2025
585d151
Merge branch 'trunk' of https://github.com/WordPress/wordpress-develo…
westonruter Jun 12, 2025
3550454
Rename novalidate_form to novalidate
westonruter Jun 12, 2025
234cd63
Use 6.8.2 as introduced version
westonruter Jun 12, 2025
e0e8b5f
fixup! Rename novalidate_form to novalidate
westonruter Jun 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/wp-includes/comment-template.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2446,6 +2446,7 @@ function wp_list_comments( $args = array(), $comments = null ) {
* @since 4.6.0 Introduced the 'action' argument.
* @since 4.9.6 Introduced the 'cookies' default comment field.
* @since 5.5.0 Introduced the 'class_container' argument.
* @since n.e.x.t Introduced the 'novalidate_form' argument.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this placeholder version since I wasn't sure how long it would take to merge this or if we'd target it for 6.8.2. But seems like it makes sense for a minor release, especially with the current relaxation of the criteria.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree; 6.8.2 seems reasonable. But with all the changes recently, I honestly don't have any idea...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 234cd63

*
* @param array $args {
* Optional. Default arguments and form fields to override.
Expand All @@ -2467,6 +2468,7 @@ function wp_list_comments( $args = array(), $comments = null ) {
* Default 'Your email address will not be published.'.
* @type string $comment_notes_after HTML element for a message displayed after the textarea field.
* @type string $action The comment form element action attribute. Default '/wp-comments-post.php'.
* @type bool $novalidate_form Whether the novalidate attribute is added to the comment from. Default false.
* @type string $id_form The comment form element id attribute. Default 'commentform'.
* @type string $id_submit The comment submit element id attribute. Default 'submit'.
* @type string $class_container The comment form container class attribute. Default 'comment-respond'.
Expand Down Expand Up @@ -2646,6 +2648,7 @@ function comment_form( $args = array(), $post = null ) {
),
'comment_notes_after' => '',
'action' => site_url( '/wp-comments-post.php' ),
'novalidate_form' => false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this would be clearer as novalidate_att, and indicate that this is the behavior of the novalidate attribute. I'm on the fence about this, because we can't really avoid the double-negative. I'm not sure there's any foolproof way to communicate how the false state of a negative attribute is expressed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I picked this name to parallel to keys like class_container, id_form, and name_submit which is pattern is attribute name followed by the thing the attribute is for.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense; it just doesn't follow the pattern used elsewhere in the same array. E.g. 'action' => "value of the action attribute".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By that argument, maybe it would just be 'novalidate' => false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency, apparently, is hard. ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It couldn't be relevant to any other element anyway, same as action, as only the form has either of these attributes. It's different for id or class when they can be on anything.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For another idea: use_novalidate (which is similar to use_desc_for_title in wp_list_categories())

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so because the other keys don't use a verb like that in this function. I like novalidate as it can't be any simpler 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just 'novalidate' makes sense. As @westonruter says, it's not valid on any other element, and it matches the method used for 'action'. Let's go with that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, there we are! 3550454

'id_form' => 'commentform',
'id_submit' => 'submit',
'class_container' => 'comment-respond',
Expand Down Expand Up @@ -2729,7 +2732,7 @@ function comment_form( $args = array(), $post = null ) {
esc_url( $args['action'] ),
esc_attr( $args['id_form'] ),
esc_attr( $args['class_form'] ),
( $html5 ? ' novalidate' : '' )
( $args['novalidate_form'] ? ' novalidate' : '' )
);

/**
Expand Down
34 changes: 34 additions & 0 deletions tests/phpunit/tests/comment/commentForm.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,4 +193,38 @@ public function test_comment_form_should_display_for_specified_post_when_passed_
$post_hidden_field = "<input type='hidden' name='comment_post_ID' value='{$post_id}' id='comment_post_ID' />";
$this->assertStringContainsString( $post_hidden_field, $form );
}

/**
* Tests novalidate attribute on the comment form.
*
* @ticket 47595
*/
public function test_comment_form_and_novalidate_attribute() {
$post_id = self::$post_id;

// By default, the novalidate is not emitted.
$form = get_echo( 'comment_form', array( array(), $post_id ) );
$p = new WP_HTML_Tag_Processor( $form );
$this->assertTrue( $p->next_tag( array( 'tag_name' => 'FORM' ) ), 'Expected FORM tag.' );
$this->assertNull( $p->get_attribute( 'novalidate' ), 'Expected FORM to not have novalidate attribute by default.' );

// Opt in to the novalidate attribute by passing an arg to comment_form().
$form = get_echo( 'comment_form', array( array( 'novalidate_form' => true ), $post_id ) );
$p = new WP_HTML_Tag_Processor( $form );
$this->assertTrue( $p->next_tag( array( 'tag_name' => 'FORM' ) ), 'Expected FORM tag.' );
$this->assertTrue( $p->get_attribute( 'novalidate' ), 'Expected FORM to have the novalidate attribute.' );

// Opt in to the novalidate attribute via the comment_form_defaults filter.
add_filter(
'comment_form_defaults',
static function ( array $defaults ): array {
$defaults['novalidate_form'] = true;
return $defaults;
}
);
$form = get_echo( 'comment_form', array( array(), $post_id ) );
$p = new WP_HTML_Tag_Processor( $form );
$this->assertTrue( $p->next_tag( array( 'tag_name' => 'FORM' ) ), 'Expected FORM tag.' );
$this->assertTrue( $p->get_attribute( 'novalidate' ), 'Expected FORM to have novalidate attribute.' );
}
}
Loading