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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested dialogs #80

Closed
renatodeleao opened this Issue Feb 21, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@renatodeleao
Copy link

renatodeleao commented Feb 21, 2018

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

@HugoGiraudel

This comment has been minimized.

Copy link
Collaborator

HugoGiraudel commented Feb 22, 2018

Hey there!

I must say never tried to open a dialog from another one, so I鈥檓 going to go with: that doesn鈥檛 surprise me that it doesn鈥榯 work. :D

It shouldn鈥檛 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

This comment has been minimized.

Copy link

renatodeleao commented Feb 22, 2018

@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

This comment has been minimized.

Copy link

renatodeleao commented Mar 31, 2018

@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()
  }
}
  1. .. 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!

@HugoGiraudel

This comment has been minimized.

Copy link
Collaborator

HugoGiraudel commented Mar 31, 2018

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

Here is the thing though, I鈥檓 not convinced updating the library to support that case is a good idea. Nested dialogs 鈥攊n 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鈥檚 only a fork away.

What do you think?

@renatodeleao

This comment has been minimized.

Copy link

renatodeleao commented Apr 3, 2018

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!

@HugoGiraudel

This comment has been minimized.

Copy link
Collaborator

HugoGiraudel commented Apr 3, 2018

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

@renatodeleao

This comment has been minimized.

Copy link

renatodeleao commented Apr 3, 2018

@HugoGiraudel Sure man, i'll gladly do it!

@HugoGiraudel HugoGiraudel changed the title Error if open modal from within another one (multiple modals open) Nested dialogs Apr 4, 2018

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