Properly apply styles and classes to the experimental form block #55755
Conversation
|
Flaky tests detected in 8be747e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17120347203
|
There was a problem hiding this comment.
This is an improvement. The style-related class names are now added to the <form tag. Spacing, color, and typography settings work on the front.
The way the attributes are removed is unconventional but it is working.
I can see that the order of the style and class attributes is not the same as in other blocks. On the front, the style is before the class. This is unexpected but I am not sure it matters.
In the editor, the typography > font family setting is not working, hence the request for changes.
When I change the font family setting, the inner blocks still use the body font family
(The font family is correct on the front).
|
Thank you for the thorough check @carolinan! |
|
Size Change: +151 B (+0.01%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
|
What is the root cause of styles and classes not being applied on both the editor side and the front end side? My understanding is that the Does it have something to do with this block being experimental? 🤔 |
It was an error of omission... In the initial POC implementation, I just forgot to add them. The |
|
I understand why. Although the blockProps hook has already been introduced, the Is there any reason to make className false? If not, I think all we need to do is make the following changes. diff --git a/packages/block-library/src/form/block.json b/packages/block-library/src/form/block.json
index 0c6451f495..b79c9f9249 100644
--- a/packages/block-library/src/form/block.json
+++ b/packages/block-library/src/form/block.json
@@ -27,7 +27,6 @@
},
"supports": {
"anchor": true,
- "className": false,
"color": {
"gradients": true,
"link": true,
:...skipping...
diff --git a/packages/block-library/src/form/block.json b/packages/block-library/src/form/block.json
index 0c6451f495..b79c9f9249 100644
--- a/packages/block-library/src/form/block.json
+++ b/packages/block-library/src/form/block.json
@@ -27,7 +27,6 @@
"anchor": true,
- "className": false,
"color": {
"gradients": true,
"link": true,
diff --git a/packages/block-library/src/form/edit.js b/packages/block-library/src/form/edit.js
index 7fded64837..7f479a9b2c 100644
--- a/packages/block-library/src/form/edit.js
+++ b/packages/block-library/src/form/edit.js
@@ -172,7 +172,6 @@ const Edit = ( { attributes, setAttributes, clientId } ) => {
) }
<form
{ ...innerBlocksProps }
- className="wp-block-form"
encType={ submissionMethod === 'email' ? 'text/plain' : null }
/>
</>
diff --git a/packages/block-library/src/form/save.js b/packages/block-library/src/form/save.js
index a824fc076d..9d944f13ef 100644
--- a/packages/block-library/src/form/save.js
+++ b/packages/block-library/src/form/save.js
@@ -10,7 +10,6 @@ const Save = ( { attributes } ) => {
return (
<form
{ ...blockProps }
- className="wp-block-form"
encType={ submissionMethod === 'email' ? 'text/plain' : null }
>
<InnerBlocks.Content />
~
~Also, this PR requires deprecation because it changes the save markup, but as reported by #56590 there is an issue with fixture tests not working correctly. |
60f9f40 to
c2a9bb0
Compare
|
You're right, the |
I think this kind of approach is mainly used when styles need to be applied to the elements inside the block rather than the wrapper. Styles and classes should be applied correctly to the block wrapper unless we are using __experimentalSkipSerialization. I pushed f8ee53e with the changes. In my testing, both global and block-specific styles appear to be applied correctly. Could you test this commit once? 4a2d95ba8a52a7ab5ccbe866a0ea3fbf.mp4 |
|
Huh... Confirmed, it works ❤️ |
|
I'm glad to know it worked correctly. The code looks good, but in order to ship this PR, I think the following two actions are necessary:
|
|
Done. |
|
Sorry, I forgot to tell you one more thing 😅 This PR is updating the |
f8ee53e to
d2350e9
Compare
|
Thank you for replying @aaronrobertshaw
The main issue I faced is in the UI.
The error: Expecting the deprecation to kick-in and work, migrating the content. What actually happens: Still getting the invalid-block error. As soon as I hit the recovery button everything works, but that's not what we expect when adding migrations |
|
Thanks for the extra details 👍 It still sounds like an issue we should be able to fix with the deprecation. Have you by any chance done some debugging around where block deprecations are applied to see what's going on in this case? My understanding is that the form block isn't being stabilized for WP6.6, so there is no urgency to have this sorted before the WP6.6. beta, correct? I'd be happy to dig deeper in a week or two if that's the case. |
Yeah, tried to understand it for weeks and couldn't understand what goes on there... so to be honest I gave up. Deprecations make my head spin, so I moved on to other issues 😅
That is correct. this is not an urgent task 👍 |
Don't worry, you aren't alone there. Happens every time I have to revisit the area.
Great, then in that case let's see what we can come up with once I can free up some bandwidth. Appreciate your patience on this one. |
|
I also investigated this issue again. I tried the deprecation suggested in this comment and it seems to work in some scenarios, but not completely. For example, this deprecation works if the original markup does not include inline styles:
However, if the markup contains inline styles, deprecation will cause an error.
|
|
Interesting #61833 submitted. It is mentioned that deprecation fails when The Form block has |
|
Any thoughts on continuing the work on this PR? |
|
The reason we can't rework this PR is that we haven't yet found a way to prevent this PR from breaking blocks. We haven't yet identified the cause, whether it's an error in our block deprecation code or a problem with the core deprecation logic itself 😅 |
7327b99 to
3e9c06a
Compare
|
OK, I think 3e9c06a fixed the problem.
We explicitly left the supports field empty to properly enforce deprecation. As a result, even if the Form block has custom values via block supports, the useBlockProps.save hook will not output inline styles. This results in the deprecation not matching and not executing. To avoid this, I explicitly generate the inline styles that useBlocksProps.save would have added, and applied them to the wrapper element. Similarly, this issue also occurred with anchor support, so I revised the anchor attribute definition to explicitly apply an ID to it. Based on my testing, deprecation should now work in all scenarios! |
| <!-- wp:form {"className":"custom-class","style":{"elements":{"link":{"color":{"text":"#ff0000"}}},"color":{"text":"#ff0000","background":"#00ffff"},"typography":{"fontSize":"30px"},"spacing":{"padding":{"top":"1px","bottom":"1px","left":"2px","right":"2px"},"margin":{"top":"3px","bottom":"3px","left":"4px","right":"4px"}}}} --> | ||
| <form id="anchor" class="wp-block-form" style="color:#ff0000;background-color:#00ffff;margin-top:3px;margin-right:4px;margin-bottom:3px;margin-left:4px;padding-top:1px;padding-right:2px;padding-bottom:1px;padding-left:2px;font-size:30px" enctype="text/plain"></form> | ||
| <!-- /wp:form --> | ||
|
|
||
| <!-- wp:form {"className":"custom-class","style":{"elements":{"link":{"color":{"text":"var:preset|color|contrast"}}},"spacing":{"padding":{"top":"var:preset|spacing|40","bottom":"var:preset|spacing|40","left":"var:preset|spacing|40","right":"var:preset|spacing|40"},"margin":{"top":"var:preset|spacing|40","bottom":"var:preset|spacing|40","left":"var:preset|spacing|40","right":"var:preset|spacing|40"}}},"backgroundColor":"accent-1","textColor":"contrast","fontSize":"x-large"} --> | ||
| <form id="anchor" class="wp-block-form" style="margin-top:var(--wp--preset--spacing--40);margin-right:var(--wp--preset--spacing--40);margin-bottom:var(--wp--preset--spacing--40);margin-left:var(--wp--preset--spacing--40);padding-top:var(--wp--preset--spacing--40);padding-right:var(--wp--preset--spacing--40);padding-bottom:var(--wp--preset--spacing--40);padding-left:var(--wp--preset--spacing--40)" enctype="text/plain"></form> | ||
| <!-- /wp:form --> |
There was a problem hiding this comment.
I have prepared two types of HTML: The first block has custom values, and the second block has preset values.
There was a problem hiding this comment.
I think I prefer the approach other blocks have taken where individual use cases are split into separate fixtures. It makes what the fixture represents and what you're testing clearer.
|
@aaronrobertshaw Ultimately, I believe we've applied the block deprecation correctly, and would appreciate your reviewing this PR for shipping 🙏 |
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for the ping and apologies for the delay 🙇
I've only had a chance to give this an initial review. At first glance it looks pretty good to me. The fixtures pass although I think keeping a 1:1 between the fixture and use case could be a good thing e.g. custom values vs presets.
The approach to not define the block supports for the deprecation makes sense.
I'll give this a further test tomorrow with a view to approve. If anyone else gives this the tick of approval before then, feel free to go with it.
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for all the hard work here 👍
I've given this more of a test:
✅ Forms created on trunk continue to look & function the same without error before migrating
✅ Deprecated forms migrate successfully and now display correctly on the frontend
✅ Fixture tests pass
Before merging this can we update the PR description so that it matches what this PR now does, has up-to-date test instructions etc?
When it comes to any PR involving deprecations I've found it really helpful to have a clear accurate history to follow if further issues arrive or even just to inform other deprecations.
That aside, this looks good to me.
🚢
What?
Properly applies styles and classes to the experimental form block. In #44214 (comment) it shows that styles don't get properly applied to the
core/formblock. This PR fixes that issue.Why?
Because it's a bug - or rather something that wasn't properly implemented in the initial PR introducing the experiment.
How?
Removes the defaultstyleandclassesattributes from the<form>element, and then adds them usingget_block_wrapper_attributes().Updated by @t-hamano:
classNameprop in the save function. This means that styles, classes, etc. from useblockProps will be respected, and block support will work properly.Testing Instructions
Added by @t-hamano:
Testing for Block Deprecation
Testing Instructions for Keyboard
Not applicable