-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix(a11y): TimePicker, DateInput a11y fixes #6568
fix(a11y): TimePicker, DateInput a11y fixes #6568
Conversation
@@ -197,6 +195,7 @@ export class TimePicker extends React.Component<TimePickerProps, TimePickerState | |||
onKeyDown={this.getInputKeyDownHandler(unit)} | |||
onKeyUp={this.getInputKeyUpHandler(unit)} | |||
role={this.props.showArrowButtons ? "spinbutton" : undefined} | |||
type="number" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a reason why we did not use this attribute, as you can see this change broke the test suite. you'll need to revert this part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that test can just be modified with this change. The test is essentially "if entry is not a number, display error", however setting type=number
disallows non-number entry in the first place and solves this case / problem from the start.
Is there a reason to not have it be type=number
when that's exactly what it is?
Tests updated: abe73d1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, if we use type=number
, we can just use the min
and max
props instead of aria-valuemin
and aria-valuemax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to dig up the original reason why we didn't use type="number"
when the component was originally written 8 years ago I and couldn't find a compelling one. Perhaps the browser support was incomplete, or there wasn't good support for doing the style overrides you've added in your recent commits. I suppose we can switch to type="number"
now, as it does seem like the right markup to use in the long run. I tested it out in the docs preview build and things seem to be working fine 👍
@@ -197,6 +195,7 @@ export class TimePicker extends React.Component<TimePickerProps, TimePickerState | |||
onKeyDown={this.getInputKeyDownHandler(unit)} | |||
onKeyUp={this.getInputKeyUpHandler(unit)} | |||
role={this.props.showArrowButtons ? "spinbutton" : undefined} | |||
type="number" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to dig up the original reason why we didn't use type="number"
when the component was originally written 8 years ago I and couldn't find a compelling one. Perhaps the browser support was incomplete, or there wasn't good support for doing the style overrides you've added in your recent commits. I suppose we can switch to type="number"
now, as it does seem like the right markup to use in the long run. I tested it out in the docs preview build and things seem to be working fine 👍
}); | ||
|
||
it("allows invalid text entry, but shows visual indicator", () => { | ||
it("disallows allows non-number text entry", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a typo:
it("disallows allows non-number text entry", () => { | |
it("disallows non-number text entry", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes lgtm, can you please run yarn lint-fix
inside packages/datetime/
?
@adidahiya Done. |
Basing a11y structure off of this example: https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-datepicker/