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

Fluid typography: convert font size inline style attributes to fluid values #44764

Merged
merged 5 commits into from
Oct 10, 2022

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Oct 7, 2022

Parent issue:

What?

This commit ensures that custom font size values that appear in the style attribute of block content are converted to fluid typography (if activated)

Why?

Fluid typography for font size presets was added in #39529

However, custom font sizes should also be automatically converted to fluid values. This is a bug

How?

By adding a filter to the render_block hook, then replacing the custom font size via a regex.

Testing Instructions

Enable fluid typography via theme.json

Example theme.json
{
	"version": 2,
	"settings": {
		"appearanceTools": true,
		"typography": {
			"fluid": true
		}
	}
}

Testing in the post and site editors, add a few blocks that have typography support, e.g., text blocks, Group, Columns... there are many 😄

Example block HTML
<!-- wp:paragraph {"style":{"typography":{"fontSize":"2em"}}} -->
<p style="font-size:2em">This is a paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:heading {"style":{"typography":{"fontSize":"4rem","fontStyle":"normal","fontWeight":"600","textTransform":"capitalize","letterSpacing":"29px","textDecoration":"underline"},"spacing":{"padding":{"top":"var:preset|spacing|50","right":"var:preset|spacing|50","bottom":"var:preset|spacing|50","left":"var:preset|spacing|50"},"margin":{"top":"var:preset|spacing|60","right":"var:preset|spacing|60","bottom":"var:preset|spacing|60","left":"var:preset|spacing|60"}},"elements":{"link":{"color":{"text":"var:preset|color|vivid-purple"}}}},"backgroundColor":"vivid-red"} -->
<h2 class="has-vivid-red-background-color has-background has-link-color" style="margin-top:var(--wp--preset--spacing--60);margin-right:var(--wp--preset--spacing--60);margin-bottom:var(--wp--preset--spacing--60);margin-left:var(--wp--preset--spacing--60);padding-top:var(--wp--preset--spacing--50);padding-right:var(--wp--preset--spacing--50);padding-bottom:var(--wp--preset--spacing--50);padding-left:var(--wp--preset--spacing--50);font-size:4rem;font-style:normal;font-weight:600;letter-spacing:29px;text-decoration:underline;text-transform:capitalize">This is a heading</h2>
<!-- /wp:heading -->

<!-- wp:group {"style":{"typography":{"fontSize":"1em"}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group" style="font-size:1em"><!-- wp:paragraph {"style":{"typography":{"fontSize":"1em"}}} -->
<p style="font-size:1em">A paragraph inside a group</p>
<!-- /wp:paragraph -->

<!-- wp:search {"label":"Search","buttonText":"Search","style":{"typography":{"fontSize":"52px"}}} /--></div>
<!-- /wp:group -->

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

<!-- wp:buttons {"style":{"typography":{"fontSize":"47px"}}} -->
<div class="wp-block-buttons has-custom-font-size" style="font-size:47px"><!-- wp:button -->
<div class="wp-block-button"><a class="wp-block-button__link wp-element-button">Test button</a></div>
<!-- /wp:button --></div>
<!-- /wp:buttons -->

For these blocks, add a custom value via the UI control and publish the page

Screen Shot 2022-10-07 at 4 07 33 pm

In the frontend, check that the inline font size style values for these blocks have been converted to fluid

Before

Screen Shot 2022-10-07 at 4 09 18 pm

After

Screen Shot 2022-10-07 at 4 08 59 pm

@ramonjd ramonjd added [Type] Bug An existing feature does not function as intended CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Typography Font and typography-related issues and PRs labels Oct 7, 2022
@ramonjd ramonjd self-assigned this Oct 7, 2022
@ramonjd ramonjd requested a review from spacedmonkey as a code owner October 7, 2022 05:11
*/
if ( ! empty( $fluid_font_size ) && $fluid_font_size !== $custom_font_size ) {
// Replaces the first instance of `font-size:$custom_font_size` with `font-size:$fluid_font_size`.
return preg_replace( '/font-size\s*:\s*' . preg_quote( $custom_font_size, '/' ) . '\s*;?/', 'font-size:' . esc_attr( $fluid_font_size ) . ';', $block_content, 1 );
Copy link
Member Author

@ramonjd ramonjd Oct 7, 2022

Choose a reason for hiding this comment

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

@adamziel 👋 I know you've been looking at anti-patterns for this kind of stuff so just wanted to run it by you.

This lovely line looks at a block's HTML content and searches for a match on font-size: $custom_font_size.

Assuming we can't yet use the HTML_Walker (yet, which I'd love to), do you see any danger here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love this approach, but I can't see a better one until the walker is merged. I have tried pretty hard to break it and I haven't found a way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for testing this @scruffian

Yeah, I had to scrub my keyboard after writing it it felt so dirty. We'll be able to replace with HTML Walker for the plugin, but not 6.1 just yet 🤞

@jasmussen
Copy link
Contributor

Can confirm this appears to work on the frontend, even if not in the editor. From the test instructions, that sounds like it's right for this PR:

fluid on

Nice work.

One question: it's still possible for a theme to have fluid: true; enabled generally as a settings.typography property, but also then still disable fluid typography on a per-preset basis, right? A la:

... 
		"typography": {
			"dropCap": false,
			"fluid": true,
			"fontSizes": [
				{
					"size": "1.75rem",
					"slug": "large",
					"fluid": false
				},
			]
		}

Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM

@bph bph added Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) and removed Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) labels Oct 7, 2022
@ramonjd
Copy link
Member Author

ramonjd commented Oct 7, 2022

Can confirm this appears to work on the frontend, even if not in the editor.

Thanks for testing @jasmussen This PR attempts to address the editor side of things: #44765

One question: it's still possible for a theme to have fluid: true; enabled generally as a settings.typography property, but also then still disable fluid typography on a per-preset basis, right? A la

Yep, exactly what you said 👍

So themers can choose to have some static presets despite the global flag.

@jasmussen
Copy link
Contributor

Bug fix works as intended then!

@andrewserong
Copy link
Contributor

Thanks for putting this one together @ramonjd! I haven't tested it yet, but just wondering what happens when this PR is applied at the same time as #44762, which adds fluid typography support for server-rendered blocks. Will it attempt to process fluid typography twice, or does it safely ignore existing clamp values?

If it does process fluid typography twice, I wonder if we could add a check that the block opts-in to the typography support, doesn't skip serialization, and also doesn't have a render method / is a static block.

Apologies if I'm overthinking it!

@ramonjd
Copy link
Member Author

ramonjd commented Oct 9, 2022

Will it attempt to process fluid typography twice, or does it safely ignore existing clamp values?

Excellent question. 10 points to Gryffindor!

We should be fine since values are parsed via gutenberg_get_typography_value_and_unit(), which only accepts value + unit values, e.g., 12.22rem.

But, I will add tests to back up this bald-face assertion.

😄

@andrewserong
Copy link
Contributor

which only accepts value + unit values, e.g., 12.22rem.

Okay, great. Thanks for confirming!

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

This is testing well for me, too, and I wasn't able to break the regex. I did notice that it only targets the first font-size within rendered markup, which I think is the ideal way for this feature to behave. For example, the Search block (which is a server-rendered block), only the first font-size is targeted in the markup. Here's the markup before / after:

Before (each element is 52px) After (only the first element gets clampified, the rest are 52px)
image image

In this case, from the sounds of it #44762 will take care of the other font-size values, so when the two PRs are combined, it sounds like they'll probably be fine.

If we wanted to be more conservative and not have double handling for server-rendered blocks, then we could always add a couple more checks to the opening if statement of gutenberg_render_typography_support (check that it's not a server-rendered block / check for skip serialization), but since there's already the check to see whether or not the found value is the same as the clampified value, and that the clampification only works on value + unit values, I think doing that's totally optional.

LGTM!

@ramonjd
Copy link
Member Author

ramonjd commented Oct 10, 2022

If we wanted to be more conservative and not have double handling for server-rendered blocks, then we could always add a couple more checks to the opening if statement of gutenberg_render_typography_support (check that it's not a server-rendered block / check for skip serialization)

I'm happy to be this defensive if you think it's warranted.

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Tested with several different blocks, included nested ones with different custom font sizes, and everything seems to work fine ✅

Code looks alright, dodgy regex and all 😄 (jokes aside, it looks pretty safe!)

@andrewserong
Copy link
Contributor

I'm happy to be this defensive if you think it's warranted.

Thanks, I did a little digging, and worked out that we can grab the block type and then check whether or not the block is dynamic, and then skip this callback with something like the following:

	$block_type  = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] );

	if ( ! isset( $block_type ) ) {
		return $block_content;
	}

	if (
		! isset( $block['attrs']['style']['typography']['fontSize'] ) ||
		$block_type->is_dynamic()
	) {
		return $block_content;
	}

However, as @tellthemachines pointed out, we can't rely on the fact of a block being a dynamic block to mean that the fluid typography will already be handled. In the case of the cover block, it never calls get_block_wrapper_attributes, and so doesn't receive the server-rendered versions of the block supports, as the server-side code for that block assumes that the block supports were already applied in the JS version when the block was serialized.

So, I think after digging into this a little bit, I'd actually vote in favour of merging this as-is, as it ensures that blocks like Cover, which slip slightly outside of the divide between static and dynamic blocks, are handled.

@ramonjd
Copy link
Member Author

ramonjd commented Oct 10, 2022

I think after digging into this a little bit, I'd actually vote in favour of merging this as-is, as it ensures that blocks like Cover, which slip slightly outside of the divide between static and dynamic blocks, are handled.

Thanks! I appreciate you looking into it.

@ramonjd ramonjd force-pushed the add/typography_render_custom_fluid_values_serverside branch from 004b2e4 to dbc2adc Compare October 10, 2022 07:27
@ndiego
Copy link
Member

ndiego commented Oct 10, 2022

@andrewserong @ramonjd I assume this will need to be backported to 6.1 when complete. Can you confirm?

@jffng
Copy link
Contributor

jffng commented Oct 10, 2022

This is testing well for me, used TT3 and also tested various default patterns which contain inline font sizes.

@ramonjd ramonjd merged commit b1d4ffd into trunk Oct 10, 2022
@ramonjd ramonjd deleted the add/typography_render_custom_fluid_values_serverside branch October 10, 2022 20:17
@github-actions github-actions bot added this to the Gutenberg 14.4 milestone Oct 10, 2022
@ramonjd ramonjd added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 10, 2022
@ramonjd
Copy link
Member Author

ramonjd commented Oct 10, 2022

@andrewserong @ramonjd I assume this will need to be backported to 6.1 when complete. Can you confirm?

Thanks for chasing that up @ndiego. The answer is yes 😄

I'd neglected to add the Backport label. Done!

We'll get the backport PRs up to WordPress/wordpress-develop ASAP

@ockham ockham added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 10, 2022
@ramonjd ramonjd added the [Priority] High Used to indicate top priority items that need quick attention label Oct 11, 2022
ramonjd added a commit that referenced this pull request Oct 11, 2022
…values (#44764)

* This commit ensures that custom font size values that appear in the style attribute of block content are converted to fluid typography (if activated)

* Adding comments

* Bail early if we don't find a custom font size

* Adding tests yo

* Updating PHP doc to describe incoming type of $raw_value (string|number)
ockham pushed a commit that referenced this pull request Oct 11, 2022
* Fluid typography: convert server-side block support values (#44762)

* For serverside rendered typography block support styles, covert font size values to fluid values in the inline styles if fluid typography is activated.
Added unit tests

* Add fluid font size to Nav Link

* Add fluid typography to Search block

* Fix fluid typography for Submenu block with open on click

* Fix fluid typography for Page List block

* Remove unnecessary parameter

* Call esc_attr only once on the whole typography string

Co-authored-by: tellthemachines <[email protected]>

* Fluid Typography: Fix bug in global styles where fluid clamp rules were not calculated for custom values (#44761)

* Fluid Typography: Fix bug where fluid clamp rules were not calculated for custom global styles values

* Add inline comments

* Add tests for JS changes

* Fluid Typography: ensure global styles preset fluid sizes are calculated correctly (#44791)

* Forked #44761

* Ensuring the font size picker select box shows the right labels

* update comment. Typescript has beaten me. It's much more convenient to use getFontSizeOptions(), but I guess we'll have to refactor.

* Adding comment about Typescript bug (YAY, it wasn't me being dumb with TS for once)

* Added tests yo

* Changeo loggo

* Create a new FontSizeSelectOption return type for getSelectedOption to:
1. Clean up type changes in #44791
2. Make TS linter be quiet

* Revert accidental changes to emptytheme

* Revert type changes and other calamities

* Remove additional size value from getToggleGroupOptions test as I believe it is no longer expected

Co-authored-by: Ramon <[email protected]>
Co-authored-by: ramonjd <[email protected]>

* Fluid typography: convert font size inline style attributes to fluid values (#44764)

* This commit ensures that custom font size values that appear in the style attribute of block content are converted to fluid typography (if activated)

* Adding comments

* Bail early if we don't find a custom font size

* Adding tests yo

* Updating PHP doc to describe incoming type of $raw_value (string|number)

* Make custom font sizes appear fluid in the block editor when fluid typography is enabled (#44765)

* Make custom font sizes appear fluid in the block editor when fluid typography is enabled

* Add tests for fluid utils

* update description

* You shall not pass with a number, well, yes, but we'll coerce it to `px` and the tests shall pass nonetheless!!!

Co-authored-by: Ben Dwyer <[email protected]>
Co-authored-by: ramonjd <[email protected]>

* Fluid typography: covert font size number values to pixels (#44807)

* Converts incoming raw font size values to value + pixel to remain consistent with the fontsizepicker component.

* Updating comments / docs

* Fluid typography: ensure fontsizes are strings or integers (#44847)

* Updating PHP doc to describe incoming type of $raw_value (string|int)
This PR also harmonizes the JS checks
And review comments from #44807 (review)
These changes have already been backported to Core in WordPress/wordpress-develop#3428

* Update changelog
Add extra test for floats
Add i18n domain

* Font sizes can be string|int|float
Updated tests and JSDoc type

* Expand tests for gutenberg_get_typography_value_and_unit
Fix typo in CHANGELOG.md

* Initial commit (#44852)

- ensure that we convert fluid font sizes to fluid values in the editor for search block block supports
- we do this by passing the getTypographyClassesAndStyles hook a flag to tell it whether to convert

Updated CHANGELOG.md
Added tests

Co-authored-by: tellthemachines <[email protected]>
Co-authored-by: Andrew Serong <[email protected]>
Co-authored-by: Robert Anderson <[email protected]>
Co-authored-by: Ben Dwyer <[email protected]>
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Oct 11, 2022
…ate 1.

Merges the following:
* [WordPress/gutenberg#44761 Gutenberg PR 44761] - Fluid Typography: Fix bug in global styles where fluid `clamp()` rules were not calculated for custom values
* [WordPress/gutenberg#44762 Gutenberg PR 44762] - Fluid Typography: Convert server-side block support values
* [WordPress/gutenberg#44764 Gutenberg PR 44764] - Fluid Typography: Convert font size inline style attributes to fluid values
* [WordPress/gutenberg#44807 Gutenberg PR 44807] - Fluid Typography: Covert font size number values to pixels
* [WordPress/gutenberg#44847 Gutenberg PR 44847] - Fluid Typography: ensure font sizes are strings or integers

Follow-up to [54280].

Props andrewserong, isabel_brison, ramonopoly.
See #56467.

git-svn-id: https://develop.svn.wordpress.org/trunk@54497 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Oct 11, 2022
…ate 1.

Merges the following:
* [WordPress/gutenberg#44761 Gutenberg PR 44761] - Fluid Typography: Fix bug in global styles where fluid `clamp()` rules were not calculated for custom values
* [WordPress/gutenberg#44762 Gutenberg PR 44762] - Fluid Typography: Convert server-side block support values
* [WordPress/gutenberg#44764 Gutenberg PR 44764] - Fluid Typography: Convert font size inline style attributes to fluid values
* [WordPress/gutenberg#44807 Gutenberg PR 44807] - Fluid Typography: Covert font size number values to pixels
* [WordPress/gutenberg#44847 Gutenberg PR 44847] - Fluid Typography: ensure font sizes are strings or integers

Follow-up to [54280].

Props andrewserong, isabel_brison, ramonopoly.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54497


git-svn-id: http://core.svn.wordpress.org/trunk@54056 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to gilzow/wordpress-performance that referenced this pull request Oct 11, 2022
…ate 1.

Merges the following:
* [WordPress/gutenberg#44761 Gutenberg PR 44761] - Fluid Typography: Fix bug in global styles where fluid `clamp()` rules were not calculated for custom values
* [WordPress/gutenberg#44762 Gutenberg PR 44762] - Fluid Typography: Convert server-side block support values
* [WordPress/gutenberg#44764 Gutenberg PR 44764] - Fluid Typography: Convert font size inline style attributes to fluid values
* [WordPress/gutenberg#44807 Gutenberg PR 44807] - Fluid Typography: Covert font size number values to pixels
* [WordPress/gutenberg#44847 Gutenberg PR 44847] - Fluid Typography: ensure font sizes are strings or integers

Follow-up to [54280].

Props andrewserong, isabel_brison, ramonopoly.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54497


git-svn-id: https://core.svn.wordpress.org/trunk@54056 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@ockham
Copy link
Contributor

ockham commented Oct 17, 2022

Removing the "Backport to WP Beta/RC" label since this has been backported as part of #44868 (and sync'd to Core per WordPress/wordpress-develop#3437).

@ockham ockham removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 17, 2022
ootwch pushed a commit to ootwch/wordpress-develop that referenced this pull request Nov 4, 2022
…ate 1.

Merges the following:
* [WordPress/gutenberg#44761 Gutenberg PR 44761] - Fluid Typography: Fix bug in global styles where fluid `clamp()` rules were not calculated for custom values
* [WordPress/gutenberg#44762 Gutenberg PR 44762] - Fluid Typography: Convert server-side block support values
* [WordPress/gutenberg#44764 Gutenberg PR 44764] - Fluid Typography: Convert font size inline style attributes to fluid values
* [WordPress/gutenberg#44807 Gutenberg PR 44807] - Fluid Typography: Covert font size number values to pixels
* [WordPress/gutenberg#44847 Gutenberg PR 44847] - Fluid Typography: ensure font sizes are strings or integers

Follow-up to [54280].

Props andrewserong, isabel_brison, ramonopoly.
See #56467.

git-svn-id: https://develop.svn.wordpress.org/trunk@54497 602fd350-edb4-49c9-b593-d223f7449a82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Typography Font and typography-related issues and PRs [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

9 participants