Skip to content
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.

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

Fixed: [UX] Clean up English on number form validation failure. #5264

Closed
jenlampton opened this issue Sep 29, 2021 · 26 comments · Fixed by backdrop/backdrop#3781
Closed

Fixed: [UX] Clean up English on number form validation failure. #5264

jenlampton opened this issue Sep 29, 2021 · 26 comments · Fixed by backdrop/backdrop#3781

Comments

@jenlampton
Copy link
Member

jenlampton commented Sep 29, 2021

Description of the bug

When entering 0 into a field that requires a positive integer, the error messaging reads
Node NID is must be higher or equal to 1.

There are several issues with this that should be resolved:

  • 2 verbs, is and must be
  • strange phrasing higher or equal to.

Steps To Reproduce

To reproduce the behavior:
0. apply the patch at backdrop/backdrop#3760

  1. Create a layout at node/5
  2. Add a block
  3. add a node ID visibility condition to the block
  4. enter 0 for your node ID

Actual behavior

the error messaging reads
Node NID is must be higher or equal to 1.

Expected behavior

how about these:

  • %name: the value must be numeric.
  • %name: the value must be greater than or equal to %min.
  • %name: the value must be less than or equal to %max.
  • %name: the value must be an integral number of steps %step from %offset.

We could also add a separate test for integrality (probably the most common use case for #step). If both #min and #step are integers, then the error would be

  • %name: the value must be an integer.

Additional information

Add any other information that could help, such as:

  • Backdrop CMS version: 1.20.0 + patch

This is a follow-up to #2668

@bugfolder bugfolder changed the title [UX] Clean up english on number form validation filure [UX] Clean up english on number form validation failure Sep 30, 2021
@jenlampton
Copy link
Member Author

At some point we changed how the front page path was saved. It used to save the system path so this function didn't bother checking aliases. But when we changed the storage of front-page, we forgot to add the alias checking back.

I'm going to assume there was a good reason for that change (Though I can't think of one, but I couldn't find an issue for it.) So this fixes backdrop_is_front_page() to account for the new front page settings :)

@indigoxela
Copy link
Member

@jenlampton I'm confused. This issue's description is about wording in a form error, but the linked PR does something completely different. Could it be that you linked the wrong PR?

@jenlampton
Copy link
Member Author

jenlampton commented Sep 30, 2021 via email

@bugfolder
Copy link

bugfolder commented Oct 1, 2021

In addition to the PR correction, I think there's further improvements that could be made to the error messages. Error messages are clearer if you quote the erroneous value in the message, particularly when the error messages are spatially separated from their corresponding element. Also, it's a little more direct to say specifically what's wrong with the erroneous input.

I suggest the following replacements in form_validate_number() (where %value is the value and other placeholders are substituted appropriately):

  • '%name must be a number' ==> 'In %name, %value is not numeric.'
  • '%name must be higher or equal to %min.' ==> 'In %name, %value is not greater than or equal to %min.'
  • '%name must be below or equal to %max.' ==> 'In %name, %value is not less than or equal to %max.'
  • '%name is not a valid number.' ==> 'In %name, %value is not an integral multiple of @step from @offset.'

Note that in the current implementation of type number, the "is not numeric" test is unreachable from a browser submission (see comment). But that's a different issue.

@klonos
Copy link
Member

klonos commented Oct 6, 2021

@bugfolder I like your suggestions, but I find "must be" better than "is not" for validation errors. In fact, I find text such as this to be kinda patronising:

In the What-have-you field, 3 is not greater than or equal to 5

@bugfolder
Copy link

Yeah, I see that. If I change it to "must be," then it becomes

In the What-have-you field, 3 must be greater than or equal to 5

which doesn't sound a whole lot better.

Saying "Entered value of 3 is not greater than or equal to 5" is less patronizing but more words.

So we could go back to "Entered value must be greater than or equal to 5."

It's nice, though, in general, to quote the actual value that was entered. This is especialy helpful when there is a long form with several errors and all the errors are collected in the message space at the top of the page. Is there a wording that works that doesn't sound patronizing but still quotes the entered value?

@klonos
Copy link
Member

klonos commented Oct 6, 2021

How about this?:

What-have-you field: the value entered must be greater than or equal to 5

(no need to mention the exact value that has been entered - the user can obviously see that)

@jenlampton
Copy link
Member Author

I also think maybe "Provided" might be more human sounding than "Entered". So, `The value provided must be greater than or equal to 5."

@klonos
Copy link
Member

klonos commented Oct 8, 2021

The way the issue title is phrased makes it a task. I think that it is a good candidate for a next bug fix release.

@bugfolder
Copy link

bugfolder commented Oct 8, 2021

Now that we're back to not quoting the value at all, how about these:

  • %name: the value must be numeric.
  • %name: the value must be greater than or equal to %min.
  • %name: the value must be less than or equal to %max.
  • %name: the value must be an integral number of steps %step from %offset.

We could also add a separate test for integrality (probably the most common use case for #step). If both #min and #step are integers, then the error would be

  • %name: the value must be an integer.

@jenlampton
Copy link
Member Author

Love it!

@stpaultim
Copy link
Member

@bugfolder - I will double check my work later tonight, but I just ran through the steps to recreate and don't think the error message is totally fixed yet.

image

The value for Node NID is must be greater than or equal to 1.

"is must" was part of the original problem, right?

@bugfolder
Copy link

Yeah. All that discussion was about the error message, but if the name of the field is "Node NID is", anything that quotes the field name is going to be awkward.

Maybe the solution here is to change the field name to just be "Node NID". (After all, we don't label the visibility condition with "Visibility condition is".)

@bugfolder
Copy link

bugfolder commented Apr 27, 2022

I've updated the PR to remove the "is". Seems like this will also ease translations in other languages, since translators can now rely on the title being just a name, rather than a sentence fragment that has to fit into multiple error messages.

A search for is' looking for similar occurrences elsewhere turned up only one similar case, in class FrontLayoutAccess (the title is Current path is). In that case, there's no validation that incorporates the title, so it can be left as-is.

Back to needs review and testing.

@quicksketch
Copy link
Member

The PR looks good to me but there's a merge conflict. Could you rebase against 1.x?

@bugfolder
Copy link

Rebased, but now a test consistently fails, so need to dig into that. (Weird that it's only bad in 7.4 but passes in 5.6 and 8.1.)

@bugfolder
Copy link

Fixed a failing test, all tests now passing.

@stpaultim
Copy link
Member

I tested using the steps to reproduce and it works for me. Should I be testing anywhere else?

image

@bugfolder
Copy link

bugfolder commented Apr 30, 2022

Should I be testing anywhere else?

This visibility condition suffices to exercise most of the different error behaviors, but you could also try other bogus entries: a non-numeric value, a non-integral value. (Spoiler alert: I've already verified these.)

The non-numeric behavior will be browser-specific: Safari lets you type 'asdf', but then blanks it upon submission, so Backdrop responds as if the field were blank, while Chrome won't even let you type non-numeric characters.

Thx for testing! 🙏

@quicksketch
Copy link
Member

I merged backdrop/backdrop#3781 into 1.x and 1.21.x. Thanks and great work @bugfolder!

backdrop/backdrop@ba2829e by @bugfolder, @jenlampton, @stpaultim, @quicksketch, @docwilmot, and @klonos.

@herbdool herbdool changed the title [UX] Clean up english on number form validation failure Fixed: [UX] Clean up English on number form validation failure. May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment