Skip to content

Duotone: Pass filters to the Post Editor #49239

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

Merged
scruffian merged 25 commits into from
Mar 29, 2023
Merged

Duotone: Pass filters to the Post Editor #49239

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
a98c60b
Duotone: Pass filters to the Post Editor
scruffian Mar 21, 2023
32355ba
remove whitespace change
scruffian Mar 21, 2023
7810e0a
remove whitespace change
scruffian Mar 21, 2023
0e6304e
Move SVGs into styles prop for EditorStyles
Mar 22, 2023
690cca4
Handle duotone presets settings in add_editor_settings
Mar 22, 2023
5a99e30
Fix php lint
Mar 22, 2023
8f9541e
Add a check for $settings['styles'] to be safe
Mar 22, 2023
2b8484b
Use SVG component instead of svg
Mar 22, 2023
6a19107
Use === instead of ==
Mar 22, 2023
6e0d5c0
output the rules for all global styles not just the used ones
scruffian Mar 22, 2023
44e46c8
Block Previews: Fix Duotone in Block Previews (#49290)
scruffian Mar 23, 2023
0b4dfe7
Rename duotone css and svg variables
Mar 23, 2023
9f84812
Move styles to the document.head with a portal
Mar 23, 2023
57471f9
Revert "Move styles to the document.head with a portal"
Mar 23, 2023
05121a2
Move EditorStyles to body for EditorCanvas iframe
Mar 23, 2023
20a6fde
Move EditorStyles to body for the global styles StylesPreview iframe
Mar 23, 2023
9b4e96c
Move EditorStyles to body for the StyleBook iframe
Mar 23, 2023
4fb10d4
Refactor editorStyles for EditorCanvas
Mar 23, 2023
5fe3835
Directly test for css in the transformStyles call since that is what …
Mar 23, 2023
32f2147
?.length is unnecessary
Mar 23, 2023
3688db6
Explain settings better
Mar 23, 2023
f426d62
Partially revert "Refactor editorStyles for EditorCanvas"
Mar 23, 2023
0dabaf4
Update useGlobalStylesOutput to use SVGs strings in styles instead of…
Mar 24, 2023
6bc19a9
Add missing __unstableType for svgs
Mar 24, 2023
a0be2bf
Add comment about required JS changes for __unstableType in PHP
Mar 24, 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
Prev Previous commit
Next Next commit
Move SVGs into styles prop for EditorStyles
  • Loading branch information
Alex Lende committed Mar 22, 2023
commit 0e6304e49b531a255a502188e857ce2111465d4c
1 change: 1 addition & 0 deletions lib/block-supports/duotone.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,3 +449,4 @@ function gutenberg_render_duotone_support( $block_content, $block ) {
add_filter( 'render_block', array( 'WP_Duotone_Gutenberg', 'render_duotone_support' ), 10, 2 );
add_action( 'wp_enqueue_scripts', array( 'WP_Duotone_Gutenberg', 'output_global_styles' ), 11 );
add_action( 'wp_footer', array( 'WP_Duotone_Gutenberg', 'output_footer_assets' ), 10 );
add_filter( 'block_editor_settings_all', array( 'WP_Duotone_Gutenberg', 'add_editor_settings' ), 10 );
28 changes: 18 additions & 10 deletions lib/class-wp-duotone-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,27 +187,35 @@ private static function get_css_custom_property_declaration( $filter_data ) {
* Outputs all necessary SVG for duotone filters, CSS for classic themes.
*/
public static function output_footer_assets() {
echo self::get_footer_assets();
}

/**
* Outputs all necessary SVG for duotone filters, CSS for classic themes, and safari rerendering hack
*/
public static function get_footer_assets() {
$assets = '';
foreach ( self::$output as $filter_data ) {

// SVG will be output on the page later.
$assets .= gutenberg_get_duotone_filter_svg( $filter_data );
$filter_svg = gutenberg_get_duotone_filter_svg( $filter_data );

echo $filter_svg;

// This is for classic themes - in block themes, the CSS is added in the head via wp_add_inline_style in the wp_enqueue_scripts action.
if ( ! wp_is_block_theme() ) {
wp_add_inline_style( 'core-block-supports', 'body{' . self::get_css_custom_property_declaration( $filter_data ) . '}' );
}
}
return $assets;
}

public static function add_editor_settings( $settings ) {
$assets = '';
foreach ( self::$output as $filter_data ) {
$assets .= gutenberg_get_duotone_filter_svg( $filter_data );
}

$settings['styles'][] = array(
'assets' => $assets,
'__unstableType' => 'svgs',
// TODO: Figure out what isGlobalStyles should be set to.
'isGlobalStyles' => true,
);

return $settings;
}

/**
* Appends the used global style duotone filter CSS Vars to the inline global styles CSS
Expand Down
1 change: 0 additions & 1 deletion lib/experimental/block-editor-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ function gutenberg_get_block_editor_settings_experimental( $settings ) {
}
}

$settings['filters'] = WP_Duotone_Gutenberg::get_footer_assets();
return $settings;
}

Expand Down
25 changes: 24 additions & 1 deletion packages/block-editor/src/components/editor-styles/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,21 @@ function useDarkThemeBodyClassName( styles ) {

export default function EditorStyles( { styles } ) {
const transformedStyles = useMemo(
() => transformStyles( styles, EDITOR_STYLES_SELECTOR ),
() =>
transformStyles(
// Assume that non-SVG styles are CSS that can be transformed.
styles.filter( ( style ) => style.__unstableType !== 'svgs' ),
EDITOR_STYLES_SELECTOR
),
[ styles ]
);
Copy link
Contributor

@andrewserong andrewserong Mar 23, 2023

Choose a reason for hiding this comment

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

This is a potentially subtle issue, but the filter here appears to be to ensure that the svg elements provided by the backend data aren't included in transformedStyles.

When this change is eventually backported, it'll likely happen at slightly different times, typically with the PHP changes landing before the JS changes.

Would it be possible or worthwhile to have the svg elements in a different part of the block editor settings, so that if the backend changes land first, the frontend doesn't get tripped up if this code hasn't landed in the JS side yet?

Copy link
Contributor

@ajlende ajlende Mar 23, 2023

Choose a reason for hiding this comment

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

This is a potentially subtle issue, but the filter here appears to be to ensure that the svg elements provided by the backend data aren't included in transformedStyles.

Hmmm... I can see how that's an issue, but I don't have any good ideas for how to solve it right now. Since we've decided to keep svgs part of styles, we're going to have to have JS changes added before PHP changes no matter what. We might just have to leave a note about it.

Copy link
Contributor

@andrewserong andrewserong Mar 24, 2023

Choose a reason for hiding this comment

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

We might just have to leave a note about it.

That sounds reasonable to me, if it's the right path forward for the feature. It'd be good to make sure the comments are clear in the PHP code for when it comes time to backport, so that we don't forget 🙂

Copy link
Contributor

@ajlende ajlende Mar 24, 2023

Choose a reason for hiding this comment

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

I'm not sure if this is what you're talking about, but I added a comment in the php in a0be2bf.

// The 'svgs' type is new in 6.3 and requires the corresponding JS changes in the EditorStyles component to work.


const transformedSvgs = useMemo(
() =>
styles
.filter( ( style ) => style.__unstableType == 'svgs' )
.map( ( style ) => style.assets )
.join( '' ),
[ styles ]
);

Expand All @@ -80,6 +94,15 @@ export default function EditorStyles( { styles } ) {
{ transformedStyles.map( ( css, index ) => (
<style key={ index }>{ css }</style>
) ) }
<svg
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 0 0"
width="0"
height="0"
focusable="false"
role="none"
dangerouslySetInnerHTML={ { __html: transformedSvgs } }
/>
Copy link
Contributor

@ajlende ajlende Mar 22, 2023

Choose a reason for hiding this comment

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

Since we're adding all this front matter in this case and we're outputting them all at once in the frontend too, there's another performance improvement that we can make with duotone filters by wrapping all the filter definitions in a single SVG wrapper like this.

Not for this PR, but as a follow-up.

Copy link
Contributor Author

@scruffian scruffian Mar 22, 2023

Choose a reason for hiding this comment

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

Why would that be more performant?

Copy link
Contributor

@ajlende ajlende Mar 23, 2023

Choose a reason for hiding this comment

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

Re-reading what I wrote, I really didn't explain that well. What I'm saying is that we can have one wrapper for all the SVG defs, so transformedSvgs would just contain the definitions.

<filter id="wp-duotone-grayscale">
	<feColorMatrix ... />
	<feComponentTransfer ...>...</feComponentTransfer>
	<feComposite ... />
</filter>
<filter id="wp-duotone-midnight">
	...
</filter>
...

And the final output would look like this.

<svg xmlns="http://www.w3.org/2000/svg" ... />
	<defs>
		<filter id="wp-duotone-grayscale">
			<feColorMatrix ... />
			<feComponentTransfer ...>...</feComponentTransfer>
			<feComposite ... />
		</filter>
		<filter id="wp-duotone-midnight">
			...
		</filter>
		...
	</defs>
</svg>

Instead of what we currently have in this PR.

<svg xmlns="http://www.w3.org/2000/svg" ... />
	<svg xmlns="http://www.w3.org/2000/svg" ... />
		<defs>
			<filter id="wp-duotone-grayscale">
				<feColorMatrix ... />
				<feComponentTransfer ...>...</feComponentTransfer>
				<feComposite ... />
			</filter>
		</defs>
	</svg>
	<svg xmlns="http://www.w3.org/2000/svg" ... />
		<defs>
			<filter id="wp-duotone-midnight">
				...
			</filter>
		</defs>
	</svg>
	...
</svg>

In the frontend, doing something similar could maybe shave off another kilobyte if there are a lot of filters on the page.

</>
);
}
7 changes: 2 additions & 5 deletions packages/edit-post/src/components/layout/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ const interfaceLabels = {
footer: __( 'Editor footer' ),
};

function Layout( { styles, filters } ) {
function Layout( { styles } ) {
const isMobileViewport = useViewportMatch( 'medium', '<' );
const isHugeViewport = useViewportMatch( 'huge', '>=' );
const isLargeViewport = useViewportMatch( 'large' );
Expand Down Expand Up @@ -248,10 +248,7 @@ function Layout( { styles, filters } ) {
<TextEditor />
) }
{ isRichEditingEnabled && mode === 'visual' && (
<VisualEditor
styles={ styles }
filters={ filters }
/>
<VisualEditor styles={ styles } />
) }
{ ! isDistractionFree && ! isTemplateMode && (
<div className="edit-post-layout__metaboxes">
Expand Down
18 changes: 5 additions & 13 deletions packages/edit-post/src/components/visual-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {
__experimentaluseLayoutStyles as useLayoutStyles,
} from '@wordpress/block-editor';
import { useEffect, useRef, useMemo } from '@wordpress/element';
import { Button, __unstableMotion as motion, SVG } from '@wordpress/components';
import { Button, __unstableMotion as motion } from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { useMergeRefs } from '@wordpress/compose';
import { arrowLeft } from '@wordpress/icons';
Expand All @@ -46,15 +46,9 @@ import { store as editPostStore } from '../../store';

const isGutenbergPlugin = process.env.IS_GUTENBERG_PLUGIN ? true : false;

function MaybeIframe( {
children,
contentRef,
shouldIframe,
styles,
style,
filters,
} ) {
function MaybeIframe( { children, contentRef, shouldIframe, styles, style } ) {
const ref = useMouseMoveTypingReset();

if ( ! shouldIframe ) {
return (
<>
Expand All @@ -73,14 +67,13 @@ function MaybeIframe( {

return (
<Iframe
head={ <EditorStyles styles={ styles } /> }
ref={ ref }
contentRef={ contentRef }
style={ { width: '100%', height: '100%', display: 'block' } }
name="editor-canvas"
>
<EditorStyles styles={ styles } />
Copy link
Member

@ellatrix ellatrix Mar 23, 2023

Choose a reason for hiding this comment

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

Why are we adding non styles to EditorStyles and then moving everything in the body? Why not add SVGs separately here?

Also what if we move this to the iframe component in the block-editor package? Wouldn't it then work automatically in all editors and allow us to remove a bunch of duplication?

Copy link
Contributor

@youknowriad youknowriad Mar 23, 2023

Choose a reason for hiding this comment

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

That was my suggestion and for me these SVG filters are styles. They're just not "CSS styles". Basically if you check the code base, in all the places we use EditorStyles or generate the "styles" key we need to do the same for the "filters". It's an indication that they are the same thing, it's just the technology that changes.

The move to body is necessary because the filters can't be inserted into head in HTML. I suggest that we could potentially use ownerDocument to solve that.

Copy link
Member

@ellatrix ellatrix Mar 23, 2023

Choose a reason for hiding this comment

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

Yes, you could use a portal. Maybe we should build EditorStyles into the Iframe component as well?

Copy link
Contributor

@ajlende ajlende Mar 23, 2023

Choose a reason for hiding this comment

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

Maybe we should build EditorStyles into the Iframe component as well?

@ellatrix Is it okay if that's handled in a follow-up? Or do you think it's important for this PR?

Copy link
Contributor

@ajlende ajlende Mar 23, 2023

Choose a reason for hiding this comment

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

Yes, you could use a portal.

I tried doing this, but it causes a flash of unstyled content while React is grabbing the ref to use for the portal. Is there any reason we can't have the <style> elements in the <body> of the document?

Copy link
Contributor

@ajlende ajlende Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also what if we move this to the iframe component in the block-editor package? Wouldn't it then work automatically in all editors and allow us to remove a bunch of duplication?

EditorStyles aren't used exclusively in the Iframe component. We'd want them to work in those cases too.

{ children }
<SVG dangerouslySetInnerHTML={ { __html: filters } } />
</Iframe>
);
}
Expand Down Expand Up @@ -111,7 +104,7 @@ function getPostContentAttributes( blocks ) {
}
}

export default function VisualEditor( { styles, filters } ) {
export default function VisualEditor( { styles } ) {
const {
deviceType,
isWelcomeGuideVisible,
Expand Down Expand Up @@ -371,7 +364,6 @@ export default function VisualEditor( { styles, filters } ) {
}
contentRef={ contentRef }
styles={ styles }
filters={ filters }
>
{ themeSupportsLayout &&
! themeHasDisabledLayoutStyles &&
Expand Down
5 changes: 1 addition & 4 deletions packages/edit-post/src/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,7 @@ function Editor( { postId, postType, settings, initialEdits, ...props } ) {
>
<ErrorBoundary>
<EditorInitialization postId={ postId } />
<Layout
styles={ styles }
filters={ settings.filters }
/>
<Layout styles={ styles } />
</ErrorBoundary>
<PostLockedModal />
</ExperimentalEditorProvider>
Expand Down