Fix some apparent a11y issues with react-dates #780

majapw · 2017-10-19T22:32:54Z

I recently pulled in the latest version of react-dates into our internal repos and discovered a few accessibility issues through our automatic testing.

The caption id was not unique! Namely it was the same on each month which was causing some problems. It also was missing the corresponding aria-labelledby attribute the table itself.
There was a text-contrast issue with the focus background color, so I modified the default styling to match what is currently used on airbnb.com. I still need to go back and make sure everything is customizable. :)
I took the opportunity to address a lot of queries in the issues on this repo and make the input a real input.

to: @ljharb @gabergg @backwardok @airbnb/webinfra

coveralls · 2017-10-19T22:39:35Z

Coverage decreased (-0.05%) to 86.738% when pulling 87331be on maja-fix-a11y-issues into a448352 on master.

coveralls · 2017-10-19T23:09:12Z

Coverage decreased (-0.05%) to 86.738% when pulling 98bd035 on maja-fix-a11y-issues into a448352 on master.

gabergg · 2017-10-19T23:35:59Z

src/components/CalendarMonth.jsx

+        <table
+          {...css(styles.CalendarMonth_table)}
+          role="presentation"
+          aria-labelledby={`CalendarMonth_caption__${toISOMonthString(month)}`}


Maybe CalendarMonth_caption__${toISOMonthString(month)} in a var just to make sure they both get updated together?

backwardok · 2017-10-19T23:35:55Z

src/components/CalendarMonth.jsx

+        <table
+          {...css(styles.CalendarMonth_table)}
+          role="presentation"
+          aria-labelledby={`CalendarMonth_caption__${toISOMonthString(month)}`}


since this has role="presentation", the label here doesn't mean as much (and also possibly won't even get read)

hmmm, does this belong in the DayPicker then? hmmm. But the DayPicker contains all the months, so perhaps it needs a different label?

Trying to follow up on https://github.com/airbnb/react-dates/pull/715/files#r139019568

I don't know that it's necessary, and I think even in general case, I don't know that the month name is as meaningful as a label outside of this table. I would imagine that the container for the picker may benefit from a label like "Date picker" or "Select a date", but the month is less valuable information for the higher level container

I'll just kill the id then

majapw · 2017-10-20T00:12:06Z

PTAL @backwardok @gabergg

coveralls · 2017-10-20T00:23:47Z

Coverage decreased (-0.05%) to 86.738% when pulling ec8ac9e on maja-fix-a11y-issues into a448352 on master.

ljharb

Can you elaborate on why the inputValue and startDateValue and endDateValue props aren't necessary?

ljharb · 2017-10-20T06:15:10Z

src/components/DateRangePickerInput.jsx

@@ -43,7 +43,6 @@ const propTypes = forbidExtraProps({
  onQuestionMark: PropTypes.func,

  startDate: PropTypes.string,
-  startDateValue: PropTypes.string,
  endDate: PropTypes.string,
  endDateValue: PropTypes.string,


did you mean to remove this prop also?

majapw · 2017-10-20T23:44:36Z

@ljharb the reason for removing the inputValue, startDateValue, and endDateValue props is that they were essentially a way to have the input element value to be in iso format even as the display format was something nicer. The effect was that a server-side submit of a form (rare, i know) would have a nice iso formatted date submitted.

Of course now that I have axed the concept of the fake input and real input (which is much better for accessibility reason), this is no longer necessary and the value of the input element is just the formatted date value when it exists.

coveralls · 2017-10-21T00:03:13Z

Coverage decreased (-0.05%) to 86.738% when pulling 6c2e062 on maja-fix-a11y-issues into a448352 on master.

coveralls · 2017-10-21T00:17:47Z

Coverage decreased (-0.05%) to 86.738% when pulling aba5a55 on maja-fix-a11y-issues into a448352 on master.

coveralls · 2017-10-21T00:28:04Z

Coverage decreased (-0.05%) to 86.738% when pulling aba5a55 on maja-fix-a11y-issues into a448352 on master.

majapw · 2017-10-21T05:30:38Z

@ljharb @backwardok @gabergg could I get a stamp on this? I think I've addressed concerns.

ljharb

LGTM pending one question

ljharb · 2017-10-23T15:52:14Z

src/components/DateInput.jsx

@@ -90,7 +88,6 @@ class DateInput extends React.Component {

    if (focused && isFocused) {
      this.inputRef.focus();
-      this.inputRef.select();


Can you elaborate in why the autoselection was removed?

imo this is actually better for a11y. It's not expected that by default, moving to a text input will select everything in it.

So before, when you focused on the input, it was select the whole text so that when you typed anything, it would just replace whatever was there. This seemed like a good approach when there was no cursor to indicate where you were in the input. However, with a cursor, it seems unnecessary behavior.

backwardok

These changes lgtm, with one comment that can be addressed separately

backwardok · 2017-10-23T17:50:06Z

src/components/DateInput.jsx

@@ -90,7 +88,6 @@ class DateInput extends React.Component {

    if (focused && isFocused) {
      this.inputRef.focus();
-      this.inputRef.select();


imo this is actually better for a11y. It's not expected that by default, moving to a text input will select everything in it.

backwardok · 2017-10-23T17:51:22Z

src/components/DateInput.jsx

@@ -181,6 +176,8 @@ class DateInput extends React.Component {
          {...css(
            styles.DateInput_input,
            readOnly && styles.DateInput_input__readOnly,
+            focused && styles.DateInput_input__focused,
+            disabled && styles.DateInput_input__disabled,
          )}
          aria-label={placeholder}


probably for another PR, but this shouldn't be set to placeholder. If a developer wanted to pass in whatever they set for the placeholder as both the label and the placeholder text (which I'd argue that they shouldn't), that should be an explicit decision and not a default one.

Yeah, let's do that in a follow-up! :)

coveralls · 2017-10-23T18:21:52Z

Coverage decreased (-82.3%) to 4.372% when pulling 2e7948d on maja-fix-a11y-issues into e2e5767 on master.

coveralls · 2017-10-23T18:53:33Z

Coverage decreased (-0.06%) to 86.662% when pulling 9e76cc7 on maja-fix-a11y-issues into e2e5767 on master.

coveralls · 2017-10-23T19:20:03Z

Coverage decreased (-0.05%) to 86.662% when pulling ae1f8d9 on maja-fix-a11y-issues into 6850c56 on master.

coveralls · 2017-10-23T19:39:58Z

Coverage decreased (-0.05%) to 86.662% when pulling 4ae1487 on maja-fix-a11y-issues into 6850c56 on master.

backwardok

Github really doesn't want you to sneak changes past me apparently 😛

majapw · 2017-10-23T19:46:22Z

I blame jordan! :P

majapw force-pushed the maja-fix-a11y-issues branch from 87331be to 98bd035 Compare October 19, 2017 22:58

majapw added the semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. label Oct 19, 2017

gabergg reviewed Oct 19, 2017

View reviewed changes

backwardok reviewed Oct 19, 2017

View reviewed changes

majapw force-pushed the maja-fix-a11y-issues branch from 98bd035 to ec8ac9e Compare October 20, 2017 00:11

ljharb reviewed Oct 20, 2017

View reviewed changes

majapw force-pushed the maja-fix-a11y-issues branch from ec8ac9e to 6c2e062 Compare October 20, 2017 23:52

majapw force-pushed the maja-fix-a11y-issues branch 2 times, most recently from fc19881 to aba5a55 Compare October 21, 2017 00:09

majapw mentioned this pull request Oct 22, 2017

Input caret should be visible when typing #783

Closed

ljharb reviewed Oct 23, 2017

View reviewed changes

backwardok previously approved these changes Oct 23, 2017

View reviewed changes

majapw dismissed backwardok’s stale review via 2e7948d October 23, 2017 18:01

majapw force-pushed the maja-fix-a11y-issues branch from aba5a55 to 2e7948d Compare October 23, 2017 18:01

majapw force-pushed the maja-fix-a11y-issues branch from 2e7948d to 9e76cc7 Compare October 23, 2017 18:36

majapw force-pushed the maja-fix-a11y-issues branch from 9e76cc7 to ae1f8d9 Compare October 23, 2017 19:01

Fix some apparent a11y issues with react-dates

4ae1487

majapw force-pushed the maja-fix-a11y-issues branch from ae1f8d9 to 4ae1487 Compare October 23, 2017 19:21

backwardok approved these changes Oct 23, 2017

View reviewed changes

majapw merged commit 1871b23 into master Oct 23, 2017

majapw deleted the maja-fix-a11y-issues branch October 23, 2017 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some apparent a11y issues with react-dates #780

Fix some apparent a11y issues with react-dates #780

majapw commented Oct 19, 2017

coveralls commented Oct 19, 2017

coveralls commented Oct 19, 2017

gabergg Oct 19, 2017

backwardok Oct 19, 2017

majapw Oct 19, 2017

backwardok Oct 19, 2017

majapw Oct 19, 2017

majapw commented Oct 20, 2017

coveralls commented Oct 20, 2017

ljharb left a comment

ljharb Oct 20, 2017

majapw Oct 20, 2017

majapw commented Oct 20, 2017

coveralls commented Oct 21, 2017

coveralls commented Oct 21, 2017

coveralls commented Oct 21, 2017

majapw commented Oct 21, 2017

ljharb left a comment

ljharb Oct 23, 2017

backwardok Oct 23, 2017

majapw Oct 23, 2017

backwardok left a comment

backwardok Oct 23, 2017

backwardok Oct 23, 2017

majapw Oct 23, 2017

coveralls commented Oct 23, 2017

coveralls commented Oct 23, 2017

coveralls commented Oct 23, 2017

coveralls commented Oct 23, 2017

backwardok left a comment

majapw commented Oct 23, 2017

Fix some apparent a11y issues with react-dates #780

Fix some apparent a11y issues with react-dates #780

Conversation

majapw commented Oct 19, 2017

coveralls commented Oct 19, 2017

coveralls commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw commented Oct 20, 2017

coveralls commented Oct 20, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw commented Oct 20, 2017

coveralls commented Oct 21, 2017

coveralls commented Oct 21, 2017

coveralls commented Oct 21, 2017

majapw commented Oct 21, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

backwardok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 23, 2017

coveralls commented Oct 23, 2017

coveralls commented Oct 23, 2017

coveralls commented Oct 23, 2017

backwardok left a comment

Choose a reason for hiding this comment

majapw commented Oct 23, 2017