Ignore style validation errors #37942
Conversation
youknowriad
requested review from
andrewserong,
aaronrobertshaw,
a team,
Mamaduka and
getdave
and removed request for
a team
gziolo
added
[Feature] Block Validation/Deprecation
|
Size Change: -154 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
@youknowriad, the change looks pretty straightforward, and happy to approve. But I'm not sure how to test this change in the editor. |
There was a problem hiding this comment.
It seems logical that blocks should not be considered invalid if the different is styles.
Like @Mamaduka i wasn't sure how best to stress test this. What cases have we identified where this was previously causing a problem? Is it only the spacer block?
| style: ( actual, expected ) => { | ||
| return isEqual( ...[ actual, expected ].map( getStyleProperties ) ); | ||
| }, | ||
| style: () => true, |
There was a problem hiding this comment.
| style: () => true, | |
| // Minor differences in style attributes should not cause blocks to be considered invalid. | |
| // Consider different `style` attributes to be equivalent (if not strictly equal). | |
| // See: https://github.com/WordPress/gutenberg/issues/37851 | |
| style: () => true, |
This change seems significant enough to warrant an inline comment.
There was a problem hiding this comment.
Thanks for getting the ball rolling on this @youknowriad 👍
From what I've tested this is working as advertised.
✅ Validation unit tests pass.
✅ Block fixture tests all pass.
✅ No validation errors occurred when pasting deprecated spacer block markup into editor (It does display as vertical however as you noted).
Dave's suggestion of an inline explanatory comment about why style differences aren't cause for invalidation is a good one.
Right now I'm ignoring style diffs at all levels of the markup
Sounds like a good first step until its proven we need to limit it only to the main block element.
The spacer deprecation that I removed is an interesting one: basically the block changes from horizontal to vertical without the invalidation
I'm not sure I'm understanding this issue completely. The deprecation looks to be concerned with the recent switch from unitless numbers to strings for the height and width attributes. From memory that deprecation was to avoid block invalidation due to the style differences and might be separate to the orientation issue. @stacimc might have additional insight.
The orientation appears to only be provided via block context and assumes without that it must be vertical. So pasting in horizontal spacer markup without that context could be seen as working as intended by displaying as vertical. Still, I think it could likely be updated in a separate PR such that the block determines the orientation based off the height/width attributes when lacking the context value.
There was a problem hiding this comment.
This is great @youknowriad, thanks for putting it together, it makes the experience of editing from the code view feel much more robust, too. In testing, when I go to update the style attribute of a block, instead of causing a block validation error, it just gracefully removes the inline style I hacked in there, since the inline style gets generated in the editor, anyway 👍
| Before | After |
|---|---|
 |
 |
Just adding to what @aaronrobertshaw mentioned about the Spacer block deprecation — since it looks like that deprecation is about migrating the height block attribute rather than the markup, once this PR lands, do we then need to figure out how to trigger a deprecation intentionally for that case, where the markup / style doesn't need changing but the attribute does?
As an aside, in case it helps anyone else testing out the Spacer block — the horizontal / width control is only available when the block is a child of a block that provides an orientation context attribute of horizontal (like the Navigation Area block). The Row block doesn't appear to provide that currently (or yet), unless I've missed something 🤔
This is part of my hesitation for this PR, while we can trigger a deprecation intentionally here, it requires an explicit code change. so I'm wondering how many cases like this we'll create for third-party blocks: this is cases where it's better to have the invalidation because it changes an important thing for the block. |
That's a good question. While we might wind up creating issues for third-party block developers there, my gut feeling is that on balance, this PR could 🤞 result in devs having to write fewer deprecations if they're messing around with what gets output to the inline styles. Hopefully this gives folks a little more flexibility to make changes without accidentally breaking block validation? |
|
We should have different classifications for validation states as proposed in #21703. So that a block can still decide to trigger a migration based on it but without breaking the block for the user if it doesn't. |
In the case of the Spacer block this can be fixed by adding something like : isEligible: ( { height, width } ) => typeof height === 'number' || typeof width === 'number'to the deprecation, so the deprecation still runs even though the block shows as valid. There will no doubt be 3rd party blocks out there with similar issues though, and potentially some that can't be fixed with the addition of an style: ( actual, expected ) => {
return ignoreStylesInValidation ? true : isEqual( ...[ actual, expected ].map( getStyleProperties ) );Maybe this can be rolled into the classification of validation states that @mtias mentioned somehow. |
|
Just noting that user report of inability to set Block is marked as invalid so Applying this PR fixes the issue. |
|
FYI - I am taking another look at this to see what can be done to implement this without fear of breaking 3rd party block deprecations that currently rely on style differences. |
|
Have started a prototype to look at making ignoring of style validation optional by block #37942 - still WIP |
@youknowriad one way around this, as mentioned above, would be to make the ignoring of styles in validation an optional block setting. That way we can role it out to core blocks incrementally, ensuring that migrations are working correctly, etc. before enabling. This also means that no 3rd party blocks are affected unless they opt into it. There is a very rough prototype of this idea here which seems to work. If there was any interest in pursuing this it would just need some tidying up of the naming, and a move of the setting of the validation rules from the block settings to the block.json. |
@andrewserong just a quick note once #39523 lands we'll be able to intentionally create invalid markup in the code editor it we want to and it will no longer immediately wipe it out |
Nice, thanks for the heads-up, Dennis! I'll keep an eye on that PR 😀 |
|
Same reasoning as here #39417 (comment) I'm going to close this and we can revisit later when there's more progress on the style engine/blocks part. |
This PR updates the block validation behavior to ignore style attribute differences entirely. To read more about why we're trying this read #37851 and #37495
Notes: