Duotone: Make it possible to define duotone settings in theme.json #34073
Conversation
scruffian
added
the
Global Styles
scruffian
requested review from
oandregal,
MaggieCabrera,
ajlende and
jorgefilipecosta
scruffian
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as code owners
lib/global-styles.php
Outdated
| $stylesheet = gutenberg_experimental_global_styles_get_stylesheet( $all ); | ||
| if ( empty( $stylesheet ) ) { | ||
| return; | ||
| } | ||
|
|
||
| do_action( 'gutenberg_experimental_global_styles', $all ); |
There was a problem hiding this comment.
This action probably needs a better name.
There was a problem hiding this comment.
We don't want to add specific filters for global styles. Can we use the existing filters for editor and front? Given that the intention is for this SVG to be embedded with the global stylesheet is makes sense that it's called directly by the theme json generator. Example of how we're doing this for other properties https://github.com/WordPress/gutenberg/pull/33812/files#diff-b03597cc3da199e5aa5a94950e8241135904853e7c3b82d758e42744878afae7R841
There was a problem hiding this comment.
I removed this and hooked directly to enqueue_block_assets instead. The problem is some of the code from gutenberg_experimental_global_styles_enqueue_assets is duplicated.
There was a problem hiding this comment.
Can you explore how it'd look like to do this in the theme json generator instead?
We've already added there some rules for the block gap to work, perhaps in can serve of inspiration. I wonder if we can do a similar thing for duotone. Something along these lines:
- while processing the block style nodes, we somehow collect the duotone filters in use
- we add a new step that takes those duotone filters in use and create the necessary SVG
Alternatively, instead of enqueueing the duotones in use, we could enqueue all of them by inspecting the settings.color.duotones for core and theme.
There was a problem hiding this comment.
I did this in b95ebb6
| @@ -16,13 +16,13 @@ | |||
| */ | |||
| function gutenberg_experimental_global_styles_get_stylesheet( $tree, $type = 'all' ) { | |||
| // Check if we can use cached. | |||
| $can_use_cached = ( | |||
| $can_use_cached = false; /*( | |||
There was a problem hiding this comment.
This is just for testing
| @@ -83,7 +83,8 @@ | |||
| }, | |||
| "__experimentalBorder": { | |||
| "radius": true | |||
| } | |||
| }, | |||
| "__experimentalSelector": ".wp-block-image img" | |||
There was a problem hiding this comment.
Not sure if this is a good idea?
There was a problem hiding this comment.
Do the other block supports still work? If they don't, we'd need to implement "selectors per property" first, which we talked about at #28913
There was a problem hiding this comment.
I think the only one that would be affected would be border radius, which seems to still work fine.
There was a problem hiding this comment.
This is problematic.
These are the testing steps:
- Using the empty theme, set the border-radius for the image block to 0px in the theme.json
- Create a post and add an image. Set its border-radius to any value.
I expected the image to change the border but it didn't. The theme style rule has higher specificity than the block's.
The border-radius rules were added in https://github.com/WordPress/gutenberg/pull/27667/files#r699061648 I'm going to pull @aaronrobertshaw thoughts on this. I'd think the fix for this would be to serialize the border-radius directly to the img element, using __experimentalSkipSerialization. Once we have fixed that in a separate PR, we could continue with this one.
There was a problem hiding this comment.
Thanks for the ping.
__experimentalSkipSerialization didn't exist yet when border-radius support was originally added to the image block. Now that it does, there's no reason we can't move the application of the border styles to the inner image element.
In fact, there is already an open PR that does this as part of adopting border color and width for the image block.
| 'path' => array( 'color', 'duotone' ), | ||
| 'value_key' => 'slug', | ||
| 'css_var_infix' => 'duotone', | ||
| 'value_wrapper' => 'url(#wp-duotone-filter-$s)', |
There was a problem hiding this comment.
This is needed so that we can modify the output of value. The problem is how can we connect the CSS variable to the SVG filter in a way that doesn't require the user to understand what's going on under the hood.
lib/block-supports/duotone.php
Outdated
| if( ! empty( $theme_json_settings['color'] ) && ! empty( $theme_json_settings['color']['duotone'] ) && ! empty( $theme_json_settings['color']['duotone']['theme'] ) ) { | ||
| foreach( $theme_json_settings['color']['duotone']['theme'] as $duotone_setting ) { | ||
| $duotone_values = get_duotone_color_values( $duotone_setting['colors'] ); | ||
| $duotone_id = 'wp-duotone-filter-' . $duotone_setting['slug']; |
There was a problem hiding this comment.
This is duplicating what's on lib/class-wp-theme-json-gutenberg.php line 188.
| * @param array $block Block object. | ||
| * @return string Filtered block content. | ||
| */ | ||
| function gutenberg_render_duotone_support( $block_content, $block ) { |
There was a problem hiding this comment.
Github is confused. None of this code is new, it's just broken into smaller functions.
|
With the TT1 theme I get this error upon opening the duotone toolbar: EDIT: Also still had the issue when I switched to skatepark on the branch from the description. Seems like it can be fixed by adding |
|
Took a look at this and this is what I see:
<style>
// CSS for each duotone filter in the global stylesheet.
:root { --wp--preset--duotone--blue-red: url(#wp-duotone-filter-blue-red); }
.has-blue-red-duotone { filter: var(--wp--preset--duotone--blue-red) !important; }
</style>
// SVG for each duotone filter in the footer.
<svg ><filter id="wp-duotone-filter-blue-red" /></svg>
// If the theme uses the duotone style in `theme.json`
.wp-block-image img { filter: var(--wp--preset--duotone--blue-red); }(Enqueueing the SVG didn't work for me but that's what I understand this PR is trying to achieve.) When a user interacts with duotone we do this instead:
<style> .wp-duotone-filter-UUID img { filter: url( #wp-duotone-filter-611b7a6b4efc5 ); } <style>
<svg ><filter id="wp-duotone-filter-UUID" /></svg>The theme-generated duotone and user-generated duotone work differently and I wonder if we can consolidate them into a single mechanism:
Note that we don't use the duotone classes output by this PR ( |
|
I've struggled to test this with the PR provided (I don't know how to build the skatepark theme). It'd be great to provide easier instructions for testing in the PR. This is what I've done: I've used the empty theme and added this is its I've also experienced a weird thing:
|
|
I was thinking about how we can change duotone to better fit how global styles currently are implemented so that we don't have to modify global styles as much to make this work. First thing we could do is inline the SVG in the CSS so we don't have two different render steps. This would make duotone work a lot more like the colors/gradients. Inlining the SVG could make duotone theme.json settings work exactly like the other settings with no modification. In the example below, we could grab the {
"settings": {
"color": {
"duotone": [
{
"filter": "url('data:image/svg+xml,<svg xmlns:xlink=\"http://www.w3.org/1999/xlink\"><defs><filter id=\"wp-duotone-filter-default-filter\"><feColorMatrix type=\"matrix\" values=\".299 .587 .114 0 0 .299 .587 .114 0 0 .299 .587 .114 0 0 0 0 0 1 0\"></feColorMatrix><feComponentTransfer color-interpolation-filters=\"sRGB\"><feFuncR type=\"table\" tableValues=\"0 0.72549019607843\"></feFuncR><feFuncG type=\"table\" tableValues=\"0 0.9843137254902\"></feFuncG><feFuncB type=\"table\" tableValues=\"0 0.61176470588235\"></feFuncB></feComponentTransfer></filter></defs></svg>#wp-duotone-filter-default-filter')",
"slug": "default-filter",
"name": "Default filter"
}
]
}
}
}However, this isn't very friendly to theme developers (and we already have another method of specifying duotone values as a list of colors). Having this conversion step from the list colors to the Since this seems like it would require the addition of a filter hook for global styles, it might be a dead end. But I figured it worth sharing in case it spurs any other ideas if we're still unsure about |
scruffian
force-pushed
the
add/duotone-to-theme-json
branch
from
ec08ac9
to
2553492
Compare
|
Size Change: +65 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
Fixed! |
Yes we definitely should do this. This feels like more of a follow up task to this PR which is already quite big. |
The reason for this is that I was following precedent - when we reference colors in theme.json we always reference CSS variables which are defined elsewhere in the theme.json rather than a slug, for example. This level of decoupling might be useful, but I don't feel strongly about it. |
If you check out https://github.com/Automattic/themes/pull/4406/files there should be nothing to build. |
| const PATHS_WITH_MERGE = { | ||
| 'color.duotone': true, |
There was a problem hiding this comment.
When I was searching through the code, I found PATHS_WITH_MERGE in one other place. Not sure why it's in both places, but we can probably add it there too to be safe.
There was a problem hiding this comment.
We should consolidate both in https://github.com/ajlende/gutenberg/blob/5ec967cc2e3f05f713e2461b6fe9d9eb2e85a4a5/packages/blocks/src/api/constants.js either in a follow-up or previously to this PR.
I fixed this in d0ffc54 but the problem is that it doesn't get matched to a preset:
Really what we want is for the duotone control to be aware that the theme is setting a filter... This needs more work... |
I'm not really sure about this. So far we don't do any processing with any of the values themes provide via |
|
The duotone filter a theme sets won't be visible in the duotone control until #34178 lands. When it does, we can update the duotone control to take the theme data from the
I wonder if we can update the duotone control to have a value that is "clear duotone" and inlines cc @ajlende for thoughts and awareness. |
That sounds like a good idea |
Yeah, that seems like the best move for now. This is too far out for this PR, but I've also been thinking about how multiple filters could interact. The |
|
Correct me if I'm wrong, but wouldn't the filter: none; solution just mean two filters are applied, the initial one and a second one to unset it? It seems like a hack. Would it need deprecations if we find a better solution down the road? |
We're trying to allow themes to set default filters for blocks (Automattic/themes#4406), so some CSS gets generated for our presets. .wp-block-cover {
filter: url(#wp-duotone-red-filter);
}We want to allow someone to clear the duotone filter even when the default is to have a filter applied, so we have to use |
|
No, I understand, but it seems like clearing a duotone filter should clear it fully, and not just override-to-none. It's not a strong opinion and I'll defer. I was hoping for some theme.json magic where the default is set there, and then unset on a per-block basis. |
|
There's a difference between hitting the clear button which would reset to the default filter and having a "swatch" that unsets the duotone filter. Maybe I still don't follow? |
|
It's also very possible I'm missing nuance. Maybe not the best comparison, but as an example I could imagine a group where a default margin of 10px has been set by the theme, and I then select that group in my post and set the margin to 0. In the rendered markup, I would then expect it to say someting along the lines of |
|
The default styles are written to a stylesheet, not the inline <style>
.wp-block-cover {
filter: url(#wp-duotone-red-filter);
}
</style>
<style>
.wp-block-cover.wp-duotone-none {
filter: none;
}
</style> |
|
Thanks for the clarification. I'll trust you all on this one 👍 |
|
I made it possible to "disable" a duotone filter that comes from theme.json:
It probably need some polish but it works at least :) |
Based on the global styles work by @pablohoneyhoney, can we use the following disabled and active style instead?
Here, the dark black and white preset is selected, but the first option in the preset row is a "none". |
|
Great idea, thanks @jasmussen. I've added this:
|
| aria-label={ __( 'Disable duotone' ) } | ||
| tooltipText={ __( 'Disable duotone' ) } | ||
| style={ { | ||
| background: '#FFF url("data:image/svg+xml;utf8,<svg xmlns=\'http://www.w3.org/2000/svg\' viewBox=\'0 0 24 24\'><path d=\'M12 2C6.5 2 2 6.5 2 12s4.5 10 10 10 10-4.5 10-10S17.5 2 12 2zM3 12c0-5 4-9 9-9 2.2 0 4.3.8 5.8 2.2L5.2 17.8C3.8 16.3 3 14.2 3 12zm9 9c-2.4 0-4.5-.9-6.2-2.4L18.6 5.8C20.1 7.5 21 9.6 21 12c0 5-4 9-9 9z\' fill-rule=\'evenodd\' clip-rule=\'evenodd\' /></svg>" )', |
There was a problem hiding this comment.
We'll likely want to extract this one and make it either a reusable component to be used by the Global Styles system (whose mockups the icon came from), or put it in the Icon library and publish it with the others, even though it doesn't specifically match the icon grid.
There was a problem hiding this comment.
Yeah, I think that will require quite a significant reworking of how this component works though :)
There was a problem hiding this comment.
I agree with @jasmussen here. I can see this added as a special option of CircularOptionPicker. It would be useful for other components that use the picker like the regular gradient and color pickers.
As far as I'm aware, right now there isn't a way to unset a gradient or color background either (on the cover block, for example). So having an unset value doesn't seem essential to me for this PR, and may be something worth moving into a follow-up PR instead of this one. There we could discuss a unified way that will work for those other cases as well without holding this PR up.
|
Nice work, thanks for trying that. The icon does highlight that the checkmark overlay to indicate selected swatch is problematic (I feel like I've seen a separate ticket for problems with that check as well). Should we try adopting the new active-swatch style as well? This one:
That's a 1.5px outset $gray-900 box-shadow. The "none" icon should be $gray-600. |
|
I opened a new PR #34667 that scopes things down to just rendering SVGs and creating block classes for duotone. It changes how styles selectors are generated for duotone filters and seems like a good first step from which we can open follow-up PRs for the additional things. |
|
With #34667 merged, I think this can be closed. |
Description
This allows users to apply a duotone filter to all image blocks. To make this possible, we need to output CSS variables and SVG filters for all the duotone filters in the theme.json file. Many of the changes in this PR are simply refactoring the duotone.php library into smaller reusable functions.
This is a fix for #33642
How has this been tested?
Screenshots
Top section - duotone applied directly to the image.
Bottom section - duotone applied by the theme.
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).