Modal: Fix backdrop not readjusting when height changes #15881

hnrch02 · 2015-02-24T00:09:15Z

Fixes #15136.
Closes #15345.
Closes #15314.
Refs #14724, #14927.

Effectively reverts #14724 but still fixes #13816. The backdrop will now again be appended to the body and not the .modal container.
Also includes a workaround for when an opening accordion makes the modal overflow to minimize jumping on the dialog. (It's more of a proof of concept, I understand if we take it out before merging.)

/cc @cvrebert @fat @XhmikosR

Sorry it took so long guys.

cvrebert · 2015-02-24T10:31:07Z

How does this fix #15106 exactly? handleUpdate() is still undocumented.

DaSchTour · 2015-02-24T12:00:08Z

I thought handleUpdate() wouldn't be necessary anymore?

…c content height Fixes twbs#15106 — Document how to properly handle modals with dynamic content height X-Ref: twbs#15881

hnrch02 · 2015-02-25T00:46:38Z

To clarify: handleUpdate is no longer needed to adjust the height of the backdrop, it now only centers the modal by applying padding when the modal is overflowing. So you'd only need to call it if the height change made the modal display a scrollbar when it before didn't have one. That's not a new problem though, it's been around for much longer.

cvrebert · 2015-02-25T07:50:17Z

js/tests/unit/modal.js

@@ -167,7 +167,7 @@ $(function () {
        assert.notEqual($('#modal-test').length, 0, 'modal insterted into dom')
        $('.contents').click()
        assert.ok($('#modal-test').is(':visible'), 'modal visible')
-        $('#modal-test .modal-backdrop').click()


Forgive my confusion, but why isn't this $('.modal-backdrop').click()? Isn't clicking on the backdrop rather than the modal itself the whole point of this test?

Well, you'll never click on the backdrop. The event listener for the click event is on the modal element.

[Ref twbs#15881] Use Explicit Values for javascript variables rather than chained ones. From twbs#15881 (comment)

[Ref #15881] Use Explicit JS Variable Declarations rather than Chained

cvrebert · 2015-03-02T02:36:14Z

While #15910 did add a note about needing to call handleUpdate(), the API docs are still missing an entry for it.

hnrch02 · 2015-03-02T10:35:41Z

True, but since handleUpdate can't be called with the string argument to $.fn.modal it would be kinda out of place there.

fat · 2015-03-03T02:10:41Z

js/modal.js

@@ -64,6 +66,28 @@
    this.resize()

    this.$element.on('click.dismiss.bs.modal', '[data-dismiss="modal"]', $.proxy(this.hide, this))
+    this.$element.on('show.bs.collapse hide.bs.collapse', function (e) {


i feel like this is making a bloated method, worse – can we move some of this logic into a handler

maybe like

this.$element.on('show.bs.collapse hide.bs.collapse', this.adjustDialogOnShowHide)

or some better name 👍

Fixes #15136. Closes #15345. Closes #15314. Refs #14724, #14927.

fat · 2015-03-03T04:49:42Z

lgtm

Modal: Fix backdrop not readjusting when height changes

cvrebert · 2015-03-03T04:56:28Z

True, but since handleUpdate can't be called with the string argument to $.fn.modal it would be kinda out of place there.

@hnrch02 I think that's an oversight in and of itself. We don't intentionally make folks jump through the .data('bs.foo') hoop with anything else...

hnrch02 · 2015-03-03T05:00:36Z

@cvrebert So you suggest we add $().modal('handleUpdate')?

cvrebert · 2015-03-03T05:02:45Z

Yeah.

hnrch02 · 2015-03-03T05:36:25Z

It is actually already accessible that way, we just need to document it.

DaSchTour · 2015-03-03T15:09:06Z

So when can we expect 3.3.4?

mdo · 2015-03-29T03:40:30Z

Looks like the CSS changes here caused #16148. Are they necessary?

allspain · 2015-04-07T22:11:42Z

so is there a fix for this & #16148 being worked on?

cvrebert · 2015-04-07T23:42:53Z

@allspain "This" is a pull request, so it's not clear what problem you're referring to.

cvrebert · 2015-04-07T23:44:23Z

@hnrch02 Can you answer mdo's question?

allspain · 2015-04-07T23:53:54Z

Sorry I was referring @mdo 's question, and the bug (#16148) this pull request may have introduced. Should have been more concise.

hnrch02 · 2015-05-08T21:40:12Z

@mdo Just wanna follow up here: I merely reverted the CSS to the pre 3.3.0/#15136 state. #16148 is a problem that exists since at least v2.

hnrch02 added the js label Feb 24, 2015

hnrch02 added this to the v3.3.4 milestone Feb 24, 2015

This was referenced Feb 24, 2015

Modal Backdrop (nearly) never fits #15136

Closed

Appending content to a modal that is already displayed does not update the backdrop height #15418

Closed

This was referenced Feb 24, 2015

Causes the backdrop of modal extend around the body #15345

Closed

fix the modal bug #15314

Closed

kkirsche mentioned this pull request Feb 24, 2015

[Fixes #15106] Document how to properly handle modals with dynamic height #15910

Closed

hnrch02 force-pushed the fix-modal-backdrop branch from 29a5135 to 21b4e24 Compare February 25, 2015 00:53

cvrebert reviewed Feb 25, 2015
View reviewed changes

kkirsche pushed a commit to kkirsche/bootstrap that referenced this pull request Feb 25, 2015

[Ref twbs#15881] Use Explicit Values rather than Chain

238be65

[Ref twbs#15881] Use Explicit Values for javascript variables rather than chained ones. From twbs#15881 (comment)

kkirsche mentioned this pull request Feb 25, 2015

[Ref #15881] Use Explicit JS Variable Declarations rather than Chained #15923

Merged

hnrch02 added a commit that referenced this pull request Feb 26, 2015

Merge pull request #15923 from kkirsche/useExplicitVariableDeclaration

cac6cfc

[Ref #15881] Use Explicit JS Variable Declarations rather than Chained

hnrch02 mentioned this pull request Feb 26, 2015

fixes #13816 Modal dialog closes when dragging and releasing mouse button outside dialog #14724

Merged

hnrch02 force-pushed the fix-modal-backdrop branch from 21b4e24 to 0a393d6 Compare March 1, 2015 13:37

hnrch02 mentioned this pull request Mar 1, 2015

Bootstrap 3 modal leaves blank space on mobile browsers #15957

Closed

fat reviewed Mar 3, 2015
View reviewed changes

Modal: Fix backdrop not readjusting when height changes

f5beebe

Fixes #15136. Closes #15345. Closes #15314. Refs #14724, #14927.

hnrch02 force-pushed the fix-modal-backdrop branch from 0a393d6 to f5beebe Compare March 3, 2015 04:27

hnrch02 added a commit that referenced this pull request Mar 3, 2015

Merge pull request #15881 from twbs/fix-modal-backdrop

200f6fb

Modal: Fix backdrop not readjusting when height changes

hnrch02 merged commit 200f6fb into master Mar 3, 2015

hnrch02 deleted the fix-modal-backdrop branch March 3, 2015 04:54

cvrebert mentioned this pull request Mar 3, 2015

v3.3.4 ship list #15624

Closed

kevinoid mentioned this pull request Mar 3, 2015

With $modal, Bootstrap 3.3.0 works, Bootstrap 3.3.1 does not angular-ui/bootstrap#3184

Closed

chuckd mentioned this pull request Mar 3, 2015

Modal backdrop broken with bootstrap 3.3.1 angular-ui/bootstrap#2970

Closed

hnrch02 mentioned this pull request Mar 5, 2015

Fix modal backdrop overlaying the modal's scrollbar #14927

Merged

This was referenced Mar 6, 2015

Update for Bootstrap 3.3: render a .modal-backdrop inside of its .modal. react-bootstrap/react-bootstrap#330

Closed

event.stopPropagation does not work for a modal('show') #16056

Closed

cvrebert mentioned this pull request Mar 13, 2015

.modal-backdrop is not appearing because _modals.scss is missing "bottom: 0" twbs/bootstrap-sass#865

Closed

imcotton mentioned this pull request Mar 17, 2015

Update to support angular 1.3 angular-ui/bootstrap#2830

Closed

leftclickben mentioned this pull request Apr 14, 2015

Modal behind backdrop when parent is position:relative with z-index #16148

Closed

cvrebert mentioned this pull request May 6, 2015

Change location of modal backdrop to fix z-index issue #16432

Closed

marlo-longley mentioned this pull request Aug 12, 2022

Bootstrap modal z-index issue sul-dlss/SearchWorks#1004

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modal: Fix backdrop not readjusting when height changes #15881

Modal: Fix backdrop not readjusting when height changes #15881

hnrch02 commented Feb 24, 2015

cvrebert commented Feb 24, 2015

DaSchTour commented Feb 24, 2015

hnrch02 commented Feb 25, 2015

cvrebert Feb 25, 2015

hnrch02 Feb 25, 2015

cvrebert commented Mar 2, 2015

hnrch02 commented Mar 2, 2015

fat Mar 3, 2015

fat commented Mar 3, 2015

cvrebert commented Mar 3, 2015

hnrch02 commented Mar 3, 2015

cvrebert commented Mar 3, 2015

hnrch02 commented Mar 3, 2015

DaSchTour commented Mar 3, 2015

mdo commented Mar 29, 2015

allspain commented Apr 7, 2015

cvrebert commented Apr 7, 2015

cvrebert commented Apr 7, 2015

allspain commented Apr 7, 2015

hnrch02 commented May 8, 2015

Modal: Fix backdrop not readjusting when height changes #15881

Modal: Fix backdrop not readjusting when height changes #15881

Conversation

hnrch02 commented Feb 24, 2015

cvrebert commented Feb 24, 2015

DaSchTour commented Feb 24, 2015

hnrch02 commented Feb 25, 2015

cvrebert Feb 25, 2015

Choose a reason for hiding this comment

hnrch02 Feb 25, 2015

Choose a reason for hiding this comment

cvrebert commented Mar 2, 2015

hnrch02 commented Mar 2, 2015

fat Mar 3, 2015

Choose a reason for hiding this comment

fat commented Mar 3, 2015

cvrebert commented Mar 3, 2015

hnrch02 commented Mar 3, 2015

cvrebert commented Mar 3, 2015

hnrch02 commented Mar 3, 2015

DaSchTour commented Mar 3, 2015

mdo commented Mar 29, 2015

allspain commented Apr 7, 2015

cvrebert commented Apr 7, 2015

cvrebert commented Apr 7, 2015

allspain commented Apr 7, 2015

hnrch02 commented May 8, 2015