Block styles: remove __unstableElementContext in favour of useStyleOverride #54493
Conversation
ellatrix
requested review from
fluiddot,
dcalhoun and
geriux
as code owners
ellatrix
force-pushed
the
remove/element-context
branch
from
978acea to
9312d25
Compare
|
Size Change: -339 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work @ellatrix, this is testing well for me on desktop, and the useStyleOverride hook looks like a good and centralised approach for each of the block supports to use for injecting their CSS 👍
✅ Layout block support outputs block-level layout rules correctly
✅ Position support outputs block-level rules correctly
✅ Elements support appears to output block-level rules correctly with links, headings, and buttons
✅ Gallery block appears to work correctly on desktop, with custom block spacing values output correctly
Is the only blocker for now the failing native tests? I wasn't too sure what the outstanding issue is for the mobile apps based on the comment in the Gallery edit.js file. To my knowledge, I don't think that the mobile apps allow control over block-level block spacing, so I wonder if there's another way to skip that logic from firing on mobile? 🤔
andrewserong
added
the
[Feature] Design Tools
There was a problem hiding this comment.
Very nice @ellatrix 👍
The layout, position, section elements, and gallery block gap styling all work as advertised on desktop for me.
@andrewserong's suggested tweaks might be a good finishing touch.
Mostly I'm thinking to ensure that for follow-up features, folks know to reach for this instead of injecting style tags
Perhaps if we could flesh out the PR description a little that might help provide better discoverability and more details on why this approach is preferred.
Is the only blocker for now the failing native tests?
Some of the failures appeared to be some run-of-the-mill timeouts. Others do look related though.
ellatrix
force-pushed
the
remove/element-context
branch
from
38a9678 to
2a7c9ec
Compare
|
I saw the previous component was is a |
There was a problem hiding this comment.
Thanks for the follow-ups @ellatrix! This is getting closer 👍
In re-testing, I notice that the position support styles don't appear to be applied anymore in my test markup. From logging out useStyleOverride it looks like the hook is receiving the generated styles correctly, but the rules don't appear to be applied in the editor. Here's a screenshot of a Group block that is set to sticky, and has the wp-container-9 classname applied to it. The block should be sticky, but instead scrolls off the top of the screen:
From logging out what gets sent to useStyleOverride it looks like it should be receiving the right styles:
From re-testing trunk, it appears the issue is somewhere in this PR, but I couldn't quite work out where!
Edit: there appears to be a collision happening between id values being used in different block supports. On trunk, this doesn't matter as two different block supports can happily have the same id of 9, for example (in this case, between the layout and position block supports).
I've left a comment on the position support, but in principle, I don't think useInstanceId is unique enough as an id across all block supports or blocks. With a lone integer it's possible for there to be collisions between different ids, which seems to result in some styles sometimes being overwritten. It looks like when we set an id we need to also include a string prefix to ensure it really is unique? I.e. position-block-support-${ id } or something like that. Is there a preferred approach for folks to use, to avoid collisions?
Since this is pretty subtle behaviour, it could be a good opportunity to write up some guidelines for intended usage for the hook, either in a code comment alongside the hook, or in a readme.
Once the position styles / id collisions are fixed up, the features otherwise appear to be testing well!
| <BlockListBlock { ...props } className={ className } /> | ||
| </> | ||
| ); | ||
| useStyleOverride( { id, css } ); |
There was a problem hiding this comment.
From a bit of hacking around, I think useInstanceId isn't unique enough for an id, as it appears to be unique to the component instance, not a unique integer across the runtime environment.
For each block support / usage of useStyleOveride, we might need to add a prefix, so something like:
useStyleOverride( { id: `position-styles-${ id }`, css } );
There was a problem hiding this comment.
I changed it to useId, which should fix the issue?
There was a problem hiding this comment.
Unfortunately no, useId doesn't appear to work properly since it seems to include colon characters.
| @@ -400,7 +400,7 @@ const elementTypes = [ | |||
| */ | |||
| const withElementsStyles = createHigherOrderComponent( | |||
| ( BlockListBlock ) => ( props ) => { | |||
| const blockElementsContainerIdentifier = `wp-elements-${ useInstanceId( | |||
| const blockElementsContainerIdentifier = `wp-elements-${ useId( | |||
| BlockListBlock | |||
There was a problem hiding this comment.
I changed it to useId, which should fix the issue?
Thanks for the updates @ellatrix, but unfortunately this still doesn't appear to be working for me, and now elements styles aren't working in addition to the position support. It looks like useId outputs strings in an unusual format, so container classnames are now wp-container-:r1p: which breaks selectors as it means the output of the CSS (without escaping) looks like illegal pseudo-classes:
Would a string prefix per block support + numeric id approach work / be unique enough? Once we've settled on a good approach, it'd be good to document it, since this is proving tricky to come up with a reliable id! 🙂
In case it helps with the testing, here's some sample block markup that includes a Group block with elements styles for headings, links and buttons, following by a sticky Group block, and then some paragraphs so that you can see whether the Group block is sticky:
Test block markup
<!-- wp:group {"style":{"elements":{"link":{"color":{"text":"#dd7878"}},"heading":{"color":{"text":"#440000"}},"button":{"color":{"background":"#b83232","text":"#f6bcbc"}}}},"layout":{"type":"constrained","contentSize":"320px","wideSize":"480px"}} -->
<div class="wp-block-group has-link-color"><!-- wp:paragraph -->
<p>A paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:cover {"overlayColor":"secondary","align":"wide","layout":{"type":"constrained"}} -->
<div class="wp-block-cover alignwide"><span aria-hidden="true" class="wp-block-cover__background has-secondary-background-color has-background-dim-100 has-background-dim"></span><div class="wp-block-cover__inner-container"><!-- wp:heading -->
<h2 class="wp-block-heading">A heading</h2>
<!-- /wp:heading -->
<!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">A title with <a href="http://wordpress.org">a link</a></p>
<!-- /wp:paragraph -->
<!-- wp:buttons -->
<div class="wp-block-buttons"><!-- wp:button -->
<div class="wp-block-button"><a class="wp-block-button__link wp-element-button">Button A</a></div>
<!-- /wp:button -->
<!-- wp:button -->
<div class="wp-block-button"><a class="wp-block-button__link wp-element-button">Button B</a></div>
<!-- /wp:button -->
<!-- wp:button -->
<div class="wp-block-button"><a class="wp-block-button__link wp-element-button">Button C</a></div>
<!-- /wp:button --></div>
<!-- /wp:buttons --></div></div>
<!-- /wp:cover --></div>
<!-- /wp:group -->
<!-- wp:group {"style":{"elements":{"link":{"color":{"text":"var:preset|color|base"}}},"spacing":{"padding":{"right":"var:preset|spacing|30","left":"var:preset|spacing|30","top":"var:preset|spacing|30","bottom":"var:preset|spacing|30"}},"position":{"type":"sticky","top":"0px"}},"backgroundColor":"vivid-red","textColor":"base","layout":{"type":"constrained"}} -->
<div class="wp-block-group has-base-color has-vivid-red-background-color has-text-color has-background has-link-color" style="padding-top:var(--wp--preset--spacing--30);padding-right:var(--wp--preset--spacing--30);padding-bottom:var(--wp--preset--spacing--30);padding-left:var(--wp--preset--spacing--30)"><!-- wp:paragraph -->
<p>A paragraph</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:paragraph -->
<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras gravida ut metus non ornare. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Fusce hendrerit dolor eget porta tincidunt. Nullam sit amet massa euismod, interdum ipsum non, viverra sapien. Pellentesque tincidunt mauris libero, vulputate venenatis purus mollis eget. Etiam erat ligula, sollicitudin at lobortis sit amet, ultricies eu elit. Etiam cursus dui nulla, ut lacinia leo tempor ac. Etiam sit amet porta mauris. Ut aliquet in libero at tincidunt. Mauris vel mauris libero. Nulla at cursus mi, vel luctus risus. Fusce gravida efficitur purus, ac auctor massa facilisis at.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Donec nec magna ac tellus dictum gravida. Curabitur tristique justo nec dui faucibus, vitae pulvinar nunc consequat. Quisque sem est, scelerisque in purus nec, tincidunt varius quam. Phasellus porta et sapien ac fringilla. Vestibulum ut tempus odio. Donec quis metus quis lorem consectetur placerat. Praesent ac mauris at orci laoreet ornare. Duis elementum magna rutrum lacus consequat interdum. Cras lacinia turpis eget est imperdiet commodo.</p>
<!-- /wp:paragraph -->Test markup screenshots:
| Trunk (heading, link, buttons styles applied) | This PR (heading, link, buttons styles aren't applied) |
|---|---|
 |
 |
| <BlockListBlock { ...props } className={ className } /> | ||
| </> | ||
| ); | ||
| useStyleOverride( { id, css } ); |
There was a problem hiding this comment.
Unfortunately no, useId doesn't appear to work properly since it seems to include colon characters.
|
Oh, I tested the gap style, and that was working because it's not using the id in the CSS 🤦♀️
The id is actually not a problem for |
Ah, gotcha! Yes, I see, the difference between the id used in the classname and CSS versus the id that's used for the style override itself 👍 Let me know when you'd like me to give this another test 🙂 |
I had another idea, feel free to ignore if it's no good! Would it work to have The drawback might be that the consumer wouldn't have a way of intentionally overriding a particular style based on a particular id, but if most supports only call I'm sure there's nuance I'm missing there, so just a thought! 😄 |
|
Sure maybe an optional ID? |
ellatrix
force-pushed
the
remove/element-context
branch
from
f631677 to
9a302fa
Compare
|
@andrewserong What do you think of the latest version? |
There was a problem hiding this comment.
Thanks for the updates @ellatrix, this is feeling pretty close to me. Just ran into two small issues:
- We can drop passing the
idfor the layout block support, as its id is still an instance-based integer, so the fallback id withinuseStyleOverrideshould work better I think - It looks like the
useStyleOverrideuseEffect is being called too frequently due tostylebeing a fresh object in each call
I've left a couple of comments. Thanks for continuing on with this one!
| useEffect( () => { | ||
| if ( ! style ) return; | ||
| const id = style.id || fallbackId; | ||
| setStyleOverride( id, style ); | ||
| return () => { | ||
| deleteStyleOverride( id ); | ||
| }; | ||
| }, [ fallbackId, style, setStyleOverride, deleteStyleOverride ] ); |
There was a problem hiding this comment.
style is an object and useStyleOverride is typically called with a fresh object in each call, so it looks like this useEffect is firing too frequently. E.g. if I log out just before setStyleOverride while scrolling the editor, it looks like it gets called a lot:
2023-10-03.10.00.40.mp4
Would it work to destructure the style object before the useEffect and then use each of those values in the useEffect dependency array, before calling setStyleOverride with a new object? Perhaps something like the following (I haven't tried this, so just an idea):
const { css, id, assets, __unstableType } = style;
useEffect( () => {
if ( ! css && ! assets ) return;
const _id = id || fallbackId;
setStyleOverride( _id, { assets, css, id, __unstableType } );
return () => {
deleteStyleOverride( id );
};
}, [ assets, css, fallbackId, id, setStyleOverride, deleteStyleOverride, __unstableType ] );I might be missing some details there, though!
There was a problem hiding this comment.
Yeah, that's right. Annoying that we have to know all the properties of the style object though. Alternatively we leave it up to the user to of this hook to memo, but that makes it annoying to use. Adjusted.
ellatrix
force-pushed
the
remove/element-context
branch
from
9a302fa to
45b8cf5
Compare
There was a problem hiding this comment.
Looking pretty good @ellatrix! Just left a comment about where we pass in the id, I think there might be another place to use the fallback _id.
One blocker to landing this PR is that it expands the scope of the issue identified in #54956 where using setStyleOverride appears to result in styles no longer being output or applied for block previews. To test I added a query loop / post template block with a Group block within it, and then in that Group block set the Heading and Link colors (which uses the elements block support). Then, within that Group block, further down the hierarchy I added a Heading block.
On trunk the Heading and Link styles apply in the previews within the query loop. With this PR applied, the Heading and Link styles no longer apply on unselected items within the query loop:
| Trunk | This PR |
|---|---|
 |
 |
So, it looks like we'll likely need to come up with a fix for #54956 before landing this one 🤔
CC: @tellthemachines, just to link together the discussions about that issue 🙂
Some test markup
<!-- wp:query {"queryId":9,"query":{"perPage":"4","pages":0,"offset":0,"postType":"post","order":"desc","orderBy":"date","author":"","search":"","exclude":[],"sticky":"","inherit":false}} -->
<div class="wp-block-query"><!-- wp:post-template {"layout":{"type":"default","columnCount":3}} -->
<!-- wp:group {"style":{"elements":{"heading":{"color":{"text":"#f44343"}},"link":{"color":{"text":"var:preset|color|primary"}}},"color":{"text":"#4f0808"},"border":{"color":"#929df3","width":"3px","radius":"10px"}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group has-border-color has-text-color has-link-color" style="border-color:#929df3;border-width:3px;border-radius:10px;color:#4f0808"><!-- wp:columns {"verticalAlignment":"center"} -->
<div class="wp-block-columns are-vertically-aligned-center"><!-- wp:column {"verticalAlignment":"center","width":"25%"} -->
<div class="wp-block-column is-vertically-aligned-center" style="flex-basis:25%"><!-- wp:post-featured-image {"isLink":true,"aspectRatio":"auto"} /--></div>
<!-- /wp:column -->
<!-- wp:column {"verticalAlignment":"center","width":"75%"} -->
<div class="wp-block-column is-vertically-aligned-center" style="flex-basis:75%"><!-- wp:heading {"level":6} -->
<h6 class="wp-block-heading">A heading</h6>
<!-- /wp:heading -->
<!-- wp:post-title {"isLink":true} /--></div>
<!-- /wp:column --></div>
<!-- /wp:columns --></div>
<!-- /wp:group -->
<!-- /wp:post-template --></div>
<!-- /wp:query -->| if ( ! css && ! assets ) return; | ||
| const _id = id || fallbackId; | ||
| setStyleOverride( _id, { | ||
| id, |
There was a problem hiding this comment.
Should this line be referencing the fallback if present, too?
| id, | |
| id: _id, |
There was a problem hiding this comment.
I'm not sure? Why should we set the fallback id?
There was a problem hiding this comment.
I wasn't too sure, but it looks like it works fine without setting the fallback id, so doesn't seem like we need to do anything here 🙂
ellatrix
force-pushed
the
remove/element-context
branch
from
45b8cf5 to
a2e546f
Compare
|
@andrewserong I rebased the PR 😊 |
andrewserong
force-pushed
the
remove/element-context
branch
from
a2e546f to
7af4ec5
Compare
There was a problem hiding this comment.
Thanks for your patience and for continuing on with this PR @ellatrix! Sorry I missed your ping a few weeks ago. I've just given this a re-test and it's all working nicely for me now:
✅ Duotone styles are working correctly
✅ Layout styles are working correctly
✅ Position styles are working correctly
✅ Element styles are working correctly
✅ Gallery gap styles are working correctly
✅ Updating styles within a query loop (especially Duotone) is still working with these changes applied
There was a merge conflict with position.js, so I've just given this another rebase, hope you don't mind.
All looking good now, I think this is in a good place to land if you do!
LGTM ✨
| if ( ! css && ! assets ) return; | ||
| const _id = id || fallbackId; | ||
| setStyleOverride( _id, { | ||
| id, |
There was a problem hiding this comment.
I wasn't too sure, but it looks like it works fine without setting the fallback id, so doesn't seem like we need to do anything here 🙂
|
Flaky tests detected in 7af4ec5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6819106211
|
andrewserong
force-pushed
the
remove/element-context
branch
from
7af4ec5 to
ffcb60c
Compare
|
I've just given this another rebase to resolve conflicts with #55762. Seems to still be working well for me! |
|
This PR had a small but nice noticeable impact on the "first block" metric in the post editor 👏 |
I actually used it for something else here: But it was
|
What?
Previously: #52888, #54466
Why?
We can use useStyleOverride everywhere now.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast