From 7e23c56746654fedf83d2e1680fbd2f4673fee3f Mon Sep 17 00:00:00 2001 From: Morgan Zolob Date: Wed, 11 Dec 2019 17:29:42 -0800 Subject: [PATCH 1/3] Fix motion popups sometimes getting stuck (#157) If the popup transitioned from visible to invisible before the status reached 'motion', the popup would get stuck in a visible state. This changes the code to immediately set status to 'stable' during a transition from visible to invisible if the status is something before 'motion'. Also fixes an issue where an animation frame could change the status to something invalid after a transition from visible to invisible. --- src/Popup.tsx | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Popup.tsx b/src/Popup.tsx index c956b44c..51d48030 100644 --- a/src/Popup.tsx +++ b/src/Popup.tsx @@ -118,7 +118,14 @@ class Popup extends Component { // Init render should always be stable newState.status = 'stable'; } else if (visible !== prevVisible) { - newState.status = visible || supportMotion(mergedMotion) ? null : 'stable'; + if ( + visible || + (supportMotion(mergedMotion) && ['motion', 'AfterMotion', 'stable'].includes(status)) + ) { + newState.status = null; + } else { + newState.status = 'stable'; + } if (visible) { newState.alignClassName = null; @@ -136,6 +143,9 @@ class Popup extends Component { const { status } = this.state; const { getRootDomNode, visible, stretch } = this.props; + // If there is a pending state update, cancel it, a new one will be set if necessary + this.cancelFrameState(); + if (visible && status !== 'stable') { switch (status) { case null: { From cbcd96809fb0952d5d2a1ab460cacf528f55a1b4 Mon Sep 17 00:00:00 2001 From: Morgan Zolob Date: Wed, 15 Jan 2020 16:33:33 -0800 Subject: [PATCH 2/3] Add tests --- src/Popup.tsx | 5 ++- tests/popup.test.jsx | 85 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 tests/popup.test.jsx diff --git a/src/Popup.tsx b/src/Popup.tsx index 51d48030..ce28a08c 100644 --- a/src/Popup.tsx +++ b/src/Popup.tsx @@ -250,7 +250,10 @@ class Popup extends Component { } cancelFrameState = () => { - raf.cancel(this.nextFrameId); + if (this.nextFrameId) { + raf.cancel(this.nextFrameId); + this.nextFrameId = null; + } }; renderPopupElement = () => { diff --git a/tests/popup.test.jsx b/tests/popup.test.jsx new file mode 100644 index 00000000..1dfff1ba --- /dev/null +++ b/tests/popup.test.jsx @@ -0,0 +1,85 @@ +import React from 'react'; +import { mount } from 'enzyme'; +import raf from 'raf'; +import Popup from '../src/Popup'; + +jest.mock('raf', () => { + const rafMock = jest.fn(() => 1); + rafMock.cancel = jest.fn(); + return rafMock; +}); + +describe('Popup', () => { + afterEach(() => { + raf.mockClear(); + raf.cancel.mockClear(); + }); + + describe('Popup getDerivedStateFromProps status behavior', () => { + it('returns stable on init', () => { + const props = { visible: false }; + const state = { prevVisible: null, status: 'something' }; + + expect(Popup.getDerivedStateFromProps(props, state).status).toBe('stable'); + }); + + it('does not change when visible is unchanged', () => { + const props = { visible: true }; + const state = { prevVisible: true, status: 'something' }; + + expect(Popup.getDerivedStateFromProps(props, state).status).toBe('something'); + }); + + it('returns null when visible is changed to true', () => { + const props = { visible: true }; + const state = { prevVisible: false, status: 'something' }; + + expect(Popup.getDerivedStateFromProps(props, state).status).toBe(null); + }); + + it('returns stable when visible is changed to false and motion is not supported', () => { + const props = { visible: false }; + const state = { prevVisible: true, status: 'something' }; + + expect(Popup.getDerivedStateFromProps(props, state).status).toBe('stable'); + }); + + it('returns null when visible is changed to false and motion is started', () => { + const props = { + visible: false, + motion: { + motionName: 'enter', + }, + }; + const state = { prevVisible: true, status: 'motion' }; + + expect(Popup.getDerivedStateFromProps(props, state).status).toBe(null); + }); + + it('returns stable when visible is changed to false and motion is not started', () => { + const props = { + visible: false, + motion: { + motionName: 'enter', + }, + }; + const state = { prevVisible: true, status: 'beforeMotion' }; + + expect(Popup.getDerivedStateFromProps(props, state).status).toBe('stable'); + }); + }); + + it('Popup cancels pending animation frames on update', () => { + const wrapper = mount( + +
popup content
+ , + ); + + expect(raf).toHaveBeenCalledTimes(1); + expect(raf.cancel).not.toHaveBeenCalled(); + + wrapper.setProps({ visible: false }); + expect(raf.cancel).toHaveBeenCalledTimes(1); + }); +}); From fca787b6bde7c44e803bbf0fdf5e7f340509d7c8 Mon Sep 17 00:00:00 2001 From: Morgan Zolob Date: Wed, 22 Jan 2020 16:39:25 -0800 Subject: [PATCH 3/3] Remove if statement around raf.cancel --- src/Popup.tsx | 5 +---- tests/popup.test.jsx | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/Popup.tsx b/src/Popup.tsx index ce28a08c..51d48030 100644 --- a/src/Popup.tsx +++ b/src/Popup.tsx @@ -250,10 +250,7 @@ class Popup extends Component { } cancelFrameState = () => { - if (this.nextFrameId) { - raf.cancel(this.nextFrameId); - this.nextFrameId = null; - } + raf.cancel(this.nextFrameId); }; renderPopupElement = () => { diff --git a/tests/popup.test.jsx b/tests/popup.test.jsx index 1dfff1ba..221e35a7 100644 --- a/tests/popup.test.jsx +++ b/tests/popup.test.jsx @@ -77,8 +77,8 @@ describe('Popup', () => { ); expect(raf).toHaveBeenCalledTimes(1); - expect(raf.cancel).not.toHaveBeenCalled(); + raf.cancel.mockClear(); wrapper.setProps({ visible: false }); expect(raf.cancel).toHaveBeenCalledTimes(1); });