Some improvements for screen readers [a11y] #715

erin-doyle · 2017-09-14T21:38:48Z

Doing some screen reader testing on this and identified some things that could make the reading a little more clear/helpful:

The table was being interpreted as a data table versus a layout table and was reading extraneous info about the number of columns and rows in the current cell happened to be. The cell already nicely reads the full date and day of the week and therefore there's no need for the screen reader to associate the column headers with the cell.
The DayPickerRangeController did not seem to be properly setting the chooseAvailableStartDate phrase after first mounting so only after picking a start date and moving to the end date would the reader nicely read "it's available" for the date options.
I did also notice Issue [a11y] Disabled dates aren't actually disabled in markup #713 where unavailable dates are not reading as "unavailable" but I haven't figured out what's causing that yet and since it's already a known issue wanted to move forward with these changes before moving on to trying to help out with that one.

erin-doyle · 2017-09-14T21:40:54Z

src/components/CalendarMonth.jsx

+        >
+          <strong>{monthTitle}</strong>
+        </div>
+        <table role="presentation">


Added the role of presentation here to stop the screen reader from reading all of the details of the columns and rows of the table WAI-ARIA use of presentation role.

erin-doyle · 2017-09-14T21:47:31Z

src/components/CalendarMonth.jsx

+          className="CalendarMonth__caption js-CalendarMonth__caption"
+        >
+          <strong>{monthTitle}</strong>
+        </div>


Changed <caption> to <div> as apparently this is a structural type element that cues screen readers that this is a data vs layout type table (see NOTE on "layout tables" and the caption element here: H39: Using caption elements to associate data table captions with data tables).

See in DayPicker.jsx that it now points here for the aria-describedby and thus continues to read the title as it was before with the <caption>.

erin-doyle · 2017-09-14T21:49:28Z

src/components/DayPicker.jsx

@@ -825,6 +825,9 @@ export default class DayPicker extends React.Component {
      <div
        className={dayPickerClassNames}
        style={dayPickerStyle}
+        role="application"


Added a role of application here as it seemed to match up with the WAI-ARIA example shown here: For application landmarks with static prose.

erin-doyle · 2017-09-14T21:51:40Z

src/components/DayPickerRangeController.jsx

+
+    // initialize phrases
+    // set the appropriate CalendarDay phrase based on focusedInput
+    const chooseAvailableDate = getChooseAvailableDatePhrase(phrases, focusedInput);


It seemed like the chooseAvailableDate phrase was only getting set on update of the focus or the phrases so I thought adding the setting of it in componentDidMount would address initialization.

erin-doyle · 2017-09-14T21:52:34Z

test/components/SingleDatePickerInput_spec.jsx

@@ -21,41 +21,45 @@ describe('SingleDatePickerInput', () => {
  describe('clear date', () => {
    describe('props.showClearDate is falsey', () => {
      it('does not have .SingleDatePickerInput__clear-date class', () => {
-        const wrapper = shallow(<SingleDatePickerInput showClearDate={false} />);
+        const wrapper = shallow(<SingleDatePickerInput id="date" showClearDate={false} />);


All the changes here were just to clean up some prop type warnings that were bugging me. Hope that's ok!

coveralls · 2017-09-14T22:05:13Z

Coverage increased (+0.06%) to 86.558% when pulling d26aade on erin-doyle:a11y-improvements into c464d43 on airbnb:master.

majapw · 2017-09-15T16:51:49Z

This is awesome! I will take a look at the code today. :) Thanks!

FYI @backwardok

backwardok · 2017-09-15T17:37:58Z

src/components/DayPicker.jsx

@@ -825,6 +825,9 @@ export default class DayPicker extends React.Component {
      <div
        className={dayPickerClassNames}
        style={dayPickerStyle}
+        role="application"
+        aria-label="Calendar"


We should make sure that this is something that can be localized

backwardok · 2017-09-15T17:49:29Z

src/components/DayPicker.jsx

@@ -825,6 +825,9 @@ export default class DayPicker extends React.Component {
      <div
        className={dayPickerClassNames}
        style={dayPickerStyle}
+        role="application"
+        aria-label="Calendar"
+        aria-describedby="CalendarMonth__caption"


I'm curious if this could be confusing - since it will be most likely just read when the user comes to the calendar the first time, the description will always be the first month. They will come to an area that will read something like, "Calendar, application, September". Seems like a more appropriate description would be something like "Date picker"

Hmm well, going back and testing this again to be sure, it seems as if it's not even reading the aria-describedby. I must have assumed that when I heard "September 2017" it was the aria-describedby but really it was just the reading of the selected CalendarDay. Upon changing the text it's now more obvious that it's not being read. Perhaps it's just not supported by the browser screen reader combinations I'm testing on (Mac with VoiceOver on Chrome, Firefox, Safari)? I so far can't find a good reason for why it's not being read...

I wonder if it's really necessary or if the aria-label here and the aria-describedby on the input telling the user how to interact with the calendar is enough information? If you think a describedby is crucial here let me know and I'll keep working on why it's not being read, otherwise I can just remove it.

@majapw and @backwardok, I just wanted to bump this and ask again, what are your thoughts about whether to skip adding an aria-describedby on the role="application" div or to figure out why it's not being read and fix it. If the answer is to add it and fix it, what should the text be?

Sorry for the delay! I think it would be fine to bypass adding the aria-describedby here. If we were to have some sort of visual text that also explained the calendar, or there was text that explained some calendar instructions specifically for screen reader users, then that might be useful, but I think it's probably ok without it.

No worries! I have added the aria-label text to the localizable phrases and removed the aria-describedby.

As for having something visual and read out loud explaining the calendar instructions, there already is the keyboard shortcuts modal which is opened by entering ? or clicking on the ? at the bottom of the calendar. The instructions for getting to the keyboard shortcuts modal is read via the aria-describedby on the DateInput:

Press the down arrow key to interact with the calendar and select a date. Press the question mark key to get the keyboard shortcuts for changing dates.

So the instructions seem pretty clear and complete from my testing as a visually able, keyboard only, and screen-reader user.

coveralls · 2017-10-03T16:10:43Z

Coverage decreased (-0.07%) to 86.425% when pulling e63f0cd on erin-doyle:a11y-improvements into c464d43 on airbnb:master.

ljharb · 2017-10-03T16:13:19Z

@erin-doyle would you mind rebasing on the command line and force pushing, so as to remove any merge commits?

coveralls · 2017-10-03T16:18:32Z

Coverage increased (+0.07%) to 86.425% when pulling 9856f30 on erin-doyle:a11y-improvements into 0f6cccc on airbnb:master.

… to the table thus differentiating the table as being for layout purposes instead of data for the sake of screen readers.

…r with appropriate aria attributes to better assist screen readers.

…rase based on the focusedInput which previously was not being set unless the focusedInput or phrases had changed.

erin-doyle · 2017-10-03T17:30:50Z

oh jeez I forget to rebase every time... Ok it should be cleaned up now.

coveralls · 2017-10-03T17:38:01Z

Coverage increased (+0.07%) to 86.425% when pulling 479d6be on erin-doyle:a11y-improvements into 0f6cccc on airbnb:master.

coveralls · 2017-10-03T17:39:01Z

Coverage increased (+0.07%) to 86.425% when pulling 479d6be on erin-doyle:a11y-improvements into 0f6cccc on airbnb:master.

backwardok · 2017-10-05T00:23:18Z

src/components/DayPickerRangeController.jsx

@@ -174,6 +183,22 @@ export default class DayPickerRangeController extends React.Component {
    this.setDayPickerRef = this.setDayPickerRef.bind(this);
  }

+  componentDidMount() {


Since the determination of which phrase to show is based off of the props and doesn't rely on the actual state of the component (waiting for the input to have focus), you can do this logic in the constructor and set phrases to the new object in this.state.

If you put that logic in componentDidMount, which gets called after render, setting state will trigger another render.

(it'd then also need the same logic in componentWillReceiveProps)

Yep that totally makes sense, I've moved that logic to the constructor and removed componentDidMount. As for componentWillReceiveProps that logic was already there. The bit I added was to make sure that phrase was being initialized at component load time instead of having to wait until props changed.

…nentDidMount() and into the constructor to prevent extraneous rendering.

coveralls · 2017-10-05T20:24:49Z

Coverage increased (+0.04%) to 86.396% when pulling 92df2a6 on erin-doyle:a11y-improvements into 0f6cccc on airbnb:master.

coveralls · 2017-10-05T20:37:47Z

Coverage increased (+0.04%) to 86.396% when pulling 0f00b97 on erin-doyle:a11y-improvements into 0f6cccc on airbnb:master.

majapw

Looks great to me!

majapw · 2017-10-05T20:41:21Z

src/components/DayPickerRangeController.jsx

@@ -132,6 +132,15 @@ const defaultProps = {
  isRTL: false,
 };

+const getChooseAvailableDatePhrase = (phrases, focusedInput) => {


slightly prefer:

function getChooseAvailableDatePhrase(phrases, focusedInput) { ... }

over an arrow function but not a blocker

erin-doyle commented Sep 14, 2017

View reviewed changes

backwardok reviewed Sep 15, 2017

View reviewed changes

Erin Doyle added 6 commits October 3, 2017 13:27

Changed the <caption> tag to a <div> and added a role of presentation…

38ebd1a

… to the table thus differentiating the table as being for layout purposes instead of data for the sake of screen readers.

Added a role of application to the containing element of the DayPicke…

d3318cb

…r with appropriate aria attributes to better assist screen readers.

Cleaned up some prop type warnings

fb817d3

Cleaned up some more prop type warnings

c0b3abb

Added a componentDidMount that initializes the chooseAvailableDate ph…

2f6b34a

…rase based on the focusedInput which previously was not being set unless the focusedInput or phrases had changed.

Added the Calendar application label to the localizable phrases.

479d6be

ljharb assigned backwardok Oct 3, 2017

backwardok reviewed Oct 5, 2017

View reviewed changes

Moved the initializing of the chooseAvailableDate phrase out of compo…

92df2a6

…nentDidMount() and into the constructor to prevent extraneous rendering.

Updated tests

0f00b97

ljharb approved these changes Oct 5, 2017

View reviewed changes

majapw approved these changes Oct 5, 2017

View reviewed changes

majapw merged commit 9f48d0e into react-dates:master Oct 5, 2017

erin-doyle deleted the a11y-improvements branch October 6, 2017 00:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some improvements for screen readers [a11y] #715

Some improvements for screen readers [a11y] #715

erin-doyle commented Sep 14, 2017

erin-doyle Sep 14, 2017

erin-doyle Sep 14, 2017

erin-doyle Sep 14, 2017

erin-doyle Sep 14, 2017

erin-doyle Sep 14, 2017

coveralls commented Sep 14, 2017

majapw commented Sep 15, 2017

backwardok Sep 15, 2017

backwardok Sep 15, 2017

erin-doyle Sep 16, 2017

erin-doyle Sep 27, 2017

backwardok Oct 3, 2017

erin-doyle Oct 3, 2017

coveralls commented Oct 3, 2017

ljharb commented Oct 3, 2017

coveralls commented Oct 3, 2017

erin-doyle commented Oct 3, 2017

coveralls commented Oct 3, 2017

coveralls commented Oct 3, 2017

backwardok Oct 5, 2017

ljharb Oct 5, 2017

erin-doyle Oct 5, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

majapw left a comment

majapw Oct 5, 2017

Some improvements for screen readers [a11y] #715

Some improvements for screen readers [a11y] #715

Conversation

erin-doyle commented Sep 14, 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

coveralls commented Sep 14, 2017

majapw commented Sep 15, 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

Choose a reason for hiding this comment

coveralls commented Oct 3, 2017

ljharb commented Oct 3, 2017

coveralls commented Oct 3, 2017

erin-doyle commented Oct 3, 2017

coveralls commented Oct 3, 2017

coveralls commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment