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

Duotone: Pass filters to the Post Editor #49239

Merged
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 5 commits
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
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 );
25 changes: 25 additions & 0 deletions lib/class-wp-duotone-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,31 @@ public static function output_footer_assets() {
}
}

public static function add_editor_settings( $settings ) {
$assets = '';
$presets = 'body{';
foreach ( self::$output as $filter_data ) {
scruffian marked this conversation as resolved.
Show resolved Hide resolved
$assets .= gutenberg_get_duotone_filter_svg( $filter_data );
$presets .= self::get_css_custom_property_declaration( $filter_data );
}
$presets .= '}';

$settings['styles'][] = array(
'assets' => $assets,
'__unstableType' => 'svgs',
'isGlobalStyles' => false,
);
ajlende marked this conversation as resolved.
Show resolved Hide resolved

$settings['styles'][] = array(
'css' => $presets,
'__unstableType' => 'presets',
// These styles are no longer generated by global styles, so this must be false or they will be stripped out.
'isGlobalStyles' => false,
);

return $settings;
}

/**
* Appends the used global style duotone filter CSS Vars to the inline global styles CSS
*/
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 } ) {
ajlende marked this conversation as resolved.
Show resolved Hide resolved
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

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

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

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

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

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

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

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.

</>
);
}
2 changes: 1 addition & 1 deletion 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 @@ -67,12 +67,12 @@ function MaybeIframe( { children, contentRef, shouldIframe, styles, style } ) {

return (
<Iframe
head={ <EditorStyles styles={ styles } /> }
ajlende marked this conversation as resolved.
Show resolved Hide resolved
ref={ ref }
contentRef={ contentRef }
style={ { width: '100%', height: '100%', display: 'block' } }
name="editor-canvas"
>
<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.

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

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

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

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

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 }
</Iframe>
);
Expand Down