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

[EuiFieldNumber] Default to step="any" and check native validity on blur #6760

Merged
merged 4 commits into from
May 11, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented May 9, 2023

Summary

This PR is a follow-up to our existing EuiFieldNumber enhancement that added detection of native browser invalidity (#6741, 37c35ea).

Specifically, this PR is meant to address a comment left in a Kibana upgrade: elastic/kibana#155208 (comment).

Problem

Initially I thought the :invalid issue on EuiFieldNumbers intended for decimal usage was already a problem beforehand. As it turns out, it had not been a visual problem because

  1. On blur, <input type="number">s with an undefined step update their validity status to accept the decimal as valid (default browser behavior). Screencap of said behavior:
    screencap

  2. Our :focus styles (blue underline) were taking precedence over the :invalid styles (red underline) and as such the invalid styling was never actually visible for decimal inputs. When we added the icon, the "invalid" state suddenly became visible. And then on blur, both focus and invalid styles would go away 💀

Solution

This PR smooths out UX issues around the step behavior for number inputs that contain decimals by defaulting to step="any". After discussing and talking through the default browser behavior with @ryankeairns and @1Copenut, we determined that while EUI normally prefers to stick as close to browser implementation as possible, in this case the browser implementation felt clunky and bad, and this was clearly a case where changing the default would result in significant UX improvement.

Note that with step="any", the stepper increment/decrement still defaults to 1 which is the same behavior as before.

This PR also adds the native validity check to onBlur as well, since browsers do apparently use that event (as well as change/keyup) to determine validity.

QA

General checklist

  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • A changelog entry exists and is marked appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes

cee-chen added 2 commits May 9, 2023 10:04
- so that reported tooltips don't disappear immediately on outside click
@cee-chen cee-chen requested a review from 1Copenut May 9, 2023 17:38
@cee-chen
Copy link
Contributor Author

cee-chen commented May 9, 2023

@1Copenut do you mind reviewing this since you have context from our rubber ducky session yesterday?

@cee-chen cee-chen marked this pull request as ready for review May 9, 2023 17:39
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6760/

… `aria-invalid`

- I was previously being a little hesitant about checking for undefined to allow consumers to override native invalid state if possible, but on second thought this makes no sense as the other invalid states use `||`
@cee-chen
Copy link
Contributor Author

@1Copenut I should have addressed the aria-invalid always being false (even when native state was invalid). I was previously being a little hesitant & allowing consumer isInvalid={false} to take precedence/override native invalid state, but on second thought this makes no sense as the other invalid UI states don't use the same ?? vs || logic.

Also, the staging link in the PR description is fixed now 🤦

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6760/

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! I did some testing with keyboard and VO and NVDA with preferred browsers. All behaved correctly. I also found some interesting tidbits about e as a shorthand for "ten to the power of..." Screen readers announce this correctly and the input is valid as long as e is followed by a number. I'll post some screenshots in a comment for posterity.

@1Copenut
Copy link
Contributor

Some interesting learnings about e as a mathematical shortcut:

55e is an invalid number with step equals any
55e6 is a valid number with step equals any. Screen readers announce it as 55 times 10 to the power of 6.
Negative 55e6 is a valid number with step equals any. Screen readers announce it as Negative 55 times 10 to the power of 6.

@cee-chen
Copy link
Contributor Author

Very cool testing! I'll fly us all out in-person for celebratory drinks if the day ever comes that a user in Kibana needs e for their number inputs 😂

@cee-chen cee-chen enabled auto-merge (squash) May 11, 2023 16:10
@cee-chen cee-chen disabled auto-merge May 11, 2023 16:10
@cee-chen cee-chen merged commit a532d4e into elastic:main May 11, 2023
@cee-chen cee-chen deleted the fieldnumber/validity branch May 11, 2023 16:10
tkajtoch added a commit to elastic/kibana that referenced this pull request May 18, 2023
## Summary

`@elastic/[email protected]` ⏩ `@elastic/[email protected]`

---

## [`80.0.0`](https://github.com/elastic/eui/tree/v80.0.0)

- Improved the contrast ratio of meta labels within
`EuiSelectableTemplateSitewide` to meet WCAG AA guidelines.
([#6761](elastic/eui#6761))
- Added `vulnerabilityManagementApp` glyph to `EuiIcon`
([#6762](elastic/eui#6762))
- Added `logoVulnerabilityManagement` icon to `EuiIcon`
([#6763](elastic/eui#6763))
- Added `onPanelChange` callback to `EuiContextMenu` to provide consumer
access to `panelId` and `direction`.
([#6767](elastic/eui#6767))

**Bug fixes**

- Fixed `EuiComboBox` so `append` and `prepend` icon buttons are full
height and vertically centered.
([#6766](elastic/eui#6766))
- Improved the uniformity of dropdown components by hiding the dropdown
icon of disabled `EuiComboBox`s.
([#6768](elastic/eui#6768))

**Breaking changes**

- `EuiFieldNumber` now defaults the `step` prop to `"any"`
([#6760](elastic/eui#6760))
- EUI now globally resets a default Chromium browser style that was
decreasing the opacity of disabled `select` items.
([#6768](elastic/eui#6768))

---------

Co-authored-by: Kibana Machine <[email protected]>
delanni pushed a commit to delanni/kibana that referenced this pull request May 25, 2023
## Summary

`@elastic/[email protected]` ⏩ `@elastic/[email protected]`

---

## [`80.0.0`](https://github.com/elastic/eui/tree/v80.0.0)

- Improved the contrast ratio of meta labels within
`EuiSelectableTemplateSitewide` to meet WCAG AA guidelines.
([elastic#6761](elastic/eui#6761))
- Added `vulnerabilityManagementApp` glyph to `EuiIcon`
([elastic#6762](elastic/eui#6762))
- Added `logoVulnerabilityManagement` icon to `EuiIcon`
([elastic#6763](elastic/eui#6763))
- Added `onPanelChange` callback to `EuiContextMenu` to provide consumer
access to `panelId` and `direction`.
([elastic#6767](elastic/eui#6767))

**Bug fixes**

- Fixed `EuiComboBox` so `append` and `prepend` icon buttons are full
height and vertically centered.
([elastic#6766](elastic/eui#6766))
- Improved the uniformity of dropdown components by hiding the dropdown
icon of disabled `EuiComboBox`s.
([elastic#6768](elastic/eui#6768))

**Breaking changes**

- `EuiFieldNumber` now defaults the `step` prop to `"any"`
([elastic#6760](elastic/eui#6760))
- EUI now globally resets a default Chromium browser style that was
decreasing the opacity of disabled `select` items.
([elastic#6768](elastic/eui#6768))

---------

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants