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

Jump to bottom

useBlockPreview: Try outputting EditorStyles to ensure local style overrides are rendered #55288

Merged
merged 4 commits into from
Oct 15, 2023
Merged

useBlockPreview: Try outputting EditorStyles to ensure local style overrides are rendered #55288

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e744fb
useBlockPreview: Try alternative fix for displaying local style overr…
andrewserong Oct 12, 2023
1d03683
Avoid duplicate styles, fix rendering issues in Safari
andrewserong Oct 13, 2023
f2ca821
Add more explanatory comments
andrewserong Oct 13, 2023
f076055
Remove additional check for styles within the block preview, as it is…
andrewserong Oct 15, 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
22 changes: 21 additions & 1 deletion packages/block-editor/src/components/block-preview/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import deprecated from '@wordpress/deprecated';
*/
import { ExperimentalBlockEditorProvider } from '../provider';
import AutoHeightBlockPreview from './auto';
import EditorStyles from '../editor-styles';
import { store as blockEditorStore } from '../../store';
import { BlockListItems } from '../block-list';

Expand Down Expand Up @@ -92,6 +93,20 @@ export function BlockPreview( {
*/
export default memo( BlockPreview );

function InnerStyles() {
const { styles } = useSelect( ( select ) => {
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
// Retrieve settings (and styles) from the preview's block editor store.
// Since `useBlockPreview` has already cleared out the parent's styles,
// these styles should only contain styles generated by the preview.
const settings = select( blockEditorStore ).getSettings();
return {
styles: settings.styles,
};
}, [] );

return <EditorStyles styles={ styles } />;
Copy link
Member

Choose a reason for hiding this comment

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

How are these styles being added right now, or previously before the bug?
Why does EditorStyles not include these already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prior to the bug being introduced the styles were output via a portal, so even in a child preview with its own block editor provider, the generated styles were being output by the parent. However, since moving over to the block editor store-based state approach for style overrides, this preview no-longer propagates (or renders) any styles generated by the blocks within its preview, because the local-to-the-preview state isn't being used anywhere. Or to put it differently setStyleOverride will only update the preview's block editor store, and not the parent environment's block editor store, which is where EditorStyles is currently used.

The proposed fix in this PR is to ensure that the preview also outputs EditorStyles, but only those styles generated from within the preview.

I've updated the PR description with the following:

Prior to this PR, for things that use useBlockPreview, the styles that are generated within that preview were not output anywhere. This is because ExperimentalBlockEditorProvider uses its own registry, so is designed to have values passed down to it, but not to propagate values outside of itself. So any calls to setStyleOverride would update state within the local block editor provider, and not be captured by the parent's EditorStyles.

}

/**
* This hook is used to lightly mark an element as a block preview wrapper
* element. Call this hook and pass the returned props to the element to mark as
Expand All @@ -113,7 +128,11 @@ export function useBlockPreview( { blocks, props = {}, layout } ) {
[]
);
const settings = useMemo(
() => ( { ...originalSettings, __unstableIsPreviewMode: true } ),
() => ( {
...originalSettings,
styles: undefined, // Clear styles included by the parent settings, as they are already output by the parent's EditorStyles.
__unstableIsPreviewMode: true,
} ),
[ originalSettings ]
);
const disabledRef = useDisabled();
Expand All @@ -128,6 +147,7 @@ export function useBlockPreview( { blocks, props = {}, layout } ) {
value={ renderedBlocks }
settings={ settings }
>
<InnerStyles />
<BlockListItems renderAppender={ false } layout={ layout } />
</ExperimentalBlockEditorProvider>
);
Expand Down
33 changes: 33 additions & 0 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 @@ -37,11 +37,22 @@ import { scopeSelector } from '../components/global-styles/utils';
import { useBlockSettings } from './utils';
import { default as StylesFiltersPanel } from '../components/global-styles/filters-panel';
import { useBlockEditingMode } from '../components/block-editing-mode';
import { __unstableUseBlockElement as useBlockElement } from '../components/block-list/use-block-props/use-block-refs';
import { store as blockEditorStore } from '../store';
import { unlock } from '../lock-unlock';

const EMPTY_ARRAY = [];

// Safari does not always update the duotone filter when the duotone colors
// are changed. This browser check is later used to force a re-render of the block
// element to ensure the duotone filter is updated. The check is included at the
// root of this file as it only needs to be run once per page load.
const isSafari =
window?.navigator.userAgent &&
window.navigator.userAgent.includes( 'Safari' ) &&
! window.navigator.userAgent.includes( 'Chrome' ) &&
! window.navigator.userAgent.includes( 'Chromium' );

extend( [ namesPlugin ] );

function useMultiOriginPresets( { presetSetting, defaultSetting } ) {
Expand Down Expand Up @@ -223,6 +234,7 @@ const withDuotoneControls = createHigherOrderComponent(
);

function DuotoneStyles( {
clientId,
id: filterId,
selector: duotoneSelector,
attribute: duotoneAttr,
Expand Down Expand Up @@ -278,6 +290,8 @@ function DuotoneStyles( {
useDispatch( blockEditorStore )
);

const blockElement = useBlockElement( clientId );

useEffect( () => {
if ( ! isValidFilter ) return;

Expand All @@ -294,12 +308,30 @@ function DuotoneStyles( {
__unstableType: 'svgs',
} );

// Safari does not always update the duotone filter when the duotone colors
// are changed. When using Safari, force the block element to be repainted by
// the browser to ensure any changes are reflected visually. This logic matches
// that used on the site frontend in `block-supports/duotone.php`.
if ( blockElement && isSafari ) {
const display = blockElement.style.display;
// Switch to `inline-block` to force a repaint. In the editor, `inline-block`
// is used instead of `none` to ensure that scroll position is not affected,
// as `none` results in the editor scrolling to the top of the block.
blockElement.style.display = 'inline-block';
// Simply accessing el.offsetHeight flushes layout and style
// changes in WebKit without having to wait for setTimeout.
// eslint-disable-next-line no-unused-expressions
blockElement.offsetHeight;
blockElement.style.display = display;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is very annoying. How is this related to the original issue? Has this always been an issue in safari? Would be great to find a fix that doesn't rely on blockElement.

Copy link
Contributor

@ajlende ajlende Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Safari has a history of not correctly updating HTML elements with SVG filters applied. The approach used here of accessing el.offsetHeight was previously used when Safari wouldn't update the element correctly even on the first pass of rendering the page. That particular bug has been fixed now in Safari, but I guess it's broken now for updating CSS after the first pass.

It would be worthwhile to report this issue to https://bugs.webkit.org and add it to the list in #47302.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked the list of open webkit issues, and it looks like it's already tracked (from way back in 2012!) in this one: https://bugs.webkit.org/show_bug.cgi?id=99996 so I've added it to the list in #47302

This is very annoying. How is this related to the original issue? Has this always been an issue in safari? Would be great to find a fix that doesn't rely on blockElement.

It is annoying that we have to include browser-specific workarounds like this, but for now I think keeping this within Duotone and only when the styles are updated is probably the right place for it (at least for WP 6.4). As far as I can tell it's always been an issue in Safari, but the changes from outputting style tags via a portal to using the state-based styles in WP 6.4 appears to have made the issue more prominent. At least we've got the workaround in place for now... thanks again for the troubleshooting tips @ajlende!


return () => {
deleteStyleOverride( filterId );
deleteStyleOverride( `duotone-${ filterId }` );
};
}, [
isValidFilter,
blockElement,
colors,
selector,
filterId,
Expand Down Expand Up @@ -378,6 +410,7 @@ const withDuotoneStyles = createHigherOrderComponent(
<>
{ shouldRender && (
<DuotoneStyles
clientId={ props.clientId }
id={ filterClass }
selector={ selector }
attribute={ attribute }
Expand Down
Loading