Skip to content

Loading…

Dialog: Fixes for #7862 - dialog accessibility #704

Closed
wants to merge 7 commits into from

5 participants

@larowlan

a11y changes as follows:
Modal dialogs have an internal wrapper with role=document to sandbox a11y users to the modal
Non-modal dialogs don't get role=dialog
Refocussing window whilst modal dialog is active returns focus to first tabbable element in modal dialog.
.dialog('open') method takes second optional argument 'triggerEl' when present focus is returned to this element when dialog is closed.
Accessibility consultant Everett Zufelt has reviewed these changes.

@RobLoach

Looks pretty good to me!... Is there any reason why triggerEl couldn't be handled by an option? parentFocus or something? Is passing in behavioural parameters on "open" a normal pattern used throughout jQuery UI?

Other than that, I'd say this is a great enhancement.

Yeah I grappled with that and failed. I couldn't see how to get the triggering element from the open method as I didn't have an event object. If there's a way to get it that I don't know of (I wouldn't consider myself a power jquery user) can you let me know?

@scottgonzalez
jQuery Foundation member

@RobLoach: Adding an option for the trigger element would be bad because it's state that only applies to while the dialog is open and there may be multiple triggers for the same dialog.

@scottgonzalez scottgonzalez commented on the diff
ui/jquery.ui.dialog.js
@@ -110,16 +110,22 @@ $.widget("ui.dialog", {
})
.appendTo( "body" ),
+ // Add a document wrapper inside the dialog for a11y.
@scottgonzalez jQuery Foundation member

This will be the first place that we set document or application role in jQuery UI. Can you explain why this is needed and whether it affects all AT or just specific ones?

Hi Scott
Everett Zufelt said this was needed to sandbox the screen reader to the modal dialog.
He's the best one to answer this.
LR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez scottgonzalez commented on the diff
ui/jquery.ui.dialog.js
@@ -148,9 +154,15 @@ $.widget("ui.dialog", {
.addClass( "ui-dialog-buttonset" )
.appendTo( uiDialogButtonPane );
+ if (options.modal) {
@scottgonzalez jQuery Foundation member

Is there anything we need to do for non-modal dialog to provide context?

Everett noted that ideally the modal would be a sibling to its triggering element but obviously thats not achievable.
He was happy with this removed from non-modal dialogs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez scottgonzalez commented on the diff
ui/jquery.ui.dialog.js
((10 lines not shown))
uiDialog.attr({
- role: "dialog",
- "aria-labelledby": uiDialogTitle.attr( "id" )
+ "aria-labelledby": uiDialogTitle.attr( "id" ),
+ "aria-describedby": uiDialogContent.attr( "id" )
@scottgonzalez jQuery Foundation member

What is the benefit of providing the entire content as the description? Do users not just interact with the contents of the dialog right away since that's where focus should be?

Again, Everett said this meant the screen reader read the contents to the user once the dialog was active.
(I'm not much help here, just the coder - Everett's the expert!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez
jQuery Foundation member

We shouldn't have a parameter for open() to define the trigger element. We should just find the active element and store that automatically.

@scottgonzalez scottgonzalez commented on an outdated diff
ui/jquery.ui.dialog.js
@@ -732,6 +754,19 @@ $.extend( $.ui.dialog.overlay, {
}
});
+ // allow refocussing topmost dialog when window receives focus
+ $( window ).bind( "focus.dialog-overlay", function( event ) {
@scottgonzalez jQuery Foundation member

Use this.window. However, you should be aware that a lot of the overlay code will be changing in 1.10, see #452

Everett has commented that this is causing some issues refocussing the browser chrome when the dialog and screen reader are active. Most likely the whole chunk will go.

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

I agree on the open parameter, but couldn't work it out - I'll re-roll with the active logic you've suggested

@ezufelt

Teted w/ FF13 / JAWS13 and working about as well as any modal/modeless dialogs I have experienced. Would like the dialog to be inserted in DOM as sibling of trigger, but understand that this might not be feasible. Would help screen-reader users with context.

@larowlan larowlan Dialog: Removes return focus to dialog when window regains focus, use…
… activeElement for triggerEl storage. Partial fix for #7862 - modal accessibility
37fef2c
@larowlan

Re-rolled without the 'return focus from window' logic and using document.activeElement to grab the focussed button as the triggerEl

@scottgonzalez scottgonzalez commented on the diff
ui/jquery.ui.dialog.js
@@ -299,6 +317,10 @@ $.widget("ui.dialog", {
return;
}
+ if (document.activeElement) {
+ this.triggerEl = $( document.activeElement )
@scottgonzalez jQuery Foundation member

this.document[0].activeElement for cross-window support. Also, have you run into cases where there is no active element? That should only occur if the document hasn't loaded yet.

Yeah, document.activeElement does work if no element is focussed, ie if I just click the trigger. But seeing as though this is for a11y, in most cases we'd assume they were tabbing and focussing anyway. Happy to do it another way if there's a better approach.

@scottgonzalez jQuery Foundation member

What browsers are you seeing this happen in? Can you provide a demo page showing this? Browsers should report <body> when no element is focused, assuming the document has loaded. If you click on a button to open the dialog, then the button may not have focus, but that's fine because as a mouse user, you don't care if focus goes back to the body. If you tab to the button and press enter to open the dialog, then the button should be the active element, which is what we want.

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

Any updates on this?

@ezufelt
@scottgonzalez
jQuery Foundation member

There are a few things holding this up. There are a few questions that are still unanswered, such as when would document.activeElement ever be null and why we need to set the role to document on a wrapper. There are issues that were pointed out but never fixed, such as changing document to this.document[ 0 ]. There are no tests to verify the functionality and prevent regressions. These are all things that we will happily investigate and fix ourselves when we have time, but unfortunately right now we don't. We're focused on all of the surrounding infrastructure for the 1.9 release and we're only fixing major bugs right now, as we're already in the RC phase of 1.9.

As mentioned before, dialog is not a priority in 1.9, but we're committed to getting all of the dialog accessibility and API improvements into 1.10 and released in time for Drupal 8's code freeze. Dialog will be high priority for 1.10 and will be one of the first things I work on. I would expect the updates to be done before Drupal's feature freeze, but we won't be able to get the release done by then. We'll likely do a milestone release with the updated dialog prior to the feature freeze. As a reminder, you can always view our roadmap to see what our current and future priorities are (the 1.10 release was split into 1.10/1.11 specifically to get dialog into a release in time for Drupal 8).

I really do appreciate the effort from @larowlan, @ezufelt, @RobLoach, and everyone else that's been helping out. I feel bad about the timing, but I will be much more responsive and helpful as soon as 1.9.0 is out the door (hopefully just another week or 2).

@larowlan

Thanks for the refresher on what needs doing Scott.
I'll try and pick this up again next week.

Lee

@larowlan

Hi Scott
All going to plan I should be finished on http://drupal.org/node/731724 next week so will come back to this.

Lee

@jzaefferer
jQuery Foundation member

@larowlan good timing! Just opened #794 - I hope you can chime in there.

@jzaefferer
jQuery Foundation member

Feedback on #794 so far is good, so closing this.

@jzaefferer jzaefferer closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 1, 2012
  1. @larowlan

    Dialog: Possible to tab outside modal dialog. Partial fix for #7862 -…

    larowlan committed
    … dialog: modal accessibility
Commits on Jul 8, 2012
  1. @larowlan

    Merge upstream changes

    larowlan committed
Commits on Jul 9, 2012
  1. @larowlan

    Dialog: Wrap dialog in div with role=document, allow re-focussing of …

    larowlan committed
    …triggering element on dialog close. Partial fix for #7862 - dialog: modal accessibility
Commits on Jul 14, 2012
  1. @larowlan
Commits on Jul 15, 2012
  1. @larowlan

    Dialog: Don't assign role=dialog for non-modal dialogs, don't use rol…

    larowlan committed
    …e=document for non modals. Partial fix for #7862 - modal accessibility
Commits on Jul 18, 2012
  1. @larowlan

    Dialog: Return focus to dialog when window regains focus. Partial fix…

    larowlan committed
    … for #7862 - modal accessibility
Commits on Jul 19, 2012
  1. @larowlan

    Dialog: Removes return focus to dialog when window regains focus, use…

    larowlan committed
    … activeElement for triggerEl storage. Partial fix for #7862 - modal accessibility
This page is out of date. Refresh to see the latest.
Showing with 28 additions and 6 deletions.
  1. +1 −1 demos/dialog/animated.html
  2. +1 −1 demos/dialog/modal-form.html
  3. +26 −4 ui/jquery.ui.dialog.js
View
2 demos/dialog/animated.html
@@ -28,7 +28,7 @@
});
$( "#opener" ).click(function() {
- $( "#dialog" ).dialog( "open" );
+ $( "#dialog" ).dialog( "open", $(this));
return false;
});
});
View
2 demos/dialog/modal-form.html
@@ -109,7 +109,7 @@
$( "#create-user" )
.button()
.click(function() {
- $( "#dialog-form" ).dialog( "open" );
+ $( "#dialog-form" ).dialog( "open");
});
});
</script>
View
30 ui/jquery.ui.dialog.js 100644 → 100755
@@ -110,16 +110,22 @@ $.widget("ui.dialog", {
})
.appendTo( "body" ),
+ // Add a document wrapper inside the dialog for a11y.
@scottgonzalez jQuery Foundation member

This will be the first place that we set document or application role in jQuery UI. Can you explain why this is needed and whether it affects all AT or just specific ones?

Hi Scott
Everett Zufelt said this was needed to sandbox the screen reader to the modal dialog.
He's the best one to answer this.
LR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ uiDialogDocument = options.modal ? ( this.uiDialogDocument = $( "<div>" ) )
+ .attr( "role", "document")
+ .addClass( "ui-dialog-document" )
+ .appendTo( uiDialog) : null,
+
uiDialogContent = this.element
.show()
.removeAttr( "title" )
.addClass( "ui-dialog-content ui-widget-content" )
- .appendTo( uiDialog ),
+ .appendTo( options.modal ? uiDialogDocument : uiDialog ),
uiDialogTitlebar = ( this.uiDialogTitlebar = $( "<div>" ) )
.addClass( "ui-dialog-titlebar ui-widget-header " +
"ui-corner-all ui-helper-clearfix" )
- .prependTo( uiDialog ),
+ .prependTo( options.modal ? uiDialogDocument : uiDialog ),
uiDialogTitlebarClose = $( "<a href='#'></a>" )
.addClass( "ui-dialog-titlebar-close ui-corner-all" )
@@ -148,9 +154,15 @@ $.widget("ui.dialog", {
.addClass( "ui-dialog-buttonset" )
.appendTo( uiDialogButtonPane );
+ if (options.modal) {
@scottgonzalez jQuery Foundation member

Is there anything we need to do for non-modal dialog to provide context?

Everett noted that ideally the modal would be a sibling to its triggering element but obviously thats not achievable.
He was happy with this removed from non-modal dialogs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ // We should only sandbox user to dialog if modal (a11y).
+ uiDialog.attr({
+ role: "dialog"
+ });
+ }
uiDialog.attr({
- role: "dialog",
- "aria-labelledby": uiDialogTitle.attr( "id" )
+ "aria-labelledby": uiDialogTitle.attr( "id" ),
+ "aria-describedby": uiDialogContent.attr( "id" )
@scottgonzalez jQuery Foundation member

What is the benefit of providing the entire content as the description? Do users not just interact with the contents of the dialog right away since that's where focus should be?

Again, Everett said this meant the screen reader read the contents to the user once the dialog was active.
(I'm not much help here, just the coder - Everett's the expert!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
});
uiDialogTitlebar.find( "*" ).add( uiDialogTitlebar ).disableSelection();
@@ -252,6 +264,12 @@ $.widget("ui.dialog", {
$.ui.dialog.maxZ = maxZ;
}
+ // return the focus to the triggering element for a11y.
+ if (this.triggerEl) {
+ this.triggerEl.focus();
+ this.triggerEl = false;
+ }
+
return this;
},
@@ -299,6 +317,10 @@ $.widget("ui.dialog", {
return;
}
+ if (document.activeElement) {
+ this.triggerEl = $( document.activeElement )
@scottgonzalez jQuery Foundation member

this.document[0].activeElement for cross-window support. Also, have you run into cases where there is no active element? That should only occur if the document hasn't loaded yet.

Yeah, document.activeElement does work if no element is focussed, ie if I just click the trigger. But seeing as though this is for a11y, in most cases we'd assume they were tabbing and focussing anyway. Happy to do it another way if there's a better approach.

@scottgonzalez jQuery Foundation member

What browsers are you seeing this happen in? Can you provide a demo page showing this? Browsers should report <body> when no element is focused, assuming the document has loaded. If you click on a button to open the dialog, then the button may not have focus, but that's fine because as a mouse user, you don't care if focus goes back to the body. If you tab to the button and press enter to open the dialog, then the button should be the active element, which is what we want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+
var hasFocus,
options = this.options,
uiDialog = this.uiDialog;
Something went wrong with that request. Please try again.