Breaking change in 0.4.9: clicking the container closes the modal

[robinmetral]

Hi,

First, thanks a lot for this library!

Just wanted to report that 32140dd (released in 0.4.9) introduced a major breaking change:

Screen.Recording.2021-12-12.at.16.19.01.mov

Note: I'm running the docs locally in this recording. You can't see it in the deployed version, which I assume is running an older version of Micromodal.

🐛 What the issue is

The bug is in this onClick method:

https://github.com/ghosh/Micromodal/blob/a55c5fd2b64250f4569ba068b24001c3e7ac6d4e/lib/src/index.js#L135-L144

Line 138 (newly added) is the problem here, since most modals will follow the demo markup and look like this:

<div id="modal-1" aria-hidden="true">
  <div class="modal__overlay" tabindex="-1" data-micromodal-close>
    <div class="modal__container" role="dialog" aria-modal="true">
      <!-- content -->
    </div>
  </div>
</div>

Any click event on the container (for example its padding, or between children elements) will effectively close the modal because the container's parent (e.target.parentNode) is the overlay—which has the closeTrigger.

Reproducing

1. clone the repo
2. yarn
3. yarn lib:build (make sure the latest lib is used by the docs)
4. yarn docs:dev (run the docs)
5. try to click the container, notice the bug

I'm happy to submit a PR to fix this, but I don't have context on why this change was applied in the first place¹. Can we just remove this line? Or were you trying to fix something else with it?

Footnotes

1. The changelog says 🐞 BUGFIX Correctly closes modal when clicking on nested close elements, which I don't really understand without context ↩

[ignatiusnikulsons]

Can confirm, having the same issue in 0.4.10.

[Kilbourne]

A workaround is adding extra wrapper between modal__overlay and modal__container.

I did a search and the change was applied to fix this issue : Close button doesn't work when contains any elements.

A simple solution could be to check if the parent hasn't the modal__overlay class.
A better one would be to add a new data-micromodal-overlay attribute to differentiate between close triggers: check the anchestors for data-micromodal-close; do not check the parent (only the target) for data-micromodal-overlay

[ignatiusnikulsons]

Yes, another wrapper was an obvious temporary solution, but it's kind of painful when a project has 100+ modals with different layout and design options.

For now, I've just reverted back to 0.4.8 until there will be a fix.

[jclark-vh]

The work around of adding the extra div wrapper causes modal windows that need to scroll, to not scroll.

[robbytx]

@jclark-vh I'm not seeing the issue with scrolling, so I suspect that the issue is specific to your CSS.

As long as the micromodal-container is the div with the (max-)height and overflow(-y): auto, then it should scroll appropriately, unless you've given the wrapper div an explicitly larger height and something like overflow: hidden.

[auriel-klasovksy]

To fix this, I have changed the onClick event to only check event.target (not its parent) and also have placed the preventDefault and stopPropagation calls outside the if statement. like this:

onClick (event) {
      if (event.target.hasAttribute(this.config.closeTrigger)) {
        this.closeModal(event)
      }
      event.preventDefault()
      event.stopPropagation()
    }

This way the event will only fire when the user clicks on an element with the closeTrigger attribute. I honestly don't know why it was not done this way to begin with, hope I am not introducing new bugs to my site.

[cfxd]

Bummer that this broke in 0.4.10 but I solved it by creating a full size pseudo element over the child elements. Worked this time!

[heyflo]

See in #505 for a quick & simple fix 👍

[dkniffin]

I can confirm this is still occurring in the latest version (0.5.2), as seen in the codepen here: Codepen example

This might also be related to #287 or have a similar fix.

If anyone wants to open a PR for this, I'd be happy to review it. Otherwise, I'll work on a fix soon.