Selectors API: Make duotone selectors fallback and be scoped #49423
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aaronrobertshaw
added
the
[Feature] Design Tools
2 tasks
|
Size Change: +623 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
fix/selectors-api-use-in-duotone
branch
from
a0fde9e to
520ea49
Compare
scruffian
reviewed
Mar 29, 2023
| return _wp_array_get( $block_type->selectors, array( 'filter', 'duotone' ), $fallback_selector ); | ||
| } | ||
|
|
||
| // Selectors API, not available, check for old experimental selector. |
There was a problem hiding this comment.
Is it worth saying that this is left in for backwards compatibility, and should be removed one day?
3 tasks
oandregal
force-pushed
the
fix/selectors-api-use-in-duotone
branch
from
520ea49 to
4f4fc3c
Compare
oandregal
added
[Type] Bug
oandregal
reviewed
Mar 29, 2023
lib/class-wp-duotone-gutenberg.php
Outdated
| @@ -349,7 +349,26 @@ public static function render_duotone_support( $block_content, $block ) { | |||
| $filter_id = gutenberg_get_duotone_filter_id( array( 'slug' => $slug ) ); | |||
|
|
|||
| // Build the CSS selectors to which the filter will be applied. | |||
| $selector = WP_Theme_JSON_Gutenberg::scope_selector( '.' . $filter_id, $duotone_selector ); | |||
| $scopes = explode( ',', '.' . $filter_id ); | |||
There was a problem hiding this comment.
I wish I hadn't to paste this here, but this needs to work differently than the WP_Theme_JSON_Gutenberg::scope_selector (do not add an extra space).
Ideas for a follow-up PR:
-
The usage of this internal utility has expanded to: settings, duotone, WP_Theme_JSON class. It's worth creating a utility function somewhere. Where?
wp_scope_css_selector( $scope, $selector )as a name? -
The consumer should be responsible for adding or not adding spaces, not the function itself:
wp_scope_css_selector( $scope . ' ', $selector )if the consumer wants spaces.wp_scope_css_selector( $scope . ' ', $selector )if the consumer doesn't want spaces.
Thoughts?
|
Flaky tests detected in 7e7bf1c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4567826409
|
oandregal
reviewed
Mar 29, 2023
oandregal
requested review from
spacedmonkey,
ellatrix and
ajitbohra
as code owners
oandregal
approved these changes
Mar 29, 2023
Before: .wp-duotone .wp-block-name After: .wp-duotone.wp-blockname
oandregal
force-pushed
the
fix/selectors-api-use-in-duotone
branch
from
cc6e325 to
64a505a
Compare
scruffian
reviewed
Mar 29, 2023
| * | ||
| * @return {string} Scoped selector. | ||
| */ | ||
| function scopeSelector( scope, selector ) { |
There was a problem hiding this comment.
I think it probably makes more sense to keep this function but just modify it.
scruffian
reviewed
Mar 30, 2023
lib/block-supports/duotone.php
Outdated
| @@ -450,3 +471,4 @@ function gutenberg_render_duotone_support( $block_content, $block ) { | |||
| 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 ); | |||
| add_filter( 'block_type_metadata_settings', 'gutenberg_migrate_experimental_duotone_support_flag', 10, 2 ); | |||
There was a problem hiding this comment.
This looks like a useful technique for migrating more settings over.
There was a problem hiding this comment.
The only problem is that there's no longer any way of knowing if you have experimental support or not. It's causing me problems because the selector that we choose is different depending on whether or not it's experimental.
// May be true even if `filter.duotone` isn't set.
const duotoneSupport = getBlockSupport(
blockType,
'filter.duotone',
false
);
// May be set even if `filter.duotone` is also set.
const experimentalDuotone = getBlockSupport(
blockType,
'color.__experimentalDuotone',
false
);
// Will always produce a selector even if `filter` or `filter.duotone` isn't set.
const duotoneSelector = getBlockCSSSelector(
blockType,
'filter.duotone',
{ fallback: true }
);EDIT: We could have color.__experimentalDuotone override filter.duotone, but that seems wrong to me. If they're both set, I would expect filter.duotone to override color.__experimentalDuotone.
There was a problem hiding this comment.
I'm not sure I follow.
In the filter, we were only setting filter.duotone if it hasn't already been set and the color.experimentalDuotone has been.
A block author shouldn't be using both properties in their raw block.json so if we have color.experimentalDuotone set at all, then it should be a case from when duotone was experimental.
- In your first example, the check for
filter.duotonetells us if there is support. - The second example if the default is removed or changed to
nullwould tell us if the duotone support was the experimental version. - The third snippet with fallback true would always produce a selector regardless of any support check. We would guard such a call with a check that the
color.experimentalDuotonesupport flag wasn't set.
There was a problem hiding this comment.
My thinking was that by leaving the __experimentalDuotone property we could determine when to use that as the selector. I can see from this discussion where I missed adding the guard check to the duotone.js HoC.
ajlende
reviewed
Mar 30, 2023
added 9 commits
March 30, 2023 10:32
ajlende
approved these changes
Mar 30, 2023
There was a problem hiding this comment.
@aaronrobertshaw The changes I made ended up being kind of big again, so I didn't want to merge right away without someone else taking a look. I have nothing else to add, so if things are working well for you, go ahead and merge it.
Comment on lines
315
to
320
| // Backwards compatibility for color.__experimentalDuotone. This will | ||
| // have priority over filter.duotone support. Unfortunately we can't | ||
| // prefer filter.duotone because it gets set when __experimentalDuotone | ||
| // is set via a block_type_metadata_settings hook. It shouldn't be too | ||
| // much of a problem because I would expect consumers to not use both | ||
| // at the same time. |
There was a problem hiding this comment.
I had to do this to fix the experimental selectors in the editor being applied to the root of the block instead of the selector set in __experimentalSelector.
There was a problem hiding this comment.
I'm not sure we're on the same page here. The supports.filter.duotone property will only get set if it hasn't been set already and we have supports.color.__experimentalDuotone set.
So supports.filter.duotone will tell us if we have duotone support one way or another. Checking if we have the supports.color.__experimentalDuotone flag set should tell us if we need to handle an experimental duotone selector or use the selectors API requesting selectors.filter.duotone
|
I've updated the comments added around backwards compatibility of We can also return earlier if there is no duotone support so I relocated those guard conditions as well in 3346b9a I've retested this: ✅ Global styles can be applied to all current blocks opting into I'll get some fresh eyes on this PR and look to merge it later this afternoon. |
glendaviesnz
approved these changes
Mar 31, 2023
There was a problem hiding this comment.
I have retested after your latest changes @aaronrobertshaw and the following still works for me:
✅ Global styles can be applied to all current blocks opting into __experimentalDuotone
✅ Global styles are applied correctly for the Image block that uses supports.filter.duotone
✅ Individual block styles in the editor can be applied for blocks using __experimentalDuotone
✅ Individual block styles work for the Image block using supports.filter.duotone
✅ Duotone filters via global styles apply for all blocks on frontend
✅ Individual block duotone filters override global duotone filters on the frontend
|
I just cherry-picked this PR to the release/15.5 branch to get it included in the next release: fa6e2a6 |
ryanwelcher
removed
the
Backport to Gutenberg RC
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related:
What?
This PR fixes global-level duotone (see #49393 (comment) for details) and make duotone work like any other selector.
Why?
How?
Make duotone selectors work like any other selector:
Updates the selectors API functions to scope the old
supports.color.__experimentalDuotoneselector automatically.Stabilize duotone support flag under
filter.duotoneblock_type_metadata_settingsfilter to migrate oldcolor.__experimentalDuotoneflag.Test
selectors.filter.duotone) and two cover blocks (supports.color.__experimentalDuotone).purple-greenduotone for one of the images and one of the covers.The expected result is that the blocks with block-level duotone have it applied and the other ones use the global-level duotone in any context (front-end, post editor, site editor).
No other
imgelements of the page have any duotone applied (check the user avatar at the top-right, for example).Front-end:
Post editor: