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

Server Styles - fix deduplication of style rules. #24486

Merged
merged 4 commits into from
Aug 13, 2020

Conversation

Addison-Stavlo
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo commented Aug 11, 2020

Description

This PR fixes style rule deduplication by adding an extra step to normalize style rules to follow the same format. (Thanks @mcsf for pointing this out #23007 (comment))

Currently in server styles there are some issues with deduplication:

$new_styles = implode( ' ', array_unique( explode( ' ', $current_styles . ' ' . $styles_to_add ) ) );

At this point in the process, $current_styles and $styles_to_add may or may not have different formats in spacing convention. ex:
$current_styles may be of the form one:thing;another:thing;.
$styles_to_add may be of the form one: thing; another: thing;.

The line above acting on these inputs would output: "one:thing;another:thing; one: thing; another:"

This happens because when these are then exploded by ' ', style keys in the second string are split from their values while the first string is not split at all. Additionally, since the convention in spacing is different inside the values themselves, even if these were split properly the dedupe would not work as 'one:thing' !== 'one: thing'.

This PR introduces an extra step in the process to normalize the values so dedupe works as intended, and the final output of $new_styles would be properly deduped and of the form one: thing; another: thing;

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Aug 11, 2020

Size Change: +157 B (0%)

Total Size: 1.16 MB

Filename Size Change
build/block-editor/index.js 125 kB +123 B (0%)
build/block-editor/style-rtl.css 10.6 kB -13 B (0%)
build/block-editor/style.css 10.6 kB -13 B (0%)
build/block-library/index.js 132 kB +12 B (0%)
build/block-library/style-rtl.css 7.49 kB -9 B (0%)
build/block-library/style.css 7.49 kB -9 B (0%)
build/components/index.js 200 kB +28 B (0%)
build/edit-post/style-rtl.css 5.63 kB +19 B (0%)
build/edit-post/style.css 5.63 kB +19 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.44 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.97 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-library/editor-rtl.css 8.36 kB 0 B
build/block-library/editor.css 8.36 kB 0 B
build/block-library/theme-rtl.css 729 B 0 B
build/block-library/theme.css 730 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.4 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 9.68 kB 0 B
build/core-data/index.js 11.8 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.45 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.23 kB 0 B
build/edit-navigation/index.js 10.9 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.js 304 kB 0 B
build/edit-site/index.js 17 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 9.38 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.js 45.3 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.79 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.33 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.71 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@Addison-Stavlo Addison-Stavlo force-pushed the fix/server-styles-dedupe-style-rules branch from e47c461 to 5e99bc8 Compare August 11, 2020 15:16
@Addison-Stavlo Addison-Stavlo self-assigned this Aug 11, 2020
@Addison-Stavlo Addison-Stavlo added the [Type] Bug An existing feature does not function as intended label Aug 11, 2020
@Addison-Stavlo Addison-Stavlo marked this pull request as ready for review August 11, 2020 15:22
@mcsf
Copy link
Contributor

mcsf commented Aug 12, 2020

Hi, @Addison-Stavlo! I took the liberty of pushing a commit (I felt I had to try it, otherwise I'd be providing vague feedback), so let me know what you think. Notes / motivation:

  • The expected output in the tests was inconsistent (e.g. in certain cases prop:val was expected with no whitespace, or a className string would expect a trailing space), which hinted at inconsistencies in the processing itself.

  • I was a bit worried that gutenberg_apply_block_supports was growing to do work in an ad hoc way, with specific conditionals.

  • It might make for a more maintainable function in the future if the parallels between class and style handling are evident, so both are now handled in similar sequences.

  • Finally, I had some initial difficulty keeping track of what was in array form and what had already been stringified (which also raised questions on what needed escaping), so I kept everything in array form while it's being processed and only stringified and piped through esc_attr at the very end.

My only risks / open questions:

  • I haven't stress-tested this for evaluating performance. The array-based operations might or might not have an impact.

  • If we have any doubts, we can drop the whitespace-normalization of CSS rules, and instead strive for Gutenberg as a whole to always output styles with consistent spacing.

I hope pushing a commit wasn't out of line. What do you think?

Copy link
Contributor Author

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @mcsf!

I will try to test them a bit more thoroughly later today. Im totally fine with you pushing the commit (as long as the commits aren't squashed and I can revert to the old one if ever needed), but this looks promising!

Im generally prefer to avoid regex where available as it isn't always easily readable, but this seems simple enough.

lib/block-supports/index.php Show resolved Hide resolved
@Addison-Stavlo
Copy link
Contributor Author

I got the time to test this a bit more. It works great! Thanks for jumping in here @mcsf, I like how this solution works and cleans things up quite a bit. 🥳

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixing and humouring my questions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants