From 2e279f57644e1f0e9c39adedd790bad0c2c78ce5 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Tue, 10 Aug 2021 13:22:20 -0400 Subject: [PATCH 01/18] try using next block as anchor --- .../src/components/block-tools/block-popover.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/block-editor/src/components/block-tools/block-popover.js b/packages/block-editor/src/components/block-tools/block-popover.js index 8371cfff9bbba8..c148c8a3ff73d7 100644 --- a/packages/block-editor/src/components/block-tools/block-popover.js +++ b/packages/block-editor/src/components/block-tools/block-popover.js @@ -10,7 +10,7 @@ import classnames from 'classnames'; import { useState, useCallback, useRef, useEffect } from '@wordpress/element'; import { isUnmodifiedDefaultBlock } from '@wordpress/blocks'; import { Popover } from '@wordpress/components'; -import { useDispatch, useSelect } from '@wordpress/data'; +import { useDispatch, useSelect, select as dataSelect } from '@wordpress/data'; import { useShortcut } from '@wordpress/keyboard-shortcuts'; import { useViewportMatch } from '@wordpress/compose'; import { getScrollContainer } from '@wordpress/dom'; @@ -34,6 +34,8 @@ function selector( select ) { isCaretWithinFormattedText, getSettings, getLastMultiSelectedBlockClientId, + getNextBlockClientId, + getBlockOrder, } = select( blockEditorStore ); return { isNavigationMode: isNavigationMode(), @@ -43,6 +45,8 @@ function selector( select ) { hasMultiSelection: hasMultiSelection(), hasFixedToolbar: getSettings().hasFixedToolbar, lastClientId: getLastMultiSelectedBlockClientId(), + getNextBlockClientId, + getBlockOrder, }; } @@ -63,13 +67,14 @@ function BlockPopover( { hasMultiSelection, hasFixedToolbar, lastClientId, + getNextBlockClientId, + getBlockOrder, } = useSelect( selector, [] ); const isInsertionPointVisible = useSelect( ( select ) => { const { isBlockInsertionPointVisible, getBlockInsertionPoint, - getBlockOrder, } = select( blockEditorStore ); if ( ! isBlockInsertionPointVisible() ) { @@ -132,6 +137,9 @@ function BlockPopover( { const lastSelectedElement = useBlockElement( lastClientId ); const capturingElement = useBlockElement( capturingClientId ); + const nextElement = useBlockElement( getNextBlockClientId( clientId ) ); + const isFirstBlock = getBlockOrder()[ 0 ] === clientId; + const popoverScrollRef = usePopoverScroll( __unstableContentRef ); if ( @@ -155,6 +163,11 @@ function BlockPopover( { let anchorRef = node; + // If this is the first block and in the site editor, try using second block as anchor. + if ( dataSelect( 'core/edit-site' ) && isFirstBlock && nextElement ) { + anchorRef = nextElement; + } + if ( hasMultiSelection ) { // Wait to render the popover until the bottom reference is available // as well. From cbdfbed7809539c0f3b56f562ba101606d95f988 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Wed, 11 Aug 2021 11:18:36 -0400 Subject: [PATCH 02/18] place popover below block if available when sticky --- .../src/components/block-tools/block-popover.js | 17 ++--------------- packages/components/src/popover/utils.js | 13 ++++++++++++- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/packages/block-editor/src/components/block-tools/block-popover.js b/packages/block-editor/src/components/block-tools/block-popover.js index c148c8a3ff73d7..8371cfff9bbba8 100644 --- a/packages/block-editor/src/components/block-tools/block-popover.js +++ b/packages/block-editor/src/components/block-tools/block-popover.js @@ -10,7 +10,7 @@ import classnames from 'classnames'; import { useState, useCallback, useRef, useEffect } from '@wordpress/element'; import { isUnmodifiedDefaultBlock } from '@wordpress/blocks'; import { Popover } from '@wordpress/components'; -import { useDispatch, useSelect, select as dataSelect } from '@wordpress/data'; +import { useDispatch, useSelect } from '@wordpress/data'; import { useShortcut } from '@wordpress/keyboard-shortcuts'; import { useViewportMatch } from '@wordpress/compose'; import { getScrollContainer } from '@wordpress/dom'; @@ -34,8 +34,6 @@ function selector( select ) { isCaretWithinFormattedText, getSettings, getLastMultiSelectedBlockClientId, - getNextBlockClientId, - getBlockOrder, } = select( blockEditorStore ); return { isNavigationMode: isNavigationMode(), @@ -45,8 +43,6 @@ function selector( select ) { hasMultiSelection: hasMultiSelection(), hasFixedToolbar: getSettings().hasFixedToolbar, lastClientId: getLastMultiSelectedBlockClientId(), - getNextBlockClientId, - getBlockOrder, }; } @@ -67,14 +63,13 @@ function BlockPopover( { hasMultiSelection, hasFixedToolbar, lastClientId, - getNextBlockClientId, - getBlockOrder, } = useSelect( selector, [] ); const isInsertionPointVisible = useSelect( ( select ) => { const { isBlockInsertionPointVisible, getBlockInsertionPoint, + getBlockOrder, } = select( blockEditorStore ); if ( ! isBlockInsertionPointVisible() ) { @@ -137,9 +132,6 @@ function BlockPopover( { const lastSelectedElement = useBlockElement( lastClientId ); const capturingElement = useBlockElement( capturingClientId ); - const nextElement = useBlockElement( getNextBlockClientId( clientId ) ); - const isFirstBlock = getBlockOrder()[ 0 ] === clientId; - const popoverScrollRef = usePopoverScroll( __unstableContentRef ); if ( @@ -163,11 +155,6 @@ function BlockPopover( { let anchorRef = node; - // If this is the first block and in the site editor, try using second block as anchor. - if ( dataSelect( 'core/edit-site' ) && isFirstBlock && nextElement ) { - anchorRef = nextElement; - } - if ( hasMultiSelection ) { // Wait to render the popover until the bottom reference is available // as well. diff --git a/packages/components/src/popover/utils.js b/packages/components/src/popover/utils.js index 697a3fceeb6dde..f2a9fc9bf8a569 100644 --- a/packages/components/src/popover/utils.js +++ b/packages/components/src/popover/utils.js @@ -190,9 +190,20 @@ export function computePopoverYAxisPosition( const stickyPosition = stickyRect.top + height - relativeOffsetTop; if ( anchorRect.top <= stickyPosition ) { + // If there is enough room below the block, return coordinates for the bottom position. + // Otherwise, use the stickyPosition. + if ( + anchorRect.bottom + relativeOffsetTop + height + HEIGHT_OFFSET < + window.innerHeight + ) { + return { + yAxis: 'bottom', + popoverTop: anchorRect.bottom, + }; + } return { yAxis, - popoverTop: Math.min( anchorRect.bottom, stickyPosition ), + popoverTop: stickyPosition, }; } } From 0ad2b7ef6af44e56ed1f8198faa75ed1df949fcc Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Fri, 13 Aug 2021 10:18:46 -0400 Subject: [PATCH 03/18] if starts at bottom of block, keep there --- packages/components/src/popover/index.js | 9 ++- packages/components/src/popover/utils.js | 91 ++++++++++++++---------- 2 files changed, 60 insertions(+), 40 deletions(-) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index a30dfe4fb19faa..3f8e12d3325071 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -12,6 +12,7 @@ import { useState, useLayoutEffect, forwardRef, + useEffect, } from '@wordpress/element'; import { getRectangleFromRange } from '@wordpress/dom'; import deprecated from '@wordpress/deprecated'; @@ -266,6 +267,7 @@ const Popover = ( const anchorRefFallback = useRef( null ); const contentRef = useRef( null ); const containerRef = useRef(); + const startsSticky = useRef( null ); const isMobileViewport = useViewportMatch( 'medium', '<' ); const [ animateOrigin, setAnimateOrigin ] = useState(); const slot = useSlot( __unstableSlotName ); @@ -273,6 +275,10 @@ const Popover = ( const [ containerResizeListener, contentSize ] = useResizeObserver(); noArrow = isExpanded || noArrow; + useEffect( () => { + startsSticky.current = null; + }, anchorRef?.current ); + useLayoutEffect( () => { if ( isExpanded ) { setClass( containerRef.current, 'is-without-arrow', noArrow ); @@ -352,7 +358,8 @@ const Popover = ( relativeOffsetTop, boundaryElement, __unstableForcePosition, - __unstableForceXAlignment + __unstableForceXAlignment, + startsSticky ); if ( diff --git a/packages/components/src/popover/utils.js b/packages/components/src/popover/utils.js index f2a9fc9bf8a569..d90c8e983cd282 100644 --- a/packages/components/src/popover/utils.js +++ b/packages/components/src/popover/utils.js @@ -159,18 +159,18 @@ export function computePopoverXAxisPosition( /** * Utility used to compute the popover position over the yAxis * - * @param {Object} anchorRect Anchor Rect. - * @param {Object} contentSize Content Size. - * @param {string} yAxis Desired yAxis. - * @param {string} corner Desired corner. - * @param {boolean} stickyBoundaryElement The boundary element to use when - * switching between sticky and normal - * position. - * @param {Element} anchorRef The anchor element. - * @param {Element} relativeOffsetTop If applicable, top offset of the - * relative positioned parent container. - * @param {boolean} forcePosition Don't adjust position based on anchor. + * @param {Object} anchorRect Anchor Rect. + * @param {Object} contentSize Content Size. + * @param {string} yAxis Desired yAxis. + * @param {string} corner Desired corner. + * @param {boolean} stickyBoundaryElement The boundary element to use when switching between sticky + * and normal position. + * @param {Element} anchorRef The anchor element. + * @param {Element} relativeOffsetTop If applicable, top offset of the relative positioned + * parent container. + * @param {boolean} forcePosition Don't adjust position based on anchor. * + * @param {boolean|null} startsSticky Whether the popover started in the sticky position. * @return {Object} Popover xAxis position and constraints. */ export function computePopoverYAxisPosition( @@ -181,7 +181,8 @@ export function computePopoverYAxisPosition( stickyBoundaryElement, anchorRef, relativeOffsetTop, - forcePosition + forcePosition, + startsSticky ) { const { height } = contentSize; @@ -189,17 +190,23 @@ export function computePopoverYAxisPosition( const stickyRect = stickyBoundaryElement.getBoundingClientRect(); const stickyPosition = stickyRect.top + height - relativeOffsetTop; - if ( anchorRect.top <= stickyPosition ) { - // If there is enough room below the block, return coordinates for the bottom position. - // Otherwise, use the stickyPosition. - if ( - anchorRect.bottom + relativeOffsetTop + height + HEIGHT_OFFSET < - window.innerHeight - ) { - return { - yAxis: 'bottom', - popoverTop: anchorRect.bottom, - }; + if ( anchorRect.top <= stickyPosition || startsSticky.current ) { + if ( startsSticky.current === null || startsSticky.current ) { + startsSticky.current = true; + // If there is enough room below the block, return coordinates for the bottom position. + // Otherwise, use the stickyPosition. + if ( + anchorRect.bottom + + relativeOffsetTop + + height + + HEIGHT_OFFSET < + window.innerHeight + ) { + return { + yAxis: 'bottom', + popoverTop: anchorRect.bottom, + }; + } } return { yAxis, @@ -208,6 +215,10 @@ export function computePopoverYAxisPosition( } } + if ( startsSticky.current === null ) { + startsSticky.current = false; + } + // y axis alignment choices let anchorMidPoint = anchorRect.top + anchorRect.height / 2; @@ -285,22 +296,22 @@ export function computePopoverYAxisPosition( } /** - * Utility used to compute the popover position and the content max width/height - * for a popover given its anchor rect and its content size. + * Utility used to compute the popover position and the content max width/height for a popover given + * its anchor rect and its content size. * - * @param {Object} anchorRect Anchor Rect. - * @param {Object} contentSize Content Size. - * @param {string} position Position. - * @param {boolean} stickyBoundaryElement The boundary element to use when - * switching between sticky and normal - * position. - * @param {Element} anchorRef The anchor element. - * @param {number} relativeOffsetTop If applicable, top offset of the - * relative positioned parent container. - * @param {Element} boundaryElement Boundary element. - * @param {boolean} forcePosition Don't adjust position based on anchor. - * @param {boolean} forceXAlignment Don't adjust alignment based on YAxis + * @param {Object} anchorRect Anchor Rect. + * @param {Object} contentSize Content Size. + * @param {string} position Position. + * @param {boolean} stickyBoundaryElement The boundary element to use when switching between + * sticky and normal position. + * @param {Element} anchorRef The anchor element. + * @param {number} relativeOffsetTop If applicable, top offset of the relative positioned + * parent container. + * @param {Element} boundaryElement Boundary element. + * @param {boolean} forcePosition Don't adjust position based on anchor. + * @param {boolean} forceXAlignment Don't adjust alignment based on YAxis * + * @param {boolean|null} startsSticky Whether this popover started in the sticky position. * @return {Object} Popover position and constraints. */ export function computePopoverPosition( @@ -312,7 +323,8 @@ export function computePopoverPosition( relativeOffsetTop, boundaryElement, forcePosition, - forceXAlignment + forceXAlignment, + startsSticky ) { const [ yAxis, xAxis = 'center', corner ] = position.split( ' ' ); @@ -324,7 +336,8 @@ export function computePopoverPosition( stickyBoundaryElement, anchorRef, relativeOffsetTop, - forcePosition + forcePosition, + startsSticky ); const xAxisPosition = computePopoverXAxisPosition( anchorRect, From db799171522c8680dac10f2fa54e986199cb54cc Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Fri, 13 Aug 2021 11:34:00 -0400 Subject: [PATCH 04/18] fix edge cases --- packages/components/src/popover/utils.js | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/components/src/popover/utils.js b/packages/components/src/popover/utils.js index d90c8e983cd282..cac79c79c752ce 100644 --- a/packages/components/src/popover/utils.js +++ b/packages/components/src/popover/utils.js @@ -192,29 +192,40 @@ export function computePopoverYAxisPosition( if ( anchorRect.top <= stickyPosition || startsSticky.current ) { if ( startsSticky.current === null || startsSticky.current ) { + // Enable startsSticky behavior. If the popover starts where it would be placed in the + // sticky position, we want to place it below the anchor if there is room. This + // prevents the block popover from hindering interactions with the block. Furthermore, + // we want to keep this positioned below the block for as long as possible while + // scrolling to prevent jumpiness of the popover in the editor. startsSticky.current = true; - // If there is enough room below the block, return coordinates for the bottom position. - // Otherwise, use the stickyPosition. - if ( + + // If there is enough room below the block to host the popover. + const canFitUnderneath = anchorRect.bottom + relativeOffsetTop + height + HEIGHT_OFFSET < - window.innerHeight - ) { + window.innerHeight; + + if ( canFitUnderneath ) { return { yAxis: 'bottom', popoverTop: anchorRect.bottom, }; } + // If the popover can no longer fit underneath the anchor, we break the startsSticky + // behavior. + startsSticky.current = false; } + // Default sticky behavior. return { yAxis, - popoverTop: stickyPosition, + popoverTop: Math.min( anchorRect.bottom, stickyPosition ), }; } } + // If this is still null, the popover did not start in the sticky position. if ( startsSticky.current === null ) { startsSticky.current = false; } From c9beddecf448c0ec450ab1b2cd1128a65fd0812f Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Fri, 13 Aug 2021 13:48:50 -0400 Subject: [PATCH 05/18] add bottom sticky position --- packages/components/src/popover/utils.js | 37 ++++++++++++------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/packages/components/src/popover/utils.js b/packages/components/src/popover/utils.js index cac79c79c752ce..07ef1895814b23 100644 --- a/packages/components/src/popover/utils.js +++ b/packages/components/src/popover/utils.js @@ -190,6 +190,9 @@ export function computePopoverYAxisPosition( const stickyRect = stickyBoundaryElement.getBoundingClientRect(); const stickyPosition = stickyRect.top + height - relativeOffsetTop; + const bottomStickyPosition = + stickyRect.bottom - height - relativeOffsetTop; + if ( anchorRect.top <= stickyPosition || startsSticky.current ) { if ( startsSticky.current === null || startsSticky.current ) { // Enable startsSticky behavior. If the popover starts where it would be placed in the @@ -199,33 +202,31 @@ export function computePopoverYAxisPosition( // scrolling to prevent jumpiness of the popover in the editor. startsSticky.current = true; - // If there is enough room below the block to host the popover. - const canFitUnderneath = - anchorRect.bottom + - relativeOffsetTop + - height + - HEIGHT_OFFSET < - window.innerHeight; - - if ( canFitUnderneath ) { + // Keep returning the bottom position / bottom sticky positions until the top of the + // anchor is below the bottom sticky position. + if ( anchorRect.top <= bottomStickyPosition + height ) { return { yAxis: 'bottom', - popoverTop: anchorRect.bottom, + popoverTop: Math.min( + bottomStickyPosition, + anchorRect.bottom + ), }; } - // If the popover can no longer fit underneath the anchor, we break the startsSticky - // behavior. + // At this point the bottom sticky position is higher than the anchor's top + // position and we can reset to default behavior. startsSticky.current = false; + } else { + // Default sticky behavior. + return { + yAxis, + popoverTop: Math.min( anchorRect.bottom, stickyPosition ), + }; } - // Default sticky behavior. - return { - yAxis, - popoverTop: Math.min( anchorRect.bottom, stickyPosition ), - }; } } - // If this is still null, the popover did not start in the sticky position. + // If startsSticky.current is still null, the popover did not start in the sticky position. if ( startsSticky.current === null ) { startsSticky.current = false; } From 284f5d9cf7a269fdcf376ef5692544a66d3283a7 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Fri, 13 Aug 2021 14:18:42 -0400 Subject: [PATCH 06/18] fix jumpy bug on using toolbar --- 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 3f8e12d3325071..ddd1ab1a9a2f40 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -277,7 +277,7 @@ const Popover = ( useEffect( () => { startsSticky.current = null; - }, anchorRef?.current ); + }, [ anchorRef ] ); useLayoutEffect( () => { if ( isExpanded ) { From 2306fcec529a5434fa768ca8e4147cbe5776a034 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Tue, 17 Aug 2021 08:38:00 -0400 Subject: [PATCH 07/18] revert to default behavior as soon as there is room. no bottom sticky necessary --- packages/components/src/popover/utils.js | 33 ++++++++++++------------ 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/packages/components/src/popover/utils.js b/packages/components/src/popover/utils.js index 07ef1895814b23..b79d23b6b90046 100644 --- a/packages/components/src/popover/utils.js +++ b/packages/components/src/popover/utils.js @@ -190,31 +190,30 @@ export function computePopoverYAxisPosition( const stickyRect = stickyBoundaryElement.getBoundingClientRect(); const stickyPosition = stickyRect.top + height - relativeOffsetTop; - const bottomStickyPosition = - stickyRect.bottom - height - relativeOffsetTop; - if ( anchorRect.top <= stickyPosition || startsSticky.current ) { if ( startsSticky.current === null || startsSticky.current ) { - // Enable startsSticky behavior. If the popover starts where it would be placed in the - // sticky position, we want to place it below the anchor if there is room. This - // prevents the block popover from hindering interactions with the block. Furthermore, - // we want to keep this positioned below the block for as long as possible while - // scrolling to prevent jumpiness of the popover in the editor. + // Enable startsSticky behavior. If the popover starts where it would be placed in + // the sticky position, we want to place it below the anchor if there is room. This + // prevents the block popover from hindering interactions with the block. This + // stays below the anchor until there is either room above the anchor or no longer + // any room below to host the popover. startsSticky.current = true; - // Keep returning the bottom position / bottom sticky positions until the top of the - // anchor is below the bottom sticky position. - if ( anchorRect.top <= bottomStickyPosition + height ) { + // Return the bottom position unless there is no room below or there is now adequate + // room above the anchor for default behavior. + const isRoomAbove = anchorRect.top >= stickyPosition; + const isNoRoomBelow = + anchorRect.bottom >= + stickyRect.bottom - height - relativeOffsetTop; + + if ( ! ( isRoomAbove || isNoRoomBelow ) ) { return { yAxis: 'bottom', - popoverTop: Math.min( - bottomStickyPosition, - anchorRect.bottom - ), + popoverTop: anchorRect.bottom, }; } - // At this point the bottom sticky position is higher than the anchor's top - // position and we can reset to default behavior. + + // At this point there is no longer any need to retain the startsSticky behavior. startsSticky.current = false; } else { // Default sticky behavior. From 9efdfdbc341991f3cc82987212ba149f135db7a7 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Tue, 17 Aug 2021 08:55:47 -0400 Subject: [PATCH 08/18] refactor logic and comment --- packages/components/src/popover/index.js | 2 ++ packages/components/src/popover/utils.js | 23 ++++++++++------------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index ddd1ab1a9a2f40..8efa483b72bac0 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -275,6 +275,8 @@ const Popover = ( const [ containerResizeListener, contentSize ] = useResizeObserver(); noArrow = isExpanded || noArrow; + // When the anchorRef changes we need to reset the startsSticky ref so we may start the + // evaluation over for the new anchor. useEffect( () => { startsSticky.current = null; }, [ anchorRef ] ); diff --git a/packages/components/src/popover/utils.js b/packages/components/src/popover/utils.js index b79d23b6b90046..3ed4bebfc302ed 100644 --- a/packages/components/src/popover/utils.js +++ b/packages/components/src/popover/utils.js @@ -190,23 +190,21 @@ export function computePopoverYAxisPosition( const stickyRect = stickyBoundaryElement.getBoundingClientRect(); const stickyPosition = stickyRect.top + height - relativeOffsetTop; - if ( anchorRect.top <= stickyPosition || startsSticky.current ) { + if ( anchorRect.top <= stickyPosition ) { if ( startsSticky.current === null || startsSticky.current ) { // Enable startsSticky behavior. If the popover starts where it would be placed in // the sticky position, we want to place it below the anchor if there is room. This // prevents the block popover from hindering interactions with the block. This - // stays below the anchor until there is either room above the anchor or no longer - // any room below to host the popover. + // stays below the anchor until sticky behavior breaks (there is room above the + // anchor) or no longer any room below to host the popover. startsSticky.current = true; - // Return the bottom position unless there is no room below or there is now adequate - // room above the anchor for default behavior. - const isRoomAbove = anchorRect.top >= stickyPosition; - const isNoRoomBelow = - anchorRect.bottom >= + // Return the bottom position if there is room. + const isRoomBelow = + anchorRect.bottom <= stickyRect.bottom - height - relativeOffsetTop; - if ( ! ( isRoomAbove || isNoRoomBelow ) ) { + if ( isRoomBelow ) { return { yAxis: 'bottom', popoverTop: anchorRect.bottom, @@ -225,10 +223,9 @@ export function computePopoverYAxisPosition( } } - // If startsSticky.current is still null, the popover did not start in the sticky position. - if ( startsSticky.current === null ) { - startsSticky.current = false; - } + // If we get to this line the popover is not in sticky position, so we break any behavior + // associated with startsSticky. + startsSticky.current = false; // y axis alignment choices let anchorMidPoint = anchorRect.top + anchorRect.height / 2; From b7ea822ea8e5280c58da66241badb1ed770bd590 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Tue, 17 Aug 2021 09:02:05 -0400 Subject: [PATCH 09/18] remove unnecessary else --- packages/components/src/popover/utils.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/components/src/popover/utils.js b/packages/components/src/popover/utils.js index 3ed4bebfc302ed..d8708a730a284c 100644 --- a/packages/components/src/popover/utils.js +++ b/packages/components/src/popover/utils.js @@ -213,13 +213,12 @@ export function computePopoverYAxisPosition( // At this point there is no longer any need to retain the startsSticky behavior. startsSticky.current = false; - } else { - // Default sticky behavior. - return { - yAxis, - popoverTop: Math.min( anchorRect.bottom, stickyPosition ), - }; } + // Default sticky behavior. + return { + yAxis, + popoverTop: Math.min( anchorRect.bottom, stickyPosition ), + }; } } From 0499cf486750a58fd01e77ced15cf99f9f5fa0d4 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Tue, 17 Aug 2021 15:47:18 -0400 Subject: [PATCH 10/18] gross selectors --- packages/components/src/popover/index.js | 11 +-- packages/components/src/popover/utils.js | 107 +++++++++++------------ 2 files changed, 53 insertions(+), 65 deletions(-) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index 8efa483b72bac0..a30dfe4fb19faa 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -12,7 +12,6 @@ import { useState, useLayoutEffect, forwardRef, - useEffect, } from '@wordpress/element'; import { getRectangleFromRange } from '@wordpress/dom'; import deprecated from '@wordpress/deprecated'; @@ -267,7 +266,6 @@ const Popover = ( const anchorRefFallback = useRef( null ); const contentRef = useRef( null ); const containerRef = useRef(); - const startsSticky = useRef( null ); const isMobileViewport = useViewportMatch( 'medium', '<' ); const [ animateOrigin, setAnimateOrigin ] = useState(); const slot = useSlot( __unstableSlotName ); @@ -275,12 +273,6 @@ const Popover = ( const [ containerResizeListener, contentSize ] = useResizeObserver(); noArrow = isExpanded || noArrow; - // When the anchorRef changes we need to reset the startsSticky ref so we may start the - // evaluation over for the new anchor. - useEffect( () => { - startsSticky.current = null; - }, [ anchorRef ] ); - useLayoutEffect( () => { if ( isExpanded ) { setClass( containerRef.current, 'is-without-arrow', noArrow ); @@ -360,8 +352,7 @@ const Popover = ( relativeOffsetTop, boundaryElement, __unstableForcePosition, - __unstableForceXAlignment, - startsSticky + __unstableForceXAlignment ); if ( diff --git a/packages/components/src/popover/utils.js b/packages/components/src/popover/utils.js index d8708a730a284c..47af5bfa14c486 100644 --- a/packages/components/src/popover/utils.js +++ b/packages/components/src/popover/utils.js @@ -159,18 +159,17 @@ export function computePopoverXAxisPosition( /** * Utility used to compute the popover position over the yAxis * - * @param {Object} anchorRect Anchor Rect. - * @param {Object} contentSize Content Size. - * @param {string} yAxis Desired yAxis. - * @param {string} corner Desired corner. - * @param {boolean} stickyBoundaryElement The boundary element to use when switching between sticky - * and normal position. - * @param {Element} anchorRef The anchor element. - * @param {Element} relativeOffsetTop If applicable, top offset of the relative positioned - * parent container. - * @param {boolean} forcePosition Don't adjust position based on anchor. + * @param {Object} anchorRect Anchor Rect. + * @param {Object} contentSize Content Size. + * @param {string} yAxis Desired yAxis. + * @param {string} corner Desired corner. + * @param {boolean} stickyBoundaryElement The boundary element to use when switching between sticky + * and normal position. + * @param {Element} anchorRef The anchor element. + * @param {Element} relativeOffsetTop If applicable, top offset of the relative positioned + * parent container. + * @param {boolean} forcePosition Don't adjust position based on anchor. * - * @param {boolean|null} startsSticky Whether the popover started in the sticky position. * @return {Object} Popover xAxis position and constraints. */ export function computePopoverYAxisPosition( @@ -181,8 +180,7 @@ export function computePopoverYAxisPosition( stickyBoundaryElement, anchorRef, relativeOffsetTop, - forcePosition, - startsSticky + forcePosition ) { const { height } = contentSize; @@ -191,28 +189,34 @@ export function computePopoverYAxisPosition( const stickyPosition = stickyRect.top + height - relativeOffsetTop; if ( anchorRect.top <= stickyPosition ) { - if ( startsSticky.current === null || startsSticky.current ) { - // Enable startsSticky behavior. If the popover starts where it would be placed in - // the sticky position, we want to place it below the anchor if there is room. This - // prevents the block popover from hindering interactions with the block. This - // stays below the anchor until sticky behavior breaks (there is room above the - // anchor) or no longer any room below to host the popover. - startsSticky.current = true; - - // Return the bottom position if there is room. - const isRoomBelow = - anchorRect.bottom <= - stickyRect.bottom - height - relativeOffsetTop; - - if ( isRoomBelow ) { - return { - yAxis: 'bottom', - popoverTop: anchorRect.bottom, - }; - } - - // At this point there is no longer any need to retain the startsSticky behavior. - startsSticky.current = false; + // If a popover cannot be positioned above the anchor, even after scrolling, we must + // ensure we use the bottom position instead of the popover slot. This prevents the + // popover from always restricting block content and interaction while selected if the + // block is near the top of the site editor. + + // Find the editor styles wrapper so we can know its scroll position. + let stylesWrapper = anchorRef?.ownerDocument.querySelector( + '.editor-styles-wrapper' + ); + if ( ! stylesWrapper ) { + const canvasFrame = anchorRef?.parentElement.querySelector( + 'iframe[name="editor-canvas"]' + ); + stylesWrapper = canvasFrame?.contentWindow.document.querySelector( + '.editor-styles-wrapper' + ); + } + + // If there is no room above the block in the canvas, return the bottom position for the + // popover. + const scrollRoomAbove = stylesWrapper?.scrollTop; + const noRoomAboveInCanvas = + height >= scrollRoomAbove + anchorRect.top - HEIGHT_OFFSET; + if ( noRoomAboveInCanvas ) { + return { + yAxis: 'bottom', + popoverTop: anchorRect.bottom, + }; } // Default sticky behavior. return { @@ -222,10 +226,6 @@ export function computePopoverYAxisPosition( } } - // If we get to this line the popover is not in sticky position, so we break any behavior - // associated with startsSticky. - startsSticky.current = false; - // y axis alignment choices let anchorMidPoint = anchorRect.top + anchorRect.height / 2; @@ -306,19 +306,18 @@ export function computePopoverYAxisPosition( * Utility used to compute the popover position and the content max width/height for a popover given * its anchor rect and its content size. * - * @param {Object} anchorRect Anchor Rect. - * @param {Object} contentSize Content Size. - * @param {string} position Position. - * @param {boolean} stickyBoundaryElement The boundary element to use when switching between - * sticky and normal position. - * @param {Element} anchorRef The anchor element. - * @param {number} relativeOffsetTop If applicable, top offset of the relative positioned - * parent container. - * @param {Element} boundaryElement Boundary element. - * @param {boolean} forcePosition Don't adjust position based on anchor. - * @param {boolean} forceXAlignment Don't adjust alignment based on YAxis + * @param {Object} anchorRect Anchor Rect. + * @param {Object} contentSize Content Size. + * @param {string} position Position. + * @param {boolean} stickyBoundaryElement The boundary element to use when switching between + * sticky and normal position. + * @param {Element} anchorRef The anchor element. + * @param {number} relativeOffsetTop If applicable, top offset of the relative positioned + * parent container. + * @param {Element} boundaryElement Boundary element. + * @param {boolean} forcePosition Don't adjust position based on anchor. + * @param {boolean} forceXAlignment Don't adjust alignment based on YAxis * - * @param {boolean|null} startsSticky Whether this popover started in the sticky position. * @return {Object} Popover position and constraints. */ export function computePopoverPosition( @@ -330,8 +329,7 @@ export function computePopoverPosition( relativeOffsetTop, boundaryElement, forcePosition, - forceXAlignment, - startsSticky + forceXAlignment ) { const [ yAxis, xAxis = 'center', corner ] = position.split( ' ' ); @@ -343,8 +341,7 @@ export function computePopoverPosition( stickyBoundaryElement, anchorRef, relativeOffsetTop, - forcePosition, - startsSticky + forcePosition ); const xAxisPosition = computePopoverXAxisPosition( anchorRect, From b90c937776e95ffd3428ce6133315511294185b9 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Wed, 18 Aug 2021 09:56:40 -0400 Subject: [PATCH 11/18] pass content wrapper through to popover as prop --- .../data/data-core-block-editor.md | 8 ++ .../components/block-tools/block-popover.js | 4 + .../src/components/iframe/index.js | 4 + packages/block-editor/src/store/actions.js | 7 ++ packages/block-editor/src/store/reducer.js | 8 ++ packages/block-editor/src/store/selectors.js | 4 + packages/components/src/popover/index.js | 4 +- packages/components/src/popover/utils.js | 102 +++++++++--------- 8 files changed, 86 insertions(+), 55 deletions(-) diff --git a/docs/reference-guides/data/data-core-block-editor.md b/docs/reference-guides/data/data-core-block-editor.md index 3cc75b50481683..9c26dcb9185c2c 100644 --- a/docs/reference-guides/data/data-core-block-editor.md +++ b/docs/reference-guides/data/data-core-block-editor.md @@ -440,6 +440,10 @@ _Returns_ - `number`: Number of blocks in the post, or number of blocks with name equal to blockName. +### getIframedEditorWrapper + +Undocumented declaration. + ### getInserterItems Determines the items that appear in the inserter. Includes both static @@ -1392,6 +1396,10 @@ _Parameters_ - _clientId_ `string`: The block's clientId. - _hasControlledInnerBlocks_ `boolean`: True if the block's inner blocks are controlled. +### setIframedEditorWrapper + +Undocumented declaration. + ### setNavigationMode Generators that triggers an action used to enable or disable the navigation mode. diff --git a/packages/block-editor/src/components/block-tools/block-popover.js b/packages/block-editor/src/components/block-tools/block-popover.js index 8371cfff9bbba8..889bf0c2481d90 100644 --- a/packages/block-editor/src/components/block-tools/block-popover.js +++ b/packages/block-editor/src/components/block-tools/block-popover.js @@ -34,6 +34,7 @@ function selector( select ) { isCaretWithinFormattedText, getSettings, getLastMultiSelectedBlockClientId, + getIframedEditorWrapper, } = select( blockEditorStore ); return { isNavigationMode: isNavigationMode(), @@ -43,6 +44,7 @@ function selector( select ) { hasMultiSelection: hasMultiSelection(), hasFixedToolbar: getSettings().hasFixedToolbar, lastClientId: getLastMultiSelectedBlockClientId(), + iframedEditorWrapper: getIframedEditorWrapper(), }; } @@ -63,6 +65,7 @@ function BlockPopover( { hasMultiSelection, hasFixedToolbar, lastClientId, + iframedEditorWrapper, } = useSelect( selector, [] ); const isInsertionPointVisible = useSelect( ( select ) => { @@ -213,6 +216,7 @@ function BlockPopover( { // Observe movement for block animations (especially horizontal). __unstableObserveElement={ node } shouldAnchorIncludePadding + __unstableContentWrapper={ iframedEditorWrapper } > { ( shouldShowContextualToolbar || isToolbarForced ) && (
= scrollRoomAbove + anchorRect.top - HEIGHT_OFFSET; - if ( noRoomAboveInCanvas ) { - return { - yAxis: 'bottom', - popoverTop: anchorRect.bottom, - }; + if ( editorWrapper ) { + // If a popover cannot be positioned above the anchor, even after scrolling, we must + // ensure we use the bottom position instead of the popover slot. This prevents the + // popover from always restricting block content and interaction while selected if the + // block is near the top of the site editor. + + // If there is no room above the block in the canvas, return the bottom position for the + // popover. + const scrollRoomAbove = editorWrapper.scrollTop; + const noRoomAboveInCanvas = + height >= scrollRoomAbove + anchorRect.top - HEIGHT_OFFSET; + if ( noRoomAboveInCanvas ) { + return { + yAxis: 'bottom', + popoverTop: anchorRect.bottom, + }; + } } // Default sticky behavior. return { @@ -306,18 +297,19 @@ export function computePopoverYAxisPosition( * Utility used to compute the popover position and the content max width/height for a popover given * its anchor rect and its content size. * - * @param {Object} anchorRect Anchor Rect. - * @param {Object} contentSize Content Size. - * @param {string} position Position. - * @param {boolean} stickyBoundaryElement The boundary element to use when switching between - * sticky and normal position. - * @param {Element} anchorRef The anchor element. - * @param {number} relativeOffsetTop If applicable, top offset of the relative positioned - * parent container. - * @param {Element} boundaryElement Boundary element. - * @param {boolean} forcePosition Don't adjust position based on anchor. - * @param {boolean} forceXAlignment Don't adjust alignment based on YAxis - * + * @param {Object} anchorRect Anchor Rect. + * @param {Object} contentSize Content Size. + * @param {string} position Position. + * @param {boolean} stickyBoundaryElement The boundary element to use when switching between + * sticky and normal position. + * @param {Element} anchorRef The anchor element. + * @param {number} relativeOffsetTop If applicable, top offset of the relative positioned + * parent container. + * @param {Element} boundaryElement Boundary element. + * @param {boolean} forcePosition Don't adjust position based on anchor. + * @param {boolean} forceXAlignment Don't adjust alignment based on YAxis + * @param {Element|null} editorWrapper Element that wraps the editor content. Used to access + * scroll position to determine sticky behavior. * @return {Object} Popover position and constraints. */ export function computePopoverPosition( @@ -329,7 +321,8 @@ export function computePopoverPosition( relativeOffsetTop, boundaryElement, forcePosition, - forceXAlignment + forceXAlignment, + editorWrapper ) { const [ yAxis, xAxis = 'center', corner ] = position.split( ' ' ); @@ -341,7 +334,8 @@ export function computePopoverPosition( stickyBoundaryElement, anchorRef, relativeOffsetTop, - forcePosition + forcePosition, + editorWrapper ); const xAxisPosition = computePopoverXAxisPosition( anchorRect, From 1308ed9c829a175f12a91c08b77bccc55a4be071 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Wed, 18 Aug 2021 11:05:24 -0400 Subject: [PATCH 12/18] minor refactor --- packages/components/src/popover/utils.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/components/src/popover/utils.js b/packages/components/src/popover/utils.js index 8ba68536f3589c..59ba29bb7a5ce0 100644 --- a/packages/components/src/popover/utils.js +++ b/packages/components/src/popover/utils.js @@ -197,12 +197,13 @@ export function computePopoverYAxisPosition( // popover from always restricting block content and interaction while selected if the // block is near the top of the site editor. - // If there is no room above the block in the canvas, return the bottom position for the - // popover. - const scrollRoomAbove = editorWrapper.scrollTop; - const noRoomAboveInCanvas = - height >= scrollRoomAbove + anchorRect.top - HEIGHT_OFFSET; - if ( noRoomAboveInCanvas ) { + const isRoomAboveInCanvas = + height + HEIGHT_OFFSET < + editorWrapper.scrollTop + anchorRect.top; + const isRoomBelowVisually = + anchorRect.bottom + height + relativeOffsetTop <= + stickyRect.bottom; + if ( ! isRoomAboveInCanvas && isRoomBelowVisually ) { return { yAxis: 'bottom', popoverTop: anchorRect.bottom, From 03404b1960883b763771ea8954b9a027f5da79e1 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Thu, 19 Aug 2021 08:59:12 -0400 Subject: [PATCH 13/18] use bottom sticky if necessary --- packages/components/src/popover/utils.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/packages/components/src/popover/utils.js b/packages/components/src/popover/utils.js index 59ba29bb7a5ce0..291bb50ef0fb82 100644 --- a/packages/components/src/popover/utils.js +++ b/packages/components/src/popover/utils.js @@ -188,9 +188,11 @@ export function computePopoverYAxisPosition( if ( stickyBoundaryElement ) { const stickyRect = stickyBoundaryElement.getBoundingClientRect(); - const stickyPosition = stickyRect.top + height - relativeOffsetTop; + const stickyPositionTop = stickyRect.top + height - relativeOffsetTop; + const stickyPositionBottom = + stickyRect.bottom - height - relativeOffsetTop; - if ( anchorRect.top <= stickyPosition ) { + if ( anchorRect.top <= stickyPositionTop ) { if ( editorWrapper ) { // If a popover cannot be positioned above the anchor, even after scrolling, we must // ensure we use the bottom position instead of the popover slot. This prevents the @@ -200,20 +202,20 @@ export function computePopoverYAxisPosition( const isRoomAboveInCanvas = height + HEIGHT_OFFSET < editorWrapper.scrollTop + anchorRect.top; - const isRoomBelowVisually = - anchorRect.bottom + height + relativeOffsetTop <= - stickyRect.bottom; - if ( ! isRoomAboveInCanvas && isRoomBelowVisually ) { + if ( ! isRoomAboveInCanvas ) { return { yAxis: 'bottom', - popoverTop: anchorRect.bottom, + popoverTop: Math.min( + anchorRect.bottom, + stickyPositionBottom + ), }; } } // Default sticky behavior. return { yAxis, - popoverTop: Math.min( anchorRect.bottom, stickyPosition ), + popoverTop: Math.min( anchorRect.bottom, stickyPositionTop ), }; } } From af1b01b53dd9889eb6488b74fee5ea6fb7c59480 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Thu, 19 Aug 2021 09:42:51 -0400 Subject: [PATCH 14/18] update names, comments, JSDocs --- .../data/data-core-block-editor.md | 8 -------- .../src/components/block-tools/block-popover.js | 10 ++++++---- .../block-editor/src/components/iframe/index.js | 4 +++- packages/block-editor/src/store/actions.js | 9 +++++++-- packages/block-editor/src/store/reducer.js | 14 +++++++++++--- packages/block-editor/src/store/selectors.js | 11 +++++++++-- packages/components/src/popover/index.js | 4 ++-- packages/components/src/popover/utils.js | 6 ++++++ 8 files changed, 44 insertions(+), 22 deletions(-) diff --git a/docs/reference-guides/data/data-core-block-editor.md b/docs/reference-guides/data/data-core-block-editor.md index 9c26dcb9185c2c..3cc75b50481683 100644 --- a/docs/reference-guides/data/data-core-block-editor.md +++ b/docs/reference-guides/data/data-core-block-editor.md @@ -440,10 +440,6 @@ _Returns_ - `number`: Number of blocks in the post, or number of blocks with name equal to blockName. -### getIframedEditorWrapper - -Undocumented declaration. - ### getInserterItems Determines the items that appear in the inserter. Includes both static @@ -1396,10 +1392,6 @@ _Parameters_ - _clientId_ `string`: The block's clientId. - _hasControlledInnerBlocks_ `boolean`: True if the block's inner blocks are controlled. -### setIframedEditorWrapper - -Undocumented declaration. - ### setNavigationMode Generators that triggers an action used to enable or disable the navigation mode. diff --git a/packages/block-editor/src/components/block-tools/block-popover.js b/packages/block-editor/src/components/block-tools/block-popover.js index 889bf0c2481d90..0108fd4694934e 100644 --- a/packages/block-editor/src/components/block-tools/block-popover.js +++ b/packages/block-editor/src/components/block-tools/block-popover.js @@ -34,7 +34,7 @@ function selector( select ) { isCaretWithinFormattedText, getSettings, getLastMultiSelectedBlockClientId, - getIframedEditorWrapper, + __unstableGetIframedEditorCanvasWrapper, } = select( blockEditorStore ); return { isNavigationMode: isNavigationMode(), @@ -44,7 +44,7 @@ function selector( select ) { hasMultiSelection: hasMultiSelection(), hasFixedToolbar: getSettings().hasFixedToolbar, lastClientId: getLastMultiSelectedBlockClientId(), - iframedEditorWrapper: getIframedEditorWrapper(), + iframedEditorCanvasWrapper: __unstableGetIframedEditorCanvasWrapper(), }; } @@ -65,7 +65,7 @@ function BlockPopover( { hasMultiSelection, hasFixedToolbar, lastClientId, - iframedEditorWrapper, + iframedEditorCanvasWrapper, } = useSelect( selector, [] ); const isInsertionPointVisible = useSelect( ( select ) => { @@ -216,7 +216,9 @@ function BlockPopover( { // Observe movement for block animations (especially horizontal). __unstableObserveElement={ node } shouldAnchorIncludePadding - __unstableContentWrapper={ iframedEditorWrapper } + // Used to safeguard sticky position behavior against cases where it would permanently + // obscure specific sections of a block. + __unstableEditorCanvasWrapper={ iframedEditorCanvasWrapper } > { ( shouldShowContextualToolbar || isToolbarForced ) && (
Date: Thu, 19 Aug 2021 10:53:56 -0400 Subject: [PATCH 15/18] add basic store tests --- .../block-editor/src/store/test/actions.js | 12 ++++++++ .../block-editor/src/store/test/reducer.js | 28 +++++++++++++++++++ .../block-editor/src/store/test/selectors.js | 11 ++++++++ 3 files changed, 51 insertions(+) diff --git a/packages/block-editor/src/store/test/actions.js b/packages/block-editor/src/store/test/actions.js index c651122f77ad72..2bdd1a929b0f39 100644 --- a/packages/block-editor/src/store/test/actions.js +++ b/packages/block-editor/src/store/test/actions.js @@ -57,6 +57,7 @@ const { updateSettings, selectionChange, validateBlocksToTemplate, + __unstableSetIframedEditorCanvasWrapper, } = actions; describe( 'actions', () => { @@ -1531,4 +1532,15 @@ describe( 'actions', () => { expect( result ).toEqual( false ); } ); } ); + + describe( '__unstableSetIframedEditorCanvasWrapper', () => { + it( 'should return the SET_IFRAMED_EDITOR_CANVAS_WRAPPER action (string) and input node (object)', () => { + const node = {}; + const result = __unstableSetIframedEditorCanvasWrapper( node ); + expect( result ).toEqual( { + type: 'SET_IFRAMED_EDITOR_CANVAS_WRAPPER', + node, + } ); + } ); + } ); } ); diff --git a/packages/block-editor/src/store/test/reducer.js b/packages/block-editor/src/store/test/reducer.js index b4ad1c12ec1980..ccffcb7e81d33a 100644 --- a/packages/block-editor/src/store/test/reducer.js +++ b/packages/block-editor/src/store/test/reducer.js @@ -33,6 +33,7 @@ import { blockListSettings, lastBlockAttributesChange, lastBlockInserted, + iframedEditorCanvasWrapper, } from '../reducer'; describe( 'state', () => { @@ -2969,4 +2970,31 @@ describe( 'state', () => { expect( state ).toEqual( expectedState ); } ); } ); + + describe( 'iframedEditorCanvasWrapper()', () => { + it( 'should return null if nothing has been set', () => { + const initialState = iframedEditorCanvasWrapper( undefined, {} ); + expect( initialState ).toBe( null ); + } ); + + it( 'should set action.node when type "SET_IFRAMED_EDITOR_CANVAS_WRAPPER"', () => { + const node = {}; + const action = { + type: 'SET_IFRAMED_EDITOR_CANVAS_WRAPPER', + node, + }; + const state = iframedEditorCanvasWrapper( undefined, action ); + expect( state ).toBe( node ); + } ); + + it( 'should not change state with other action types', () => { + const initialState = {}; + const action = { + type: 'FOOBAR', + node: {}, + }; + const state = iframedEditorCanvasWrapper( initialState, action ); + expect( state ).toBe( initialState ); + } ); + } ); } ); diff --git a/packages/block-editor/src/store/test/selectors.js b/packages/block-editor/src/store/test/selectors.js index 0e67cc0f1d5515..4f64d1028713be 100644 --- a/packages/block-editor/src/store/test/selectors.js +++ b/packages/block-editor/src/store/test/selectors.js @@ -77,6 +77,7 @@ const { __unstableGetClientIdsTree, __experimentalGetPatternTransformItems, wasBlockJustInserted, + __unstableGetIframedEditorCanvasWrapper, } = selectors; describe( 'selectors', () => { @@ -3842,3 +3843,13 @@ describe( '__unstableGetClientIdsTree', () => { ] ); } ); } ); + +describe( '__unstableGetIframedEditorCanvasWrapper', () => { + it( 'should return the iframedEditorCanvasWrapper state', () => { + const state = { + iframedEditorCanvasWrapper: {}, + }; + const result = __unstableGetIframedEditorCanvasWrapper( state ); + expect( result ).toBe( state.iframedEditorCanvasWrapper ); + } ); +} ); From 169bcbf849bed7e54e000267bd87fdf486898d94 Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Tue, 24 Aug 2021 08:51:23 -0400 Subject: [PATCH 16/18] remove store goo --- .../components/block-tools/block-popover.js | 6 ++-- .../src/components/iframe/index.js | 9 ++---- packages/block-editor/src/store/actions.js | 12 -------- packages/block-editor/src/store/reducer.js | 16 ----------- packages/block-editor/src/store/selectors.js | 11 -------- .../block-editor/src/store/test/actions.js | 12 -------- .../block-editor/src/store/test/reducer.js | 28 ------------------- .../block-editor/src/store/test/selectors.js | 11 -------- 8 files changed, 5 insertions(+), 100 deletions(-) diff --git a/packages/block-editor/src/components/block-tools/block-popover.js b/packages/block-editor/src/components/block-tools/block-popover.js index 0108fd4694934e..59bad178220509 100644 --- a/packages/block-editor/src/components/block-tools/block-popover.js +++ b/packages/block-editor/src/components/block-tools/block-popover.js @@ -24,6 +24,7 @@ import Inserter from '../inserter'; import { store as blockEditorStore } from '../../store'; import { __unstableUseBlockElement as useBlockElement } from '../block-list/use-block-props/use-block-refs'; import { usePopoverScroll } from './use-popover-scroll'; +import { wrapperState } from '../iframe'; function selector( select ) { const { @@ -34,7 +35,6 @@ function selector( select ) { isCaretWithinFormattedText, getSettings, getLastMultiSelectedBlockClientId, - __unstableGetIframedEditorCanvasWrapper, } = select( blockEditorStore ); return { isNavigationMode: isNavigationMode(), @@ -44,7 +44,6 @@ function selector( select ) { hasMultiSelection: hasMultiSelection(), hasFixedToolbar: getSettings().hasFixedToolbar, lastClientId: getLastMultiSelectedBlockClientId(), - iframedEditorCanvasWrapper: __unstableGetIframedEditorCanvasWrapper(), }; } @@ -65,7 +64,6 @@ function BlockPopover( { hasMultiSelection, hasFixedToolbar, lastClientId, - iframedEditorCanvasWrapper, } = useSelect( selector, [] ); const isInsertionPointVisible = useSelect( ( select ) => { @@ -218,7 +216,7 @@ function BlockPopover( { shouldAnchorIncludePadding // Used to safeguard sticky position behavior against cases where it would permanently // obscure specific sections of a block. - __unstableEditorCanvasWrapper={ iframedEditorCanvasWrapper } + __unstableEditorCanvasWrapper={ wrapperState.wrapperNode } > { ( shouldShowContextualToolbar || isToolbarForced ) && (
{ @@ -1532,15 +1531,4 @@ describe( 'actions', () => { expect( result ).toEqual( false ); } ); } ); - - describe( '__unstableSetIframedEditorCanvasWrapper', () => { - it( 'should return the SET_IFRAMED_EDITOR_CANVAS_WRAPPER action (string) and input node (object)', () => { - const node = {}; - const result = __unstableSetIframedEditorCanvasWrapper( node ); - expect( result ).toEqual( { - type: 'SET_IFRAMED_EDITOR_CANVAS_WRAPPER', - node, - } ); - } ); - } ); } ); diff --git a/packages/block-editor/src/store/test/reducer.js b/packages/block-editor/src/store/test/reducer.js index ccffcb7e81d33a..b4ad1c12ec1980 100644 --- a/packages/block-editor/src/store/test/reducer.js +++ b/packages/block-editor/src/store/test/reducer.js @@ -33,7 +33,6 @@ import { blockListSettings, lastBlockAttributesChange, lastBlockInserted, - iframedEditorCanvasWrapper, } from '../reducer'; describe( 'state', () => { @@ -2970,31 +2969,4 @@ describe( 'state', () => { expect( state ).toEqual( expectedState ); } ); } ); - - describe( 'iframedEditorCanvasWrapper()', () => { - it( 'should return null if nothing has been set', () => { - const initialState = iframedEditorCanvasWrapper( undefined, {} ); - expect( initialState ).toBe( null ); - } ); - - it( 'should set action.node when type "SET_IFRAMED_EDITOR_CANVAS_WRAPPER"', () => { - const node = {}; - const action = { - type: 'SET_IFRAMED_EDITOR_CANVAS_WRAPPER', - node, - }; - const state = iframedEditorCanvasWrapper( undefined, action ); - expect( state ).toBe( node ); - } ); - - it( 'should not change state with other action types', () => { - const initialState = {}; - const action = { - type: 'FOOBAR', - node: {}, - }; - const state = iframedEditorCanvasWrapper( initialState, action ); - expect( state ).toBe( initialState ); - } ); - } ); } ); diff --git a/packages/block-editor/src/store/test/selectors.js b/packages/block-editor/src/store/test/selectors.js index 4f64d1028713be..0e67cc0f1d5515 100644 --- a/packages/block-editor/src/store/test/selectors.js +++ b/packages/block-editor/src/store/test/selectors.js @@ -77,7 +77,6 @@ const { __unstableGetClientIdsTree, __experimentalGetPatternTransformItems, wasBlockJustInserted, - __unstableGetIframedEditorCanvasWrapper, } = selectors; describe( 'selectors', () => { @@ -3843,13 +3842,3 @@ describe( '__unstableGetClientIdsTree', () => { ] ); } ); } ); - -describe( '__unstableGetIframedEditorCanvasWrapper', () => { - it( 'should return the iframedEditorCanvasWrapper state', () => { - const state = { - iframedEditorCanvasWrapper: {}, - }; - const result = __unstableGetIframedEditorCanvasWrapper( state ); - expect( result ).toBe( state.iframedEditorCanvasWrapper ); - } ); -} ); From 2c4acb32ae21ad2bca3f732f6bb157611c84773b Mon Sep 17 00:00:00 2001 From: Addison-Stavlo Date: Tue, 24 Aug 2021 10:38:17 -0400 Subject: [PATCH 17/18] use the contentRef --- .../block-editor/src/components/block-tools/block-popover.js | 3 +-- packages/block-editor/src/components/iframe/index.js | 3 --- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/block-editor/src/components/block-tools/block-popover.js b/packages/block-editor/src/components/block-tools/block-popover.js index 59bad178220509..cb180d550b1451 100644 --- a/packages/block-editor/src/components/block-tools/block-popover.js +++ b/packages/block-editor/src/components/block-tools/block-popover.js @@ -24,7 +24,6 @@ import Inserter from '../inserter'; import { store as blockEditorStore } from '../../store'; import { __unstableUseBlockElement as useBlockElement } from '../block-list/use-block-props/use-block-refs'; import { usePopoverScroll } from './use-popover-scroll'; -import { wrapperState } from '../iframe'; function selector( select ) { const { @@ -216,7 +215,7 @@ function BlockPopover( { shouldAnchorIncludePadding // Used to safeguard sticky position behavior against cases where it would permanently // obscure specific sections of a block. - __unstableEditorCanvasWrapper={ wrapperState.wrapperNode } + __unstableEditorCanvasWrapper={ __unstableContentRef.current } > { ( shouldShowContextualToolbar || isToolbarForced ) && (
Date: Wed, 25 Aug 2021 08:07:53 -0400 Subject: [PATCH 18/18] Update packages/block-editor/src/components/block-tools/block-popover.js --- .../block-editor/src/components/block-tools/block-popover.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/block-editor/src/components/block-tools/block-popover.js b/packages/block-editor/src/components/block-tools/block-popover.js index cb180d550b1451..59d9ae24a71d3a 100644 --- a/packages/block-editor/src/components/block-tools/block-popover.js +++ b/packages/block-editor/src/components/block-tools/block-popover.js @@ -215,7 +215,7 @@ function BlockPopover( { shouldAnchorIncludePadding // Used to safeguard sticky position behavior against cases where it would permanently // obscure specific sections of a block. - __unstableEditorCanvasWrapper={ __unstableContentRef.current } + __unstableEditorCanvasWrapper={ __unstableContentRef?.current } > { ( shouldShowContextualToolbar || isToolbarForced ) && (