Skip to content

feat: Add invalid prop to Input, which causes the border to be red#25225

Closed
behowell wants to merge 2 commits into
microsoft:masterfrom
behowell:invalid/input
Closed

feat: Add invalid prop to Input, which causes the border to be red#25225
behowell wants to merge 2 commits into
microsoft:masterfrom
behowell:invalid/input

Conversation

@behowell

@behowell behowell commented Oct 13, 2022

Copy link
Copy Markdown
Contributor

New Behavior

  • Add an invalid prop to Input
  • Add styling to cause the border to become red when invalid is set
    • image
  • Add screener tests for the invalid prop
  • Add a story for the invalid prop

Related Issue(s)

@behowell behowell self-assigned this Oct 13, 2022
@fabricteam

fabricteam commented Oct 13, 2022

Copy link
Copy Markdown
Collaborator

📊 Bundle size report

🤖 This report was generated against 530866cad542b2a1c4976badf7cdfdc5ec4920cf

@size-auditor

size-auditor Bot commented Oct 13, 2022

Copy link
Copy Markdown

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 530866cad542b2a1c4976badf7cdfdc5ec4920cf (build)

@behowell behowell marked this pull request as ready for review October 14, 2022 18:41
@behowell behowell requested review from a team and spmonahan as code owners October 14, 2022 18:41
*
* It is recommended to include an error message linked by `aria-errormessage`, or use `InputField` to handle it.
*/
invalid?: boolean;

@behowell behowell Oct 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I considered a few possible alternative approaches, and wanted to see if people preferred any of them:

  1. What's in this PR: add an invalid prop, which causes the border to be red.
    • Pros: Simple, straightforward API for Input on its own.
    • Cons: When used with InputField, it gives you two ways to say the same thing: <InputField invalid /> is the same as <InputField validationState="error" />
  2. Add a validationState?: 'success' | 'warning' | 'error' prop to Input, which matches Field's prop.
    • Pros: Uses the same way to describe the error state between Input and InputField.
    • Cons: The values "success" and "warning" don't do anything when used on Input alone, but they need to be allowed in order to work with InputField.
  3. Have the border be red when aria-invalid is true (which gets set by Field when validationState="error").
    • Pros: Simplicity, no new props, and relatively straightforward semantics.
    • Cons: Ties an aria prop to visuals. There's no way to have a red border without aria-invalid, and no way to have aria-invalid without a red border.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might be a bad suggestion!

What do you think about using the ⁠Constraint Validation API? This would allow us to target styles via the :invalid pseudo-selector (and :valid too). ⁠Here's an example of controlling an element's validity.

I took a look at Edge's accessibility tree inspector and setting the validity this way results in the same invalid use entry computed value as setting aria-invalid.

invalid_user_entry

Pros:

  1. Uses native platform capabilitites/we don't have to manage aria-*
  2. We get access to :valid/:invalid. Styling could be applied via Field.
  3. This functionality could be part of Field. Currently none of the inputs have validation behavior built in because we delegate all of it to Field.
  4. It meets our browser support matrix requirements: https://caniuse.com/constraint-validation

Cons:

  1. Need to use refs and mix imperative DOM APIs with declarative React code
  2. Seems more complicated than other proposals.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The idea is that Field will continue to control all the validation behavior, including applying styles and we make no changes to the input controls or Field API since we can already set all the necessary validation states on Field today.

@behowell behowell Oct 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I spent some time prototyping it using the form validation APIs. It seems like a potentially good idea... However, the main blocker is that the :invalid pseudo-class is only on the <input> element, but we need to change the border on the div that surrounds the input. Unless I'm mistaken, I don't think that's possible without :has (https://caniuse.com/css-has), which is still not in the CSS standard.

Also, so far Field has been agnostic to the method of form validation used: it's just a place to report the results, but doesn't favor any particular API. I'm not sure if building in calls to setCustomValidity would interfere with people who want to use the Constraint validation API in other ways.

Other more minor issues:

  • Field doesn't require that there be an error message; or the error message could be custom JSX. But setCustomValidity requires a string error message. This might be a non-issue if we just hardcode the string to "invalid" e.g. setCustomValidity('invalid') and use aria-errormessage to point to the real error message... but I'm not sure if that's a usable workaround.
  • The form validation APIs only work for <input>, <fieldset>, <textarea>, <button>, and a few others. Notably, it doesn't work for <div>, which is used by RadioGroup. That's not the end of the world though, since RadioGroup doesn't currently have any visuals for invalid (and maybe RadioGroup should be using fieldset....)

@behowell behowell Oct 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I talked to @smhigley offline, and it seems like option 3 above (using aria-invalid) might be a good option. It's somewhat of a middle ground between fully custom props (options 1-2), and the true form validation APIs. It's the simplest to implement, and has the least impact on public APIs or the behavior of Field.

I had been worried about using an aria prop for styling, but the MDN docs for aria-invalid even mention using it for styling:

If you are creating your own form validation scripts, make sure to include aria-invalid on invalid form controls, along with styling (use the [aria-invalid="true"] attribute selector) and messaging (with aria-errormessage) to help users understand where the mistake is and how they can fix it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The aria-invalid version is implemented here: #25252

I'm going to close this PR in favor of the other one. Thanks for the feedback!

@behowell behowell marked this pull request as draft October 14, 2022 19:23
@behowell behowell closed this Oct 14, 2022
@behowell behowell deleted the invalid/input branch February 15, 2024 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants