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.

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

Fix duotone theme cache #36236

Merged
merged 21 commits into from
Dec 17, 2021
Merged

Fix duotone theme cache #36236

merged 21 commits into from
Dec 17, 2021

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Nov 5, 2021

Description

Fix #36208

Duotone filters were getting rendered via a side-effect of the global stylesheet generation. Because the stylesheets were cached, the duotone filter rendering wasn't called when the cache hit.

This adds a separate cache for the SVG filters which have to be rendered in the footer of the document, separate from the stylesheet.

How has this been tested?

  1. Ensure that your environment doesn't have any of these consts set:
    $can_use_cached = (
    ( empty( $types ) ) &&
    ( ! defined( 'WP_DEBUG' ) || ! WP_DEBUG ) &&
    ( ! defined( 'SCRIPT_DEBUG' ) || ! SCRIPT_DEBUG ) &&
    ( ! defined( 'REST_REQUEST' ) || ! REST_REQUEST ) &&
    ! is_admin()
    );
  2. Apply a duotone filter to a theme via the theme.json file:
{
    "styles": {
        "blocks": {
            "core/image": {
                "filter": {
                    "duotone": "var(--wp--preset--duotone--default-filter)"
                }
            }
        }
    }
}
{
    "settings":  {
        "color":  {
            "duotone":  [
                {
                    "colors": [ "#000", "#B9FB9C" ],
                    "slug": "default-filter",
                    "name": "Default filter"
                }
            ]
        }
    }
}
  1. See that the duotone filters are loaded on all images and persist on subsequent reloads

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@ajlende ajlende marked this pull request as ready for review November 5, 2021 01:55
@MaggieCabrera
Copy link
Contributor

Thank you for this, so quick! This seems to fix the cache issues I was seeing on Skatepark myself

@oandregal
Copy link
Member

Hey, thanks for creating the PR. This looks a bit involved and I have my brain cycles focused on today's freeze. Would you mind if I come back to this on Monday? It's a bug that we need to fix so we have more leeway than for features.

Copy link
Contributor Author

@ajlende ajlende left a comment

Choose a reason for hiding this comment

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

Added a few comments to help with reviewing

@@ -451,6 +486,8 @@ function gutenberg_render_duotone_support( $block_content, $block ) {
wp_add_inline_style( $filter_id, $filter_style );
wp_enqueue_style( $filter_id );

gutenberg_render_duotone_filter( $filter_preset );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously this was handled by gutenberg_render_duotone_filter_preset (above), but now has to be its own separate call.

*/
function gutenberg_register_duotone_support( $block_type ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole function was just moved below without modification.

*/
function gutenberg_render_duotone_filter_preset( $preset ) {
Copy link
Contributor Author

@ajlende ajlende Nov 11, 2021

Choose a reason for hiding this comment

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

Split this into a few functions

  • gutenberg_get_duotone_filter_property for generating the filter url() property
  • gutenberg_get_duotone_filter_svg for creating SVG
  • gutenberg_render_duotone_filter for rendering the SVG in the appropriate place

This way the SVG can be generated and saved in get_svg_filters below which can be cached.

The old gutenberg_render_duotone_filter_preset still exists below for value_func and for backwards compatibility.

lib/block-supports/duotone.php Outdated Show resolved Hide resolved
@ajlende ajlende self-assigned this Nov 12, 2021
@ajlende ajlende added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Bug An existing feature does not function as intended labels Nov 12, 2021
@oandregal
Copy link
Member

Hey, I wasn't able to look at this yet for reasons. I hope to be able to look at this when the beta1 is in a better place (probably next week?).

One overall comment I'd like to share is that we need to be careful with changes to code that landed in WordPress. For example, changes to code in lib/block-supports/duotone.php that was part of WordPress 5.8 (see) needs to either stay the same (in terms of name, parameters, etc) or be deprecated (some examples in this file).

@ajlende
Copy link
Contributor Author

ajlende commented Nov 23, 2021

Hey, I wasn't able to look at this yet for reasons.

No worries, there is a lot going on for the beta release and this is just a bug fix that can go in any time. Feel free to add me on any reviews for things, especially around the color palette work, if you need some help with reviews.

One overall comment I'd like to share is that we need to be careful with changes to code that landed in WordPress. For example, changes to code in lib/block-supports/duotone.php that was part of WordPress 5.8 (see) needs to either stay the same (in terms of name, parameters, etc) or be deprecated (some examples in this file).

Yep, all functions that existed before still exist with the same signature. I noticed a documentation problem with one of them that I fixed, and I updated some of my review comments to help point to where they all moved since it isn't obvious from the diff.

@jasmussen jasmussen requested review from a team and nerrad and removed request for a team December 16, 2021 07:41
@oandregal
Copy link
Member

Hey, I've been looking at this during the morning and I have a couple of minor details that I'll push shortly if you don't mind (where code lives and function names to make sure backporting is easy).

I need to review the duotone block supports a bit more and understand what's new in 5.9 and what existed in 5.8. My current understanding of how duotone works and how this PR changes it:

  1. The block styles defined in theme.json that use the filters: .wp-block-image{ filter: var(--wp--preset--duotone--<slug>)}. Uses the value directly from theme.json. Work as before.

  2. The CSS Custom properties to be used in theme.json: --wp--preset--duotone--<slug>: url(#wp-duotone-<slug>). Uses gutenberg_render_duotone_filter_preset. Work as before.

  3. The SVGs that use the ID. <svg id="wp-duotone-<slug>" />:

    • Before: side-effect of calling gutenberg_render_duotone_filter_preset => hooked the SVG generation to wp_body_open (right after body tag is opened).
    • After: hooks the SVG generation to either admin_footer or wp_footer filters (right before the body tag is closed), that uses a new public function gutenberg_get_global_styles_svg_filters to render them.

Is this correct? If so, I've got a couple of questions:

  • Can we remove the code that render the SVGs using the wp_body_open filter if we're using now admin_footer or wp_footer filters instead?
  • I have a vague memory that there were some issues depending on where we rendered the SVGs. Is before the closing body a good place? Does it work for all browsers?

@oandregal
Copy link
Member

oandregal commented Dec 17, 2021

@ajlende got to test this a bit and this is what I ran into. Env: WordPress 5.8, so I could test this PR.

I've used the TwentyTwentyTwo theme and put this bit under styles.blocks:

			"core/image": {
				"filter": {
					"duotone": "var(--wp--preset--duotone--foreground-and-background)"
				}
			}

It worked for the front & post editor (reloading the page still showed me the images and they used that filter). The images in the site editor didn't get the filter, though.

Then I've tried with a duotone coming from core:

			"core/image": {
				"filter": {
					"duotone": "var(--wp--preset--duotone--blue-red)"
				}
			}

and I ran into the same issue as in trunk (the image disappears) in the frontend & post editor. The site editor images loaded but had no filters applied.

@oandregal
Copy link
Member

@ajlende I've pushed 81ae91e that fixes the issue with default presets, so the only thing missing would be to make sure that this works in the site editor as well?

@oandregal
Copy link
Member

I've tested on trunk and the filters are not loaded for images in the site editor either. This can be looked independently from this PR.

@oandregal oandregal force-pushed the fix/duotone-theme-cache branch from a05b51d to 2109308 Compare December 17, 2021 16:42
@oandregal
Copy link
Member

Can we remove the code that render the SVGs using the wp_body_open filter if we're using now admin_footer or wp_footer filters instead?

Removed at 2109308 We still need to use gutenberg_render_duotone_filter to render the custom filters the user may create by itself and attach to an image.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

I've tested this in the following scenarios:

  • custom filter added by the user
  • default filter used by the theme through theme.json
  • theme filter used by the theme through theme.json

And it works well in the post and frontend. There's some issue in the site editor but it exists in trunk already, so can be looked up separately.

I've pushed some code changes here, so I'll let @ajlende merge this one when he feels is right, in case some of what I've done has caused havoc somewhere.

@ajlende ajlende force-pushed the fix/duotone-theme-cache branch from fa40cfc to 43167b0 Compare December 17, 2021 18:52
@ajlende ajlende force-pushed the fix/duotone-theme-cache branch from 43167b0 to fa1c821 Compare December 17, 2021 21:06
@ajlende
Copy link
Contributor Author

ajlende commented Dec 17, 2021

I've tested on trunk and the filters are not loaded for images in the site editor either. This can be looked independently from this PR.

Yeah, this was a know issue I'm taking another look at now. #35331

@ajlende
Copy link
Contributor Author

ajlende commented Dec 17, 2021

We discussed these a bit on Slack, but I'll summarize here for future reference.

Can we remove the code that render the SVGs using the wp_body_open filter if we're using now admin_footer or wp_footer filters instead?

One of the places we render duotone filters is for the user-defined custom per-block filters and the other is for the filters defined in theme.json. If we can somehow register the custom filters into some global store and access them from wp_get_global_styles_svg_filters then we can use the singular footer render. But that seems like it would be a bit complicated, so we'll leave that improvement for another day.

@ajlende
Copy link
Contributor Author

ajlende commented Dec 17, 2021

I have a vague memory that there were some issues depending on where we rendered the SVGs. Is before the closing body a good place? Does it work for all browsers?

Rendering in wp_body_open was a fix for Safari incorrectly rendering filters that appeared in the DOM after the content that they were applied to. Safari won't render filters at all if they're placed in the head of the document, so wp_body_open is what I landed on.

I updated the footer rendering to be in wp_body_open and updated the inline documentation for why that was chosen to make it a little more clear.

@ajlende ajlende merged commit 42cd5bb into trunk Dec 17, 2021
@ajlende ajlende deleted the fix/duotone-theme-cache branch December 17, 2021 23:02
@github-actions github-actions bot added this to the Gutenberg 12.3 milestone Dec 17, 2021
@scruffian
Copy link
Contributor

Thanks for merging this!

@noisysocks noisysocks added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jan 17, 2022
@Mamaduka
Copy link
Member

Cherry-picked for 5.9.1.

@Mamaduka Mamaduka removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Feb 16, 2022
Mamaduka pushed a commit that referenced this pull request Feb 16, 2022
Co-authored-by: André <583546+oandregal@users.noreply.github.com>
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Feb 17, 2022
This changeset is a backport for the following Gutenberg PRs:

- Fix duotone theme cache: WordPress/gutenberg#36236
- Fix duotone render in non-fse themes gutenberg: WordPress/gutenberg#37954
- Duotone: Allow users to specify custom filters gutenberg: WordPress/gutenberg#38442

Props oandregal, scruffian, Mamaduka.
Merges [52757] to the 5.9 branch.
See #55179.


git-svn-id: https://develop.svn.wordpress.org/branches/5.9@52759 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Feb 17, 2022
This changeset is a backport for the following Gutenberg PRs:

- Fix duotone theme cache: WordPress/gutenberg#36236
- Fix duotone render in non-fse themes gutenberg: WordPress/gutenberg#37954
- Duotone: Allow users to specify custom filters gutenberg: WordPress/gutenberg#38442

Props oandregal, scruffian, Mamaduka.
Merges [52757] to the 5.9 branch.
See #55179.

Built from https://develop.svn.wordpress.org/branches/5.9@52759


git-svn-id: http://core.svn.wordpress.org/branches/5.9@52348 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Feb 17, 2022
This changeset is a backport for the following Gutenberg PRs:

- Fix duotone theme cache: WordPress/gutenberg#36236
- Fix duotone render in non-fse themes gutenberg: WordPress/gutenberg#37954
- Duotone: Allow users to specify custom filters gutenberg: WordPress/gutenberg#38442

Props oandregal, scruffian, Mamaduka.
Merges [52757] to the 5.9 branch.
See #55179.

Built from https://develop.svn.wordpress.org/branches/5.9@52759


git-svn-id: https://core.svn.wordpress.org/branches/5.9@52348 1a063a9b-81f0-0310-95a4-ce76da25c4cd
whereiscodedude pushed a commit to whereiscodedude/wpss that referenced this pull request Sep 18, 2022
This changeset is a backport for the following Gutenberg PRs:

- Fix duotone theme cache: WordPress/gutenberg#36236
- Fix duotone render in non-fse themes gutenberg: WordPress/gutenberg#37954
- Duotone: Allow users to specify custom filters gutenberg: WordPress/gutenberg#38442

Props oandregal, scruffian, Mamaduka.
Merges [52757] to the 5.9 branch.
See #55179.

Built from https://develop.svn.wordpress.org/branches/5.9@52759
VenusPR added a commit to VenusPR/Wordpress_Richard that referenced this pull request Mar 9, 2023
This changeset is a backport for the following Gutenberg PRs:

- Fix duotone theme cache: WordPress/gutenberg#36236
- Fix duotone render in non-fse themes gutenberg: WordPress/gutenberg#37954
- Duotone: Allow users to specify custom filters gutenberg: WordPress/gutenberg#38442

Props oandregal, scruffian, Mamaduka.
Merges [52757] to the 5.9 branch.
See #55179.

Built from https://develop.svn.wordpress.org/branches/5.9@52759


git-svn-id: http://core.svn.wordpress.org/branches/5.9@52348 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duotone: Caching breaks duotones set through theme.json
6 participants