Properly apply styles and classes to the experimental form block #55755
Conversation
aristath
added
[Type] Bug
|
Flaky tests detected in b2734fa. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9176149963
|
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: +93 B (+0.01%) Total Size: 1.75 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. |
aristath
force-pushed
the
try/fix-form-styles
branch
from
60f9f40
to
c2a9bb0
Compare
aristath
requested review from
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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 |
aristath
force-pushed
the
try/fix-form-styles
branch
from
f8ee53e
to
d2350e9
Compare
There was a problem hiding this comment.
Thanks for the ping @t-hamano 👍
Under normal circumstances, the deprecation here should work. The bug this PR aims to fix though causes some serious problems though.
We're running into an issue where the deprecation doesn't work well in the form block. Ideally, we should investigate the cause, but what do you think about not adding deprecation to blocks that are marked as "experimental"?
100% we should investigate the cause. I don't think we can or should avoid that.
Regarding the experimental nature of the block to date. I agree the fact it is experimental should give us more freedoms but I still believe we owe it to early adopters to prevent breakages where possible.
In this case, adding a working deprecation is possible, so my vote is that we do so.
I've tried to fix the deprecation implementation for days but I can't figure it out.
@aristath I believe I've tracked down the root of the issue with why the current version of the deprecation isn't working. I'll try and summarize it all here clearly but it is a tricky area, so feel free to challenge anything, ask questions etc.
Deprecations only run when their old version of the block's save function generates the same content as what's saved in the block's markup.
The catch is that it does this through a series of utils: validateBlock > getSaveContent > getSaveElement.
The last of those, getSaveElement, will generate the content from the save function first, it then applies any extra props via the blocks.getSaveContent.extraProps filters. The block supports in the editor are applied via that filter.
This is why originally the block supports appeared to work in the editor and not on the frontend. It is also why then the deprecation is determined to not generate "valid" block markup and so isn't applied.
This is where the nature of the mistake in the original implementation really starts to cause some problems.
While I'm not very familiar with this area of the code, I don't think we can change the order of the application of block supports. It is likely being relied on as is and changing it to accommodate a bug fix is probably a non-starter.
There is an approach that has worked for me while hacking at this PR locally.
- Remove the block supports config from the deprecation (well, make it empty and comment as to why, to demonstrate this is deliberate despite the block originally having supports enabled).
- Manually define the block support attributes within the deprecation's attributes. This is required to maintain the data for the block support styles so they get migrated. Again, the hacked attributes should be heavily documented inline.
I'm running out of time today but in the simple testing I've done so far for a few different types of color; background, text, link etc. and the font size typography setting. It successfully migrates for me.
Proposal
- Update the deprecation to omit the supports config and documented why.
- Manually add the attribute definitions for all the block supports to the deprecation and document why again
- Add a fixture for the form block that covers this deprecation i.e. one that has the old markup with only the
wp-block-formclass despite containing block support data e.g.backgroundColorin its attributes.
* trunk: (273 commits) Remove preffered style variations legacy support (#58930) Style theme variations: add property extraction and merge utils (#58803) Migrate `change-detection` to Playwright (#58767) Update Changelog for 17.6.6 Docs: Clarify the status of the wp-block-styles theme support, and its intent (#58915) Use `data_wp_context` helper in core blocks and remove `data-wp-interactive` object (#58943) Try double enter for details block. (#58903) Template revisions API: move from experimental to compat/6.4 (#58920) Editor: Remove inline toolbar preference (#58945) Clean up link control CSS. (#58934) Font Library: Show error message when no fonts found to install (#58914) Block Bindings: lock editing of blocks by default (#58787) Editor: Remove the 'all' rendering mode (#58935) Pagination Numbers: Add `data-wp-key` to pagination numbers if enhanced pagination is enabled (#58189) Close link preview if collapsed selection when creating link (#58896) Fix incorrect useAnchor positioning when switching from virtual to rich text elements (#58900) Upgrade Floating UI packages, fix nested iframe positioning bug (#58932) Site editor: fix start patterns store selector (#58813) Revert "Rich text: pad multiple spaces through en/em replacement (#56341)" (#58792) Documentation: Clarify the performance reference commit and how to pick it (#58927) ...
|
Hey @aaronrobertshaw, thank you for looking into this!
You mentioned that you got it working locally... Could you please post some code? Doesn't need to be complete, just enough for me to understand what's on your mind and what worked for you structurally. |
|
My apologies for not adding code snippets, I'm a bit short on time and bandwidth at the moment.
What I mean, is that the deprecation defines its attributes. Those deprecation attributes could be updated to include the attributes that the block supports inject. For example, the background color support adds a number of attributes such as. If we only updated the deprecation's
I didn't stash the local changes as they were so far from complete but I'll put together something quick to illustrate the thrust of what I'm proposing. Example diffdiff --git a/packages/block-library/src/form/deprecated.js b/packages/block-library/src/form/deprecated.js
index 4109ddf442..5db53b66ec 100644
--- a/packages/block-library/src/form/deprecated.js
+++ b/packages/block-library/src/form/deprecated.js
@@ -5,37 +5,18 @@ import { InnerBlocks, useBlockProps } from '@wordpress/block-editor';
const deprecated = [
{
- supports: {
- anchor: true,
- className: false,
- color: {
- gradients: true,
- link: true,
- __experimentalDefaultControls: {
- background: true,
- text: true,
- link: true,
- },
- },
- spacing: {
- margin: true,
- padding: true,
- },
- typography: {
- fontSize: true,
- lineHeight: true,
- __experimentalFontFamily: true,
- __experimentalTextDecoration: true,
- __experimentalFontStyle: true,
- __experimentalFontWeight: true,
- __experimentalLetterSpacing: true,
- __experimentalTextTransform: true,
- __experimentalDefaultControls: {
- fontSize: true,
- },
- },
- __experimentalSelector: 'form',
- },
+ // The block supports here are deliberately empty despite this
+ // deprecated version of the block having adopted block supports.
+ // The attributes added by these supports have been manually
+ // added to this deprecated version's attributes definition so
+ // that the data isn't lost on migration. All this is so that the
+ // automatic application of block support classes doesn't occur
+ // as this version of the block had a bug that overrode those
+ // classes. If those block support classes are applied during the
+ // deprecation process, this deprecation doesn't match and won't
+ // run.
+ // @see https://github.com/WordPress/gutenberg/pull/55755
+ supports: {},
attributes: {
submissionMethod: {
type: 'string',
@@ -51,6 +32,12 @@ const deprecated = [
email: {
type: 'string',
},
+ // The following attributes have been added to match the block
+ // supports at the time of the deprecation. See above for details.
+ backgroundColor: {
+ type: 'string',
+ },
+ // ... rest of block support attributes.
},
save( { attributes } ) {
const blockProps = useBlockProps.save();
I hope that helps 🤞 |
|
This PR will also fix #59045. |
* trunk: (78 commits) Components: Use `Element.scrollIntoView()` instead of `dom-scroll-into-view` (#59085) DataViews: Correctly display featured image that don't have image sizes (#59111) Elements: Fix block instance element styles for links applying to buttons (#59114) Allow editing of image block alt and title attributes in content only mode (#58998) Add toggle for grid types and stabilise Grid block variation. (#59051) Global Styles: fix console error in block preview (#59112) Navigation: Avoid using embedded records from fallback API (#59076) Refactor responsive logic for grid column spans. (#59057) Editor: Unify Mode Switcher component between post and site editor (#59100) Move StopEditingAsBlocksOnOutsideSelect to Root (#58412) Fix logic error in #58951 (#59101) Block-editor: Auto-register block commands (#59079) Fix small typo in rich text reference guide (#59089) Components: Add lint rules for theme color CSS var usage (#59022) Enter editing mode via Enter or Spacebar (#58795) Updating Storybook to v7.6.15 (latest) (#59074) CustomSelectControl (v1 & v2): Fix errors in unit test setup (#59038) Add stylelint rule to prevent theme CSS vars outside of wp-components (#59020) ColorPicker: Style without accessing InputControl internals (#59069) Pattern block: batch replacing actions (#59075) ...
There was a problem hiding this comment.
Thanks for continuing to iterate on this @aristath 👍
The deprecation looks good and the attributes contained within it match those provided by block supports currently.
I think the only thing we need now is a fixture that covers this deprecated version of the form block.
aristath
dismissed
carolinan’s stale review
All requested changes from this review have been applied
aristath
force-pushed
the
try/fix-form-styles
branch
from
69bcc8a
to
b0645e0
Compare
This reverts commit b0645e0.
|
I removed the deprecation as it was not working. I don't know how to build this deprecation and make it work both in real-world use as well as in automated tests. If I make it pass the automated tests, then it doesn't work when I try to do it in the editor itself and vice-versa. 🤷 Granted, the deprecation bug in this PR is an edge-case, but it's been a blocker for this PR for months so I completely removed it, as I find no way forward if we try to enforce it. |
Can you expand at all on what wasn't working with the deprecation? Some steps to replicate the problem would be helpful.
I think this is a bit of a grey area. If we can sort out the deprecation and not break blocks for early adopters, that sounds like our best bet. From memory, the deprecations were pretty close last I reviewed this PR.
If there is a disconnect between our fixture tests and how deprecations are working in the editor, that sounds like something we should probably get to the bottom of to maintain confidence in those tests. I'm a little tied up on other things for WP 6.6 at the moment but I'd be more than happy to help out after the 6.6 beta. |
|
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 Fform block has |
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?
styleandclassesattributes from the<form>element, and then adds them usingget_block_wrapper_attributes().Testing Instructions
Testing Instructions for Keyboard
Not applicable