From 3af877f8738bdb301a8a04eccac52ae98beef42a Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Tue, 16 Aug 2022 18:47:08 +0800 Subject: [PATCH 1/8] Ensure Popover hooks that call update are using a fresh version of the function by specifying it in the deps --- packages/components/src/popover/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index b530f2e51d4c61..c6f78e56d37e56 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -364,7 +364,7 @@ const Popover = ( refs.floating.current, update ); - }, [ anchorRef, anchorRect, getAnchorRect ] ); + }, [ anchorRef, anchorRect, getAnchorRect, update ] ); // This is only needed for a smooth transition when moving blocks. useLayoutEffect( () => { @@ -377,7 +377,7 @@ const Popover = ( return () => { observer.disconnect(); }; - }, [ __unstableObserveElement ] ); + }, [ __unstableObserveElement, update ] ); // If the reference element is in a different ownerDocument (e.g. iFrame), // we need to manually update the floating's position as the reference's owner @@ -409,7 +409,7 @@ const Popover = ( defaultView.removeEventListener( 'resize', updateFrameOffset ); } }; - }, [ ownerDocument ] ); + }, [ ownerDocument, update ] ); const mergedFloatingRef = useMergeRefs( [ floating, From 9a394c3cf7c8b5cc7017846de3bfdfb1e42f243a Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Tue, 16 Aug 2022 18:48:20 +0800 Subject: [PATCH 2/8] Also call update in `updateFrameOffset` to be called --- packages/components/src/popover/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index c6f78e56d37e56..bee73119cc9288 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -400,6 +400,7 @@ const Popover = ( }; updateFrameOffset(); defaultView.addEventListener( 'resize', updateFrameOffset ); + update(); } return () => { From 8fd32406c43dfc982ce40000c7b20b1f77bc22c3 Mon Sep 17 00:00:00 2001 From: ntsekouras Date: Tue, 16 Aug 2022 16:07:01 +0300 Subject: [PATCH 3/8] move update call --- packages/components/src/popover/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index bee73119cc9288..2447f5de6a4bd8 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -397,10 +397,10 @@ const Popover = ( updateFrameOffset = () => { const iframeRect = frameElement.getBoundingClientRect(); frameOffset.current = { x: iframeRect.left, y: iframeRect.top }; + update(); }; updateFrameOffset(); defaultView.addEventListener( 'resize', updateFrameOffset ); - update(); } return () => { From d35abcc6cf1729fce4c8a9a9f669b405ef803664 Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Wed, 17 Aug 2022 10:38:34 +0800 Subject: [PATCH 4/8] Ensure frameOffset.current is defined on first render --- packages/components/src/popover/index.js | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index 2447f5de6a4bd8..76ae959151b4ac 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -118,6 +118,15 @@ const MaybeAnimatedWrapper = forwardRef( const slotNameContext = createContext(); +const getFrameOffset = ( document ) => { + const frameElement = document?.defaultView?.frameElement; + if ( ! frameElement ) { + return; + } + const iframeRect = frameElement.getBoundingClientRect(); + return { x: iframeRect.left, y: iframeRect.top }; +}; + const Popover = ( { range, @@ -191,7 +200,7 @@ const Popover = ( * Store the offset in a ref, due to constraints with floating-ui: * https://floating-ui.com/docs/react-dom#variables-inside-middleware-functions. */ - const frameOffset = useRef(); + const frameOffset = useRef( getFrameOffset( ownerDocument ) ); const middleware = [ frameOffset.current || offset @@ -388,15 +397,13 @@ const Popover = ( } const { defaultView } = ownerDocument; - const { frameElement } = defaultView; ownerDocument.addEventListener( 'scroll', update ); let updateFrameOffset; - if ( frameElement ) { + if ( defaultView ) { updateFrameOffset = () => { - const iframeRect = frameElement.getBoundingClientRect(); - frameOffset.current = { x: iframeRect.left, y: iframeRect.top }; + frameOffset.current = getFrameOffset( ownerDocument ); update(); }; updateFrameOffset(); From 6a697ab001e887e73f832cc05acf621cef0dc35b Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Wed, 17 Aug 2022 10:45:39 +0800 Subject: [PATCH 5/8] Update changelog --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index e6bba25be6d8cd..3d0002b5554054 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -9,6 +9,7 @@ ### Bug Fix - `Popover`: fix and improve opening animation ([#43186](https://github.com/WordPress/gutenberg/pull/43186)). +- `Popover`: fix incorrect deps in hooks resulting in incorrect positioning after calling `update` ([#43267](https://github.com/WordPress/gutenberg/pull/43267/)). ### Enhancements From d91a073efd2c227655fbab8a30a9d472c81ff40d Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Wed, 17 Aug 2022 11:02:20 +0800 Subject: [PATCH 6/8] Add comment about deps --- packages/components/src/popover/index.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index 76ae959151b4ac..ea3d5997d49251 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -192,6 +192,10 @@ const Popover = ( } return documentToReturn ?? document; + + // 'reference' and 'refs.floating' are refs and don't need to be listed + // as dependencies (see https://github.com/WordPress/gutenberg/pull/41612) + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ anchorRef, anchorRect, getAnchorRect ] ); /** From 351e66c2073e4e8d4a80e8ea4c357289cdfb3247 Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Wed, 17 Aug 2022 17:36:15 +0800 Subject: [PATCH 7/8] Check for frameElement before binding to resize event --- packages/components/src/popover/index.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index ea3d5997d49251..fb524ad07802f9 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -405,7 +405,8 @@ const Popover = ( ownerDocument.addEventListener( 'scroll', update ); let updateFrameOffset; - if ( defaultView ) { + const hasFrameElement = !! ownerDocument?.defaultView?.frameElement; + if ( hasFrameElement ) { updateFrameOffset = () => { frameOffset.current = getFrameOffset( ownerDocument ); update(); From 433c550fd2f7b7a944fdaa5972e0334f758700d1 Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Wed, 17 Aug 2022 17:46:55 +0800 Subject: [PATCH 8/8] Move getFrameOffset to utils file --- packages/components/src/popover/index.js | 15 +++++---------- packages/components/src/popover/utils.js | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index fb524ad07802f9..6716e4cd92dfc5 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -42,7 +42,11 @@ import { Path, SVG } from '@wordpress/primitives'; import Button from '../button'; import ScrollLock from '../scroll-lock'; import { Slot, Fill, useSlot } from '../slot-fill'; -import { positionToPlacement, placementToMotionAnimationProps } from './utils'; +import { + getFrameOffset, + positionToPlacement, + placementToMotionAnimationProps, +} from './utils'; /** * Name of slot in which popover should fill. @@ -118,15 +122,6 @@ const MaybeAnimatedWrapper = forwardRef( const slotNameContext = createContext(); -const getFrameOffset = ( document ) => { - const frameElement = document?.defaultView?.frameElement; - if ( ! frameElement ) { - return; - } - const iframeRect = frameElement.getBoundingClientRect(); - return { x: iframeRect.left, y: iframeRect.top }; -}; - const Popover = ( { range, diff --git a/packages/components/src/popover/utils.js b/packages/components/src/popover/utils.js index 05bf818aa969d1..90cd3d84d047cd 100644 --- a/packages/components/src/popover/utils.js +++ b/packages/components/src/popover/utils.js @@ -81,3 +81,27 @@ export const placementToMotionAnimationProps = ( placement ) => { transition: { duration: 0.1, ease: [ 0, 0, 0.2, 1 ] }, }; }; + +/** + * @typedef FrameOffset + * @type {Object} + * @property {number} x A numerical value representing the horizontal offset of the frame. + * @property {number} y A numerical value representing the vertical offset of the frame. + */ + +/** + * Returns the offset of a document's frame element. + * + * @param {Document} document A document. This will usually be the document within an iframe. + * + * @return {FrameOffset|undefined} The offset of the document's frame element, + * or undefined if the document has no frame element. + */ +export const getFrameOffset = ( document ) => { + const frameElement = document?.defaultView?.frameElement; + if ( ! frameElement ) { + return; + } + const iframeRect = frameElement.getBoundingClientRect(); + return { x: iframeRect.left, y: iframeRect.top }; +};