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.

Sign up for GitHub

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

fix(Modal): scrolling disabled when modal unmounts #2394

Closed
wants to merge 1 commit into from
Closed

fix(Modal): scrolling disabled when modal unmounts #2394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/modules/Modal/Modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,8 @@ class Modal extends Component {
static Description = ModalDescription
static Actions = ModalActions

scrollingWasDefinedAtMountTime = false

componentWillUnmount() {
debug('componentWillUnmount()')
this.handlePortalUnmount()
Expand Down Expand Up @@ -217,7 +219,13 @@ class Modal extends Component {
mountNode.classList.remove('blurring')
mountNode.classList.remove('dimmable')
mountNode.classList.remove('dimmed')
mountNode.classList.remove('scrolling')

// Restore scrolling to original value before mount.
if (this.scrollingWasDefinedAtMountTime) {
mountNode.classList.add('scrolling')
} else {
mountNode.classList.remove('scrolling')
}

cancelAnimationFrame(this.animationRequestId)
_.invoke(this.props, 'onUnmount', e, this.props)
Expand All @@ -229,6 +237,8 @@ class Modal extends Component {
const { dimmer } = this.props
const mountNode = this.getMountNode()

this.scrollingWasDefinedAtMountTime = mountNode.classList.contains('scrolling')

if (dimmer) {
mountNode.classList.add('dimmable')
mountNode.classList.add('dimmed')
Expand Down
47 changes: 47 additions & 0 deletions test/specs/modules/Modal/Modal-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ describe('Modal', () => {
it('renders to the document body', () => {
wrapperMount(<Modal open />)
assertBodyContains('.ui.modal')
wrapper.unmount()
})

it('renders child text', () => {
Expand All @@ -70,6 +71,7 @@ describe('Modal', () => {
document.querySelector('.ui.modal')
.innerText
.should.equal('child text')
wrapper.unmount()
})

it('renders child components', () => {
Expand All @@ -80,6 +82,7 @@ describe('Modal', () => {
.querySelector('.ui.modal')
.querySelector('[data-child]')
.should.not.equal(null, 'Modal did not render the child component.')
wrapper.unmount()
})

it("spreads the user's style prop on the Modal", () => {
Expand All @@ -90,6 +93,7 @@ describe('Modal', () => {

element.style.should.have.property('marginTop', '1em')
element.style.should.have.property('top', '0px')
wrapper.unmount()
})

describe('actions', () => {
Expand All @@ -99,6 +103,7 @@ describe('Modal', () => {
assertBodyContains('.ui.modal')
domEvent.click('.ui.modal .actions .button')
assertBodyContains('.ui.modal', false)
wrapper.unmount()
})

it('calls shorthand onActionClick callback', () => {
Expand All @@ -109,6 +114,7 @@ describe('Modal', () => {
onActionClick.should.not.have.been.called()
domEvent.click('.ui.modal .actions .button')
onActionClick.should.have.been.calledOnce()
wrapper.unmount()
})
})

Expand All @@ -123,13 +129,15 @@ describe('Modal', () => {

onActionClick.should.have.been.calledOnce()
onActionClick.should.have.been.calledWithMatch(event, props)
wrapper.unmount()
})
})

describe('open', () => {
it('is not open by default', () => {
wrapperMount(<Modal />)
assertBodyContains('.ui.modal.open', false)
wrapper.unmount()
})

it('is passed to Portal open', () => {
Expand Down Expand Up @@ -157,21 +165,25 @@ describe('Modal', () => {
it('does not show the modal when false', () => {
wrapperMount(<Modal open={false} />)
assertBodyContains('.ui.modal', false)
wrapper.unmount()
})

it('does not show the dimmer when false', () => {
wrapperMount(<Modal open={false} />)
assertBodyContains('.ui.dimmer', false)
wrapper.unmount()
})

it('shows the dimmer when true', () => {
wrapperMount(<Modal open dimmer />)
assertBodyContains('.ui.dimmer')
wrapper.unmount()
})

it('shows the modal when true', () => {
wrapperMount(<Modal open />)
assertBodyContains('.ui.modal')
wrapper.unmount()
})

it('shows the modal and dimmer on changing from false to true', () => {
Expand All @@ -183,6 +195,7 @@ describe('Modal', () => {

assertBodyContains('.ui.modal')
assertBodyContains('.ui.dimmer')
wrapper.unmount()
})

it('hides the modal and dimmer on changing from true to false', () => {
Expand All @@ -194,13 +207,15 @@ describe('Modal', () => {

assertBodyContains('.ui.modal', false)
assertBodyContains('.ui.dimmer', false)
wrapper.unmount()
})
})

describe('basic', () => {
it('adds basic to the modal className', () => {
wrapperMount(<Modal basic open />)
assertBodyContains('.ui.basic.modal')
wrapper.unmount()
})
})

Expand All @@ -211,6 +226,7 @@ describe('Modal', () => {
sizes.forEach((size) => {
wrapperMount(<Modal size={size} open />)
assertBodyContains(`.ui.${size}.modal`)
wrapper.unmount()
})
})
})
Expand All @@ -225,6 +241,7 @@ describe('Modal', () => {
it('is present by default', () => {
wrapperMount(<Modal open />)
assertBodyContains('.ui.dimmer')
wrapper.unmount()
})
})

Expand All @@ -242,18 +259,21 @@ describe('Modal', () => {
it('adds a dimmer to the body', () => {
wrapperMount(<Modal open dimmer />)
assertBodyContains('.ui.page.modals.dimmer.transition.visible.active')
wrapper.unmount()
})
})

describe('false', () => {
it('does not render a dimmer', () => {
wrapperMount(<Modal open dimmer={false} />)
assertBodyClasses('dimmable dimmed blurring', false)
wrapper.unmount()
})

it('does not add any dimmer classes to the body', () => {
wrapperMount(<Modal open dimmer={false} />)
assertBodyClasses('dimmable dimmed blurring', false)
wrapper.unmount()
})
})

Expand All @@ -271,6 +291,7 @@ describe('Modal', () => {
it('adds a dimmer to the body', () => {
wrapperMount(<Modal open dimmer='blurring' />)
assertBodyContains('.ui.page.modals.dimmer.transition.visible.active')
wrapper.unmount()
})
})

Expand All @@ -288,6 +309,7 @@ describe('Modal', () => {
it('adds an inverted dimmer to the body', () => {
wrapperMount(<Modal open dimmer='inverted' />)
assertBodyContains('.ui.inverted.page.modals.dimmer.transition.visible.active')
wrapper.unmount()
})
})
})
Expand All @@ -299,6 +321,7 @@ describe('Modal', () => {

wrapper.find('#trigger').simulate('click')
spy.should.have.been.calledOnce()
wrapper.unmount()
})

it('is not called on body click', () => {
Expand All @@ -307,6 +330,7 @@ describe('Modal', () => {

domEvent.click('body')
spy.should.not.have.been.called()
wrapper.unmount()
})
})

Expand All @@ -322,62 +346,71 @@ describe('Modal', () => {

domEvent.click('.ui.dimmer')
spy.should.have.been.calledOnce()
wrapper.unmount()
})

it('is called on click outside of the modal', () => {
wrapperMount(<Modal onClose={spy} defaultOpen />)

domEvent.click(document.querySelector('.ui.modal').parentNode)
spy.should.have.been.calledOnce()
wrapper.unmount()
})

it('is not called on click inside of the modal', () => {
wrapperMount(<Modal onClose={spy} defaultOpen />)

domEvent.click(document.querySelector('.ui.modal'))
spy.should.not.have.been.called()
wrapper.unmount()
})

it('is not called on body click', () => {
wrapperMount(<Modal onClose={spy} defaultOpen />)

domEvent.click('body')
spy.should.not.have.been.calledOnce()
wrapper.unmount()
})

it('is called when pressing escape', () => {
wrapperMount(<Modal onClose={spy} defaultOpen />)

domEvent.keyDown(document, { key: 'Escape' })
spy.should.have.been.calledOnce()
wrapper.unmount()
})

it('is not called when the open prop changes to false', () => {
wrapperMount(<Modal onClose={spy} defaultOpen />)

wrapper.setProps({ open: false })
spy.should.not.have.been.called()
wrapper.unmount()
})

it('is not called when open changes to false programmatically', () => {
wrapperMount(<Modal onClose={spy} defaultOpen />)

wrapper.setProps({ open: false })
spy.should.not.have.been.called()
wrapper.unmount()
})

it('is not called on dimmer click when closeOnDimmerClick is false', () => {
wrapperMount(<Modal onClose={spy} defaultOpen closeOnDimmerClick={false} />)

domEvent.click('.ui.dimmer')
spy.should.not.have.been.called()
wrapper.unmount()
})

it('is not called on body click when closeOnDocumentClick is false', () => {
wrapperMount(<Modal onClose={spy} defaultOpen closeOnDocumentClick={false} />)

domEvent.click(document.body)
spy.should.not.have.been.called()
wrapper.unmount()
})
})

Expand All @@ -388,6 +421,7 @@ describe('Modal', () => {
document.body.childElementCount.should.equal(1)
domEvent.keyDown(document, { key: 'Escape' })
document.body.childElementCount.should.equal(0)
wrapper.unmount()
})

it('closes the modal when true and Escape is pressed', () => {
Expand All @@ -396,6 +430,7 @@ describe('Modal', () => {
document.body.childElementCount.should.equal(1)
domEvent.keyDown(document, { key: 'Escape' })
document.body.childElementCount.should.equal(0)
wrapper.unmount()
})

it('does not close the modal when false and Escape is pressed', () => {
Expand All @@ -404,6 +439,7 @@ describe('Modal', () => {
document.body.childElementCount.should.equal(1)
domEvent.keyDown(document, { key: 'Escape' })
document.body.childElementCount.should.equal(1)
wrapper.unmount()
})
})

Expand All @@ -417,13 +453,15 @@ describe('Modal', () => {
document.body.childElementCount.should.equal(1)
domEvent.click(document.body)
document.body.childElementCount.should.equal(0)
wrapper.unmount()
})
it('does not close the modal on document click when false', () => {
wrapperMount(<Modal defaultOpen closeOnDocumentClick={false} />)

document.body.childElementCount.should.equal(1)
domEvent.click(document.body)
document.body.childElementCount.should.equal(1)
wrapper.unmount()
})
})

Expand All @@ -434,23 +472,27 @@ describe('Modal', () => {

wrapperMount(<Modal mountNode={mountNode} open>foo</Modal>)
assertNodeContains(mountNode, '.ui.modal')
wrapper.unmount()
})
})

describe('closeIcon', () => {
it('is not present by default', () => {
wrapperMount(<Modal open>foo</Modal>)
assertBodyContains('.ui.modal .icon', false)
wrapper.unmount()
})

it('defaults to `close` when boolean', () => {
wrapperMount(<Modal open closeIcon>foo</Modal>)
assertBodyContains('.ui.modal .icon.close')
wrapper.unmount()
})

it('is present when passed', () => {
wrapperMount(<Modal open closeIcon='bullseye'>foo</Modal>)
assertBodyContains('.ui.modal .icon.bullseye')
wrapper.unmount()
})

it('triggers onClose when clicked', () => {
Expand All @@ -459,6 +501,7 @@ describe('Modal', () => {
wrapperMount(<Modal onClose={spy} open closeIcon='bullseye'>foo</Modal>)
domEvent.click('.ui.modal .icon.bullseye')
spy.should.have.been.calledOnce()
wrapper.unmount()
})
})

Expand All @@ -476,6 +519,7 @@ describe('Modal', () => {
it('does not add the scrolling class to the body by default', () => {
wrapperMount(<Modal open />)
assertBodyClasses('scrolling', false)
wrapper.unmount()
})

it('adds the scrolling class to the body when taller than the window', (done) => {
Expand All @@ -484,6 +528,7 @@ describe('Modal', () => {

requestAnimationFrame(() => {
assertBodyClasses('scrolling')
wrapper.unmount()
done()
})
})
Expand All @@ -500,6 +545,7 @@ describe('Modal', () => {

requestAnimationFrame(() => {
assertBodyClasses('scrolling', false)
wrapper.unmount()
done()
})
})
Expand All @@ -520,6 +566,7 @@ describe('Modal', () => {
wrapper.setProps({ open: true })
requestAnimationFrame(() => {
assertBodyClasses('scrolling')
wrapper.unmount()
done()
})
})
Expand Down