Dialog a11y by pvagner · Pull Request #1652 · matrix-org/matrix-react-sdk

pvagner · 2017-12-06T10:28:56Z

This enhances dialogs accessibility according to the Aria Best practices.

A little breakdown of the changes

@davidtheclark focus-trap-react is being used to force keyboard focus staying within the dialog while one is showing
When the dialog is showing the rest of the riot app is hidden to screen readers using aria-hidden property so it is not possible to navigate into it by using screen reader specific features.
Since focus is now trapped inside the dialog I went through all the dialogs inspecting their states and attempted to ensure initial focus is set for all of them when mounted.
BaseDialog now has contentId prop components depending on it can set in order to properly associate a textual description to the dialog it-self via the aria-describedby property.
onKeyDown handler within the BaseDialog component only handles esc keypresses as handling the enter key is leading to issue All Riot-Web dialogs have partially broken keyboard support element-hq/element-web#5736
Fixed a follow up issue where some dialogs might get reloaded when focusing AccessibleButton component immediatelly after dismissing the dialog by pressing the enter key.
Made sure dialogs which are collecting data thus have other focusable controls besides buttons embed a form so we can get the onEnterPressed functionality back via onFormSubmit where it makes sense.
Conditionally set role="alert" property on the regions displaying error messages so these will get reported automagically by most screen reading apps as they appear.
Looked through and attempted to add missing labels to some form elements.

It's super awesome BaseDialog is getting attention elsewhere too as the DeactivateAccountDialog will also receive these accessibility changes once PR #1631 lands.

Signed-off-by: Stefan Parviainen <pafcu@iki.fi>

- Wrapped all the modals inside a react-focus-trap component disabling keyboard navigation outside the modal dialogs - Disabled our custom key handling at dialog level. Cancelling on esc key is now handled via FocusTrap component. - Removed onEnter prop from the BaseDialog component. Dialogs that submit data all now embed a form with onSubmit handler. And since keyboard focus is now managed better via FocusTrap it no longer makes sense for the other dialog types. Fixes element-hq/element-web#5736 - Set aria-hidden on the matrixChat outer node when showing dialogs to disable navigating outside the modals by using screen reader specific features.

leaving it in the Modal manager. We are using Modal manager to load other components not just BaseDialog and its subclasses and they might require different keyboard handling. Also depend on focus-trap-react rather than react-focus-trap for locking keyboard focus inside the dialog. The experience is much nicer and even the FocusTrap element it-self no longer gains the focus. On a side note using the FocusTrap element outside the dialog (on its parent) stops it from working properly.

Also added initial focus where it has not been set.

was also activating normal button that might just have received the system focus as a result of the key press and the other way round. The most obvious occurence of this issue is that dialogs were reappearing when dismissed by pressing the enter key.

create group dialog.

pafcu · 2017-12-06T15:56:20Z

+    },
+
+    componentDidMount: function() {
+        this.applicationNode = document.getElementById('matrixchat');


Perhaps add a comment here that this is to hide the rest of the application from screen readers while the dialog is open

Done in 4171675

pafcu · 2017-12-06T15:59:41Z

    restProps.onClick = onClick;
-    restProps.onKeyUp = function(e) {
-        if (e.keyCode == 13 || e.keyCode == 32) return onClick(e);
+    restProps.onKeyDown = function(e) {


Perhaps document here why it needs to be onKeyDown, and what is special about keyCode 13 and 32

Found a related issue so done along with its fix in 839f938

hide content outside of the BaseDialog to screen reader users.

fixing behaviour when pressing the enter key breaks behaviour when pressing space to activate the buttons. So we are now handling enter onKeyDown and space onKeyUp. Also briefly explained the situation with comments.

pafcu · 2017-12-07T17:21:01Z

Still needs linting errors to be fixed

lukebarnard1

This looks really good. Thanks very much you two for doing this!

lukebarnard1 · 2017-12-12T10:22:29Z

+
+    componentDidMount: function() {
+        // Retrieve the root node of the Riot application outside the dialog
+        this.applicationNode = document.getElementById('matrixchat');


It would be best if this was done in the context of Modal, which is aware of how many dialogs are currently in the stack. At the point a dialog is closed, we can check the number of dialogs open and set aria-hidden to false if there are none remaining.

Originally I wanted it to have it the way you are suggesting. I have started by using FocusTrap component on the dialog parent div element. I have found out FocusTrap is not working well when placed outside the dialog (i.e. managed via the Modal manager) so I moved it into the dialog. I think hiding the main application node complements trapping the keyboard focus it's why I have added it into the dialog not to its parent. Later on I have discovered we are sometimes loading other elements e.g. spinner into the modal. I was unable to assure if all the components loaded into the modal should trap the keyboard focus and disable screen reader access to the main application so this is only supporting my doupt. If you are sure we are only loading keyboard navigable stuff into the modal then I'm happy to move the aria-hide stuff into the Modal but I won't be moving FocusTrap thingy as it is not working very well outside the dialog. If there is a chance that we might occassionally load something into the modal that can't be navigated by the keyboard alone then implementing aria-hidden at this level would cause screen reader users to be locked in the modal without possible exit path out of it.

If there is a chance that we might occassionally load something into the modal that can't be navigated by the keyboard

I'd say that would be a bug anyway.

If we don't check the number of dialogs open before deciding whether to apply aria-hidden to the root element, we might end up with races and sometimes the aria-hidden property will be set as expected.

lukebarnard1 · 2017-12-12T10:33:10Z


        return (
-            <div onKeyDown={this._onKeyDown} className={this.props.className}>
+            <FocusTrap onKeyDown={this._onKeyDown} className={this.props.className} role="dialog" aria-labelledby='mx_BaseDialog_title' aria-describedby={this.props.contentId}>


This line is quite long, could you break it up over several lines?

Also, the element IDs here assume that there is only one dialog being shown at one time. This is strictly true at the moment and I suppose that it's sufficiently unlikely for there to be more than one for us to ignore this for now. Also, I can't think of a simple, low-overhead way to make both IDs unique short of actively modifying the IDs of the children of BaseDialog.

Long line split in ab0ff9b
As for the possible component ID clashes I guess Modal only allows single dialog to be rendered into the DOM so at the DOM level this should hopefully not be an issue. If I have missed something and in deed this might be dangerous or inappropriate in other way then I can append some randomly generated number at the end of these IDs. Should I do that?

Sorry, to be clear I think that we can ignore this on exactly the basis you mentioned - Modal only allows a single dialog to be rendered.

lukebarnard1 · 2017-12-12T11:11:05Z

                </div>
                <div className="mx_Dialog_buttons">
-                    <button ref="button" className="mx_Dialog_primary" onClick={this.props.onFinished}>
+                    <button className="mx_Dialog_primary" onClick={this.props.onFinished} autoFocus={this.props.focus}>


I'm not sure where focus is coming from. Should autoFocus not always be true?

Error dialog has focus prop that is true by default. In fact I haven't introduced it my-self. It used button's ref to set the focus conditionally in the componentDidMount function and I have changed it to use autoFocus prop of the button component instead as I like it better. This is just cosmetic change and the functionality should not be affected.

My mistake, ErrorDialog indeed has this property defined - I was obviously looking in the from file somehow.

Thanks for changing that - autoFocus is a clear improvement over using refs.

lukebarnard1 · 2017-12-12T11:12:59Z

-                <div>
-                    <div>{ this.state.authError.message || this.state.authError.toString() }</div>
+                <div id='mx_Dialog_content'>
+                    <div role={(this.state.authError.message || this.state.authError.toString()) ? "alert" : ""}>{ this.state.authError.message || this.state.authError.toString() }</div>


I think the existence of this div should be conditional on whether there is a message to display. That way, we only have a single conditional and the role can always be "alert".

In the ErrorDialog the condition checking the error message turned out to be redundant as the same check is performed a few lines above. But thanks for the idea, I have checked the other occurences of this too.

lukebarnard1 · 2017-12-12T11:21:16Z


+    componentDidMount: function() {
+        if (this.refs.bugreportLink) {
+            this.refs.bugreportLink.focus();


Should we not be auto-focussing the "Continue anyway" button instead?

I like to focus the bug report link when it's available and focus the button when that is not there. As I think we should encourage people to report their issues when they find some. Focusing the button would mean screen reader users have to shift+tab in order to focus that link. I assume by default almost every keyboard user navigates by pressing the tab key not shift+tab. Still this is my feeling only and if you like me to change it then I will do it.

Okay, fair reasoning. This dialog shouldn't come up with any frequency anyway.

lukebarnard1 · 2017-12-12T11:35:08Z

+    // We need to consume enter onKeyDown and space onKeyUp
+    // otherwise we are risking also activating other keyboard focusable elements
+    // that might receive focus as a result of the AccessibleButtonClick action
+    restProps.onKeyDown = function(e) {


I'm confused as to why we would treat SPACE and ENTER differently here. I'd expect them to both be handled onKeyDown because if for some bizarre reason pressing space results in focus arriving on a button, we really shouldn't be handling onKeyUp for space at that point.

I can't explain this very well I was playing with this trying out various combinations of onKeyDown and onKeyUp and what I think is most important is that we stop propagating and prevent default of both onKeyDown and onKeyUp. I am not going to change this unless you can show me an example which is working well in both these cases:

Go to the Riot settings, use the tab key to navigate to the Logout button, press either enter key or space key and see the dialog. The issue here is that depending on the default browser behaviour the default dialog button receives the focus as the dialog appears and it then gets activated.

If the logout dialog has not been dismissed then press the tab key until the Cancel button receives the focus. Now test how if you can close the dialog by pressing both the enter key and the space bar key. The issue here is that the logout button in the settings might receive the focus as the dialog is closing and if the proper event is not consumed the dialog might reactivate.

You need to test both these cases with space key and the enter key.
I know this feels very broken however we are only handling the differences imposed by the fact we have various button types i.e. the native button and our own AccessibleButton and the resulting behaviour of their differences.

OK, so what's actually going on here is that for a actual HTML button (eg. the cancel button in the Sign Out dialog), activating it with enter swallows the keydown event but propagates a keyup event. Activating it with space swallows both, which is why it happens to work in this configuration. This is still a little buggy though because the spacebar press still propagates out when activating the 'sign out' button with the spacebar (the settings page scrolls down as the dialog appears).

This may be an argument for changing everything to consistently use onKeyDown (it was originally changed on onKeyUp by me here: #742 (comment)). The problem with using keyup for space and keydown for enter will be that we may get this same problem again (ie. the accessiblebutton captures keydown but propagates keyup which triggers something else that it shouldn't do.

I'd probably argue for switching to keydown everywhere since this empirically what a native button does, but I'd also like to split this change out of this PR, because the rest of this PR looks great and it's already very large. In future it's probably better to split these changes up into smaller PRs.

Please try the following two sets of steps

Take 1

Apply this PR

Edit AccessibleButton.js file in a way so both space and enter key presses will trigger onKeyDown and remove onKeyUp all together

Build the resulting app

Navigate to the Riot settings.

When the Settings window comes up keep pressing the tab key until Logout button receives the focus

Press the space bar key

Results

Damn! We were expecting a confirmation dialog however we have just logged out because of the underlying issue :(

Expected results

Before logging out we should be able to confirm our choice within the question dialog.

Take 2

Apply this PR

Edit AccessibleButton.js file in such a way that both enter and space bar keypresses will trigger onKeyDown i.e. onClick will be executed when either of them is pressed onKeyDown.

Keep onKeyUp method present however change it so it will do nothing more except of e.preventDefault() and e.stopPropagation()

Continue with the steps as if you were following take 1.

Results

Damn! We were expecting a confirmation dialog however we have just logged out because of the underlying issue :(

Expected results

Before logging out we should be able to confirm our choice within the question dialog.

So I can't explain it very well however by some tryal and error this is the only walid implementation as far as I can test.

Yep, I suspect this is because we're using keydown/keyup inconsistently in some other file, hence suggesting we should use one of them consistently throughout the app.

@dbkr isn't the point that browsers don't do onKeyUp vs. onKeyDown consistently the actual problem here? And by mimicking the weirdness as in the PR, we get something that works but that feels wrong.

I don't know - if this is the case I think we should find out what this inconsistency is and document it in a comment in this block of code, otherwise it's going to continue to confuse people.

Will it help if I change that inline comments to say something like this?

// We need to consume enter onKeyDown and space onKeyUp // otherwise we are risking also activating other keyboard focusable elements // that might receive focus as a result of the AccessibleButtonClick action // It's because we are using html buttons at a few places e.g. inside dialogs // And divs which we report as role button to assistive technologies. // Browsers handle space and enter keypresses differently and we are only adjusting to the // inconsistencies here

OK, it at least gives a hint as to what's going on. Can we fix the fact that the spacebar keydown still leaks out when activating an accessible button with the spacebar though?

lukebarnard1 · 2017-12-12T11:36:55Z

                    </div>
                </form>
-                <div className="error">
+                <div className="error" role={ this.props.errorText ? "alert" : ""}>


Again, I think it'd be easier to read if the existence of the error div was conditional on the errorText and the role was always "alert".

Changed all of InteractiveAuthEntryComponents to display the error div conditionally.

lukebarnard1 · 2017-12-12T11:37:19Z

                    onCaptchaResponse={this._onCaptchaResponse}
                />
-                <div className="error">
+                <div className="error" role={ this.props.errorText ? "alert" : ""}>


The same applies here (see above comment).

lukebarnard1 · 2017-12-12T11:37:35Z

                            />
                        </form>
-                        <div className="error">
+                        <div className="error" role={this.state.errorText ? "alert" : ""}>


See above comment.

lukebarnard1 · 2017-12-12T11:39:46Z

        }
    },

+    focus: function() {


I'm not sure what would call this, could you clarify?

Please see the comment at line 64. Apparently InteractiveAuthEntryComponents should implement that if they need it. I've got an inspiration from PasswordAuthEntry component otherwise I would just do this inside componentDidMount function.

Hmm. Okay, I've managed to find where this is actually called. We're doing fun things in InteractiveAuth to call it.

its ref inside onComponentDidMount function. This is shorter better and has been requested.

pvagner · 2017-12-20T09:21:03Z

@lukebarnard1 Excuse me for the delay. I think the suggested changes are now pushed.

try to explain this key handling inconsistency with some additional comments as per the review discussion.

dbkr

Sorry, there's quite a lot of different changes in this PR and every time I look at it, I notice more things.

dbkr · 2018-02-07T11:59:08Z


    _reRender() {
+        // Retrieve the root node of the Riot application outside the modal
+        let applicationNode = document.getElementById('matrixchat');


Hmm, unfortunately we can't assume that the application will have called our root element matrixchat. Ideally these dialogs should all be in an element inside MatrixChat rather than just added to the document, but possibly the best compromise for now would be to dispatch a app-visible message with a true/false flag that MatrixChat can listen for and set its aria-hidden flag. This is also overriding React and so is likely to lead to subtle bugs like the aria-hidden flag disappearing if React re-renders the MatrixChat component.

dbkr · 2018-02-07T16:23:00Z

                    <TintableSvg src="img/icons-close-button.svg" width="35" height="35" />
                </AccessibleButton>
-                <div className={'mx_Dialog_title ' + this.props.titleClass}>
+                <div className={'mx_Dialog_title ' + this.props.titleClass} id='mx_BaseDialog_title'>


Why does this need an extra id for this rather just using mx_Dialog_title?

We need an ID not a CSS class in order to pass as an aria-labelled-by on the root node. So other than to supply the same ID at line 78 and line 86 there are no other intentions here. Perhaps it can be improved somehow but I'd appreciate a hint on how to go about doing it. Perhaps it might just be because I am not very experienced.

Oh, I completely missed that it was an ID and not a class.

dbkr · 2018-02-07T16:31:40Z

+                className={this.props.className}
+                role="dialog"
+                aria-labelledby='mx_BaseDialog_title'
+                aria-describedby={this.props.contentId}


This doesn't seem quite right to me - looks like this this should point to a description of what the thing is: the content will also include form controls and whatever else is in the dialog.

Yes by reading aria docs or best practices we can learn this should be pointing to a DOM node describing the dialog i.e. for simple yes/no dialog it is its whole content excluding the buttons. For more complex dialogs it would be some other node nested inside the dialog. If we were using it preciselly the way as suggested we will have to delegate setup of this prop to the component where the description is added. Thus we have opted for a compromise by using the whole dialog content as its accessible description. All of Riot dialogs have the descriptive text at the top thus usefull text would be presented by the assistive tools first
. Note AT users don't have to listen to the whole description, they can interupt it naturally whenever they are confortable to continue working with the app.

OK. When making compromises like this, we generally comment them so we know what compromises have been made and why, otherwise everyone reading the code will end up asking the same question I asked.

dbkr · 2018-02-07T16:39:20Z

-                                { _t('Block users on other matrix homeservers from joining this room') }
-                                <br />
-                                ({ _t('This setting cannot be changed later!') })
-                            </label>


I assume removing this wasn't intentional?

@dbkr

removed while trying to solve merge conflicts (thx @dbkr)

…ctions rather than assuming we can find and manipulate the node directly

dbkr

Thanks!

pafcu and others added 9 commits November 29, 2017 21:13

Make Dialogs more accessible

cb5c9f2

Signed-off-by: Stefan Parviainen <pafcu@iki.fi>

Add missing id

437a440

Signed-off-by: Stefan Parviainen <pafcu@iki.fi>

Applied aria-describedby to all other dialogs that are using BaseDialog.

a31af39

Also added initial focus where it has not been set.

Forgot to turn div displaying error text into an aria-alert in the

60ada30

create group dialog.

Add a missing language string

9146fde

Merge branch 'develop' into dialog-a11y

f9b0243

pafcu reviewed Dec 6, 2017

View reviewed changes

pvagner added 2 commits December 7, 2017 09:44

Add some comments explaining how we are using aria-hidden property to

4171675

hide content outside of the BaseDialog to screen reader users.

Argh! Reworked AccessibleButton key handling again. It turned out by

839f938

fixing behaviour when pressing the enter key breaks behaviour when pressing space to activate the buttons. So we are now handling enter onKeyDown and space onKeyUp. Also briefly explained the situation with comments.

Elliminate lint errors and warnings

321b1ad

lukebarnard1 mentioned this pull request Dec 12, 2017

Use BaseDialog for DeactivateAccountDialog #1631

Closed

lukebarnard1 suggested changes Dec 12, 2017

View reviewed changes

pvagner added 5 commits December 12, 2017 18:55

BaseDialog: split a very long line

ab0ff9b

Address review request comments

642675c

Merge branch 'develop' into dialog-a11y

9f5857a

Move aria-hidden management from the BaseDialog component to the Modal

20c485d

SetEmailDialog: use autoFocus prop on the EditableText rather than using

f2ca02e

its ref inside onComponentDidMount function. This is shorter better and has been requested.

pvagner added 2 commits January 5, 2018 11:45

Consume all combinations of space / enter, keyDown / keyUp presses and

cf472c7

try to explain this key handling inconsistency with some additional comments as per the review discussion.

Merge branch 'develop' into dialog-a11y

14991af

dbkr suggested changes Feb 7, 2018

View reviewed changes

pvagner added 3 commits February 8, 2018 21:16

Ooops, restore a bit of RoomCreateDialog content I have accidentally

8f97e94

removed while trying to solve merge conflicts (thx @dbkr)

Reimplement setting aria-hidden on the main app node by dispatching a…

4105709

…ctions rather than assuming we can find and manipulate the node directly

Add comments explaining our non standard usage of aria-described-by

5e9368e

dbkr approved these changes Feb 13, 2018

View reviewed changes

dbkr merged commit 580b68a into matrix-org:develop Mar 20, 2018

turt2live mentioned this pull request Jul 8, 2019

BaseDialog-based components assume <kbd>Enter</kbd> on any tab-indexed item = affirmative element-hq/element-web#6336

Closed

Uh oh!

Conversation

pvagner commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

A little breakdown of the changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pafcu commented Dec 7, 2017

Uh oh!

lukebarnard1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvagner Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Take 1

Results

Expected results

Take 2

Results

Expected results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pvagner commented Dec 6, 2017 •

edited

Loading

pvagner Jan 3, 2018 •

edited

Loading