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

Add ARIA attributes for better accessibility #33

Closed
basham opened this Issue Jul 28, 2015 · 18 comments

Comments

5 participants
@basham

basham commented Jul 28, 2015

While react-day-picker works with the keyboard, its semantics could be improved by adding the appropriate ARIA attributes, so screen readers will better interpret how the component works.

Probably the best React exemplar I've found which achieves this nicely is react-widgets Calendar. However, I see the customizability of the react-day-picker project being much more ideal. I really like the clean, small ES6 implementation with no dependencies outside of React.

The following are some suggestions based on my research. Some suggestions are a result of adding back certain semantics which were lost due to switching from <table> to <div>. Suggestions are organized by the element to which it affects, identified by its class name.

.DayPicker-Month

Authors MUST ensure that elements with role gridcell are owned by elements with role row, which in turn are owned by an element with role rowgroup, grid or treegrid.

A grid is considered editable unless otherwise specified. To make a grid read-only, set the aria-readonly attribute of the grid to true. The value of the grid element's aria-readonly attribute is implicitly propagated to all of its owned gridcell elements, and will be exposed through the accessibility API.

.DayPicker-Caption

  • Add unique id to work with .DayPicker-Month enhancements.

.DayPicker-Weekdays

.DayPicker-Weekday

.DayPicker-Body

.DayPicker-Day

gridcell elements with the aria-selected attribute set can be selected for user interaction

Additional considerations

Since aria-labelledby and aria-activedescendant reference an id, it may be good to dynamically generate unique ids for elements they reference, so you avoid the problem of having different instances of react-day-picker accidentally cross-reference each other.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jul 29, 2015

Owner

@basham thanks for taking the time to write this, it is really helpful 👍. Many suggestions are trivial to implement (see unit test below).

For supporting aria-selected and aria-disabled I need to treat selected and disabled as "special" modifiers: I'll open a separate issue for this.

title or aria-label attributes raise a problem with localization. "Today" is a default modifier but developers may want to implement other labels, such as "Bookable" or "Free" or whatever. I need to think about it, maybe adding labels prop...

let captionElements = getElementsByClassName(dayPickerNode, "DayPicker-Caption");
let monthElements = getElementsByClassName(dayPickerNode, "DayPicker-Month");
let weekdaysElements = getElementsByClassName(dayPickerNode, "DayPicker-Weekdays");
let weekdayElements = getElementsByClassName(dayPickerNode, "DayPicker-Weekday");
let bodyElements = getElementsByClassName(dayPickerNode, "DayPicker-Body");
let weekElements = getElementsByClassName(dayPickerNode, "DayPicker-Week");
let dayElements = getElementsByClassName(dayPickerNode, "DayPicker-Day");

expect(dayPickerNode.getAttribute("role")).to.equal("widget");
expect(monthElements.every(el => el.getAttribute("role") === "grid")).to.be.true;
expect(monthElements.every(el => el.getAttribute("aria-readonly"))).to.equal("true");

monthElements.forEach((el, i) => {
  let captionId = captionElements[i].getAttribute("id");
  expect(el.getAttribute("aria-labelledby")).to.equal(captionId);
});

expect(weekdaysElements.every(el => el.getAttribute("role"))).to.equal("rowgroup");
expect(weekdayElements.every(el => el.getAttribute("role") === "columnheader")).to.be.true;
expect(bodyElements.every(el => el.getAttribute("role") === "rowgroup")).to.be.true;
expect(weekElements.every(el => el.getAttribute("role") === "row")).to.be.true;
expect(dayElements.every(el => el.getAttribute("role") === "gridcell")).to.be.true;
Owner

gpbl commented Jul 29, 2015

@basham thanks for taking the time to write this, it is really helpful 👍. Many suggestions are trivial to implement (see unit test below).

For supporting aria-selected and aria-disabled I need to treat selected and disabled as "special" modifiers: I'll open a separate issue for this.

title or aria-label attributes raise a problem with localization. "Today" is a default modifier but developers may want to implement other labels, such as "Bookable" or "Free" or whatever. I need to think about it, maybe adding labels prop...

let captionElements = getElementsByClassName(dayPickerNode, "DayPicker-Caption");
let monthElements = getElementsByClassName(dayPickerNode, "DayPicker-Month");
let weekdaysElements = getElementsByClassName(dayPickerNode, "DayPicker-Weekdays");
let weekdayElements = getElementsByClassName(dayPickerNode, "DayPicker-Weekday");
let bodyElements = getElementsByClassName(dayPickerNode, "DayPicker-Body");
let weekElements = getElementsByClassName(dayPickerNode, "DayPicker-Week");
let dayElements = getElementsByClassName(dayPickerNode, "DayPicker-Day");

expect(dayPickerNode.getAttribute("role")).to.equal("widget");
expect(monthElements.every(el => el.getAttribute("role") === "grid")).to.be.true;
expect(monthElements.every(el => el.getAttribute("aria-readonly"))).to.equal("true");

monthElements.forEach((el, i) => {
  let captionId = captionElements[i].getAttribute("id");
  expect(el.getAttribute("aria-labelledby")).to.equal(captionId);
});

expect(weekdaysElements.every(el => el.getAttribute("role"))).to.equal("rowgroup");
expect(weekdayElements.every(el => el.getAttribute("role") === "columnheader")).to.be.true;
expect(bodyElements.every(el => el.getAttribute("role") === "rowgroup")).to.be.true;
expect(weekElements.every(el => el.getAttribute("role") === "row")).to.be.true;
expect(dayElements.every(el => el.getAttribute("role") === "gridcell")).to.be.true;

This was referenced Jul 29, 2015

@basham

This comment has been minimized.

Show comment
Hide comment
@basham

basham Jul 29, 2015

Figured most of these suggestions would be really easy to implement. Thanks for the quick work on this.

I also assumed that aria-disabled, aria-selected, title and aria-label would need some additional work and experimentation. Glad to see those issues are getting special attention.

basham commented Jul 29, 2015

Figured most of these suggestions would be really easy to implement. Thanks for the quick work on this.

I also assumed that aria-disabled, aria-selected, title and aria-label would need some additional work and experimentation. Glad to see those issues are getting special attention.

@basham

This comment has been minimized.

Show comment
Hide comment
@basham

basham Jul 30, 2015

I retract my suggestion of having aria-activedescendant on .DayPicker-Month. Turns out that approach would be most appropriate if you would leave focus only on .DayPicker[role="widget"] and it would never move elsewhere in the widget.

Since you're already managing focus on widget descendants, it's probably best to just leave the code as-is. Additionally, focus is a more established feature than aria-activedescendant, so what you've already done is a more backwards compatible approach.

References

basham commented Jul 30, 2015

I retract my suggestion of having aria-activedescendant on .DayPicker-Month. Turns out that approach would be most appropriate if you would leave focus only on .DayPicker[role="widget"] and it would never move elsewhere in the widget.

Since you're already managing focus on widget descendants, it's probably best to just leave the code as-is. Additionally, focus is a more established feature than aria-activedescendant, so what you've already done is a more backwards compatible approach.

References

@gpbl gpbl added the new feature label Jul 30, 2015

@gpbl gpbl added this to the v2.0.0 milestone Aug 2, 2015

@gpbl gpbl added the a11y label Aug 2, 2015

@ezufelt

This comment has been minimized.

Show comment
Hide comment
@ezufelt

ezufelt Oct 11, 2015

Would be happy to test a PR with a screen-reader.

ezufelt commented Oct 11, 2015

Would be happy to test a PR with a screen-reader.

@gpbl gpbl removed this from the v2.0.0 milestone Nov 3, 2015

@davidgilbertson

This comment has been minimized.

Show comment
Hide comment
@davidgilbertson

davidgilbertson Nov 24, 2015

I'm sure you know already, but you can easily test the compliance here: https://validator.w3.org/nu/#textarea

At the moment it's the only thing stopping our site from passing WCAG AA, so looking forward to this one!

(BTW, great component)

davidgilbertson commented Nov 24, 2015

I'm sure you know already, but you can easily test the compliance here: https://validator.w3.org/nu/#textarea

At the moment it's the only thing stopping our site from passing WCAG AA, so looking forward to this one!

(BTW, great component)

@gpbl gpbl added the help wanted label Nov 25, 2015

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Nov 25, 2015

Owner

Hi @davidgilbertson yes thank you for the link!
I won't forget this issue... I'm just a bit overwhelmed by the quantity of things i've to read and pay attention to :-)

Owner

gpbl commented Nov 25, 2015

Hi @davidgilbertson yes thank you for the link!
I won't forget this issue... I'm just a bit overwhelmed by the quantity of things i've to read and pay attention to :-)

@davidgilbertson

This comment has been minimized.

Show comment
Hide comment
@davidgilbertson

davidgilbertson Nov 25, 2015

If I am able to get this change set up as a task in my project, would you consider a pull request?

davidgilbertson commented Nov 25, 2015

If I am able to get this change set up as a task in my project, would you consider a pull request?

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Nov 25, 2015

Owner

I would really appreciate any help, @davidgilbertson!

Owner

gpbl commented Nov 25, 2015

I would really appreciate any help, @davidgilbertson!

@davidgilbertson

This comment has been minimized.

Show comment
Hide comment
@davidgilbertson

davidgilbertson Nov 25, 2015

OK it looks like I can get this as a task in my next sprint. To be clear I plan to fix these three issues thrown by the w3 validator:

  • Bad value widget for attribute role on element div.
  • Element attr not allowed as child of element div in this context.
  • An element with role=row must be contained in, or owned by, an element with role=treegrid or role=grid or role=rowgroup.

Should come up a few weeks from now.

davidgilbertson commented Nov 25, 2015

OK it looks like I can get this as a task in my next sprint. To be clear I plan to fix these three issues thrown by the w3 validator:

  • Bad value widget for attribute role on element div.
  • Element attr not allowed as child of element div in this context.
  • An element with role=row must be contained in, or owned by, an element with role=treegrid or role=grid or role=rowgroup.

Should come up a few weeks from now.

@davidgilbertson

This comment has been minimized.

Show comment
Hide comment
@davidgilbertson

davidgilbertson Nov 25, 2015

I'm interested, what is <attr> in the source? I've never seen that before and can't find any info on it.

davidgilbertson commented Nov 25, 2015

I'm interested, what is <attr> in the source? I've never seen that before and can't find any info on it.

gpbl added a commit that referenced this issue Nov 25, 2015

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Nov 25, 2015

Owner

OMG @davidgilbertson I think that attr was meant abbr 😭 fixed with #106! Great you've found it!

Owner

gpbl commented Nov 25, 2015

OMG @davidgilbertson I think that attr was meant abbr 😭 fixed with #106! Great you've found it!

@davidgilbertson

This comment has been minimized.

Show comment
Hide comment
@davidgilbertson

davidgilbertson Nov 25, 2015

Happens to the best of us. :)

davidgilbertson commented Nov 25, 2015

Happens to the best of us. :)

gpbl added a commit that referenced this issue Nov 26, 2015

Merge pull request #106 from gpbl/attr
Correct attr → abbr (#33)
@ragesoss

This comment has been minimized.

Show comment
Hide comment
@ragesoss

ragesoss Mar 1, 2016

Hi! This is just to say that my project has really appreciate this react-day-picker, and we're looking forward to improvements in accessibility. One of my colleagues uses a screen reader, and cannot use the calendar in our app. There are, in particular, two issues for screen readers:

  • Day cells are not clickable
  • There is no indication of selected/not-selected/disabled state.
    • This is what adding the aria-selected attributes would do.

Echoing @ezufelt above, I'm happy to help test any patches that might fix those issues.

ragesoss commented Mar 1, 2016

Hi! This is just to say that my project has really appreciate this react-day-picker, and we're looking forward to improvements in accessibility. One of my colleagues uses a screen reader, and cannot use the calendar in our app. There are, in particular, two issues for screen readers:

  • Day cells are not clickable
  • There is no indication of selected/not-selected/disabled state.
    • This is what adding the aria-selected attributes would do.

Echoing @ezufelt above, I'm happy to help test any patches that might fix those issues.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Mar 1, 2016

Owner

Thanks @ragesoss! There's a PR from @limscoder that may add some improvements for ARIA, but I'm not sure if it fixes all the issues. Going to check it out soon.

Owner

gpbl commented Mar 1, 2016

Thanks @ragesoss! There's a PR from @limscoder that may add some improvements for ARIA, but I'm not sure if it fixes all the issues. Going to check it out soon.

@davidgilbertson

This comment has been minimized.

Show comment
Hide comment
@davidgilbertson

davidgilbertson Mar 1, 2016

FWIW...

After thinking about this for a while, if I couldn't see (or was a keyboard navigator), I would much rather just type a date than navigate a calendar, so we are aria-hiding the calendar and just having an input field to type date.

davidgilbertson commented Mar 1, 2016

FWIW...

After thinking about this for a while, if I couldn't see (or was a keyboard navigator), I would much rather just type a date than navigate a calendar, so we are aria-hiding the calendar and just having an input field to type date.

@ragesoss

This comment has been minimized.

Show comment
Hide comment
@ragesoss

ragesoss Mar 1, 2016

@davidgilbertson That makes sense for picking a single date — I know that's what my colleague Helaine prefers when it's a matter of one date — but will it work for a calendar that is being used to select multiple dates? In my app, we use it with three states and lots of selected dates at once.
day picker

ragesoss commented Mar 1, 2016

@davidgilbertson That makes sense for picking a single date — I know that's what my colleague Helaine prefers when it's a matter of one date — but will it work for a calendar that is being used to select multiple dates? In my app, we use it with three states and lots of selected dates at once.
day picker

@basham

This comment has been minimized.

Show comment
Hide comment
@basham

basham Mar 1, 2016

@davidgilbertson I work for Indiana University, and consulting with our assistive technologies team, they say the same thing. A text input is much simpler to use for a keyboard user. And a text input is much easier to understand for those navigating with only a screen reader.

Assuming that's the case and assuming the intention is to select one date at a time, I think it's less imperative that this date picker can be controlled with a keyboard. I think we need to think about what is the best way to provide equivalent controls with a text input. Perhaps this text control is another project. Maybe it is a sub-component of this project.

My current thought is that Facebook's date input for events is a rather fantastic exemplar. The up/down arrow keys toggles through the month/day/year based on the placement of the keyboard cursor, providing very quick non-calendar-based input.

Facebook's event date text input

basham commented Mar 1, 2016

@davidgilbertson I work for Indiana University, and consulting with our assistive technologies team, they say the same thing. A text input is much simpler to use for a keyboard user. And a text input is much easier to understand for those navigating with only a screen reader.

Assuming that's the case and assuming the intention is to select one date at a time, I think it's less imperative that this date picker can be controlled with a keyboard. I think we need to think about what is the best way to provide equivalent controls with a text input. Perhaps this text control is another project. Maybe it is a sub-component of this project.

My current thought is that Facebook's date input for events is a rather fantastic exemplar. The up/down arrow keys toggles through the month/day/year based on the placement of the keyboard cursor, providing very quick non-calendar-based input.

Facebook's event date text input

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Mar 1, 2016

Owner

@ragesoss is right saying that for multiple days input fields would not be enough.
I think we can't escape it, this component must support users adopting assistive technologies.

There are already three pull requests (two of them are rather old) waiting to be analyzed and merged. It seems, however, they require a good amount of work 😅 I'm committed to work again on this soon!

Owner

gpbl commented Mar 1, 2016

@ragesoss is right saying that for multiple days input fields would not be enough.
I think we can't escape it, this component must support users adopting assistive technologies.

There are already three pull requests (two of them are rather old) waiting to be analyzed and merged. It seems, however, they require a good amount of work 😅 I'm committed to work again on this soon!

@gpbl gpbl modified the milestone: v2 Mar 2, 2016

@gpbl gpbl removed this from the v2 milestone May 16, 2016

@gpbl gpbl added this to the v3 milestone Oct 9, 2016

@gpbl gpbl modified the milestones: v4, v5 Feb 10, 2017

@gpbl gpbl closed this Feb 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment