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 all 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
8 changes: 7 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 @@ -113,7 +114,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 +133,7 @@ export function useBlockPreview( { blocks, props = {}, layout } ) {
value={ renderedBlocks }
settings={ settings }
>
<EditorStyles />
<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