Block editor: Alt+F10 shouldn't scroll to top (#19896)

* Add e2e test * Leave fixed position until position can be set
WordPress · Jan 28, 2020 · c49b051 · c49b051
1 parent bc38a3d
commit c49b051
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 5 deletions.
diff --git a/packages/block-editor/src/components/block-list/insertion-point.js b/packages/block-editor/src/components/block-list/insertion-point.js
@@ -124,6 +124,7 @@ export default function InsertionPoint( {
 			focusOnMount={ false }
 			className="block-editor-block-list__insertion-point-popover"
 			__unstableSlotName="block-toolbar"
+			__unstableFixedPosition={ false }
 		>
 			<div className="block-editor-block-list__insertion-point" style={ { width: inserterElement.offsetWidth } }>
 				<Indicator clientId={ inserterClientId } />

diff --git a/packages/block-editor/src/components/block-list/style.scss b/packages/block-editor/src/components/block-list/style.scss
@@ -539,7 +539,6 @@
 
 .block-editor-block-list__insertion-point-popover {
 	z-index: z-index(".block-editor-block-list__insertion-point-popover");
-	position: absolute;
 
 	.components-popover__content {
 		background: none;
@@ -551,7 +550,6 @@
 
 .components-popover.block-editor-block-list__block-popover {
 	z-index: z-index(".block-editor-block-list__block-popover");
-	position: absolute;
 
 	.components-popover__content {
 		margin: 0 !important;

diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js
@@ -246,6 +246,7 @@ const Popover = ( {
 	__unstableSlotName = SLOT_NAME,
 	__unstableAllowVerticalSubpixelPosition,
 	__unstableAllowHorizontalSubpixelPosition,
+	__unstableFixedPosition = true,
 	/* eslint-enable no-unused-vars */
 	...contentProps
 } ) => {
@@ -268,6 +269,7 @@ const Popover = ( {
 			setStyle( containerRef.current, 'left' );
 			setStyle( contentRef.current, 'maxHeight' );
 			setStyle( contentRef.current, 'maxWidth' );
+			setStyle( containerRef.current, 'position' );
 			return;
 		}
 
@@ -292,15 +294,17 @@ const Popover = ( {
 				contentRect.current = contentRef.current.getBoundingClientRect();
 			}
 
-			const { offsetParent, ownerDocument } = containerRef.current;
 			let relativeOffsetTop = 0;
 
 			// If there is a positioned ancestor element that is not the body,
 			// subtract the position from the anchor rect. If the position of
 			// the popover is fixed, the offset parent is null or the body
 			// element, in which case the position is relative to the viewport.
 			// See https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/offsetParent
-			if ( offsetParent && offsetParent !== ownerDocument.body ) {
+			if ( ! __unstableFixedPosition ) {
+				setStyle( containerRef.current, 'position', 'absolute' );
+
+				const { offsetParent } = containerRef.current;
 				const offsetParentRect = offsetParent.getBoundingClientRect();
 
 				relativeOffsetTop = offsetParentRect.top;
@@ -310,6 +314,8 @@ const Popover = ( {
 					anchor.width,
 					anchor.height
 				);
+			} else {
+				setStyle( containerRef.current, 'position' );
 			}
 
 			const {

diff --git a/packages/e2e-tests/specs/editor/various/navigable-toolbar.test.js b/packages/e2e-tests/specs/editor/various/navigable-toolbar.test.js
@@ -40,6 +40,30 @@ describe.each( [ [ 'unified', true ], [ 'contextual', false ] ] )(
 			await pressKeyWithModifier( 'alt', 'F10' );
 			expect( await isInBlockToolbar() ).toBe( true );
 		} );
+
+		if ( ! isUnifiedToolbar ) {
+			it( 'should not scroll page', async () => {
+				while ( await page.evaluate( () =>
+					wp.dom.getScrollContainer( document.activeElement ).scrollTop === 0
+				) ) {
+					await page.keyboard.press( 'Enter' );
+				}
+
+				await page.keyboard.type( 'a' );
+
+				const scrollTopBefore = await page.evaluate( () =>
+					wp.dom.getScrollContainer( document.activeElement ).scrollTop
+				);
+
+				await pressKeyWithModifier( 'alt', 'F10' );
+				expect( await isInBlockToolbar() ).toBe( true );
+
+				const scrollTopAfter = await page.evaluate( () =>
+					wp.dom.getScrollContainer( document.activeElement ).scrollTop
+				);
+
+				expect( scrollTopBefore ).toBe( scrollTopAfter );
+			} );
+		}
 	}
 );
-