Make WordPress Core

Opened 6 years ago

Closed 3 years ago

#47018 closed enhancement (fixed)

No error message provided upon incomplete input in Tags Administration Screen

Reported by: conner_bw's profile conner_bw Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.1.1
Component: Taxonomy Keywords: has-screenshots input-validation
Focuses: ui, accessibility, javascript, administration Cc:

Description

During a recent accessibility audit at Pressbooks, we were informed there were problems with the taxonomy editor.

If a user leaves the required 'name' input field blank and presses the submit button, the field becomes highlighted in red, but no error message is provided.

Source: https://github.com/WordPress/wordpress-develop/blob/5cc7acd1abc12e5f65f3dc67bfa1d27f683ee2df/src/wp-admin/edit-tags.php#L434

WordPress should provide an error message with instructions to help users correct the incomplete submission.

See: https://developer.mozilla.org/en-US/docs/Learn/HTML/Forms/Form_validation#Customized_error_messages

Attachments (8)

adding-a-tag-with-missing-name-field-with-javascript-disabled.jpg (20.4 KB) - added by birgire 6 years ago.
adding-a-tag-with-missing-name-field-with-javascript-enabled.jpg (23.9 KB) - added by birgire 6 years ago.
47018.diff (3.2 KB) - added by afercia 3 years ago.
47018.png (44.8 KB) - added by afercia 3 years ago.
47018.2.diff (4.4 KB) - added by afercia 3 years ago.
47018.3.diff (3.4 KB) - added by afercia 3 years ago.
47018.4.diff (1.7 KB) - added by peterwilsoncc 3 years ago.
only spaces.png (62.0 KB) - added by afercia 3 years ago.
Submitting the form with the Name field containing only spaces

Download all attachments as: .zip

Change History (41)

#2 @birgire
6 years ago

  • Keywords has-screenshots added

Thanks for the ticket @conner_bw

For reference I add:

Sorry for the duplicated screenshot in adding-a-tag-with-missing-name-field-with-javascript-enabled.2.jpg. (I can't remove it)

Related ticket #42937 for the missing success message.

#3 @conner_bw
6 years ago

If it helps, this is how we (temporarily, until issue is resolved) fixed it in Pressbooks:

https://github.com/pressbooks/pressbooks/commit/0e9e143f0e96d1589278eb8954264bec7a651574

#4 @afercia
6 years ago

Sorry for the duplicated screenshot

Removed it for you :)

@conner_bw thanks for your report. User input validation is an important topic and required by at least a couple WCAG requirements to meet level AA (error identification, correction suggestions). As it is now, input validation in WordPress is definitely insufficient both server side and client side.

It's certainly possible to try to fix the few cases where validateForm() is used. Wondering if a more general solution would be worth it. It would require some big refactoring though, especially in the way input fields are generated.

Re: the browsers constraint validation API: WordPress still supports Internet Explorer 11, which unfortunately doesn't support the reportValidity() method.

Will propose to discuss the general issue during next accessibility team meeting on Slack, on Friday at 15:00 UTC. Everybody's welcome.

#5 @SergeyBiryukov
6 years ago

  • Component changed from General to Taxonomy
  • Focuses ui administration added

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


6 years ago

#7 @afercia
6 years ago

  • Keywords input-validation added

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


6 years ago

#9 @afercia
6 years ago

  • Milestone changed from Awaiting Review to Future Release

Discussed during today's accessibility bug scrub. General consensus was there's no great point in fixing one single instance of insufficient user input validation.

To our knowledge, at the moment WordPress doesn't have a solid user input validation mechanism and maybe it's time to consider to build one. This would be a big project though, that probably needs to be addressed in a broader discussion. Moving to Future Release for now.

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


6 years ago

#11 @afercia
6 years ago

Related: #47105.

#12 @afercia
3 years ago

Looking back into this, seems to me there's some some parts of the related code are pretty old and maybe some overlap has occurred over time, which is perfectly understandable in a so large project with lots of legacy ode.

In src/js/_enqueues/admin/tags.js:

  • When submitting the form, window.validateForm is called to check whether the 'Name' input field is empty.
  • If it's empty, the code in tags.js returns false. thus preventing the Ajax call and the form submission.

Actually, this prevents the 'add tag' Ajax action from doing its job. Seems everything we need is already there:

  • The Ajax response properly returns success or error messages.
  • window.wpAjax parses the response and appends the messages at the top of the page.
  • There are already audible messages for screen readers via wp.a11y.speak.
  • The form submission is already prevented.

All the above doesn't run just because of the return false in tags.js. I do realize the current code 'saves' one Ajax request when the field is empty but that comes at the cost of missing feedback for users.

Removing the return false does allow the Ajax request to run and return the error messages. Seems to me this would be more inline with the purpose of the existing Ajax response.

@afercia
3 years ago

#13 @afercia
3 years ago

  • Focuses javascript added
  • Keywords has-patch added
  • Milestone changed from Future Release to 6.0

47018.diff

  • Allows the add tag Ajax request to run also when the form is submitted with the Name field left empty.
  • This way, the response is now able to return error messages.
  • Note that this will trigger an Ajax call every time the form is submitted with the 'Name' field empty.

Additionally:

  • Updates the CSS classes of the notices: 'error' and 'updated' are legacy and shouldn't be used any longer.
  • Adds audible messages for 3 more cases so that they're now announced by screen readers:
    • No permissions error: 'Sorry, you are not allowed to do that.'
    • Broken error: 'Something went wrong.'
    • When the response returns an error with a custom message.
  • Updates the styling of the error red 'border', ot make it more consistent with the 'border' used for the focus style.
  • Adds wp-a11y as dependency for the wp-ajax-response script.

See attached screenshot for the error message and the new red border style.

Moving to 6.0 consideration.

@afercia
3 years ago

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


3 years ago

#15 @ryokuhi
3 years ago

  • Keywords needs-testing added

We reviewed this ticket today during the accessibiltiy team's bug-scrub.

I'm adding the needs-testing label, so that, if the patch is valid, it can be committed before the next release.

As ticket #54394, also milestoned for 6.0, adds a "core way" to mark required fields, maybe this is a nice use case for the new functions.

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


3 years ago

#17 @peterwilsoncc
3 years ago

  • Keywords needs-refresh added

47018.diff seems to contain a lot of things that are off-focus for this ticket. I'm in favour of the changes going in but they need to be on a dedicated ticket so the testing instructions aren't misleading.

As it is, the patch may affect more than the tags/categories/term screen so a wider range of testing is needed, basically anything that uses wp-ajax-response.js


Testing the new term form:

  • AJAX request was made on error
  • The error message was displayed as expected

There's a slight but noticeable delay between the field getting a red border and the error message displaying. The result is a little disjointed as an experience, it seems like there are two errors rather than one.

I think this needs a little more work before it goes in.

#18 @afercia
3 years ago

  • Keywords needs-testing removed

@peterwilsoncc thanks for reviewing.

The changes to wp-ajax-response.js are mostly 'cosmetic' changes about legacy CSS class names, indentation, and coding standards. The only functional changes are the new calls to wp.a11y.speak() which shouldn't break anything.

However, besides the slight delay between the field getting a red border and the error message displaying, I see there's one more problem. The input field will now get a red border for any error response, which is not correct. It should only happen for the error with code empty_term_name. I'd agree this needs some more work.

@afercia
3 years ago

#19 @afercia
3 years ago

  • Keywords needs-testing added; needs-refresh removed

47018.2.diff builds on the previous patch and:

  • Adds specific error code (when available) to the add-tag ajax action .
  • Checks for that error code to add the red border to the input field.
  • Calls validateForm() when the response error is returned, so that there's no delay between the field getting a red border and the error message displaying.

#20 @peterwilsoncc
3 years ago

The changes to ajax-response.js will need to go in on a separate ticket.

The same JavaScript is used on the media, user, updater and any page with a list table. While ensuring AJAX responses are announced is ambiguously a good thing, it needs to go on another ticket to ensure adequate testing can take place.

A lot of the pages using ajax-response.js have existing calls to wp.a11y.speak() so I don't want to risk adding double-up announcements on a ticket with another focus.

@afercia
3 years ago

#21 @afercia
3 years ago

47018.3.diff

  • Removes the changes to ajax-response.js.
  • Trims the input field value in window.validateForm to pair what happens on the PHP and JS sides.

To test:

  • Leave the 'Name' field empty and submit the form.
  • Check a notice appears with the message 'A name is required for this term.'
  • Check the 'Name' field gets a red border (actually, a CSS box-shadow).
  • Use your browser's dev tools inspector and check the polite ARIA live region with ID a11y-speak-polite contains the text 'A name is required for this term.'
  • Refresh the page.
  • Enter only a few spaces into the 'Name' field empty and submit the form.
  • Repeat the steps above.

Note: when submitting the form, the submit button text gets remove. That's a separate issue tracked in #48030.

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

#22 @afercia
3 years ago

Created a new ticket for the enhancements to ajax-response.js. See #55537.

#23 @peterwilsoncc
3 years ago

In 47018.4.diff I've removed all changes except those in tags.js and the associated ajax action.

  • validateForm is used throughout the admin so trimming could cause false errors on other forms
  • the CSS change will also affect other invalid forms so should be done on dedicated ticket
  • removed the new wp-a11y dependency for inclusion in #55537.

This ticket was mentioned in PR #2525 on WordPress/wordpress-develop by peterwilsoncc.


3 years ago
#24

Trac ticket:

#25 @peterwilsoncc
3 years ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 53088:

Taxonomy: Show error message for terms without a name.

Display an error message to users if they attempt to create a term without a name via the admin-ajax add-tag action. This improves the accessibility of the screen by avoiding the use of color alone to indicate an error.

Props conner_bw, birgire, afercia.
Fixes #47018.

#27 @afercia
3 years ago

  • Keywords needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

@peterwilsoncc thanks for your review and commit. I'm going to reopen this ticket because there are a few things I'd like to bring in for some better consideration.

1

removed the new wp-a11y dependency for inclusion in #55537.

The ajax-response.js script already contains two calls to wp.a11y.speak. They were added in a previous commit, see [52170]. However, that change missed to add the wp-a11y dependency. That's unsafe and should be fixed. There's no guarantee that #55537 will make it for 6.0. The dependency should have been kept or at least a new ticket should have been created.

2

validateForm is used throughout the admin so trimming could cause false errors on other forms

I'm not sure that's correct. In core, there are two JavaScript functions named validateForm:

  • window.validateForm in common.js
  • window.wpAjax.validateForm in ajax-response.js

The first one is used here in tags.js. I couldn't find any other usage in core. I think it's safe to add trim(). If we don't, submitting the form with the 'Name' field that contains only spaces will display an admin notice at the top of the page but the red border will not be added to the input field. I'm not sure that would be a good experience for users. See attached screenshot.

@afercia
3 years ago

Submitting the form with the Name field containing only spaces

#28 @afercia
3 years ago

Created a new ticket for the CSS change for invalid forms, see #55541.

#29 @peterwilsoncc
3 years ago

I've created #55544 as an appropriate place to update the script registration. Including such items on commits for unrelated tickets can lead to reintroducing the bug if the commit needs to be reverted.

validateFile() may also be used by plugins, adding the trim() requires more consideration on a dedicated ticket even if tags.js is probably the only place it's used. If the term name needs to be trimmed, it should be done in tags.js. This can be done on this ticket.

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


3 years ago

#31 @peterwilsoncc
3 years ago

  • Keywords has-patch removed

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


3 years ago

#33 @costdev
3 years ago

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

Most of the remaining items related to this ticket have been separated to other tickets:

  • For the CSS change, see #55541.
  • For the dependency, see #55544.

However, there is still a possible outstanding change which would implement trim() on the term name.

With 6.0 Beta 1 released on Tuesday, we are now past Feature Freeze for the 6.0 cycle. The implementation of trim() should be separated out to a related bug (defect) ticket to stay in line with Core's release process.

I'll close out this ticket as fixed.

Note: See TracTickets for help on using tickets.