From e1a98590a9278f9f7e7956f3865f315208a88de9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20H=C3=B8egh?= Date: Wed, 28 Feb 2024 09:40:59 +0100 Subject: [PATCH] fix(HeightAnimation): enhance calculation of height (#3335) By using `position: absolute` on the first pain styles, we got a wrong height calculation. --- .../HeightAnimationInstance.ts | 28 ++- .../__tests__/HeightAnimationInstance.test.ts | 230 +++++++++++++++--- .../stories/HeightAnimation.stories.tsx | 63 +++++ .../height-animation/useHeightAnimation.tsx | 26 +- 4 files changed, 288 insertions(+), 59 deletions(-) diff --git a/packages/dnb-eufemia/src/components/height-animation/HeightAnimationInstance.ts b/packages/dnb-eufemia/src/components/height-animation/HeightAnimationInstance.ts index a5cbda2a300..3c6e77083f5 100644 --- a/packages/dnb-eufemia/src/components/height-animation/HeightAnimationInstance.ts +++ b/packages/dnb-eufemia/src/components/height-animation/HeightAnimationInstance.ts @@ -44,6 +44,12 @@ export default class HeightAnimation { isAnimating: boolean __currentHeight: number + firstPaintStyle = { + visibility: 'hidden', + opacity: '0', // prevents before/after elements to be visible + height: 'auto', + } + constructor(opts: HeightAnimationOptions = {}) { this.isInBrowser = typeof window !== 'undefined' this.setState('init') @@ -164,14 +170,6 @@ export default class HeightAnimation { getHeight() { return parseFloat(String(this.elem?.clientHeight)) || null } - firstPaintStyle() { - return { - position: 'absolute', - visibility: 'hidden', - opacity: '0', // prevents before/after elements to be visible - height: 'auto', - } - } getUnknownHeight() { if (!this.elem) { return null @@ -181,13 +179,15 @@ export default class HeightAnimation { return this.__currentHeight } + const width = this.elem.clientWidth const clonedElem = this.elem.cloneNode(true) as HTMLElement this.elem.parentNode?.insertBefore(clonedElem, this.elem.nextSibling) for (const key in this.firstPaintStyle) { clonedElem.style[key] = this.firstPaintStyle[key] } - clonedElem.style.height = 'auto' + clonedElem.style.width = width ? `${String(width)}px` : 'auto' // set width because of the "position: absolute" + clonedElem.style.position = 'absolute' // not a part of the "firstPaintStyle" const height = parseFloat(String(clonedElem.clientHeight)) || @@ -228,6 +228,7 @@ export default class HeightAnimation { return } + this.stop() this.isAnimating = true // make the animation @@ -275,7 +276,7 @@ export default class HeightAnimation { const toHeight = this.getUnknownHeight() this.addEndEvent((e) => { - if (e.target === e.currentTarget) { + if (e.target === e.currentTarget || !e.currentTarget) { this.setState('opened') this.readjust() } @@ -299,7 +300,7 @@ export default class HeightAnimation { const fromHeight = this.getHeight() this.addEndEvent((e) => { - if (e.target === e.currentTarget) { + if (e.target === e.currentTarget || !e.currentTarget) { if (this.elem) { this.elem.style.visibility = 'hidden' this.elem.style.overflowY = 'clip' @@ -342,7 +343,10 @@ export default class HeightAnimation { this.callAnimationStart() this.addEndEvent((e) => { - if (this.state === 'adjusting' && e.target === e.currentTarget) { + if ( + this.state === 'adjusting' && + (e.target === e.currentTarget || !e.currentTarget) + ) { if (this.elem) { this.elem.style.height = 'auto' } diff --git a/packages/dnb-eufemia/src/components/height-animation/__tests__/HeightAnimationInstance.test.ts b/packages/dnb-eufemia/src/components/height-animation/__tests__/HeightAnimationInstance.test.ts index 4ab4cf39d6e..93a42b545a7 100644 --- a/packages/dnb-eufemia/src/components/height-animation/__tests__/HeightAnimationInstance.test.ts +++ b/packages/dnb-eufemia/src/components/height-animation/__tests__/HeightAnimationInstance.test.ts @@ -1,3 +1,4 @@ +import { wait } from '../../../core/jest/jestSetup' import HeightAnimationInstance from '../HeightAnimationInstance' import { simulateAnimationEnd, @@ -37,6 +38,20 @@ describe('HeightAnimationInstance', () => { expect(inst.elem).toBeUndefined() }) + it('firstPaintStyle should have these properties', () => { + const inst = new HeightAnimationInstance() + expect(inst.firstPaintStyle).toEqual({ + height: 'auto', + opacity: '0', + visibility: 'hidden', + }) + expect(inst.firstPaintStyle).not.toEqual( + expect.objectContaining({ + position: 'absolute', + }) + ) + }) + it('getHeight should return height', () => { const inst = new HeightAnimationInstance() inst.setElement(element) @@ -46,55 +61,115 @@ describe('HeightAnimationInstance', () => { expect(inst.getHeight()).toBe(100) }) - it('getUnknownHeight should return proper height', () => { - const inst = new HeightAnimationInstance() - inst.setElement(element) + describe('getUnknownHeight', () => { + it('should return proper height', () => { + const inst = new HeightAnimationInstance() + inst.setElement(element) - mockHeight(100, element) + mockHeight(100, element) - expect(inst.getUnknownHeight()).toBe(100) - }) + expect(inst.getUnknownHeight()).toBe(100) + }) - it('open should call getUnknownHeight', () => { - const inst = new HeightAnimationInstance() + it('should create a cloned element', async () => { + const inst = new HeightAnimationInstance() + inst.setElement(element) - jest.spyOn(inst, 'getUnknownHeight').mockImplementation(jest.fn()) + mockHeight(100, element) - inst.setElement(element) - inst.setState('closed') - inst.open() + const addedNodes = [] + const removedNodes = [] + + const observer = new MutationObserver((mutationsList) => { + for (const mutation of mutationsList) { + if (mutation.type === 'childList') { + if (mutation.removedNodes?.length) { + removedNodes.push(mutation.removedNodes) + } + if (mutation.addedNodes?.length) { + addedNodes.push(mutation.addedNodes) + } + } + } + }) - expect(inst.getUnknownHeight).toHaveBeenCalledTimes(1) - }) + observer.observe(document.body, { + childList: true, + }) - it('getUnknownHeight should use cached height during animation', () => { - const inst = new HeightAnimationInstance() + inst.getUnknownHeight() - mockHeight(100, element) + await wait(1) - expect(inst.__currentHeight).toBe(undefined) - expect(inst.isAnimating).toBe(undefined) + observer.disconnect() - inst.setElement(element) - inst.setState('closed') - inst.open() + expect(addedNodes).toHaveLength(1) + expect(removedNodes).toHaveLength(1) + }) - expect(inst.isAnimating).toBe(true) - expect(inst.__currentHeight).toBe(100) + it('should create a cloned element with firstPaintStyle styles', async () => { + const inst = new HeightAnimationInstance() + inst.setElement(element) - mockHeight(200, element) + mockHeight(100, element) - inst.getUnknownHeight() + const styles = [] - expect(inst.__currentHeight).toBe(100) + const observer = new MutationObserver((mutationsList) => { + for (const mutation of mutationsList) { + if (mutation.type === 'childList') { + if (mutation.addedNodes?.length) { + styles.push(mutation.addedNodes[0]) + } + } + } + }) + + observer.observe(document.body, { + childList: true, + }) + + inst.getUnknownHeight() + + await wait(1) + + observer.disconnect() + + expect(styles).toHaveLength(1) + expect(styles[0].getAttribute('style')).toBe( + 'visibility: hidden; opacity: 0; height: auto; width: auto; position: absolute;' + ) + }) - delete inst.elem + it('should use cached height during animation', () => { + const inst = new HeightAnimationInstance() - expect(inst.getUnknownHeight()).toBe(null) + mockHeight(100, element) - inst.callAnimationEnd() + expect(inst.__currentHeight).toBe(undefined) + expect(inst.isAnimating).toBe(undefined) - expect(inst.__currentHeight).toBe(undefined) + inst.setElement(element) + inst.setState('closed') + inst.open() + + expect(inst.isAnimating).toBe(true) + expect(inst.__currentHeight).toBe(100) + + mockHeight(200, element) + + inst.getUnknownHeight() + + expect(inst.__currentHeight).toBe(100) + + delete inst.elem + + expect(inst.getUnknownHeight()).toBe(null) + + inst.callAnimationEnd() + + expect(inst.__currentHeight).toBe(undefined) + }) }) describe('start', () => { @@ -146,6 +221,83 @@ describe('HeightAnimationInstance', () => { expect(onStart).toHaveBeenCalledTimes(0) expect(onEnd).toHaveBeenCalledTimes(0) }) + + it('should call stop', () => { + const inst = new HeightAnimationInstance() + inst.setElement(element) + + jest.spyOn(inst, 'stop').mockImplementation(jest.fn()) + inst.start(100, 200) + + expect(inst.stop).toHaveBeenCalledTimes(1) + }) + + it('should set reqId1 and reqId2', () => { + const inst = new HeightAnimationInstance() + inst.setElement(element) + + inst.start(100, 200) + + expect(inst.reqId1).toBe(1) + expect(inst.reqId2).toBeUndefined() + + nextAnimationFrame() + + expect(inst.reqId1).toBe(1) + expect(inst.reqId2).toBe(1) + }) + + it('should set height style', () => { + const inst = new HeightAnimationInstance() + inst.setElement(element) + + inst.start(100, 200) + + expect(inst.elem.style.height).toBe('') + + nextAnimationFrame() + + expect(inst.elem.style.height).toBe('100px') + + nextAnimationFrame() + + expect(inst.elem.style.height).toBe('200px') + }) + + it('should set not height style when element is missing', () => { + const inst = new HeightAnimationInstance() + inst.setElement(element) + const elem = inst.elem + + inst.start(100, 200) + + expect(elem.style.height).toBe('') + + nextAnimationFrame() + + expect(elem.style.height).toBe('100px') + + inst.elem = undefined // here we remove the element during the second animation frame + nextAnimationFrame() + + expect(elem.style.height).toBe('100px') + }) + + it('should not run when element is not set', () => { + const inst = new HeightAnimationInstance() + inst.start(100, 200) + expect(inst.reqId1).toBeUndefined() + expect(inst.reqId2).toBeUndefined() + }) + + it('should set isAnimating to true', () => { + const inst = new HeightAnimationInstance() + inst.setElement(element) + + inst.start(100, 200) + + expect(inst.isAnimating).toBe(true) + }) }) describe('open', () => { @@ -153,7 +305,19 @@ describe('HeightAnimationInstance', () => { globalThis.bypassTime = 1 }) - it('should call setAsOpen when criterias are met', () => { + it('should call getUnknownHeight', () => { + const inst = new HeightAnimationInstance() + + jest.spyOn(inst, 'getUnknownHeight').mockImplementation(jest.fn()) + + inst.setElement(element) + inst.setState('closed') + inst.open() + + expect(inst.getUnknownHeight).toHaveBeenCalledTimes(1) + }) + + it('should call setAsOpen when criteria are met', () => { const inst = new HeightAnimationInstance() inst.setElement(element) @@ -256,7 +420,7 @@ describe('HeightAnimationInstance', () => { globalThis.bypassTime = 1 }) - it('should call setAsClosed when criterias are met', () => { + it('should call setAsClosed when criteria are met', () => { const inst = new HeightAnimationInstance() inst.setElement(element) diff --git a/packages/dnb-eufemia/src/components/height-animation/stories/HeightAnimation.stories.tsx b/packages/dnb-eufemia/src/components/height-animation/stories/HeightAnimation.stories.tsx index 86b6ba3eb08..e2c4926b4dd 100644 --- a/packages/dnb-eufemia/src/components/height-animation/stories/HeightAnimation.stories.tsx +++ b/packages/dnb-eufemia/src/components/height-animation/stories/HeightAnimation.stories.tsx @@ -87,3 +87,66 @@ const StyledSection = styled(Section)` transform: translateY(0); } ` + +export function HeightAnimationKeepInDOM() { + const Example = () => { + const [openState, setOpenState] = React.useState(true) + const [contentState, setContentState] = React.useState(false) + + const onChangeHandler = ({ checked }) => { + setOpenState(checked) + } + + // console.log('contentState', contentState) + + return ( + <> + + Open/close + + { + setContentState(checked) + }} + space={{ top: true, bottom: true }} + > + Change height inside + + + + +
+

Your content

+
+ {contentState &&

More content

} + + + + ) + } + + const StyledSection = styled(Section)` + .content-element { + transition: transform 1s var(--easing-default); + transform: translateY(-2rem); + + padding: 4rem 0; + } + + .dnb-height-animation--parallax .content-element { + transform: translateY(0); + } + ` + + return +} diff --git a/packages/dnb-eufemia/src/components/height-animation/useHeightAnimation.tsx b/packages/dnb-eufemia/src/components/height-animation/useHeightAnimation.tsx index 7070c63577d..19a5ecd6cf5 100644 --- a/packages/dnb-eufemia/src/components/height-animation/useHeightAnimation.tsx +++ b/packages/dnb-eufemia/src/components/height-animation/useHeightAnimation.tsx @@ -154,7 +154,7 @@ export function useHeightAnimation( const firstPaintStyle = ((open && !isVisible && !isAnimating && - instRef.current?.firstPaintStyle()) || + instRef.current?.firstPaintStyle) || {}) as React.CSSProperties const isInDOM = open || isVisible @@ -170,6 +170,11 @@ export function useHeightAnimation( } function useOpenClose({ open, instRef, isInitialRenderRef, targetRef }) { + const isTest = + typeof process !== 'undefined' && + process.env.NODE_ENV === 'test' && + typeof globalThis.IS_TEST === 'undefined' + useLayoutEffect(() => { instRef.current.setElement(targetRef.current) @@ -182,19 +187,12 @@ function useOpenClose({ open, instRef, isInitialRenderRef, targetRef }) { } else { instRef.current.setAsClosed() } - } - }, [open, targetRef, instRef, isInitialRenderRef]) - - const isTest = - typeof process !== 'undefined' && - process.env.NODE_ENV === 'test' && - typeof globalThis.IS_TEST === 'undefined' - - useLayoutEffect(() => { - if (open) { - instRef.current.open() } else { - instRef.current.close() + if (open) { + instRef.current.open() + } else { + instRef.current.close() + } } // For testing purposes, we need to trigger the transitionend event @@ -202,7 +200,7 @@ function useOpenClose({ open, instRef, isInitialRenderRef, targetRef }) { const event = new CustomEvent('transitionend') targetRef.current?.dispatchEvent(event) } - }, [open, instRef, isInitialRenderRef, targetRef, isTest]) + }, [open, targetRef, instRef, isInitialRenderRef, isTest]) useLayoutEffect(() => { const run = () => {