Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jan 28, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c2bca7
Add e2e test
ellatrix Jan 25, 2020
277b6bb
Leave fixed position until position can be set
ellatrix Jan 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/block-editor/src/components/block-list/insertion-point.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 } />
Expand Down
2 changes: 0 additions & 2 deletions packages/block-editor/src/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
10 changes: 8 additions & 2 deletions packages/components/src/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ const Popover = ( {
__unstableSlotName = SLOT_NAME,
__unstableAllowVerticalSubpixelPosition,
__unstableAllowHorizontalSubpixelPosition,
__unstableFixedPosition = true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about the growing number of unstable props for this component. This makes me think there's an audit work that's needed to rethink/consolidate these props. It becomes very hard to understand how this component works.

The "modifiers" approach popper.js uses could absorb a lot of these I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's concerning, but I don't see an easy way around it for now. These unstable props allow us to experiment with the popover without committing to an API.

  • __unstableSticky can probably be stabilised at some point.
  • __unstableSlotName should ideally go away.
  • __unstableAllowVerticalSubpixelPosition and __unstableAllowHorizontalSubpixelPosition when Chrome fixes the positioning bug (they're working on it), or if we can find a different workaround.
  • __unstableFixedPosition could be stabilised eventually.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Noting that "could be stabilized" is meant to be a differentiator between what we consider to be "experimental" vs. "unstable".

An unstable API is one which serves as a means to an end. It is not desired to ever be converted into a public API.

https://github.com/WordPress/gutenberg/blob/master/docs/contributors/coding-guidelines.md#experimental-and-unstable-apis

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They all do serve as means to an end. If it's interesting for other popovers, we can consider making it experimental or stable though. I think we should avoid marking anything stable or experimental if we're thinking about using a third party library.

/* eslint-enable no-unused-vars */
...contentProps
} ) => {
Expand All @@ -268,6 +269,7 @@ const Popover = ( {
setStyle( containerRef.current, 'left' );
setStyle( contentRef.current, 'maxHeight' );
setStyle( contentRef.current, 'maxWidth' );
setStyle( containerRef.current, 'position' );
return;
}

Expand All @@ -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;
Expand All @@ -310,6 +314,8 @@ const Popover = ( {
anchor.width,
anchor.height
);
} else {
setStyle( containerRef.current, 'position' );
}

const {
Expand Down
26 changes: 25 additions & 1 deletion packages/e2e-tests/specs/editor/various/navigable-toolbar.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
} );
}
}
);