From 2284ddf42e719732acd9c491fe0847eea3540443 Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Wed, 24 Aug 2022 10:21:18 +0800 Subject: [PATCH 1/5] Add a context provider to keep track of the popover root document, and update the popover on scroll of that document --- packages/components/src/popover/index.js | 151 +++++++++++------- .../src/popover/root-document-context.js | 9 ++ 2 files changed, 98 insertions(+), 62 deletions(-) create mode 100644 packages/components/src/popover/root-document-context.js diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index c40804e1164e23..ff0dd08385b63f 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -48,6 +48,10 @@ import { positionToPlacement, placementToMotionAnimationProps, } from './utils'; +import { + useRootDocumentContext, + RootDocumentProvider, +} from './root-document-context'; /** * Name of slot in which popover should fill. @@ -394,6 +398,26 @@ const Popover = ( }; }, [ __unstableObserveElement, update ] ); + const rootDocument = useRootDocumentContext(); + + // If the root document is scrolled trigger an update of the popover + // position. This covers cases where there might be that's a child of + // another popover, and the outermost popover references an element + // in an iframe. + useLayoutEffect( () => { + if ( + ! rootDocument || + rootDocument === referenceOwnerDocument || + rootDocument === document + ) { + return; + } + + rootDocument.addEventListener( 'scroll', update ); + + return () => rootDocument?.removeEventListener( 'scroll', update ); + }, [ referenceOwnerDocument, rootDocument, 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 // document scrolls. Also update the frame offset if the view resizes. @@ -428,7 +452,7 @@ const Popover = ( defaultView.removeEventListener( 'resize', updateFrameOffset ); } }; - }, [ referenceOwnerDocument, update ] ); + }, [ referenceOwnerDocument, update, rootDocument ] ); const mergedFloatingRef = useMergeRefs( [ floating, @@ -436,68 +460,71 @@ const Popover = ( forwardedRef, ] ); - // Disable reason: We care to capture the _bubbled_ events from inputs - // within popover as inferring close intent. - let content = ( - // eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions - // eslint-disable-next-line jsx-a11y/no-static-element-interactions - - { /* Prevents scroll on the document */ } - { isExpanded && } - { isExpanded && ( -
- - { headerTitle } - -
- ) } -
{ children }
- { hasArrow && ( -
- -
- ) } - + + { + // Disable reason: We care to capture the _bubbled_ events from inputs + // within popover as inferring close intent. + // eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions + // eslint-disable-next-line jsx-a11y/no-static-element-interactions + } + + { /* Prevents scroll on the document */ } + { isExpanded && } + { isExpanded && ( +
+ + { headerTitle } + +
+ ) } +
{ children }
+ { hasArrow && ( +
+ +
+ ) } + + ); if ( slot.ref ) { diff --git a/packages/components/src/popover/root-document-context.js b/packages/components/src/popover/root-document-context.js new file mode 100644 index 00000000000000..2a8815c8e8919a --- /dev/null +++ b/packages/components/src/popover/root-document-context.js @@ -0,0 +1,9 @@ +/** + * WordPress dependencies + */ +import { createContext, useContext } from '@wordpress/element'; + +const RootDocumentContext = createContext( undefined ); + +export const useRootDocumentContext = () => useContext( RootDocumentContext ); +export const RootDocumentProvider = RootDocumentContext.Provider; From f0729bdfcaa2e412909a27b1100a8f68e1a979da Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Wed, 24 Aug 2022 12:27:44 +0800 Subject: [PATCH 2/5] Add comments --- packages/components/src/popover/index.js | 53 +++++++++++-------- .../src/popover/root-document-context.js | 1 - 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index ff0dd08385b63f..d1aa3a216f7c72 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -398,26 +398,6 @@ const Popover = ( }; }, [ __unstableObserveElement, update ] ); - const rootDocument = useRootDocumentContext(); - - // If the root document is scrolled trigger an update of the popover - // position. This covers cases where there might be that's a child of - // another popover, and the outermost popover references an element - // in an iframe. - useLayoutEffect( () => { - if ( - ! rootDocument || - rootDocument === referenceOwnerDocument || - rootDocument === document - ) { - return; - } - - rootDocument.addEventListener( 'scroll', update ); - - return () => rootDocument?.removeEventListener( 'scroll', update ); - }, [ referenceOwnerDocument, rootDocument, 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 // document scrolls. Also update the frame offset if the view resizes. @@ -452,7 +432,33 @@ const Popover = ( defaultView.removeEventListener( 'resize', updateFrameOffset ); } }; - }, [ referenceOwnerDocument, update, rootDocument ] ); + }, [ referenceOwnerDocument, update ] ); + + // If the root document is scrolled trigger an update of the popover + // position. This covers cases where a popover is a child of another + // popover, and the first popover in the chain references an element + // in an iframe. + const rootReferenceDocument = useRootDocumentContext(); + useLayoutEffect( () => { + // Return early if the root document is the same as the owner document, + // as the scroll event will be listened to in other code. + const isSameDocument = rootReferenceDocument === referenceOwnerDocument; + + // Return early if this isn't an iframe document. + const isNotIframeDocument = rootReferenceDocument === document; + + if ( + ! rootReferenceDocument || + isSameDocument || + isNotIframeDocument + ) { + return; + } + + rootReferenceDocument.addEventListener( 'scroll', update ); + return () => + rootReferenceDocument?.removeEventListener( 'scroll', update ); + }, [ referenceOwnerDocument, rootReferenceDocument, update ] ); const mergedFloatingRef = useMergeRefs( [ floating, @@ -461,7 +467,10 @@ const Popover = ( ] ); let content = ( - + { // Disable reason: We care to capture the _bubbled_ events from inputs // within popover as inferring close intent. diff --git a/packages/components/src/popover/root-document-context.js b/packages/components/src/popover/root-document-context.js index 2a8815c8e8919a..8b60cb96c863c8 100644 --- a/packages/components/src/popover/root-document-context.js +++ b/packages/components/src/popover/root-document-context.js @@ -4,6 +4,5 @@ import { createContext, useContext } from '@wordpress/element'; const RootDocumentContext = createContext( undefined ); - export const useRootDocumentContext = () => useContext( RootDocumentContext ); export const RootDocumentProvider = RootDocumentContext.Provider; From 09f425673b552dc38a5501985e028207681f7326 Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Wed, 24 Aug 2022 14:51:23 +0800 Subject: [PATCH 3/5] Rename context --- packages/components/src/popover/index.js | 12 ++++++------ .../components/src/popover/root-document-context.js | 8 -------- .../src/popover/root-reference-document-context.js | 10 ++++++++++ 3 files changed, 16 insertions(+), 14 deletions(-) delete mode 100644 packages/components/src/popover/root-document-context.js create mode 100644 packages/components/src/popover/root-reference-document-context.js diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index d1aa3a216f7c72..7dead407dd2a99 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -49,9 +49,9 @@ import { placementToMotionAnimationProps, } from './utils'; import { - useRootDocumentContext, - RootDocumentProvider, -} from './root-document-context'; + useRootReferenceDocument, + RootReferenceDocumentProvider, +} from './root-reference-document-context'; /** * Name of slot in which popover should fill. @@ -438,7 +438,7 @@ const Popover = ( // position. This covers cases where a popover is a child of another // popover, and the first popover in the chain references an element // in an iframe. - const rootReferenceDocument = useRootDocumentContext(); + const rootReferenceDocument = useRootReferenceDocument(); useLayoutEffect( () => { // Return early if the root document is the same as the owner document, // as the scroll event will be listened to in other code. @@ -467,7 +467,7 @@ const Popover = ( ] ); let content = ( - @@ -533,7 +533,7 @@ const Popover = ( ) } - + ); if ( slot.ref ) { diff --git a/packages/components/src/popover/root-document-context.js b/packages/components/src/popover/root-document-context.js deleted file mode 100644 index 8b60cb96c863c8..00000000000000 --- a/packages/components/src/popover/root-document-context.js +++ /dev/null @@ -1,8 +0,0 @@ -/** - * WordPress dependencies - */ -import { createContext, useContext } from '@wordpress/element'; - -const RootDocumentContext = createContext( undefined ); -export const useRootDocumentContext = () => useContext( RootDocumentContext ); -export const RootDocumentProvider = RootDocumentContext.Provider; diff --git a/packages/components/src/popover/root-reference-document-context.js b/packages/components/src/popover/root-reference-document-context.js new file mode 100644 index 00000000000000..855954a83f667f --- /dev/null +++ b/packages/components/src/popover/root-reference-document-context.js @@ -0,0 +1,10 @@ +/** + * WordPress dependencies + */ +import { createContext, useContext } from '@wordpress/element'; + +const RootReferenceDocumentContext = createContext( undefined ); +export const useRootReferenceDocument = () => + useContext( RootReferenceDocumentContext ); +export const RootReferenceDocumentProvider = + RootReferenceDocumentContext.Provider; From af5e3b8bdb41d80788529a6f03acfd7585ac8b13 Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Wed, 24 Aug 2022 14:52:07 +0800 Subject: [PATCH 4/5] Update comment --- packages/components/src/popover/index.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index 7dead407dd2a99..3b90de04928f40 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -469,6 +469,8 @@ const Popover = ( let content = ( { From 2faf4f1f5768a8db75bc4d28a22c5ee3f6d44537 Mon Sep 17 00:00:00 2001 From: Daniel Richards Date: Wed, 24 Aug 2022 14:53:52 +0800 Subject: [PATCH 5/5] Add changelog entry --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 248802890e46d6..beee58ba8dd240 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -17,6 +17,7 @@ - `Popover`: make sure offset middleware always applies the latest frame offset values ([#43329](https://github.com/WordPress/gutenberg/pull/43329/)). - `Dropdown`: anchor popover to the dropdown wrapper (instead of the toggle) ([#43377](https://github.com/WordPress/gutenberg/pull/43377/)). - `Guide`: Fix error when rendering with no pages ([#43380](https://github.com/WordPress/gutenberg/pull/43380/)). +- `Popover`: Ensure position is correct when a nested popover's parent references an element in an iframe and the iframe is scrolled ([#43544](https://github.com/WordPress/gutenberg/pull/43544/)). ### Enhancements