Make WordPress Core

Opened 6 years ago

Last modified 21 hours ago

#47595 reopened enhancement

Re-evaluate whether comment form should still get the HTML5 novalidate attribute

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: 6.8.2 Priority: normal
Severity: normal Version: 3.6
Component: Comments Keywords: has-patch input-validation has-unit-tests fixed-major needs-dev-note dev-reviewed
Focuses: ui, accessibility, template Cc:

Description (last modified by westonruter)

Given a theme that declares theme support for html5 via:

<?php
add_theme_support(
        'html5',
        array( 'comment-form' )
);

The result is that not only are the HTML5 input types used (e.g. email) but the comment form itself also gets a novalidate attribute added to it. This prevents HTML5 client-side validation from being performed on comment form when submitting which was added due to concerns about browsers implementing validation in very different ways. Nevertheless, this concern was about browsers 9 years ago so it would be worthwhile to see if this is still a problem. (The attribute was added in #15080 via r23689.)

If novalidate remains important to keep there is a case for optionally allowing it to be removed in order to rely only on client-side validation only. The PWA feature plugin adds support for Offline Commenting. This works by having the service worker intercept the POST submission to wp-comments-post.php: it then makes the request, and if it fails fails due to the user being offline, the service worker will replay it later when the user comes back online via the BackgroundSync API. However, if client-side validation was not performed then it is possible the user omitted a required field or provided a bad email address, so when they do come back online and the comment is synced, the server would then reject it and it would be more difficult for the user then to find out that their previously-posted comment actually failed. If no filter is available for removing the novalidate attribute, then it has to get removed via hacks like using JS or via PHP with output buffering.

But again, is there still a case for adding the novalidate attribute in the first place? Is not client-side validation a better user experience (if browsers now are now consistent in validation)? For that matter, is there even a case for the format attribute and shouldn't HTML5 always be used?

Attachments (3)

47595-novalidate_form-argument.0.diff (2.7 KB) - added by westonruter 6 years ago.
Option 1: Add a novalidate_form argument to allow the novalidate attribute to be omitted
47595-remove-novalidate-form-attr.0.diff (850 bytes) - added by westonruter 6 years ago.
Option 2: Remove form novalidate attribute altogether
47595-use-html5-exclusively-and-remove-novalidate.0.diff (5.0 KB) - added by westonruter 6 years ago.
Option 3: Eliminate format argument entirely in favor of using HTML5 only, while also removing novalidate form attribute since browsers should be consistent

Download all attachments as: .zip

Change History (33)

@westonruter
6 years ago

Option 1: Add a novalidate_form argument to allow the novalidate attribute to be omitted

@westonruter
6 years ago

Option 2: Remove form novalidate attribute altogether

@westonruter
6 years ago

Option 3: Eliminate format argument entirely in favor of using HTML5 only, while also removing novalidate form attribute since browsers should be consistent

#1 @westonruter
6 years ago

  • Keywords has-patch added

#2 @westonruter
6 years ago

  • Description modified (diff)

#3 @afercia
6 years ago

  • Keywords input-validation added

See #47505, which proposes to implement a comprehensive user input validation API. I guess that should take into considerations also forms used on the front end, like the post comments one.

#4 @SergeyBiryukov
6 years ago

#47728 was marked as a duplicate.

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


6 years ago

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


6 years ago

#7 @afercia
6 years ago

  • Milestone changed from Awaiting Review to Future Release

Discussed during today's accessibility bug-scrub and agree to move this ticket to Future Release. Probably the first thing to do would be some research to see what the current support from assistive technologies is. This could be a nice, first, task to evaluate further actions.

#8 @SergeyBiryukov
6 years ago

#42661 was marked as a duplicate.

#9 @SergeyBiryukov
6 years ago

#47875 was marked as a duplicate.

#10 @SergeyBiryukov
6 years ago

#49039 was marked as a duplicate.

#11 @guddu1315
6 years ago

As explained in my ticket here: https://core.trac.wordpress.org/ticket/49039
We can keep JavaScript validation on the form. So the user does not redirect to

wp-comments-post.php

And validation messages are shown on the same page.

Version 0, edited 6 years ago by guddu1315 (next)

#12 @pfefferle
5 years ago

What about at least a filter, to have a quick win? I think there are a lot of folks that are happy to use the browser validation for their themes (including me).

Last edited 5 years ago by pfefferle (previous) (diff)

#13 @afercia
5 years ago

Related: #32510.

#14 @oglekler
2 years ago

This error message looks obsolete and needs to be avoided as much as possible for not to spoil a good impression upon users. Let's make JS validation or remove "novalidate" attribute. Both solutions look not very complicated. If there are some concerns from the accessibility perspective, it's better to ask accessibility team how it can be done better. The third possibility is a hook to get developers an opportunity to decide for themselves, but from my perspective this is unnecessary complication to many developers when this can be solved in the root.

Last edited 2 years ago by oglekler (previous) (diff)

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


2 years ago

#16 @sabernhardt
23 months ago

#58968 was marked as a duplicate.

#17 @bugnumber9
18 months ago

The default behavior is indeed anything but good UX in 2024.

It was mentioned in https://core.trac.wordpress.org/ticket/49039#comment:4 that 27% of browsers in use don't support the required attribute, but that was 4 years ago.
As of today, more than 97% of browsers in use support it: https://caniuse.com/mdn-api_htmlinputelement_required

#18 @sabernhardt
7 months ago

#62638 was marked as a duplicate.

#19 @joedolson
2 months ago

  • Milestone changed from Future Release to 6.9
  • Owner set to joedolson
  • Status changed from new to accepted

At this point in time, I find it hard to see a justification for using novalidate here. If there are still reasons to do so, please share them; otherwise, I'm thinking it's long past time to remove this.

#20 follow-up: @sabernhardt
4 weeks ago

Simply removing novalidate from the printf could be a good first bug.

However, is there any benefit in creating a filter or argument to allow adding novalidate (and/or another attribute) to the form element?

#21 @sabernhardt
4 weeks ago

  • Keywords needs-refresh added

#22 in reply to: ↑ 20 @westonruter
4 weeks ago

  • Milestone changed from 6.9 to 6.8.2

Replying to sabernhardt:

However, is there any benefit in creating a filter or argument to allow adding novalidate (and/or another attribute) to the form element?

With a tweak to my latest patch, I think this would already be the case given my latest patch, wouldn't it? My latest patch adds novalidate_form key to the $args which gets filtered by comment_form_defaults. By default it is added if the theme supports html5. Now, however, it should be just false by default, regardless of whether the theme supports html5.

If someone wanted to preserve the novalidate attribute, they could just do:

<?php
add_filter( 'comment_form_defaults', function ( $args ) {
    $args['novalidate_form'] = true;
    return $args;
} );

If that sounds good to you, I'll refresh the patch and move to commit.

This could then be shipped as part of the 6.8.2 milestone as a tiny enhancement.

#23 @sabernhardt
4 weeks ago

I think so. Personally, I would agree with either option 1 reversed (adding an argument with default false) or option 2 (removing novalidate entirely). The new argument might be a better choice, though, in case someone disagrees with the change.

This ticket was mentioned in PR #8858 on WordPress/wordpress-develop by @westonruter.


3 weeks ago
#24

  • Keywords has-unit-tests added; needs-refresh removed

The comment form currently outputs a FORM tag with a novalidate attribute if the theme supports html5. This is obsolete now that browsers have greatly improved their support for client-side validation. Omitting this attribute helps ensure that a commenter does not accidentally submit an incomplete comment form, potentially risking losing the drafted comment (e.g. if the browser's bfcache is not enabled).

With this PR, the novalidate attribute is omitted by default. If a site owner wants to retain the novalidate attribute on the comment form, they can opt-in via a filter, for example:

add_filter(
        'comment_form_defaults',
        static function ( array $defaults ): array {
                $defaults['novalidate_form'] = true;
                return $defaults;
        }
);

Or when they can pass in this argument when the theme calls comment_form():

comment_form( array( 'novalidate_form' => true ) );

Trac ticket: https://core.trac.wordpress.org/ticket/47595

#25 @westonruter
3 weeks ago

  • Owner changed from joedolson to westonruter

#26 @westonruter
8 days ago

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

In 60304:

Comments: Remove novalidate attribute from comments form by default.

Browser support for client-side validation is now reliable. Blocking the form submission when there is a missing/invalid field prevents the possibility of losing comment content when bfcache does not restore the previous page when going back.

To retain the novalidate attribute, the comment_form()'s args now accepts a novalidate key which will add the attribute when it is true. This can also be supplied by the comment_form_default_fields filter, for example:

add_filter( 'comment_form_defaults', function ( $args ) {
    $args['novalidate'] = true;
    return $args;
} );

Fixes #47595.
Props westonruter, joedolson, sabernhardt, afercia, SergeyBiryukov, guddu1315, pfefferle, oglekler, bugnumber9.

#27 @westonruter
8 days ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 6.8.2 consideration (as is referenced in a @since tag).

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


24 hours ago

#29 @audrasjb
21 hours ago

  • Keywords needs-dev-note added

#30 @audrasjb
21 hours ago

  • Keywords dev-reviewed added

Second committer review: This changeset is OK for a 6.8 branch backport.

Note: See TracTickets for help on using tickets.