Skip to content
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

Accessibility improvements #11386

Closed
janrg opened this issue Apr 26, 2017 · 21 comments
Closed

Accessibility improvements #11386

janrg opened this issue Apr 26, 2017 · 21 comments
Labels
ionitron: v3 moves the issue to the ionic-v3 repository

Comments

@janrg
Copy link

janrg commented Apr 26, 2017

Ionic version: (check one with "x")
[X] 3.x

I would like to try to contribute some accessibility improvements to ionic. So far I've identified a few issues that I intend to work on:

  • The focus management for certain elements such as toasts, loading elements and alerts seems to make it impossible for screen readers to deal with these properly. Even if focus is manually directed to the element, focus is immediately shifted to the body element interrupting the reading of the element
  • There should be an option to change the role of these items from dialog to alert depending on what they are used for. Alert component should be alertdialog by default
  • Alerts should be focus traps while active
  • Icons in the menu should be aria-hidden
  • If there is a menu button, it should perhaps have aria ownership of the menu it opens

Related issues:
#11309 #11181 #9808

@jgw96 jgw96 added the v2 label Apr 27, 2017
@jgw96
Copy link
Contributor

jgw96 commented Apr 27, 2017

Hello! This sounds awesome, cant wait for the PR's! Let us know if you need any guidance.

@awebdeveloper
Copy link

wave tool also complains about buttons without a text like hambuger button and back button

@janrg
Copy link
Author

janrg commented Apr 28, 2017

@awebdeveloper Noted, thanks.

Edit: removed descriptions of changes, as the actual modifications I've made are described below.

@janrg
Copy link
Author

janrg commented May 7, 2017

@awebdeveloper Under what circumstances have you seen the warnings about buttons? It seems to me that ion-icons automatically get an aria-label corresponding to the name of the icon.

@janrg
Copy link
Author

janrg commented May 7, 2017

Alright, I think I'm mostly done with what I had set out to do. Still doing a little more testing and then I'll make a pull request. Here's an overview of what I've done.

General

Focus Traps

I've added three functions to platform.ts:

trapFocus(HTMLElement) will trap keyboard focus within an element by identifying the first and last tab-able element in the element and looping around. Since it attached an eventlistener to the element, it is undone automatically when the element is destroyed and doesn't require cleanup

trapVirtualCursor(HTMLElement) will prevent a screen reader's virtual cursor from moving away from the element. It identifies all sibling elements of the element and sets those that are not aria-hidden to aria-hidden and additionally sets a data-traphide attribute to true (so we don't accidentally remove aria-hidden from an element during cleanup that had it before). The function could easily be extended to recursively walk up the DOM hierarchy, but given where menus and overlays appear in the DOM, I don't think it's necessary. There is a small inconvenience in that the virtual focus can still reach all parent elements of the current element but I think that is inevitable.

untrapVirtualCursor(HTMLElement) releases the above virtual cursor trap

Note that once a focus trap is destroyed, focus should always return to the element that had focus before, as long as it still exists.

Roles

Three of the roles specified by WAI-ARIA seem to be relevant for the overlays I have been looking at. Some parts of what I'm writing below are somewhat subjective.

alert elements will be read out immediately when they are created / become visible and should not require any interaction.

alertdialog elements are also read out immediately but do require interaction. This interaction should however be fairly minimal (typically one, perhaps two buttons, it's a slightly fuzzy boundary). Focus should be directed to an active element inside the alertdialog and confined to within the element while it is active.

dialog elements are not read out automatically when they appear. As I understand, labels of the dialog should technically be read out as soon as a child element receives focus, but in practice this does not happen. The most elegant solution I have found after looking at many implementation examples was to give initial focus to the element itself (-> it needs to have tabindex="-1"). Focus should be confined within the dialog while it is active.

I had to make a slightly heavier modification than expected to the markup most of the overlays, because in the current implementation, the "role" attribute sits on the outermost element of the modal (e.g. ), which contains as its first child the backdrop. If we set the focus to this element, as is required for role="dialog", a rather awkward situation arises. At least on android, TalkBack will announce the element followed by "double tap to activate". If a user follows the instruction, this is executed as a click on the backdrop which dismisses the element... So I have instead moved the relevant attributes on element in - to the -wrapper div which has the backdrop as a sibling rather than a child.

Hardware Back Button

This is only relevant for android and windows, but I added back button functionality to dismiss the modals. Tapping the back button is in this case equivalent to tapping the backdrop, so it will only dismiss the modal if enableBackDropDismiss is not set to false.

Components

Action-Sheet

The action sheet only required the mentioned general modifications (moving role attribute etc. inside by one element, storing and restoring the previously active element, trapping focus, and enabling the back button). Role remains as "dialog". The action-sheet interface for the ion-select required no further modifications.

Alert

Aside from the general modifications, the alert has an additional parameter "alertDialogRole" which defaults to "alertdialog". It can be set to dialog instead, in which case it will behave as described for dialog above instead. Since as mentioned above the line between simple and more complicated action is somewhat fuzzy, I felt that this should be user-configurable rather than automatic, with the exception of the alert ion-select interface which will always have several buttons and automatically uses the "dialog" role.

Loading

The loading element required very little modification. The role is set to alert and the spinner (if present) to aria-hidden="true". While no interaction is required, focus is still confined to the element while it was active because a) there should be no interaction with the rest of the page during that time and b) the restoring of focus to the previously active element (or the body element if the previously active one doesn't exist anymore) serves as a notice that loading has completed. The only downside of directing focus to the alert is that the announcement will be followed by "double tap to activate" but that seems preferable to allowing interaction with the site while loading is active.

Modal

The modal is complicated because there are no limits to what it can contain. I have given it a "dialog" role. I have added an additional optional parameter "ariaLabel" that will be read out once the modal receives focus. What happens if that has not been specified depends on the page structure but for a well-formatted page should be the page title.

Popover

The popover was the most awkward one to implement. It comes with the same issues as the modal, but has the additional complication that it does not necessarily have to implement a back button. Initiating a backdrop dismiss on a popover while using a screen reader was already very difficult before, but the focus trap makes it even harder. I ended up implementing an invisible button as the first child of the wrapper which will dismiss the element. The button will only be there if enableBackdropDismiss = "true" and has a default label of "Tap to dismiss" which can be changed via the "dismissLabel" parameter I have added. Activating the button wasn't possible if it was moved off screen or covered by another element, so it is now 1x1px and transparent.
As for the modal, there is also a new "ariaLabel" parameter.
The popover interface of the ion-select works fairly well with these changes, the only additional modification I have made is to set the ion-label of each radio to aria-hidden="true", so that on each swipe, the next radio incl. label is focused, otherwise screen readers will read the label, and then the label with the radio,

Toast

For toast there are three scenarios:

  • If the toast has a close button, the role is always "alertdialog", this overrides the "silent" setting (below). Focus is set to the close button and trapped. I had to implement a workaround, because somehow the button wasn't tap-able on initial focus. So now the focus is first set to the wrapper and then immediately to the close button, which is not noticeable to the user but fixes the problem.
  • Default behaviour for the toast in the absence of a close button is the "alert" role. Focus is left untouched as the toast should not impact what the user is doing at the moment, the toast text is simply read out.
  • If the new optional parameter silent is set to true, the toast will instead have no role and will be unnoticeable to a blind user. This can be used for notifications that are not important enough to interrupt what the user is doing and where not noticing them would not be an issue. Should probably be used very sparingly.

Menu

I haven't yet quite figured out the exact mechanisms for the menu, maybe someone else can have a look at this after my focus helper functions are pulled. I tried calling the virtual cursor functions in menu.ts:setOpen() but the function isn't called when the menu is opened/closed by swiping. This can be very problematic if the menu is opened with a button but closed with a swipe because at that point no part of the dom is accessible to the virtual cursor (and the only way to make things work again is to swipe open the menu). Obviously I had to remove that again.

Page Title

This is a note for future modifications. Since src/index.html supplies the HTML framework for all the pages, every page in the app will have the same <title> tag. Usually, this wouldn't even be noticeable unless the app is opened in a browser, but when using a screen reader, it becomes a nuisance: Every time the page is changed, the title of the app is read. It's trivial for a developer to change this by adding a document.title = "..." to one of the life cycle events, but it would be nice to have something working out-of-the-box (though where to take the string for the title from may require some thought.


The above changes fix #11309 and #11181. #9808 I could not reproduce. It worked for me before and it still works with the changes if the alert has its default role of alertdialog. Though there is a note in the source code:

      // note that this does not always work and bring up the keyboard on
      // devices since the focus command must come from the user's touch event
      // and ionViewDidEnter is not in the same callstack as the touch event :(

@janrg
Copy link
Author

janrg commented May 8, 2017

Also relevant:
#10168

@awebdeveloper
Copy link

@janrg plz run your sample code via wave. And notice the errors

@janrg
Copy link
Author

janrg commented May 8, 2017

@awebdeveloper Ah, I see. So what wave seems to be complaining about is that the button has no label or text inside even though the icon inside has an aria-label. I would think having an element with an aria-label should be equivalent to having text inside, and the chrome accessibility developer toolbar seems to agree with me as it's not complaining about the button. I've also tested it on Android, iOS, and ChromeVox and in each case I hear "Menu - Button". So I think this may be a case of wave either being overzealous or not picking up on the rule being fulfilled in an arguably somewhat roundabout way.

@awebdeveloper
Copy link

May be the issue is with older version of safari or chrome. Why don't you report a bug with them and see

@ankushgoyal27
Copy link

ankushgoyal27 commented Oct 18, 2017

@janrg have you committed your changes. Where I can find the fix that you made.

Thanks,
Ankush

@janrg
Copy link
Author

janrg commented Oct 18, 2017

@ankushgoyal27 yes I did 5 months ago. The pull request is here:
#11543

@sapradhan1
Copy link

@janrg for Toast, can I set the focus to toast message instead of close button. Please respond to this

@janrg
Copy link
Author

janrg commented Nov 10, 2017

@sapradhan1
If there is no close button, focus is on the text, otherwise it's on the button but the text is still read since the role is alertdialogue. Why would you want the focus on the text in that case?

@sapradhan1
Copy link

@janrg after giving role="alertdialog" its not reading the toast message. Its reading only close button. So, what I was thinking that, if we can shift the focus to toast message it will read out the message and on right swipe the focus will shift to close button.

@janrg
Copy link
Author

janrg commented Nov 10, 2017

That's strange. What platform are you on? Can't test it right now because I have just moved and haven't set everything up again yet. The problem is that what you describe would be against the alertdialog specification which states that "focus must be moved to that control when the alert dialog appears"

@AndreVicenteW
Copy link

Any news with this case? I'm having problems with talkback. I'm using Native Map, when I open menu the talkback doesn't recognize and when I open a modal the talkback continue reading the map page.

@janrg
Copy link
Author

janrg commented Feb 4, 2018

I'm still waiting to hear anything regarding my pull request. There seem to be some merge conflicts now, but unless I hear anything suggesting that a merge would be considered anytime soon, I don't think there'd be much point trying to resolve them as it doesn't seem unlikely that new conflicts would pop up before something happens regarding the merge...

@kensodemann
Copy link
Member

@janrg - at this point, I would wait until the release of v4 is complete and then refactor the PR as necessary. The changes for v4 are substantial enough that I would not suggest doing further work against v3.

@QiaoyuanMaxDeng
Copy link

QiaoyuanMaxDeng commented Oct 18, 2018

This reply is not really taking responsibility. I do appreciate ionic team support for newer version but v4 is in beta and it does not mean people should not use any v3 because of that.

Actually ionic accessibility bothers us as well. We are building business apps based on ionic framework but accessibility is so weak. It did not pass most of major accessibility test. Our team has spent two weeks so far to update a lot of handy code to pass the test. For example, ion-input does not really did well on aria-, it was using ng- related on ion-input field which is not a standard data entry element. Pure angular did all validation on input field directly but ionic team use it wrong or did not fully use it. So ion-input kind of failed most of accessibility test.

What I am saying is not complaining but to say ionic team should allow this merge in at least to support v3 first. Then think about how to migrate it to v4.

Thanks,

@adamdbradley adamdbradley added the ionitron: v3 moves the issue to the ionic-v3 repository label Nov 1, 2018
@imhoffd imhoffd removed ionitron: v3 moves the issue to the ionic-v3 repository labels Nov 28, 2018
@Ionitron Ionitron added the ionitron: v3 moves the issue to the ionic-v3 repository label Nov 28, 2018
@ionitron-bot
Copy link

ionitron-bot bot commented Nov 28, 2018

This issue has been automatically identified as an Ionic 3 issue. We recently moved Ionic 3 to its own repository. I am moving this issue to the repository for Ionic 3. Please track this issue over there.

If I've made a mistake, and if this issue is still relevant to Ionic 4, please let the Ionic Framework team know!

Thank you for using Ionic!

@ionitron-bot
Copy link

ionitron-bot bot commented Nov 28, 2018

Issue moved to: ionic-team/ionic-v3#213

@ionitron-bot ionitron-bot bot closed this as completed Nov 28, 2018
@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Nov 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ionitron: v3 moves the issue to the ionic-v3 repository
Projects
None yet
Development

No branches or pull requests