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 | Owned by: | 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.
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)
Change History (41)
#2
@
6 years ago
- Keywords has-screenshots added
Thanks for the ticket @conner_bw
For reference I add:
- the screenshot for the case of missing name when Javascript is disabled: adding-a-tag-with-missing-name-field-with-javascript-disabled.jpg
- the screenshot for the case of missing name when Javascript is enabled: adding-a-tag-with-missing-name-field-with-javascript-enabled.jpg
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
@
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
@
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.
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
#9
@
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
#12
@
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.
#13
@
3 years ago
- Focuses javascript added
- Keywords has-patch added
- Milestone changed from Future Release to 6.0
- 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 thewp-ajax-response
script.
See attached screenshot for the error message and the new red border style.
Moving to 6.0 consideration.
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
3 years ago
#15
@
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
@
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
@
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.
#19
@
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
@
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.
#21
@
3 years ago
- 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.
#23
@
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
@
3 years ago
- Owner set to peterwilsoncc
- Resolution set to fixed
- Status changed from new to closed
In 53088:
peterwilsoncc commented on PR #2525:
3 years ago
#26
#27
@
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
incommon.js
window.wpAjax.validateForm
inajax-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.
#29
@
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
This ticket was mentioned in Slack in #core by chaion07. View the logs.
3 years ago
#33
@
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:
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.
And here: https://github.com/WordPress/wordpress-develop/blob/5cc7acd1abc12e5f65f3dc67bfa1d27f683ee2df/src/wp-admin/edit-tag-form.php#L133