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: improve root portal #48803

Closed
wants to merge 1 commit into from
Closed

Block editor: improve root portal #48803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
68 changes: 58 additions & 10 deletions packages/block-editor/src/components/block-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,14 @@ import {
} from '@wordpress/compose';
import {
createContext,
useState,
useMemo,
useCallback,
useLayoutEffect,
useContext,
useRef,
Fragment,
createElement,
useReducer,
} from '@wordpress/element';

/**
Expand All @@ -41,13 +46,55 @@ import {
DEFAULT_BLOCK_EDIT_CONTEXT,
} from '../block-edit/context';

const elementContext = createContext();
const componentsContext = createContext();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be ComponentsContext?

This also confused me when I first noticed elementsContext, all the other contexts in the codebase have an uppercase first character.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not a component? :)


export const IntersectionObserver = createContext();
const pendingBlockVisibilityUpdatesPerRegistry = new WeakMap();

function useRootPortal( component ) {
const components = useContext( componentsContext );

useLayoutEffect( () => {
if ( ! components ) return;
components.add( component );
return () => {
components.delete( component );
};
} );
Copy link
Member

Choose a reason for hiding this comment

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

This seems fragile. Every time it renders it deletes the component and adds a new one. And when it does that it changes the order of the components. Because of the lack of keys, it will just re-render everything.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add keys 👍

}

function Components( { subscriber, components } ) {
const [ , forceRender ] = useReducer( () => ( {} ) );
subscriber.current = forceRender;
Copy link
Member

Choose a reason for hiding this comment

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

Writing refs during rendering is not recommended in concurrent mode. It seems to be only acceptable when it's for lazily evaluating for once.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to be only acceptable when it's for lazily evaluating for once.

What does that mean? forceRender remains constant here, so maybe it's fine?

return createElement( Fragment, null, ...components.current );
}

function ComponentRenderer( { children } ) {
const subscriber = useRef( () => {} );
const components = useRef( new Set() );
return (
<componentsContext.Provider
value={ useMemo(
() => ( {
add( component ) {
components.current.add( component );
subscriber.current();
},
delete( component ) {
components.current.delete( component );
subscriber.current();
Copy link
Member

Choose a reason for hiding this comment

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

Whenever a component that useRootPortal re-renders, it will delete the content and re-add it in useLayoutEffect, thus calling subscriber twice, which is just a forceRender function in the Components component. This results in a double-render for the portal component for every render of the consumer's component. While React is typically very fast, this does feel like an unnecessary overhead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix this.

},
} ),
[]
) }
>
<Components subscriber={ subscriber } components={ components } />
{ children }
</componentsContext.Provider>
);
}

function Root( { className, ...settings } ) {
const [ element, setElement ] = useState();
const isLargeViewport = useViewportMatch( 'medium' );
const { isOutlineMode, isFocusMode, editorMode } = useSelect(
( select ) => {
Expand Down Expand Up @@ -105,7 +152,6 @@ function Root( { className, ...settings } ) {
ref: useMergeRefs( [
useBlockSelectionClearer(),
useInBetweenInserter(),
setElement,
] ),
className: classnames( 'is-root-container', className, {
'is-outline-mode': isOutlineMode,
Expand All @@ -116,11 +162,13 @@ function Root( { className, ...settings } ) {
settings
);
return (
<elementContext.Provider value={ element }>
<IntersectionObserver.Provider value={ intersectionObserver }>
<div { ...innerBlocksProps } />
</IntersectionObserver.Provider>
</elementContext.Provider>
<IntersectionObserver.Provider value={ intersectionObserver }>
<div { ...innerBlocksProps }>
<ComponentRenderer>
{ innerBlocksProps.children }
</ComponentRenderer>
</div>
</IntersectionObserver.Provider>
);
}

Expand All @@ -135,7 +183,7 @@ export default function BlockList( settings ) {
);
}

BlockList.__unstableElementContext = elementContext;
BlockList.useRootPortal = useRootPortal;
Copy link
Member Author

Choose a reason for hiding this comment

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

Would be good to also make this unstable for now, but eventually this should be exported in the block editor package as an official API.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can start private.


function Items( {
placeholder,
Expand Down
15 changes: 3 additions & 12 deletions packages/block-editor/src/hooks/duotone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import namesPlugin from 'colord/plugins/names';
import { getBlockSupport, hasBlockSupport } from '@wordpress/blocks';
import { createHigherOrderComponent, useInstanceId } from '@wordpress/compose';
import { addFilter } from '@wordpress/hooks';
import { useMemo, useContext, createPortal } from '@wordpress/element';
import { useMemo } from '@wordpress/element';
import { useSelect } from '@wordpress/data';

/**
Expand Down Expand Up @@ -255,14 +255,6 @@ function BlockDuotoneStyles( { name, duotoneStyle, id } ) {
defaultSetting: 'color.defaultDuotone',
} );

const element = useContext( BlockList.__unstableElementContext );

// Portals cannot exist without a container.
// Guard against empty Duotone styles.
if ( ! element || ! duotoneStyle ) {
return null;
}

let colors = duotoneStyle;

if ( ! Array.isArray( colors ) && colors !== 'unset' ) {
Expand All @@ -282,13 +274,12 @@ function BlockDuotoneStyles( { name, duotoneStyle, id } ) {
duotoneSupportSelectors
);

return createPortal(
BlockList.useRootPortal(
<InlineDuotone
selector={ selectorsGroup }
id={ id }
colors={ colors }
/>,
element
/>
);
}

Expand Down
50 changes: 20 additions & 30 deletions packages/block-editor/src/hooks/layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
PanelBody,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { useContext, createPortal } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -364,7 +363,6 @@ export const withLayoutStyles = createHigherOrderComponent(
hasLayoutBlockSupport && ! disableLayoutStyles;
const id = useInstanceId( BlockListBlock );
const defaultThemeLayout = useSetting( 'layout' ) || {};
const element = useContext( BlockList.__unstableElementContext );
const { layout } = attributes;
const { default: defaultBlockLayout } =
getBlockSupport( name, layoutBlockSupportKey ) || {};
Expand Down Expand Up @@ -405,26 +403,23 @@ export const withLayoutStyles = createHigherOrderComponent(
layoutClasses
);

return (
<>
{ shouldRenderLayoutStyles &&
element &&
!! css &&
createPortal(
<LayoutStyle
blockName={ name }
selector={ selector }
css={ css }
layout={ usedLayout }
style={ attributes?.style }
/>,
element
) }
<BlockListBlock
{ ...props }
__unstableLayoutClassNames={ layoutClassNames }
BlockList.useRootPortal(
shouldRenderLayoutStyles && !! css && (
<LayoutStyle
blockName={ name }
selector={ selector }
css={ css }
layout={ usedLayout }
style={ attributes?.style }
/>
</>
)
);

return (
<BlockListBlock
{ ...props }
__unstableLayoutClassNames={ layoutClassNames }
/>
);
}
);
Expand All @@ -449,7 +444,6 @@ export const withChildLayoutStyles = createHigherOrderComponent(
const shouldRenderChildLayoutStyles =
hasChildLayout && ! disableLayoutStyles;

const element = useContext( BlockList.__unstableElementContext );
const id = useInstanceId( BlockListBlock );
const selector = `.wp-container-content-${ id }`;

Expand All @@ -472,15 +466,11 @@ export const withChildLayoutStyles = createHigherOrderComponent(
shouldRenderChildLayoutStyles && !! css, // Only attach a container class if there is generated CSS to be attached.
} );

return (
<>
{ shouldRenderChildLayoutStyles &&
element &&
!! css &&
createPortal( <style>{ css }</style>, element ) }
<BlockListBlock { ...props } className={ className } />
</>
BlockList.useRootPortal(
shouldRenderChildLayoutStyles && !! css && <style>{ css }</style>
);

return <BlockListBlock { ...props } className={ className } />;
}
);

Expand Down
20 changes: 5 additions & 15 deletions packages/block-editor/src/hooks/position.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,7 @@ import {
} from '@wordpress/components';
import { createHigherOrderComponent, useInstanceId } from '@wordpress/compose';
import { useSelect } from '@wordpress/data';
import {
useContext,
useMemo,
createPortal,
Platform,
} from '@wordpress/element';
import { useMemo, Platform } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';

/**
Expand Down Expand Up @@ -353,7 +348,6 @@ export const withPositionStyles = createHigherOrderComponent(
hasPositionBlockSupport && ! useIsPositionDisabled( props );

const id = useInstanceId( BlockListBlock );
const element = useContext( BlockList.__unstableElementContext );

// Higher specificity to override defaults in editor UI.
const positionSelector = `.wp-container-${ id }.wp-container-${ id }`;
Expand All @@ -377,15 +371,11 @@ export const withPositionStyles = createHigherOrderComponent(
!! attributes?.style?.position?.type,
} );

return (
<>
{ allowPositionStyles &&
element &&
!! css &&
createPortal( <style>{ css }</style>, element ) }
<BlockListBlock { ...props } className={ className } />
</>
BlockList.useRootPortal(
allowPositionStyles && !! css && <style>{ css }</style>
);

return <BlockListBlock { ...props } className={ className } />;
}
);

Expand Down
47 changes: 21 additions & 26 deletions packages/block-editor/src/hooks/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { useContext, useMemo, createPortal } from '@wordpress/element';
import { useMemo } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';
import {
getBlockSupport,
Expand Down Expand Up @@ -418,33 +418,28 @@ const withElementsStyles = createHigherOrderComponent(
return elementCssRules.length > 0 ? elementCssRules : undefined;
}, [ props.attributes.style?.elements ] );

const element = useContext( BlockList.__unstableElementContext );
BlockList.useRootPortal(
styles && (
<style
dangerouslySetInnerHTML={ {
__html: styles,
} }
/>
)
);

return (
<>
{ styles &&
element &&
createPortal(
<style
dangerouslySetInnerHTML={ {
__html: styles,
} }
/>,
element
) }

<BlockListBlock
{ ...props }
className={
props.attributes.style?.elements
? classnames(
props.className,
blockElementsContainerIdentifier
)
: props.className
}
/>
</>
<BlockListBlock
{ ...props }
className={
props.attributes.style?.elements
? classnames(
props.className,
blockElementsContainerIdentifier
)
: props.className
}
/>
);
}
);
Expand Down
10 changes: 1 addition & 9 deletions packages/block-library/src/gallery/gap-styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@ import {
BlockList,
__experimentalGetGapCSSValue as getGapCSSValue,
} from '@wordpress/block-editor';
import { useContext, createPortal } from '@wordpress/element';

export default function GapStyles( { blockGap, clientId } ) {
const styleElement = useContext( BlockList.__unstableElementContext );
// --gallery-block--gutter-size is deprecated. --wp--style--gallery-gap-default should be used by themes that want to set a default
// gap on the gallery.
const fallbackValue = `var( --wp--style--gallery-gap-default, var( --gallery-block--gutter-size, var( --wp--style--block-gap, 0.5em ) ) )`;
Expand All @@ -35,11 +33,5 @@ export default function GapStyles( { blockGap, clientId } ) {
gap: ${ gapValue }
}`;

const GapStyle = () => {
return <style>{ gap }</style>;
};

return gap && styleElement
? createPortal( <GapStyle />, styleElement )
: null;
BlockList.useRootPortal( gap ? <style>{ gap }</style> : null );
}