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

Make the shortcuts provider optional #54080

Merged
merged 18 commits into from
Sep 4, 2023
Merged

Make the shortcuts provider optional #54080

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a7151ed
Make the shortcut provider optional
youknowriad Aug 25, 2023
88b69b2
Bubble keydown events
youknowriad Aug 25, 2023
ab7c07c
Prevent double region navigation
youknowriad Aug 25, 2023
2a6247c
Prevent bubbling when navigation mode is handled
youknowriad Aug 25, 2023
a94eb2d
Wait to register the event handler
youknowriad Aug 28, 2023
0138f13
SSR support for keyboard shortcuts
youknowriad Aug 29, 2023
93537de
Small improvements
youknowriad Aug 29, 2023
82a856b
Small improvements
youknowriad Aug 29, 2023
687c653
Small update
youknowriad Aug 29, 2023
96d4684
Fix e2e tests
youknowriad Aug 29, 2023
e10d9d2
Prevent the iframe event from bubbling twice
youknowriad Aug 31, 2023
42015f0
Remove the event propagation
youknowriad Aug 31, 2023
dde6447
Fixes
youknowriad Aug 31, 2023
7ff56bf
Fix event listener
youknowriad Sep 1, 2023
f419388
Change how we prevent bubbling iframe events
youknowriad Sep 1, 2023
39b2f74
Fix more e2e tests
youknowriad Sep 4, 2023
95e3607
Escape command shouldn't trigger when dialogs are open
youknowriad Sep 4, 2023
cdc70bd
Retain defaultPrevented value
youknowriad Sep 4, 2023
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
92 changes: 58 additions & 34 deletions packages/block-editor/src/components/iframe/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,27 @@ import { useWritingFlow } from '../writing-flow';
import { useCompatibilityStyles } from './use-compatibility-styles';
import { store as blockEditorStore } from '../../store';

function bubbleEvent( event, Constructor, frame ) {
const init = {};

for ( const key in event ) {
init[ key ] = event[ key ];
}

if ( event instanceof frame.ownerDocument.defaultView.MouseEvent ) {
const rect = frame.getBoundingClientRect();
init.clientX += rect.left;
init.clientY += rect.top;
}
Comment on lines +41 to +45
Copy link
Contributor

@andrewserong andrewserong Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm just looking into the issue over in #55074:

Why is this guarded against a check for event instanceof frame.ownerDocument.defaultView.MouseEvent? I notice if I change this to if ( frame ) { I can get the drag position of the block drag chip (i.e. dragging from the inserter over into the editor canvas) working properly again. So it seems this is returning false for some situations where we still want the offset to be applied, so that drag positioning will be correct. In this example, I think it might mean drag events that originated from outside the iframe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really know, this code was there long before my PR. I'm guessing we should do this for all events that have "clientX" and "clientY" (we may want to exclude keyboard events for instance...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming, I'll have a play around 🙂


const newEvent = new Constructor( event.type, init );
const cancelled = ! frame.dispatchEvent( newEvent );

if ( cancelled ) {
event.preventDefault();
}
}

/**
* Bubbles some event types (keydown, keypress, and dragover) to parent document
* document to ensure that the keyboard shortcuts and drag and drop work.
Expand All @@ -39,42 +60,30 @@ import { store as blockEditorStore } from '../../store';
* should be context dependent, e.g. actions on blocks like Cmd+A should not
* work globally outside the block editor.
*
* @param {Document} doc Document to attach listeners to.
* @param {Document} iframeDocument Document to attach listeners to.
*/
function bubbleEvents( doc ) {
const { defaultView } = doc;
const { frameElement } = defaultView;

function bubbleEvent( event ) {
const prototype = Object.getPrototypeOf( event );
const constructorName = prototype.constructor.name;
const Constructor = window[ constructorName ];

const init = {};

for ( const key in event ) {
init[ key ] = event[ key ];
function useBubbleEvents( iframeDocument ) {
return useRefEffect( ( body ) => {
const { defaultView } = iframeDocument;
const { frameElement } = defaultView;
const eventTypes = [ 'dragover', 'mousemove' ];
Copy link
Member

Choose a reason for hiding this comment

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

So we're not bubbling keydown in the end?

Copy link
Member

Choose a reason for hiding this comment

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

Is this refactoring helping with anything in particular or is it a remnant from an earlier iteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the refactoring is not doing anything I can restore the previous "bubbleEvents" here but we need the inner "bubbleEvent" function to be shared so we can use it when bubbling the keydown events as a React event handler.

const handlers = {};
for ( const name of eventTypes ) {
handlers[ name ] = ( event ) => {
const prototype = Object.getPrototypeOf( event );
const constructorName = prototype.constructor.name;
const Constructor = window[ constructorName ];
bubbleEvent( event, Constructor, frameElement );
};
body.addEventListener( name, handlers[ name ] );
}

if ( event instanceof defaultView.MouseEvent ) {
const rect = frameElement.getBoundingClientRect();
init.clientX += rect.left;
init.clientY += rect.top;
}

const newEvent = new Constructor( event.type, init );
const cancelled = ! frameElement.dispatchEvent( newEvent );

if ( cancelled ) {
event.preventDefault();
}
}

const eventTypes = [ 'dragover', 'mousemove' ];

for ( const name of eventTypes ) {
doc.addEventListener( name, bubbleEvent );
}
return () => {
for ( const name of eventTypes ) {
body.removeEventListener( name, handlers[ name ] );
}
};
} );
}

function Iframe( {
Expand Down Expand Up @@ -117,7 +126,6 @@ function Iframe( {
const { documentElement } = contentDocument;
iFrameDocument = contentDocument;

bubbleEvents( contentDocument );
clearerRef( documentElement );

// Ideally ALL classes that are added through get_body_class should
Expand Down Expand Up @@ -182,6 +190,7 @@ function Iframe( {

const disabledRef = useDisabled( { isDisabled: ! readonly } );
const bodyRef = useMergeRefs( [
useBubbleEvents( iframeDocument ),
contentRef,
clearerRef,
writingFlowRef,
Expand Down Expand Up @@ -251,13 +260,28 @@ function Iframe( {
>
{ iframeDocument &&
createPortal(
// We want to prevent React events from bubbling throught the iframe
// we bubble these manually.
/* eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions */
<body
ref={ bodyRef }
className={ classnames(
'block-editor-iframe__body',
'editor-styles-wrapper',
...bodyClasses
) }
onKeyDown={ ( event ) => {
// This stopPropagation call ensures React doesn't create a syncthetic event to bubble this event
// which would result in two React events being bubbled throught the iframe.
event.stopPropagation();
const { defaultView } = iframeDocument;
const { frameElement } = defaultView;
bubbleEvent(
event,
window.KeyboardEvent,
frameElement
);
} }
Copy link
Member

Choose a reason for hiding this comment

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

Why are we listening for the React event instead of native event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the only way to basically prevent the "double event bubbling" properly. If I use stopPropagation on a regular event handler and I trigger the event on the frame, it never reached the upper event handlers in my tests.

Copy link
Member

Choose a reason for hiding this comment

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

There's something weird about this, but I guess we can investigate later.

>
{ contentResizeListener }
<StyleProvider document={ iframeDocument }>
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/components/writing-flow/use-tab-nav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ export default function useTabNav() {

if ( event.keyCode === ESCAPE && ! hasMultiSelection() ) {
event.preventDefault();
event.stopPropagation();
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
setNavigationMode( true );
return;
}
Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/autocomplete/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,8 @@ export function useAutocomplete( {
setAutocompleter( null );
setAutocompleterUI( null );
event.preventDefault();
// This prevents the block editor from handling the escape key to unselect the block.
event.stopPropagation();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These remaining stopPropagation calls were necessary because if I don't do this the "clear block selection" shortcut triggers as well which causes the block to unselect and lose focus.

What I don't understand is why we didn't have these in place in "trunk" because even in trunk, it seems that the handler of the "unselect" shortcut should trigger because we're just hitting "Escape", I'm not sure why it's not called on trunk. It seemed logical for me that we add these when dialogs are open and we only want to close the dialogs.

Copy link
Member

Choose a reason for hiding this comment

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

In that case we should check event.defaultPrevented for clearing selection. Did we forget that there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are checking that mmm, I guess it's the defaultPrevented value that might be different between trunk and this branch since we're now using a different event object. Let me check that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I think it's solved now :)

break;

case 'Enter':
Expand Down
1 change: 1 addition & 0 deletions packages/compose/src/hooks/use-dialog/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ function useDialog( options: DialogOptions ): useDialogReturn {
currentOptions.current?.onClose
) {
event.preventDefault();
event.stopPropagation();
currentOptions.current.onClose();
}
} );
Expand Down
28 changes: 11 additions & 17 deletions packages/customize-widgets/src/components/customize-widgets/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import { useState, useEffect, useRef, createPortal } from '@wordpress/element';
import { SlotFillProvider, Popover } from '@wordpress/components';
import { ShortcutProvider } from '@wordpress/keyboard-shortcuts';

/**
* Internal dependencies
Expand Down Expand Up @@ -68,21 +67,16 @@ export default function CustomizeWidgets( {
);

return (
<ShortcutProvider>
<SlotFillProvider>
<SidebarControls
sidebarControls={ sidebarControls }
activeSidebarControl={ activeSidebarControl }
>
<FocusControl
api={ api }
sidebarControls={ sidebarControls }
>
{ activeSidebar }
{ popover }
</FocusControl>
</SidebarControls>
</SlotFillProvider>
</ShortcutProvider>
<SlotFillProvider>
<SidebarControls
sidebarControls={ sidebarControls }
activeSidebarControl={ activeSidebarControl }
>
<FocusControl api={ api } sidebarControls={ sidebarControls }>
{ activeSidebar }
{ popover }
</FocusControl>
</SidebarControls>
</SlotFillProvider>
);
}
9 changes: 4 additions & 5 deletions packages/edit-post/src/components/keyboard-shortcut-help-modal/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { render, screen } from '@testing-library/react';
* WordPress dependencies
*/
import { EditorKeyboardShortcutsRegister } from '@wordpress/editor';
import { ShortcutProvider } from '@wordpress/keyboard-shortcuts';

/**
* Internal dependencies
Expand All @@ -19,10 +18,10 @@ const noop = () => {};
describe( 'KeyboardShortcutHelpModal', () => {
it( 'should match snapshot when the modal is active', () => {
render(
<ShortcutProvider>
<>
<EditorKeyboardShortcutsRegister />
<KeyboardShortcutHelpModal isModalActive toggleModal={ noop } />
</ShortcutProvider>
</>
);

expect(
Expand All @@ -34,13 +33,13 @@ describe( 'KeyboardShortcutHelpModal', () => {

it( 'should not render the modal when inactive', () => {
render(
<ShortcutProvider>
<>
<EditorKeyboardShortcutsRegister />
<KeyboardShortcutHelpModal
isModalActive={ false }
toggleModal={ noop }
/>
</ShortcutProvider>
</>
);

expect(
Expand Down
37 changes: 17 additions & 20 deletions packages/edit-post/src/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
import { useMemo } from '@wordpress/element';
import { SlotFillProvider } from '@wordpress/components';
import { store as coreStore } from '@wordpress/core-data';
import { ShortcutProvider } from '@wordpress/keyboard-shortcuts';
import { store as preferencesStore } from '@wordpress/preferences';
import { CommandMenu } from '@wordpress/commands';
import { privateApis as coreCommandsPrivateApis } from '@wordpress/core-commands';
Expand Down Expand Up @@ -156,25 +155,23 @@ function Editor( { postId, postType, settings, initialEdits, ...props } ) {
}

return (
<ShortcutProvider>
<SlotFillProvider>
<ExperimentalEditorProvider
settings={ editorSettings }
post={ post }
initialEdits={ initialEdits }
useSubRegistry={ false }
__unstableTemplate={ isTemplateMode ? template : undefined }
{ ...props }
>
<ErrorBoundary>
<CommandMenu />
<EditorInitialization postId={ postId } />
<Layout />
</ErrorBoundary>
<PostLockedModal />
</ExperimentalEditorProvider>
</SlotFillProvider>
</ShortcutProvider>
<SlotFillProvider>
<ExperimentalEditorProvider
settings={ editorSettings }
post={ post }
initialEdits={ initialEdits }
useSubRegistry={ false }
__unstableTemplate={ isTemplateMode ? template : undefined }
{ ...props }
>
<ErrorBoundary>
<CommandMenu />
<EditorInitialization postId={ postId } />
<Layout />
</ErrorBoundary>
<PostLockedModal />
</ExperimentalEditorProvider>
</SlotFillProvider>
);
}

Expand Down
21 changes: 9 additions & 12 deletions packages/edit-site/src/components/app/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import { SlotFillProvider } from '@wordpress/components';
import { UnsavedChangesWarning } from '@wordpress/editor';
import { ShortcutProvider } from '@wordpress/keyboard-shortcuts';
import { store as noticesStore } from '@wordpress/notices';
import { useDispatch } from '@wordpress/data';
import { __, sprintf } from '@wordpress/i18n';
Expand Down Expand Up @@ -35,16 +34,14 @@ export default function App() {
}

return (
<ShortcutProvider style={ { height: '100%' } }>
<SlotFillProvider>
<GlobalStylesProvider>
<UnsavedChangesWarning />
<RouterProvider>
<Layout />
<PluginArea onError={ onPluginAreaError } />
</RouterProvider>
</GlobalStylesProvider>
</SlotFillProvider>
</ShortcutProvider>
<SlotFillProvider>
<GlobalStylesProvider>
<UnsavedChangesWarning />
<RouterProvider>
<Layout />
<PluginArea onError={ onPluginAreaError } />
</RouterProvider>
</GlobalStylesProvider>
</SlotFillProvider>
);
}
29 changes: 13 additions & 16 deletions packages/edit-widgets/src/components/widget-areas-block-editor-provider/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
privateApis as blockEditorPrivateApis,
} from '@wordpress/block-editor';
import { privateApis as editPatternsPrivateApis } from '@wordpress/patterns';
import { ShortcutProvider } from '@wordpress/keyboard-shortcuts';
import { store as preferencesStore } from '@wordpress/preferences';

/**
Expand Down Expand Up @@ -98,21 +97,19 @@ export default function WidgetAreasBlockEditorProvider( {
);

return (
<ShortcutProvider>
<SlotFillProvider>
<KeyboardShortcuts.Register />
<SlotFillProvider>
<ExperimentalBlockEditorProvider
value={ blocks }
onInput={ onInput }
onChange={ onChange }
settings={ settings }
useSubRegistry={ false }
{ ...props }
>
<CopyHandler>{ children }</CopyHandler>
<PatternsMenuItems rootClientId={ widgetAreaId } />
</ExperimentalBlockEditorProvider>
</SlotFillProvider>
</ShortcutProvider>
<ExperimentalBlockEditorProvider
value={ blocks }
onInput={ onInput }
onChange={ onChange }
settings={ settings }
useSubRegistry={ false }
{ ...props }
>
<CopyHandler>{ children }</CopyHandler>
<PatternsMenuItems rootClientId={ widgetAreaId } />
</ExperimentalBlockEditorProvider>
</SlotFillProvider>
);
}
2 changes: 1 addition & 1 deletion packages/keyboard-shortcuts/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ _This package assumes that your code will run in an **ES2015+** environment. If

### ShortcutProvider

Handles callbacks added to context by `useShortcut`.
Handles callbacks added to context by `useShortcut`. Adding a provider allows to register contextual shortcuts that are only active when a certain part of the UI is focused.

_Parameters_

Expand Down
2 changes: 2 additions & 0 deletions packages/keyboard-shortcuts/src/components/shortcut-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ const { Provider } = context;

/**
* Handles callbacks added to context by `useShortcut`.
* Adding a provider allows to register contextual shortcuts
* that are only active when a certain part of the UI is focused.
*
* @param {Object} props Props to pass to `div`.
*
Expand Down
Loading