Fix default duotone preset SVG and style generation #38681
Conversation
ajlende
added
[Type] Bug
ajlende
force-pushed
the
fix/default-duotone-generation
branch
from
98a6753 to
36782a3
Compare
|
Size Change: +88 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
Thanks for all of your work on this; I look forward to this eventually being integrated into trunk. I'm not sure what you mean by an 'FSE' theme (a theme that uses exclusively blocks and no PHP templates? a theme that has a theme.json ?) Using the @annezazu 's definitions, could you clarify which themes you'd recommend to test? Tested:Twentyseventeen 2.9 (no modifications made to theme), WordPress 5.9, and gutenberg built as of 36782a3 : On page when no blocks containing a duotone were present and gutenberg activated On page when no blocks containing a duotone and When gutenberg was not activated: Note: After publishing a page with a block containing a gradient (made when 36782a3 was activated); I deactivated the plugin. TwentyTwenty (with this theme.json ) WordPress 5.9, and gutenberg built as of https://github.com/WordPress/gutenberg/pull/38681/commits /36782a3557e0f536d7dfb2082d8186ae3508d1f0 (defaultPalette, defaultGradients, or defaultDuotone all set to false): On page when no blocks containing a duotone were present and gutenberg activated On page when no blocks containing a duotone and When gutenberg was not activated: |
|
@skorasaurus thanks for taking an early look! I'm still actively working on this branch which is why it is marked as a draft and some things are not working as expected. Things were working properly until I rebased with trunk today, so I will be sorting that out tomorrow. When it is ready for review, I will move the PR out of draft, and I can request a review from you at that point. |
ajlende
force-pushed
the
fix/default-duotone-generation
branch
from
36782a3 to
18b6f62
Compare
|
If I understand the release process for 5.9.1, the changes in lib/compat/wordpress-5.9 marked as This PR depends on #36236 which didn't make it into WP 5.9, so the duotone styles are still being generated in I figured out #38701 is what caused the issue when I rebased yesterday. When running the plugin on the 5.8 branch, it causes the styles to not show up. I've rebased to just before this change for testing. |
ajlende
requested review from
spacedmonkey,
TimothyBJacobs,
ellatrix,
mkaz,
ajitbohra and
ryanwelcher
as code owners
|
Thanks for following up: TwentyTwenty (with this theme.json ) WordPress 5.9, and gutenberg built as of 18b6f62 (defaultPalette, defaultGradients, or defaultDuotone all set to false): On page when no blocks containing a duotone were present and gutenberg activated: On page when no blocks containing a duotone and When gutenberg was not activated: |
|
@skorasaurus Thanks for testing again!
I tried your theme.json file, but couldn't reproduce the variables being generated in global-styles-inline-css. My generated
|
|
Thanks again for the tip re: adding ; I've done that in 2 different testing sites and still am able to reproduce the results as I shared above. I'm not quite sure what I'm doing incorrectly. I followed Steps 1-4, as follows;
I will then copy the zip into a couple other installations, activate the plugin. |
|
@ajlende is this intended to be tested with a WordPress-develop patch too? I tested this against a clean 5.9 instance, Theme: 2017, Gutenberg activated to this branch and I see default filters added and referenced in global styles:
|
|
@gwwar and @skorasaurus I think the problem may be that you're testing against WP 5.9 instead of WP 5.8. I was incorrect when I said, "If testing against WP 5.9, you should expect to see two sets of default duotone filters prior to this change and only one with this change." What you're both seeing is actually what is expected when testing against 5.9 instead of 5.8—no SVGs, but the CSS variables still getting generated in the stylesheet. The code rendering the stylesheet is in get-global-styles-and-settings.php. The function for rendering stylesheets already exists in 5.9, so the function in the lib/compat/wordpress-5.9 folder isn't loaded for those. The function for rendering the SVGs was not included in 5.9, so it still gets included when testing against 5.9. According to an earlier conversation, the changes in lib/compat/wordpress-5.9 with the "Backport to WP Minor Release" tag will be included in 5.9.1. Because of that, any functions defined in lib/compat/wordpress-5.9 that were already included in 5.9 need to be tested against 5.8 or they won't be loaded. Hopefully my explanation makes sense, but I can elaborate and rephrase things if it doesn't 🙂 |
Right, so was there an equivalent WordPress-develop PR to test? Or should we test against the 5.9 branch directly? In terms of manual testing for the related issue, I'd expect us to need to test against the 5.8/5.9 lines with both Gutenberg enabled and disabled. It'd also be super helpful to update the summary with an example literal before/after output for a particular theme. |
You should be testing with this branch and WP 5.8 (not 5.9) which can be the wordpress-develop 5.8 branch or 5.8.3 from the releases. The files in lib/compat/wordpress-5.9 add features from the Gutenberg plugin to earlier versions, so we need the earlier version in order for the changes here to load. There isn't a patch for wordpress-develop 5.9 because that patch will be generated from the merged PRs with the "Backport to WP Minor Release" label. Maybe @gziolo can correct me if I'm wrong. Figuring out how these changes need to be included in 5.9.1 is a bit confusing.
👍 I'll add that (but maybe not until tomorrow morning, it's getting kind of late here) |
Mamaduka
removed
the
Backport to WP Minor Release
|
I've been pondering this one for a while, as — per #38681 (comment) — it needs some sort of clear decision amongst the participants of this PR, and subsequently it might need clear communication with the community in case of changes to existing behaviour. There are clear downsides with whichever decision we make, but ultimately we need to honour the fact that That said, these constraints don't apply to Duotone. Seeing as it is also the most expensive feature of the three, there is an opportunity to try something new here and not enqueue its styles when the Duotone option is disabled. I discussed this with Matías to check my judgment and he's on board. /cc @ajlende @oandregal @youknowriad ¹: Save for explicit intervention, of course. |
I want to avoid treating duotone as a special case. From a themer's perspective and from a user's perspective duotone is just another style that can be applied to their site. When the behavior of |
Fair enough, special-casing duotone was a secondary concern. And as to reverting for |
I've thought of a couple options.
I like the simplicity of the first one. From the discussion in #38299, many people already expected the options to behave like they do in this PR, so I don't know if the functionality will be missed to toggle them separately. |
|
Reading the discussion here. There are a few things that click with me. I think we shouldn't disregard the principle we set for block patterns. We've always committed to the fact that block patterns should be allowed to use "core" palettes and work cross themes. This is not something we can break I think. I also think even core duotone should be usable in block patterns. So adding The other thing I wanted to mention is that duotone is different than colors in a lot of ways and I think being consistent here is just harming us more than solving anything. It is different regardless of how
This is an important distinction because it opens a lot of opportunities for us. It means we can change how duotone is applied without breaking changes. It also means that we don't persist any classname in the markup. Based on this, I think the ideal solution here is to rollback to how duotone initially worked. Instead of treating them like "presets" where you generate classnames for themes and attach classnames to blocks... consider the styles being generated by duotone as "server side inline styles" (like we do for layout styles for instance). Basically, for each duotone used in a block, output the style by the block. The pros here are:
The potential con:
Let's also consider that that last point is something we don't want, we can introduce deduplication in the optimization phase of the generated styles. It's actually an area that some folks are working on (in a so called style engine) cc @andrewserong @ramonjd and others. So it's not specific to duotone at all and we can start an adhoc solution in duotone while expanding this to any style as well. If reverting to the old way of judged too ambitious/impactful for now, I'm fine with reverting default* to the previous behavior as well for now (WP 6.0 beta next week). |
Sounds good. Skatepark sets a duotone filter by default on all images, which presumably in that situation would cause a great deal of duplication on archive pages. |
It depends how the "default" is added. If it's a global styles (theme.json) config, it only generates the style once. |
|
Thanks @youknowriad for the thoughtful response. Hearing the specifics is helping me understand your concerns more fully. This is my understanding of the situation:
For individual blocks, we are not generating classnames for duotone or attaching the classnames to blocks—that hasn't changed. The rendering for individual blocks is necessarily different due to the kses requirement of rendering the SVGs on the server, the CSS requirement that the However, for the global styles in question gradients/colors are not that different! Duotone still generates a selector and CSS using the same code for colors/gradients. The only** difference is that duotone also has to generate the SVGs in the document body. body {
/* Generated gradient CSS variable */
--wp--preset--color--black: #000000;
/* Generated duotone CSS variable */
--wp--preset--duotone--grayscale: url('#wp-duotone-grayscale');
}
/* Generated global styles gradient */
a {
color: var(--wp--preset--color--black);
}
/* Generated global styles duotone */
.wp-block-image img {
filter: var(--wp--preset--duotone--grayscale);
}* Or use hacks to repaint the filtered content in Safari. This might be another no-go solution because I haven't read through the internals for block patterns, but could we require block patterns to always save using custom colors? Instead of this: <!-- wp:group {"gradient":"midnight"} -->
<div class="wp-block-group has-midnight-gradient-background has-background"><!-- wp:paragraph -->
<p>Grouped content with a background gradient</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->We save this: <!-- wp:group {"style":{"color":{"gradient":"linear-gradient(135deg,rgb(2,3,129) 0%,rgb(40,116,252) 95%)"}}} -->
<div class="wp-block-group has-background" style="background:linear-gradient(135deg,rgb(2,3,129) 0%,rgb(40,116,252) 100%)"><!-- wp:paragraph -->
<p>Grouped content with a background gradient</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->We could even reverse-lookup the gradient in the active palettes to convert it back when the pattern is used. That way themes that don't want to include the default palette don't have to worry about it, and block patterns will still work. With 6.0 being so close and this still not having a decision, for now this is what I will do:
|
|
Thanks for the reply, there something I'd like to clarify on your answer above. So If I'm understanding properly, we only need the presets styles when duotone is applied as a style in theme.json, am I right? Why can't we just avoid including the filters and SVGs in the body of the page unless the duotone in question is actually used in the |
For things generated by global styles, yes. But themers can still use the generated CSS custom properties in their own CSS files that we can't easily scan. For example the Archeo theme uses color presets, but could just as easily use duotone presets. https://github.com/Automattic/themes/blob/trunk/archeo/style.css#L44 It seems like a bad experience for themers if the colors work, but duotone doesn't in that scenario. |
I think it's fine personally, one of the main point of block themes is that themes don't use CSS anymore (or less) and because duotone is complex, I expect most themes will rely on block customization (block support) to add duotone to their templates. |
Worst case scenario, we can add an option to opt-in to rendering all of them if a theme author needs them. And it's a pretty simple solution that will cover a lot of cases. I'll make a follow-up to #39966 to try it out. |
|
Considering this PR is not going to be ported to a 5.9 minor, we should revert all the changes done in the |
Does it mean that we still need to backport any changes to WordPress |
|
I believe so. Note that part of this is being reverted at #39966 |
Co-authored-by: André <583546+oandregal@users.noreply.github.com>
Follow-up for [53129]. This PR backports the remaining `defaultDuotone` option changes added in the Gutenberg plugin. Related PRs: WordPress/gutenberg#38681 and WordPress/gutenberg#39966. Props oandregal, ajlende. See #55505. git-svn-id: https://develop.svn.wordpress.org/trunk@53130 602fd350-edb4-49c9-b593-d223f7449a82
Follow-up for [53129]. This PR backports the remaining `defaultDuotone` option changes added in the Gutenberg plugin. Related PRs: WordPress/gutenberg#38681 and WordPress/gutenberg#39966. Props oandregal, ajlende. See #55505. Built from https://develop.svn.wordpress.org/trunk@53130 git-svn-id: https://core.svn.wordpress.org/trunk@52719 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Follow-up for [53129]. This PR backports the remaining `defaultDuotone` option changes added in the Gutenberg plugin. Related PRs: WordPress/gutenberg#38681 and WordPress/gutenberg#39966. Props oandregal, ajlende. See #55505. Built from https://develop.svn.wordpress.org/trunk@53130 git-svn-id: http://core.svn.wordpress.org/trunk@52719 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
I am sorry to introduce myself in a conversation that's maybe already ended, however I'd like to point out one thing because I'm reading here and in other threads that duotone filters are considered and treated just like a color scheme/palette equivalent. I cannot understand how it can be for real. As @youknowriad already explained, there are some technical differences amongst the two, being Duotone filters applied server side, while a color scheme is persistent in the markup. However I think that the issue should be solved by looking at the very conceptual difference between Duotone Filters and color presets:
So, just for saying, this thing (i dont know what else to call it) should be just removed from core. It can be an additional Jetpack feature, it can be a block adding the thing, it can be a plugin or whatever, but just not core. |
Description
This is fixing two issues that both depend on having a new
defaultDuotoneoption in theme.json.Fix #38299
Prevents the respective styles and SVGs from generating when
defaultPalette,defaultGradients, ordefaultDuotoneare set tofalsein theme.json settings.In non-fse themes these settings are
falseby default. Colors and gradients haveeditor-color-paletteandeditor-gradient-presetsto enable them from #36496.In fse themes with a theme.json, these settings are
trueby default.Fix #38690
This makes duotone filters behave more like the colors and gradients.
Enables multi origin colors and duotone presets to be added to the UI.
Adds
defaultDuotoneto disable the default palette from generating.As a follow-up, we'll want to update the UI to indicate where the presets are coming from like in #35970
Testing Instructions
In both a classic (non-fse) theme and a block (fse/theme.json) theme with
defaultDuotonedisabled:In a block (fse) theme with
defaultDuotoneenabled and themeduotonepresets included:Screenshots
Classic Theme (Twenty Twenty)
Block Theme
"defaultDuotone": true(Skatepark, default)Block Theme
"defaultDuotone": false(Skatepark, edited)Types of changes
New feature, bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).