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

[WIP] Some accessibility fixes for modals #1034

Closed
wants to merge 7 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@toolness
Contributor

toolness commented Jun 16, 2015

This makes some a11y fixes for our modals (some specific to club modals):

  • The close box for our modals was just being announced as "button"; now it's "Close button".
  • The "a network error occurred" and "your submission has some problems" alerts on our modals aren't announced by screen readers because they don't have role="alert" on them, which is now fixed. Also, NVDA doesn't seem to announce any <ul>'s within alerts, so I had to convert the lists to paragraphs. Which seems dumb, but I guess it's better than not announcing them at all?
  • The "Add Your Club" button on the Clubs page was a link instead of a <button>, and because it didn't have a href attribute, it wasn't focusable with the keyboard. I changed it to a <button>.
  • The club location autocomplete field now has a <label> associated with it.
  • Changes between individual steps of the modal, e.g. going from the "add your club" form step to the "thanks for your submission" step, weren't announced by screen readers. We now define ARIA Live Regions so this is fixed.
  • The way input focus was being handled between opening and closing the modal, and navigating between the individual steps in a modal, was terrible. I had no idea where my input focus was during these transitions, and often it was in an invalid place, e.g. still on the "Add My Club" button when the "Add Your Club" modal is in focus. I've fixed this so that, at the very least, the modal itself becomes focused when shown, and the DOM element that had focus before it was shown is re-focused when the modal closes. Also, a StepView component for multi-step modals has been added, which manages the auto-focusing of elements as users transition into a new step.

Other notes:

  • The club location autocomplete field isn't very accessible to screen-readers, though at least it's fully usable via keyboard. Not really sure if this can be fixed without altering react-select and there don't seem to be any issues in that project about accessibility, aside from the delete button being pronounced "times".

Things still left to do:

  • I should add unit tests for some of this stuff, but I'm holding off on it until we've done some QA and verified that this is the behavior we want.

@toolness toolness referenced this pull request Jun 16, 2015

Closed

[ Pledge Workflow Part 2 ] Build modals #1015

0 of 3 tasks complete
@toolness

This comment has been minimized.

Show comment
Hide comment
@toolness

toolness Jun 16, 2015

Contributor

@mmmavis can you take a look at this using VoiceOver and verify that these changes work OK? I've only tested on NVDA so far and could use a second opinion.

Contributor

toolness commented Jun 16, 2015

@mmmavis can you take a look at this using VoiceOver and verify that these changes work OK? I've only tested on NVDA so far and could use a second opinion.

@mmmavis

This comment has been minimized.

Show comment
Hide comment
@mmmavis

mmmavis Jun 16, 2015

Member

yup yup! should i check out this PR or the develop branch?

Member

mmmavis commented Jun 16, 2015

yup yup! should i check out this PR or the develop branch?

@toolness

This comment has been minimized.

Show comment
Hide comment
@toolness

toolness Jun 17, 2015

Contributor

oh this pr please. you can compare how it works on screenreaders/with keyboard against staging if you like.

Contributor

toolness commented Jun 17, 2015

oh this pr please. you can compare how it works on screenreaders/with keyboard against staging if you like.

@toolness toolness referenced this pull request Jun 18, 2015

Merged

Pledge modals for the pledge workflow on Homepage #1039

0 of 2 tasks complete
@toolness

This comment has been minimized.

Show comment
Hide comment
@toolness

toolness Jun 19, 2015

Contributor

Hey any chance you could get around to testing this on VoiceOver soon? If you think the fixes are usable then I will bust up unit tests!

Contributor

toolness commented Jun 19, 2015

Hey any chance you could get around to testing this on VoiceOver soon? If you think the fixes are usable then I will bust up unit tests!

@mmmavis

This comment has been minimized.

Show comment
Hide comment
@mmmavis

mmmavis Jun 19, 2015

Member

@toolness oh yea sorry, checking it out now

Member

mmmavis commented Jun 19, 2015

@toolness oh yea sorry, checking it out now

@mmmavis

This comment has been minimized.

Show comment
Hide comment
@mmmavis

mmmavis Jun 19, 2015

Member

This makes some a11y fixes for our modals (some specific to club modals):

  • The close box for our modals was just being announced as "button"; now it's "Close button".

👍 with VoiceOver

  • The "a network error occurred" and "your submission has some problems" alerts on our modals aren't announced by screen readers because they don't have role="alert" on them, which is now fixed. Also, NVDA doesn't seem to announce any <ul>'s within alerts, so I had to convert the lists to paragraphs. Which seems dumb, but I guess it's better than not announcing them at all?

VoiceOver reads it as "Window" and I can't get inside of the div 😞 It's the same if I change them back to list form.

  • The "Add Your Club" button on the Clubs page was a link instead of a <button>, and because it didn't have a href attribute, it wasn't focusable with the keyboard. I changed it to a <button>.
  • The club location autocomplete field now has a <label> associated with it.
  • Changes between individual steps of the modal, e.g. going from the "add your club" form step to the "thanks for your submission" step, weren't announced by screen readers. We now define ARIA Live Regions so this is fixed.

After the form has been successfully submitted, VoiceOver reads the button text "Take Me To My Club" and skips everything else. 😕

  • The way input focus was being handled between opening and closing the modal, and navigating between the individual steps in a modal, was terrible. I had no idea where my input focus was during these transitions, and often it was in an invalid place, e.g. still on the "Add My Club" button when the "Add Your Club" modal is in focus. I've fixed this so that, at the very least, the modal itself becomes focused when shown, and the DOM element that had focus before it was shown is re-focused when the modal closes. Also, a StepView component for multi-step modals has been added, which manages the auto-focusing of elements as users transition into a new step.

Works 👍 with VoiceOver and keyboard.

Member

mmmavis commented Jun 19, 2015

This makes some a11y fixes for our modals (some specific to club modals):

  • The close box for our modals was just being announced as "button"; now it's "Close button".

👍 with VoiceOver

  • The "a network error occurred" and "your submission has some problems" alerts on our modals aren't announced by screen readers because they don't have role="alert" on them, which is now fixed. Also, NVDA doesn't seem to announce any <ul>'s within alerts, so I had to convert the lists to paragraphs. Which seems dumb, but I guess it's better than not announcing them at all?

VoiceOver reads it as "Window" and I can't get inside of the div 😞 It's the same if I change them back to list form.

  • The "Add Your Club" button on the Clubs page was a link instead of a <button>, and because it didn't have a href attribute, it wasn't focusable with the keyboard. I changed it to a <button>.
  • The club location autocomplete field now has a <label> associated with it.
  • Changes between individual steps of the modal, e.g. going from the "add your club" form step to the "thanks for your submission" step, weren't announced by screen readers. We now define ARIA Live Regions so this is fixed.

After the form has been successfully submitted, VoiceOver reads the button text "Take Me To My Club" and skips everything else. 😕

  • The way input focus was being handled between opening and closing the modal, and navigating between the individual steps in a modal, was terrible. I had no idea where my input focus was during these transitions, and often it was in an invalid place, e.g. still on the "Add My Club" button when the "Add Your Club" modal is in focus. I've fixed this so that, at the very least, the modal itself becomes focused when shown, and the DOM element that had focus before it was shown is re-focused when the modal closes. Also, a StepView component for multi-step modals has been added, which manages the auto-focusing of elements as users transition into a new step.

Works 👍 with VoiceOver and keyboard.

toolness added some commits Jun 29, 2015

Revert "add AccessibleAlert component."
This reverts commit 8720213.

The AccessibleAlert component makes things work with VoiceOver in
OS X Safari and Chrome, but then *breaks* things in NVDA because
it keeps announcing the empty alert instead of the dialog contents.

It's easier to just keep the code as simple as possible and rely on
UA vendors to make role="alert" work in a consistent way that doesn't
drive developers insane.
@toolness

This comment has been minimized.

Show comment
Hide comment
@toolness

toolness Jun 29, 2015

Contributor

Ok, so role="alert" is extremely frustrating because it works so differently across different browser/screenreader combinations that getting it to work in one makes it break in another. In 8720213 I implemented a weird, kludgy work-around that got things to work on OS X Safari+Chrome, but that broke things in NVDA on Windows, so I reverted the changes in 13142d4, as it's easier to just keep the code as simple as possible and rely on UA vendors to make role="alert" work in a consistent way that doesn't drive developers insane.

(Side note: for really annoying cross-browser issues like this, we can at least use UA detection as a last resort, but I don't think we can here, since there doesn't appear to be a way to detect what screen reader is being used.)

Since the current PR doesn't degrade VoiceOver performance but improves NVDA performance, I think we should leave the behavior as-is and just add unit tests for what we've got here and land these changes. What do you think @mmmavis ?

Contributor

toolness commented Jun 29, 2015

Ok, so role="alert" is extremely frustrating because it works so differently across different browser/screenreader combinations that getting it to work in one makes it break in another. In 8720213 I implemented a weird, kludgy work-around that got things to work on OS X Safari+Chrome, but that broke things in NVDA on Windows, so I reverted the changes in 13142d4, as it's easier to just keep the code as simple as possible and rely on UA vendors to make role="alert" work in a consistent way that doesn't drive developers insane.

(Side note: for really annoying cross-browser issues like this, we can at least use UA detection as a last resort, but I don't think we can here, since there doesn't appear to be a way to detect what screen reader is being used.)

Since the current PR doesn't degrade VoiceOver performance but improves NVDA performance, I think we should leave the behavior as-is and just add unit tests for what we've got here and land these changes. What do you think @mmmavis ?

@toolness

This comment has been minimized.

Show comment
Hide comment
@toolness

toolness Jul 13, 2015

Contributor

Hey @mmmavis, any idea when you might be able to get to this?

Contributor

toolness commented Jul 13, 2015

Hey @mmmavis, any idea when you might be able to get to this?

@mmmavis

This comment has been minimized.

Show comment
Hide comment
@mmmavis

mmmavis Jul 13, 2015

Member

Sorry @toolness I missed your ping earlier. I will get to this today :bowtie: (btw, is this still WIP? 😁 )

Member

mmmavis commented Jul 13, 2015

Sorry @toolness I missed your ping earlier. I will get to this today :bowtie: (btw, is this still WIP? 😁 )

@toolness

This comment has been minimized.

Show comment
Hide comment
@toolness

toolness Jul 13, 2015

Contributor

Yeah, it's still WIP, I just wanted to know what your opinion on my last comment was. If you're ok w/ the current behavior then I will start writing tests!

Contributor

toolness commented Jul 13, 2015

Yeah, it's still WIP, I just wanted to know what your opinion on my last comment was. If you're ok w/ the current behavior then I will start writing tests!

@mmmavis

This comment has been minimized.

Show comment
Hide comment
@mmmavis

mmmavis Jul 13, 2015

Member

cool, i'll check this out after the standups.

Member

mmmavis commented Jul 13, 2015

cool, i'll check this out after the standups.

@mmmavis

This comment has been minimized.

Show comment
Hide comment
@mmmavis

mmmavis Jul 13, 2015

Member

Hey @toolness , thanks for sharing links to those useful posts. 😉 It's very challenging to make a11y work across browsers/platform/screen readers so I'm fine with your solution here. (thanks for working on this!)

I know we haven't decided on our browser support but a11y-wise I'm wondering if we should focus on just a few platform combos(e.g., FF on Mac OSX with VoiceOver, Chrome on Windows with NVDA... something like that). It's 😞 to say this but if we are too committed to making the site widely accessible I'm worried it will lead to dev overhead. 😶

Member

mmmavis commented Jul 13, 2015

Hey @toolness , thanks for sharing links to those useful posts. 😉 It's very challenging to make a11y work across browsers/platform/screen readers so I'm fine with your solution here. (thanks for working on this!)

I know we haven't decided on our browser support but a11y-wise I'm wondering if we should focus on just a few platform combos(e.g., FF on Mac OSX with VoiceOver, Chrome on Windows with NVDA... something like that). It's 😞 to say this but if we are too committed to making the site widely accessible I'm worried it will lead to dev overhead. 😶

@Pomax

This comment has been minimized.

Show comment
Hide comment
@Pomax

Pomax Feb 15, 2016

Collaborator

closing, as the codebase has passed this PR by significantly.

Collaborator

Pomax commented Feb 15, 2016

closing, as the codebase has passed this PR by significantly.

@Pomax Pomax closed this Feb 15, 2016

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