Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modal event.preventDefault() for show.bs.modal: disables modals with fade class from being displayed again in V4 & V5. #34055

Closed
RichDeBourke opened this issue May 21, 2021 · 2 comments · Fixed by #34085

Comments

@RichDeBourke
Copy link

When an event listener attached to a Bootstrap modal (with the fade class) for show.bs.modal returns event.preventDefault(), the modal is not displayed (the correct behavior) the first time, but it will no longer be possible to trigger the modal. The button associated with the modal cannot cause the modal to open again.

This is because the modal.js module sets the value for whether the modal is transitioning or not before it triggers the show.bs.modal event. Once displaying a modal is blocked with preventDefault(), there's no way to reset the this._isTransitioning value.

Expected behavior

The expected behavior is that each time the show.bs.modal event is triggered, the event listener can prevent the modal from being shown, but prevention will not impact future requests.

This is how the show.bs.modal event works on V3.4.1. This is also how other show events work (at least for V5 for dropdown, collapse, popover, and offcanvas, which were tested).

Starting with V4.0 and continuing through to the current V5.0.1, after a show.bs.modal listener returns event.preventDefault(), Bootstrap no longer issues an event to the listener.

Why is this happening

The Bootstrap 4 and the Bootstrap 5 documentation recommends return event.preventDefault() as the way to stop a modal from being shown:

Bootstrap 4

$('#myModal').on('show.bs.modal', function (event) {
    if (!data) {
        return event.preventDefault() // stops modal from being shown
    }
})

Bootstrap 5

var myModal = document.getElementById('myModal')

myModal.addEventListener('show.bs.modal', function (event) {
    if (!data) {
        return event.preventDefault() // stops modal from being shown
    }
})

The code in the V4 & V5 modal.js files that processes preventDefault() is in the show function:

Bootstrap 4.6.0

show(relatedTarget) {
    if (this._isShown || this._isTransitioning) {
        return
    }

    if ($(this._element).hasClass(CLASS_NAME_FADE)) {
        this._isTransitioning = true
    }

    const showEvent = $.Event(EVENT_SHOW, {
        relatedTarget
    })

    $(this._element).trigger(showEvent)

    if (this._isShown || showEvent.isDefaultPrevented()) {
        return
    }

    this._isShown = true

Bootstrap 5.0.1

show(relatedTarget) {
    if (this._isShown || this._isTransitioning) {
        return
    }

    if (this._isAnimated()) {
        this._isTransitioning = true
    }

    const showEvent = EventHandler.trigger(this._element, EVENT_SHOW, {
        relatedTarget
    })

    if (this._isShown || showEvent.defaultPrevented) {
        return
    }

    this._isShown = true

For both V4 and V5, the first thing the show function does is to check whether the modal is already being shown (this._isShown) or if the modal is in the process of being shown (this._isTransistioning).

Both V4 and V5 then immediately set the this._isTransitioning value if the modal has the fade class. The value is set before triggering the show.bs.modal event.

If the event handler returns defaultPrevented, the show function is exited, leaving the this._isTransistioning set to true.

The next time the show function is called, the this._isTransistioning value is checked, and since it's true, the show function exits, preventing the modal from being displayed.

The solution

Move setting this._isTransistioning to after evaluating the defaultPrevented value, below the this._isShown = true line, will prevent this._isTransistioning from being set to true with no way to reset the value.

Test cases

Links for V4 and V5 JS Bin test cases are listed below:

To step through the Bootstrap code and see the issue as it happens, a breakpoint can be placed at line 103 in the V4.6.0 modal.js file or line 107 in the V5.0.1 modal.js file (the show(relatedTarget) function).

There are also test cases for V3.4.1 and V4.0.0 to show the listener working on Bootstrap 3 and not working on the first version of Bootstrap 4. A V5.0.1 version with dropdown, collapse, popopen, and offcanvas is provided to show those components working propertly.

Test conditions

  • Operating system and version — Microsoft Windows 10 Pro / Version 10.0.19041 Build 19041
  • Browser and version
    • Chrome — 90.0.4430.212 (Official Build) (64-bit)
    • Firefox — 88.0.1 (64-bit)

Note

The JS Bin examples would validate except for the use of autocomplete="off" for the checkbox inputs. autocomplete is not valid for checkboxes, but it is the recommended solution from MDN Web Docs to control the checkbox status when a page is refreshed.

@alpadev
Copy link
Contributor

alpadev commented May 22, 2021

Holy moly.. The time and efforts you put into that bug report, you could have simply opened a PR with the fix. 😅

If only every bug report looked something like this.. Amazing job you did there, really appreciate it! ❤️

alpadev added a commit that referenced this issue May 22, 2021
In case of a modal with fading enabled, a prevented show event can cause show to not showing the modal anymore.

See #34055
alpadev added a commit that referenced this issue May 22, 2021
In case of a modal with fading enabled, a prevented show event can cause show to not showing the modal anymore.

See #34055
@RichDeBourke
Copy link
Author

alpadev — thanks for creating the Pull Request and getting it into the system for processing. I’ve never done a PR, so it was easier for me to explain (in detail so there would be no questions) what the problem was, where it came from, and a general solution.

The fix for V5 is in the 5.0.2 release and it does work — I cloned one of my test cases and updated the Bootstrap links to the 5.0.2 release.

The next step is the fix for V4. Your Pull Request is in the Inbox for v4.6.1 and is waiting for XhmikosR or js-review to review the request, although it has been in the Inbox for a month now. Hopefully it will be reviewed soon.

XhmikosR added a commit that referenced this issue Jul 25, 2021
In case of a modal with fading enabled, a prevented show event can cause show to not showing the modal anymore.

See #34055

Co-authored-by: XhmikosR <xhmikosr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants