Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cover: Block may invalidate with optimized styles #28497

Closed
sirreal opened this issue Jan 26, 2021 · 5 comments · Fixed by #28501
Closed

Cover: Block may invalidate with optimized styles #28497

sirreal opened this issue Jan 26, 2021 · 5 comments · Fixed by #28501
Assignees
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress

Comments

@sirreal
Copy link
Member

sirreal commented Jan 26, 2021

Description

The Cover block with a focal point will generate some HTML like the following:

<img class="wp-block-cover__image-background wp-image-703" alt="" src="https://example.com/image.jpeg" style="object-position:0% 44%" data-object-fit="cover" data-object-position="0% 44%"/>

When a simple CSS optimization plugin runs on post save (for example, applying this to style attributes), it removes the redundant % from 0%. The following post content is stored to the database:

<img class="wp-block-cover__image-background wp-image-703" alt="" src="https://example.com/image.jpeg" style="object-position:0 44%;" data-object-fit="cover" data-object-position="0% 44%" />

When the editor is refreshed, the post content is invalid. The CSS is semantically the same, but the missing redundant % causes block invalidation to occur. The block should be resilient to this change as the style is semantically identical.

Note that #28487 makes this issue much more likely to be encountered as a 0 value must be picked in the focal point picker.

Step-by-step reproduction instructions

You need to run a css optimization plugin that applies the % removal optimization on post save.

  • Add a Cover block
  • Add a background image
  • Pick a focal point in the block sidebar
  • Save the post
  • Refresh the editor
  • Notice the invalidation (only if a plugin is optimizing the style attribute).

Expected behaviour

There should be no block invalidation.

Actual behaviour

The block is invalidated.

WordPress information

  • WordPress version: 5.6
  • Gutenberg version: 9.8.1
  • Are all plugins except Gutenberg deactivated? No
  • Are you using a default theme (e.g. Twenty Twenty-One)? Yes
@sirreal sirreal added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Feature] Blocks Overall functionality of blocks labels Jan 26, 2021
@ockham
Copy link
Contributor

ockham commented Jan 26, 2021

Thanks Jon, I'll look into this.

Block validation seems to happen in this file: https://github.com/WordPress/gutenberg/blob/c31555d4cec541db929ee5f63b900c6577513272/packages/blocks/src/api/validation/index.js

I think we can add some logic for the style attribute to allow for 0 values with no unit. We should definitely cover that behavior with unit tests (allow 0 with no unit, don't allow non-o with no units, etc). I haven't found unit test coverage for that file yet, tho.

@ockham
Copy link
Contributor

ockham commented Jan 26, 2021

@ockham
Copy link
Contributor

ockham commented Jan 26, 2021

Fortunately, invalidation seems to compare normalized values. We basically 'just' need to add the relevant logic to getNormalizedStyleValue (and cover with unit tests):

/**
* Given a style value, returns a normalized style value for strict equality
* comparison.
*
* @param {string} value Style value.
*
* @return {string} Normalized style value.
*/
export function getNormalizedStyleValue( value ) {
return (
value
// Normalize URL type to omit whitespace or quotes
.replace( REGEXP_STYLE_URL_TYPE, 'url($1)' )
);
}


For comparison, here's what CSSTidy does: https://github.com/Automattic/jetpack/blob/ece061248c19609e58fd67ba3ff69d850e6780e6/projects/plugins/jetpack/modules/custom-css/csstidy/class.csstidy_optimise.php#L373-L379

We might want to emulate that. It uses AnalyzeCssNumber, which might take a bit of effort to re-implement 😅

@sirreal
Copy link
Member Author

sirreal commented Jan 26, 2021

I believe this is the canonical source for AnalyzeCssNumber

@mtias
Copy link
Member

mtias commented Jan 26, 2021

This seems a bit convoluted and would need an exception to handle calc:

Note: Because <number-token>s are always interpreted as <number>s or <integer>s, "unitless 0" <length>s aren’t supported in calc(). That is, width: calc(0 + 5px); is invalid, even though both width: 0; and width: 5px; are valid.

From https://www.w3.org/TR/css-values-3/#calc-type-checking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants