Problem/Motivation

When a link is used to open an ajax dialog when the dialog is closed the jQuery dialog that core uses will return the focus to the item that had focus before the dialog was opened.

This does not work for contextual links. When a contextual link is used to open an ajax dialog when the dialog is closed(not a form save & page reload) the contextual link will not be visible and focus will not be returned.

This was discovered in #2784569: Settings Tray Accessibility: Improve tabbing for the off-canvas dialog but it affects all core dialogs.

Proposed resolution

When a dialog is closed that was opened from a contextual link the contextual lin button for the context

Remaining tasks

  1. Figure out desired behaviour
  2. Determine if this general Drupal dialog problem or specific to off-canvas dialog.
  3. Implement fix

User interface changes

Contextual button will be focused after the dialog is closed.

API changes

None

Data model changes

none

Original Summary

from #2784569: Settings Tray Accessibility: Improve tabbing comment 9 point 7 of @Wim Leers review

However, when I close the Settings Tray dialog, my previous location is lost/forgotten — I'm back at the root of the page, and have to tab back all the way to where I was before I triggered the "Quick Edit" contextual link.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tedbow created an issue. See original summary.

mgifford’s picture

Issue tags: +keyboard navigation
mgifford’s picture

Issue tags: -a11y

removing "a11y" tag since we aren't using it.

tedbow’s picture

I have confirmed this is a problem with contextual links and dialogs even without Settings Tray enabled. We use to not support ajax/dialog links in contextual links so it won't have been a problem.

To test

  1. Enable test modules contextual_test and dialog_renderer_test
  2. Goto /dialog_renderer-test-links
  3. Use tab to select the Normal Modal!
  4. Hit return to open the dialog
  5. Hit return to close the dialog(you should be focus on close button when open)
  6. You should be focused on the link that opened the dialog. This works great and is documented here: https://api.jqueryui.com/dialog/
  7. Tab to block contextual link
  8. Enter to open
  9. Tab to "Test link with ajax" contextual link
  10. Entity to open dialog
  11. In the background contextual links close
  12. Hit return to close the dialog(you should be focus on close button when open)
  13. Not sure where focus?!?

You can also confirm that the focus work for the off-canvas dialog when not using context links by enabling off_canvas_test and then doing steps first 6 step on the page /off-canvas-test-links

I tried to use the dialog:beforecreate event to capture the currently focused link but at that point the focus is already on the dialog I think.

I am not sure what desired behavior would be here.

tedbow’s picture

Component: settings_tray.module » contextual.module
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3.64 KB

Ok here is a fix. It will need work.

I talked to @andrewmacpherson and we agreed that focus should go to the contextual button that opened the contextual links list with the dialog link. This patch does that.

@andrewmacpherson also pointed out that we need to consider what should happen when a form is saved in the dialog and then reloads the current page. I think that should a new issue.

Need to figure out how to test this.

tedbow’s picture

Title: Return to previous tab position when exiting Off-canvas dialog » Return to tab position when exiting dialog opened from contextual link
Issue summary: View changes
FileSize
1.08 KB
4.71 KB
  1. +++ b/core/misc/dialog/dialog.ajax.js
    @@ -85,6 +85,10 @@
    +    if (ajax.hasOwnProperty('element')) {
    +      response.dialogOptions.drupalTriggerElement = ajax.element;
    +    }
    

    If the dialog is being triggered for an element this element will be lost when opening the dialog if we don't save it in dialogOptions

  2. +++ b/core/modules/contextual/js/contextual.es6.js
    @@ -263,4 +264,22 @@
    +      if (settings.hasOwnProperty('drupalTriggerElement')) {
    +        drupalTriggerElement = settings.drupalTriggerElement;
    +      }
    

    We have to save the triggering element because settings is not available in dialog:afterclose event.

This patch adds test coverage to prove the trigger button is the active element when the dialog is closed.

tedbow’s picture

In #6 I was going to add test coverage for all dialog types but I realized it is difficult to test the off-canvas dialog unless you extend \Drupal\Tests\system\FunctionalJavascript\OffCanvasTestBase

So created #2927926: Create OffCanvas testing trait to allow other modules to test using off-canvas dialog

tedbow’s picture

Updated the comment which was I copied originally.

Also provided TEST_ONLY patch to prove the button does not get focus without this fix.

The last submitted patch, 8: 2915762-8-TEST_ONLY.patch, failed testing. View results

tim.plunkett’s picture

I can't speak to the JS changes, but the test looks good.

drpal’s picture

Status: Needs review » Needs work
  1. +++ b/core/misc/dialog/dialog.ajax.es6.js
    @@ -132,6 +132,10 @@
    +    if (ajax.hasOwnProperty('element')) {
    

    Is there any value in checking to see that ajax.element is actually of a type we expect?

  2. +++ b/core/modules/contextual/js/contextual.es6.js
    @@ -4,6 +4,7 @@
    +  let drupalTriggerElement;
    

    Probably need some docs describing what this is for. Also perhaps move it closer to where it's actually used?

  3. +++ b/core/modules/contextual/js/contextual.es6.js
    @@ -263,4 +264,24 @@
    +        // Save trigger element so it will be available for 'dialog:afterclose'
    +        // event.
    

    Multiline comment format.

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.44 KB
4.98 KB

@drpal thanks for the review!

  1. added ajax.element instanceof Element
    I looked into using ajax.element instanceof HTMLElement but from what I can tell Element is supported in many more browsers than HTMLElement
  2. I changed the logic in this file now to check in dialog:beforecreate whether it is contextual link. Because there no point saving the element if the link that opened the dialog is not a contextual link.

    As result of this it make more sense to just save the trigger button which will be given focus. So I changed the var name to dialogLinkContextualButton. Added a comment and moved it closer to where it is used.

  3. fixed. But comment is always updated because of logic change.
tedbow’s picture

chatted with @drpal and here is just small change to the if statement when checking for the contextual button.

drpal’s picture

@tedbow - Yes, looks good.

+++ b/core/modules/contextual/js/contextual.es6.js
@@ -263,4 +263,30 @@
+      if (settings.hasOwnProperty('drupalTriggerElement')) {

So just a general question about this. Previously in dialog.ajax.es6.js we perform some extensive checks before adding ajax.element to drupalTriggerElement, does it make sense to move these checks to the changes in the dialog:beforecreate function?

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.