feat(NumberInput): add status validate icons

[gitdallas]

What: Closes #7730

used validated property based on TextInput using that property for such styling.

example: https://patternfly-react-pr-7806.surge.sh/components/number-input#with-status

Additional issues:

[patternfly-build]

Preview: https://patternfly-react-pr-7806.surge.sh

A11y report: https://patternfly-react-pr-7806-a11y.surge.sh

[mcoker]

LGTM! Left a comment about the possibility of using <TextInput> if we can for the number input?

Also the validated example input is readonly - should we add an onChange handler so it matches the rest of the examples? I imagine the use case for this is the same as the default examples, except with a visual cue for when the value is within threholds.

[mcoker]

LGTM! The only thing I noticed is if I try to enter a value in that input manually and hit backspace to enter a new number, it replaces the value with 0. That seems to just be an example thing in the onChange callback - up/down arrows work to change the number, so works for me!

[gitdallas]

LGTM! The only thing I noticed is if I try to enter a value in that input manually and hit backspace to enter a new number, it replaces the value with 0. That seems to just be an example thing in the onChange callback - up/down arrows work to change the number, so works for me!

@mcoker - You'll see the same thing here https://www.patternfly.org/v4/components/number-input#with-unit-and-thresholds

It has to do with the fact we are limiting it to numbers (empty string isn't a number) between 0 and 10 in these cases.

[gitdallas]

@mcarrano - in this PR i've utilized the textinput inside of the numberinput... however, there is logic within InputGroup is checking whether any (non select/textarea/textinput) children have an ID and then using that ID to set a describedby on the TextInput

This makes sense if we are using a label with an ID.. but in this case, if we put an ID on the up/down buttons for the NumberInput it will use that ID in the describedby for the TextInput which doesn't really make sense. I can't imagine why a button would be used as a describedby.

I've made it so that Button is one of the excluded types to get an ID from for describedby, but noticed there was actually a test wanting exactly that (a button ID used for describedby) https://github.com/patternfly/patternfly-react/blob/main/packages/react-core/src/components/InputGroup/__tests__/InputGroup.test.tsx#L16

I've updated the test to use a label instead. I've asked around a bit and it seems to be going back to you as the person who can make the decision on whether or not we can stop using a button ID in the inputGroup as the describedby for the input.

[mcoker]

there is logic within InputGroup is checking whether any (non select/textarea/textinput) children have an ID and then using that ID to set a describedby on the TextInput

@gitdallas IMO we should have a prop that disables that, and I'd argue making that the default when we can introduce that breaking change. The core examples show that same basic id/aria-describedby association (also using <button>s) - https://www.patternfly.org/v4/components/input-group/html#variations. I'm guessing when we created the <InputGroup> component, the automatic assigning of aria-describedby by any non-form-control with an id was based off of that and an attempt to improve the a11y of that form field. Though I'd argue that's too arbitrary of a way to describe a form control, and usage of this component has changed in a way that this logic is too simple. From my perspective, the input group component is more of a layout that puts a group of "controls" (have the common border/box formatting - dropdown, select, text input, control buttons, etc) next to one another and otherwise isn't particularly opinionated about what goes inside of it.

[gitdallas]

@mcoker - i agree about inputGroup should not be so opinionated, and found it odd to have it automatically generate describedby on a button id. As far as disabling that.. what if you did have multiple elements with IDs and you only wanted a specific one that wasn't the first (right now it just finds the first)?

To me it would make more sense to just pass the value for describedby if you want it - but at this point that would be a breaking change. Perhaps we could add a formControlDescribedBy prop where if set it would use that and otherwise it could keep doing what it's doing? But then if you don't want a describedby even though you do want an ID on something within the inputgroup.. what then? empty string for formControlDescribedBy? allow formControlDescribedBy to be string | false?

As far as the NumberInput - It's creating an inputGroup just around the [minus][input][plus] with no way to even put a label within that inputgroup. Maybe something should change there?

[mcoker]

IMO, we shouldn't build on top of the functionality, just add the option to disable it. If we have the option to pass an id to the input group to describe a form control, it's no more work to just pass that to the form control itself, and a lot more straight forward. Also, there are more things that can go in input groups that presumably need good labels (and maybe description) just like those form controls - if we do this with form select, why not any other menu like that? Dropdown, select, a custom menu/menu-toggle, etc. Similar to a text input, we would describe text input group, search input, etc. Unless we build support for that, I think this "feature" is odd and not of that much value.

As far as the NumberInput - It's creating an inputGroup just around the [minus][input][plus] with no way to even put a label within that inputgroup. Maybe something should change there?

If I understand the question, we have props for aria-label for all of the number input elements, and the ability to pass props directly to them (to add an id to the input to associate it with a form <label>'s for attribute or add aria-describedby, for example) so I think that's good?

patternfly-react/packages/react-core/src/components/NumberInput/NumberInput.tsx

Lines 45 to 55 in bb63f5f

	inputAriaLabel?: string;
	/** Aria label of the minus button */
	minusBtnAriaLabel?: string;
	/** Aria label of the plus button */
	plusBtnAriaLabel?: string;
	/** Additional properties added to the text input */
	inputProps?: any;
	/** Additional properties added to the minus button */
	minusBtnProps?: ButtonProps;
	/** Additional properties added to the plus button */
	plusBtnProps?: ButtonProps;

[gitdallas]

@mcoker - this would override the describedby passed in inputProps... but I suppose if we add your disableAssignDescribedby or something it could be used together... or maybe just letting inputProps.describedby have priority would be enough

[mcarrano]

@nicolethoen @tlabaj wanted to get your thoughts on the above convo. Looks like the enhacement is working as expected from a UX perspective, but it's unclear to me whether we should try to address the issue that @gitdallas raises about IDs. Thoughts?

[gitdallas]

TLDR on coker and my exchange, our final options were narrowed down to:

1. allow inputProps['aria-describedby'] to persist over the automatic describedby creation
   this would basically mean changing this line in inputGroup to add && child.props['aria-describedby'] === undefined and I could also make it so that if an empty string exists for inputProps[aria-describedby] it would just leave out the property all together.
2. have a new property on NumberInput component that is something like disabledAssignDescribedBy that just bypasses the feature.. with this they could still add the inputProps describedby if they wanted and it would be added. if they didn't add an inputProps describedby, the describedby prop wouldn't exist at all

this whole thing seems pretty edge casey, but need to figure something out

[gitdallas]

I went ahead and just made it so that if someone adds aria-describedby to the input props, they don't get overridden by the existing feature. Seems least invasive for this edge case. Functionality can be seen in new InputGroup tests.

[tlabaj]

Looks great so far Dallas. Just one little comment.

[tlabaj]

Thanks Dallas. LGTM

[wise-king-sullyman]

Looks perfect to me!

[nicolethoen]

I think I need to do more research about input type=number because I'm surprised that the default screen reader experience doesn't seem to include updating the user to the current value of the input when using the VO+arrow keys to change the value of the input. Same is true when updating the value using the + and - buttons. I think someone using a screen reader may need a little more information.
@thatblindgeye @jessiehuff do you have any more information about this scenario?

[thatblindgeye]

This is looking good! I had mainly some nitpicks below (let me know what you think).

In addition to @nicolethoen comment above, another issue is that the validation state only gets announced when navigating to the input, and even then only when the input is invalid (so a "warning" state won't get announced).

If the intent is to not have any validation text above/below the number input, a really quick option could be making the Number Input a live region and passing inputAriaLabel={validated} instead of "number input" (the value of the input could also be passed in, so that both the validation state and value get announced which could resolve the issue Nicole mentioned above).

[thatblindgeye]

do you have any more information about this scenario?

@nicolethoen it looks like this is partially an issue with Chromium (https://stackoverflow.com/questions/48299571/screen-reader-cannot-read-number-input-values-on-chrome).

With Firefox, changing the Number Input via Voice Over + arrow keys while in the input has the new value announced, though pressing the - or + buttons does not announce the new value. Safari acts similary to Firefox with some slight differences (works better when you just focus the input and press up/down arrow rather than "entering" the input via VO modifier + Shift + Down).

It is definitely odd that the change in value doesn't get announced in Chrome by default, even when changing the value via the actual input and not our own +/- buttons.

Adding aria live like I noted above seems to help resolve that in Chrome, but in Firefox the value ends up getting repeated by VO (pressing the minus button once would announce "89, 89, 89, 89" for me). So that may require a little more investigation.

[tlabaj]

@mcoker Should these formControlStyles be available in the number input. If so, should we open a core issue?

[thatblindgeye]

Looks good! Opened #7871 as a followup to the a11y issues since it looks like that will require more investigation to find a solution that resolves Chromium issues without introducing issues in other browsers.