Nested dialogs

[renatodeleao]

First of all, thanks and congrats for such a small, elegant and accessible dialog solution 👍

Error

I step into an error while opening a modal from within another dialog, so multiple modals opened at the same time. (everything kept working, but the following error logs to console)

Note:
If this is the expected behaviour, as in, we shouldn't open more than one dialog at once, ignore this report and close it.

Demo

Steps:

1. Open Modal one
2. Open modal two from modal 1 content
3. look at console

Error log

Maximum call stack size exceeded.
    at o ((index):72)
    at e._maintainFocus ((index):72)

Details

version: ^4.0.0

[KittyGiraudel]

Hey there!

I must say never tried to open a dialog from another one, so I’m going to go with: that doesn’t surprise me that it doesn‘t work. :D

It shouldn’t be too hard to fix though. If you want to have a look at the culprit line and submit a PR: https://github.com/edenspiekermann/a11y-dialog/blob/master/a11y-dialog.js#L276-L278. ;)

[renatodeleao]

@hugogiraudel Definitely not a common pattern (and a questionable one from a ux perspective IMO). I admit that i've faced some situations during consulting work, when the second popup acted as "kind-of-selectbox" and the action would change content of the first dialog.

I admit that this time, I just tested it for the sake of "let's see if this works" :P
It's not critical and i'll give it a shot one of these days. Cheers!

[renatodeleao]

@hugogiraudel I've back to this.

It was apparently a simple fix. If the event.target has attribute data-a11y-dialog-show that is not equal to instance node reference this.node.id then we are in the presence of another dialog, so skip the setFocusTofirstItem on this instance for now, that will be triggered later again when the second modal opens and that time it will evalute to true and focus the correct item

/**
 * Private event handler used when making sure the focus stays within the
 * currently open dialog
 *
 * @access private
 * @param {Event} event
 */
A11yDialog.prototype._maintainFocus = function (event) {
  // If the dialog is shown and the focus is not within the dialog element,
  // move it back to its first focusable child, unless another dialog is going to be opened :)
  var dialogTarget = event.target.getAttribute('data-a11y-dialog-show');
  if (this.shown && !this.node.contains(event.target) && dialogTarget === this.node.id ) {
	setFocusToFirstItem(this.node);
  }
};

This appears to work fine but...

Easy tiger

This only works for click events. But we're talking about an accessible dialog. We still need to handle other input methods, keyboard, and the problem is not at the opening but when we close.

escape click signal ever instance to fire it's .hide() method.

A11yDialog.prototype._bindKeypress = function (event) {
  // If the dialog is shown and the ESCAPE key is being pressed, prevent any
  // further effects from the ESCAPE key and hide the dialog

  // ---> we are closing all of openInstances, not the topMostOne 🤔
  if (this.shown && event.which === ESCAPE_KEY) {
  event.preventDefault();
  this.hide();
}

That is not a bad default behaviour and we could end this conversation here but...

what if want to close only the last opened?

Each instance is isolated and has no idea about the state their siblings, by design. So we can't call it a problem.
That being said a couple of workarounds came to my mind to solve this.

1. we could save all instances to a global var, and rely on DOM order to define which is o top of which

// data-a11y-dialog is applied to every dialog element
const dialogEls = Array.from(document.querySelectorAll(['data-a11y-dialog']))
window.a11yDialogs = []
dialogEls.map((el, i) => a11yDialogs[i] = new A11yDialog(el))

if (this.shown && event.which === ESCAPE_KEY) {
  event.preventDefault();
  console.log('close escape')
  // if we have the current activeElement is all fine, but the user could have clicked the dialog body, losing focus, therefore error is thown    
  let ae = document.activeElement.closest('[data-a11y-dialog]');
  if(ae){
    window.A11yDialogs.find(instance => intance.node.id === ae.id).hide()
  } else {
   // just hide the last one in array
    window.A11yDialogs[window.A11yDialogs.length - 1].hide()
  }
}

2. .. or, as I don't like to make personal affairs public, we could instead save a reference for each instance inside each Dialog node on the constructor, similar to other plugins

 /**
   * Define the constructor to instantiate a dialog
   *
   * @constructor
   * @param {Element} node
   * @param {(NodeList | Element | string)} targets
   */
  function A11yDialog (node, targets) {
  ...
  // Keep a reference of the instance on the node
   this.node._A11yDialog = this;
}

and then we could check for open dialog siblings on 'escape click'

A11yDialog.prototype.getOpenSiblings = function () {
  // is open and isn't mysefl
  this.openSiblings = $$('[data-a11y-dialog]').filter(sib => sib._A11yDialog.shown && sib.id !== this.node.id)
}

And then same on Escape click event

if (this.shown && event.which === ESCAPE_KEY) {
  event.preventDefault();
  console.log('close escape')
  // if we have the current activeElement is all fine, but the user could have clicked the dialog body, losing focus, therefore error is thown    
  let ae = document.activeElement.closest('[data-a11y-dialog]');
  if(ae){
   ae._A11yDialog.hide()
  } else {
    // just hide the last one in array
    this._getOpenSiblings()
    this.openSiblings[this.openSiblings.length -1].hide()
  }
}

Note: focusedBeforeDialog behaviour starts failing if we have >2 dialogs. I have no eloquent explanation for this yet, but saving this element on the constructor as this.focusedBeforeDialog solves it.

demo here

Final Notes

I'm still not happy with any of this solutions because they are no-solutions. We're relying on DOM order to check for the top most Dialog and i don't like the smell of that idea. I don't like to dictate how other people should organize their DOM, i don't event know if the dialog is there at DOM load.

Shameless question (request for advice/help): how would you handle it?
from top of my mind:

• Maybe we should add an order attribute data-a11y-dialog-order="" on open, by checking other opened instances
• Maybe that order attribute could be a simple timeStamp (Date.now())

I've back to this because:

1. I have a confirmation dialog situation (are you sure? Y/N) that needs to be open on submit of the first. The first part of this "blog post" deals with it as i don't need to keep it open after submit and can close them all.
2. I was thinking about making an accessible flyout menu which would fork 95% of the code of this repo. and flyout menus can be dropdowns or sidedrawers, layout is not up to me to decide, but multiple instances need to be open at the same time.

Well that's a wrap, sry for the long post! Cheers!

[KittyGiraudel]

Hey Renato! Thank you so much for your research, that’s good to see what the problems are when using multiple nested dialogs.

Here is the thing though, I’m not convinced updating the library to support that case is a good idea. Nested dialogs —in my opinion— is more of a hack than an actual regular design pattern.

That being said, I think it might be interesting to update the documentation with an explanation and a link to this comment of yours if someone ever need to handle nested dialogs. As you said, it’s only a fork away.

What do you think?

[renatodeleao]

Yup totally understandable and I agree. As i've said at the beginning this is a shady/questionable UX pattern and one that is not referenced anywhere in the native Dialog spec so it makes no sense to support it.

Even if we do want to support it, this is still a no-solution for me yet as well, and we shouldn't push that kind of code to libraries.

Sometimes we gotta to do what we gotta do to complete the job and a reference may help others doing the same. Googling [multiple modals open] shows that this is a common "problem" even in popular libraries like bootstrap (that also don't support it by default), but one that should treated as an exception, and exceptions require custom code as this one.

Thanks for taking time to read this anyway. Cheers!

[KittyGiraudel]

Thank you for taking the time to investigate this issue. If you’d like to PR the docs to add an explanation and a link to this issue, that would be fantastic. :)

[renatodeleao]

@hugogiraudel Sure, i'll gladly do it!