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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
95 changes: 61 additions & 34 deletions packages/block-editor/src/components/iframe/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,30 @@ 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

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 );
if ( init.defaultPrevented ) {
newEvent.preventDefault();
}
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 +63,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 +129,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 +193,7 @@ function Iframe( {

const disabledRef = useDisabled( { isDisabled: ! readonly } );
const bodyRef = useMergeRefs( [
useBubbleEvents( iframeDocument ),
contentRef,
clearerRef,
writingFlowRef,
Expand Down Expand Up @@ -251,13 +263,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
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>
);
}
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
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
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>
);
}
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
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
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
22 changes: 21 additions & 1 deletion packages/keyboard-shortcuts/src/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,24 @@
*/
import { createContext } from '@wordpress/element';

export const context = createContext();
const globalShortcuts = new Set();
const globalListener = ( event ) => {
for ( const keyboardShortcut of globalShortcuts ) {
keyboardShortcut( event );
}
};

export const context = createContext( {
add: ( shortcut ) => {
if ( globalShortcuts.size === 0 ) {
document.addEventListener( 'keydown', globalListener );
}
globalShortcuts.add( shortcut );
},
delete: ( shortcut ) => {
globalShortcuts.delete( shortcut );
if ( globalShortcuts.size === 0 ) {
document.removeEventListener( 'keydown', globalListener );
}
},
} );
Loading
Loading