Skip to content

Button Block: Set proper typography for inner elements #68023

Merged
ramonjd merged 7 commits into
WordPress:trunkfrom
shimotmk:fix/button-typography
Dec 31, 2024
Merged

Button Block: Set proper typography for inner elements #68023
ramonjd merged 7 commits into
WordPress:trunkfrom
shimotmk:fix/button-typography

Conversation

@shimotmk

@shimotmk shimotmk commented Dec 16, 2024

Copy link
Copy Markdown
Contributor

What?

The current typography does not work correctly with the given button styles. This fix forces inner elements to inherit the styles.

Fixes #64662
related #64869

Why?

How?

Testing Instructions

  1. Set Twenty Twenty-Four theme
  2. Place Button block
  3. Set the Appearance to Bold, etc.
    You can see that the Appearance has changed on either the editor screen or the front screen.

Testing Instructions for Keyboard

Screenshots or screencast

Before After

@github-actions

github-actions Bot commented Dec 16, 2024

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: shimotmk <shimotomoki@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

...colorProps.style,
...spacingProps.style,
...shadowProps.style,
...typographyProps.style,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm I think the .wp-block-button__link height of 100% is interfering with the typography setting writing-mode: vertical-rl;

Screenshot 2024-12-17 at 11 07 42 am

I wonder if changing it to auto would break anything else 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If writing-mode is best placed still on the wrapper, we could just change the __experimentalSkipSerialization value for typography to be an array of all the other typography features except writing mode.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good call! I didn't think of that, thanks!

@aaronrobertshaw aaronrobertshaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for wrangling this fix @shimotmk 👍

While reading through the changes I've spotted a few issues and left inline comments for those.

There also looks to be a possible opportunity to clean up the .has-custom-font-size class. It appears that was added to allow the inner element to be forced to inherit the parent font size.

Regarding the e2e failures, there are two issues;

  1. The new deprecation fixture for the v12 button needs to have a Button block as the base block rather than a Buttons block
  2. The v10 fixture has typography styles in it so the final .serialized.html result for the fixture needs to have the typography classes and styles moved to the inner element.

The last point there would be cover by running npm run fixtures:regenerate. Doing so will also leave a few other unrelated fixtures updated due to the fact their attributes have been reordered. Those changes can be discarded for this PR.

The following diff shows an example of how then fixture could be. It isn't suitable for simply committing to this PR as the fixture should cover all the typography styles (with maybe the exception of writing mode as @ramonjd highlighted).

Example diff fixing fixtures
diff --git a/test/integration/fixtures/blocks/core__button__deprecated-v10.serialized.html b/test/integration/fixtures/blocks/core__button__deprecated-v10.serialized.html
index e4c7b89c794..0bebe131629 100644
--- a/test/integration/fixtures/blocks/core__button__deprecated-v10.serialized.html
+++ b/test/integration/fixtures/blocks/core__button__deprecated-v10.serialized.html
@@ -1,3 +1,3 @@
 <!-- wp:button {"fontFamily":"cambria-georgia"} -->
-<div class="wp-block-button has-cambria-georgia-font-family"><a class="wp-block-button__link wp-element-button">My button</a></div>
+<div class="wp-block-button"><a class="wp-block-button__link has-cambria-georgia-font-family wp-element-button">My button</a></div>
 <!-- /wp:button -->
diff --git a/test/integration/fixtures/blocks/core__button__deprecated-v12.html b/test/integration/fixtures/blocks/core__button__deprecated-v12.html
index 03ac733e5c3..355f1c2019d 100644
--- a/test/integration/fixtures/blocks/core__button__deprecated-v12.html
+++ b/test/integration/fixtures/blocks/core__button__deprecated-v12.html
@@ -1,17 +1,3 @@
-<!-- wp:buttons {"align":"wide","layout":{"type":"flex","justifyContent":"center"}} -->
-<div class="wp-block-buttons alignwide"><!-- wp:button {"fontSize":"xx-large"} -->
-<div class="wp-block-button has-custom-font-size has-xx-large-font-size"><a class="wp-block-button__link wp-element-button">My button 1</a></div>
-<!-- /wp:button -->
-
-<!-- wp:button {"style":{"typography":{"fontStyle":"normal","fontWeight":"800"}}} -->
-<div class="wp-block-button" style="font-style:normal;font-weight:800"><a class="wp-block-button__link wp-element-button">My button 2</a></div>
-<!-- /wp:button -->
-
-<!-- wp:button {"style":{"typography":{"letterSpacing":"39px"}}} -->
-<div class="wp-block-button" style="letter-spacing:39px"><a class="wp-block-button__link wp-element-button">My button 3</a></div>
-<!-- /wp:button -->
-
-<!-- wp:button {"tagName":"button","style":{"typography":{"letterSpacing":"39px"}}} -->
-<div class="wp-block-button" style="letter-spacing:39px"><button type="button" class="wp-block-button__link wp-element-button">My button 4</button></div>
-<!-- /wp:button --></div>
-<!-- /wp:buttons -->
+<!-- wp:button {"fontSize":"xx-large","style":{"typography":{"fontStyle":"normal","fontWeight":"800","letterSpacing":"39px"}}} -->
+<div class="wp-block-button has-custom-font-size has-xx-large-font-size" style="font-style:normal;font-weight:800;letter-spacing:39px"><a class="wp-block-button__link wp-element-button">My Button</a></div>
+<!-- /wp:button -->
\ No newline at end of file
diff --git a/test/integration/fixtures/blocks/core__button__deprecated-v12.json b/test/integration/fixtures/blocks/core__button__deprecated-v12.json
index b6cab91f0d5..7fd537b6575 100644
--- a/test/integration/fixtures/blocks/core__button__deprecated-v12.json
+++ b/test/integration/fixtures/blocks/core__button__deprecated-v12.json
@@ -1,72 +1,20 @@
 [
 	{
-		"name": "core/buttons",
+		"name": "core/button",
 		"isValid": true,
 		"attributes": {
-			"align": "wide",
-			"layout": {
-				"type": "flex",
-				"justifyContent": "center"
+			"tagName": "a",
+			"type": "button",
+			"text": "My Button",
+			"fontSize": "xx-large",
+			"style": {
+				"typography": {
+					"fontStyle": "normal",
+					"fontWeight": "800",
+					"letterSpacing": "39px"
+				}
 			}
 		},
-		"innerBlocks": [
-			{
-				"name": "core/button",
-				"isValid": true,
-				"attributes": {
-					"tagName": "a",
-					"type": "button",
-					"text": "My button 1",
-					"fontSize": "xx-large"
-				},
-				"innerBlocks": []
-			},
-			{
-				"name": "core/button",
-				"isValid": true,
-				"attributes": {
-					"tagName": "a",
-					"type": "button",
-					"text": "My button 2",
-					"style": {
-						"typography": {
-							"fontStyle": "normal",
-							"fontWeight": "800"
-						}
-					}
-				},
-				"innerBlocks": []
-			},
-			{
-				"name": "core/button",
-				"isValid": true,
-				"attributes": {
-					"tagName": "a",
-					"type": "button",
-					"text": "My button 3",
-					"style": {
-						"typography": {
-							"letterSpacing": "39px"
-						}
-					}
-				},
-				"innerBlocks": []
-			},
-			{
-				"name": "core/button",
-				"isValid": false,
-				"attributes": {
-					"tagName": "button",
-					"type": "button",
-					"text": "My button 4",
-					"style": {
-						"typography": {
-							"letterSpacing": "39px"
-						}
-					}
-				},
-				"innerBlocks": []
-			}
-		]
+		"innerBlocks": []
 	}
 ]
diff --git a/test/integration/fixtures/blocks/core__button__deprecated-v12.parsed.json b/test/integration/fixtures/blocks/core__button__deprecated-v12.parsed.json
index 466b53ae00e..542817c8428 100644
--- a/test/integration/fixtures/blocks/core__button__deprecated-v12.parsed.json
+++ b/test/integration/fixtures/blocks/core__button__deprecated-v12.parsed.json
@@ -1,84 +1,20 @@
 [
 	{
-		"blockName": "core/buttons",
+		"blockName": "core/button",
 		"attrs": {
-			"align": "wide",
-			"layout": {
-				"type": "flex",
-				"justifyContent": "center"
+			"fontSize": "xx-large",
+			"style": {
+				"typography": {
+					"fontStyle": "normal",
+					"fontWeight": "800",
+					"letterSpacing": "39px"
+				}
 			}
 		},
-		"innerBlocks": [
-			{
-				"blockName": "core/button",
-				"attrs": {
-					"fontSize": "xx-large"
-				},
-				"innerBlocks": [],
-				"innerHTML": "\n<div class=\"wp-block-button has-custom-font-size has-xx-large-font-size\"><a class=\"wp-block-button__link wp-element-button\">My button 1</a></div>\n",
-				"innerContent": [
-					"\n<div class=\"wp-block-button has-custom-font-size has-xx-large-font-size\"><a class=\"wp-block-button__link wp-element-button\">My button 1</a></div>\n"
-				]
-			},
-			{
-				"blockName": "core/button",
-				"attrs": {
-					"style": {
-						"typography": {
-							"fontStyle": "normal",
-							"fontWeight": "800"
-						}
-					}
-				},
-				"innerBlocks": [],
-				"innerHTML": "\n<div class=\"wp-block-button\" style=\"font-style:normal;font-weight:800\"><a class=\"wp-block-button__link wp-element-button\">My button 2</a></div>\n",
-				"innerContent": [
-					"\n<div class=\"wp-block-button\" style=\"font-style:normal;font-weight:800\"><a class=\"wp-block-button__link wp-element-button\">My button 2</a></div>\n"
-				]
-			},
-			{
-				"blockName": "core/button",
-				"attrs": {
-					"style": {
-						"typography": {
-							"letterSpacing": "39px"
-						}
-					}
-				},
-				"innerBlocks": [],
-				"innerHTML": "\n<div class=\"wp-block-button\" style=\"letter-spacing:39px\"><a class=\"wp-block-button__link wp-element-button\">My button 3</a></div>\n",
-				"innerContent": [
-					"\n<div class=\"wp-block-button\" style=\"letter-spacing:39px\"><a class=\"wp-block-button__link wp-element-button\">My button 3</a></div>\n"
-				]
-			},
-			{
-				"blockName": "core/button",
-				"attrs": {
-					"tagName": "button",
-					"style": {
-						"typography": {
-							"letterSpacing": "39px"
-						}
-					}
-				},
-				"innerBlocks": [],
-				"innerHTML": "\n<div class=\"wp-block-button\" style=\"letter-spacing:39px\"><button type=\"button\" class=\"wp-block-button__link wp-element-button\">My button 4</button></div>\n",
-				"innerContent": [
-					"\n<div class=\"wp-block-button\" style=\"letter-spacing:39px\"><button type=\"button\" class=\"wp-block-button__link wp-element-button\">My button 4</button></div>\n"
-				]
-			}
-		],
-		"innerHTML": "\n<div class=\"wp-block-buttons alignwide\">\n\n\n\n\n\n</div>\n",
+		"innerBlocks": [],
+		"innerHTML": "\n<div class=\"wp-block-button has-custom-font-size has-xx-large-font-size\" style=\"font-style:normal;font-weight:800;letter-spacing:39px\"><a class=\"wp-block-button__link wp-element-button\">My Button</a></div>\n",
 		"innerContent": [
-			"\n<div class=\"wp-block-buttons alignwide\">",
-			null,
-			"\n\n",
-			null,
-			"\n\n",
-			null,
-			"\n\n",
-			null,
-			"</div>\n"
+			"\n<div class=\"wp-block-button has-custom-font-size has-xx-large-font-size\" style=\"font-style:normal;font-weight:800;letter-spacing:39px\"><a class=\"wp-block-button__link wp-element-button\">My Button</a></div>\n"
 		]
 	}
 ]
diff --git a/test/integration/fixtures/blocks/core__button__deprecated-v12.serialized.html b/test/integration/fixtures/blocks/core__button__deprecated-v12.serialized.html
index 2df6789245e..1b18b70facc 100644
--- a/test/integration/fixtures/blocks/core__button__deprecated-v12.serialized.html
+++ b/test/integration/fixtures/blocks/core__button__deprecated-v12.serialized.html
@@ -1,17 +1,3 @@
-<!-- wp:buttons {"align":"wide","layout":{"type":"flex","justifyContent":"center"}} -->
-<div class="wp-block-buttons alignwide"><!-- wp:button {"fontSize":"xx-large"} -->
-<div class="wp-block-button has-custom-font-size"><a class="wp-block-button__link has-xx-large-font-size wp-element-button">My button 1</a></div>
+<!-- wp:button {"fontSize":"xx-large","style":{"typography":{"fontStyle":"normal","fontWeight":"800","letterSpacing":"39px"}}} -->
+<div class="wp-block-button has-custom-font-size"><a class="wp-block-button__link has-xx-large-font-size wp-element-button" style="font-style:normal;font-weight:800;letter-spacing:39px">My Button</a></div>
 <!-- /wp:button -->
-
-<!-- wp:button {"style":{"typography":{"fontStyle":"normal","fontWeight":"800"}}} -->
-<div class="wp-block-button"><a class="wp-block-button__link wp-element-button" style="font-style:normal;font-weight:800">My button 2</a></div>
-<!-- /wp:button -->
-
-<!-- wp:button {"style":{"typography":{"letterSpacing":"39px"}}} -->
-<div class="wp-block-button"><a class="wp-block-button__link wp-element-button" style="letter-spacing:39px">My button 3</a></div>
-<!-- /wp:button -->
-
-<!-- wp:button {"tagName":"button","style":{"typography":{"letterSpacing":"39px"}}} -->
-<div class="wp-block-button" style="letter-spacing:39px"><button type="button" class="wp-block-button__link wp-element-button">My button 4</button></div>
-<!-- /wp:button --></div>
-<!-- /wp:buttons -->

I hope that helps 🤞

Comment thread packages/block-library/src/button/deprecated.js
Comment thread packages/block-library/src/button/deprecated.js Outdated
Comment thread packages/block-library/src/button/deprecated.js
...colorProps.style,
...spacingProps.style,
...shadowProps.style,
...typographyProps.style,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If writing-mode is best placed still on the wrapper, we could just change the __experimentalSkipSerialization value for typography to be an array of all the other typography features except writing mode.

Comment thread packages/block-library/src/button/save.js
Comment thread test/integration/fixtures/blocks/core__button__deprecated-v12.html Outdated
@shimotmk

Copy link
Copy Markdown
Contributor Author

@ramonjd @aaronrobertshaw
Thanks for reviewing!

I adjusted the attributes and made core__button__deprecated-v12.html into a single button block.

However, the data for the fourth letterSpacing setting in core__button__deprecated-v12.html cannot be migrated.

Why is that?🤔

@aaronrobertshaw

Copy link
Copy Markdown
Contributor

However, the data for the fourth letterSpacing setting in core__button__deprecated-v12.html cannot be migrated.

I'm not following this comment sorry. It looks fine to me in the lastest commit.

The current e2e failures relate to full post content fixture › core__button__deprecated-v10. As I noted in my review this fixture needing updating. The required change was also in the diff I provided.

After updating the v10 fixture, all the fixture tests pass for me.

Once that is sorted, there's still the issue with writing mode to fix up.

@shimotmk

Copy link
Copy Markdown
Contributor Author

@aaronrobertshaw
Sorry. I was mistaken. The transition was going well.

@shimotmk

Copy link
Copy Markdown
Contributor Author

@ramonjd @aaronrobertshaw
I adjusted the writing-mode to add inline styles to the outer div 🙂

Comment thread packages/block-library/src/button/edit.js
Comment thread packages/block-library/src/button/block.json
@shimotmk

Copy link
Copy Markdown
Contributor Author

@ramonjd @aaronrobertshaw
Any other things I need to fix?

@aaronrobertshaw

Copy link
Copy Markdown
Contributor

Any other things I need to fix?

@shimotmk the issue I flagged in my last two reviews still needs fixing.

The current e2e failures relate to full post content fixture › core__button__deprecated-v10. As I noted in my #68023 (review) this fixture needing updating. The required change was also in the diff I provided.

@shimotmk

Copy link
Copy Markdown
Contributor Author

@aaronrobertshaw
Thanks for the reply

Moved the has-custom-font-size class internally.
And updated test/integration/fixtures/blocks/core__button__deprecated-v10.serialized.html
Is this OK?

@ramonjd

ramonjd commented Dec 22, 2024

Copy link
Copy Markdown
Member

Thanks for the continued work on this PR @shimotmk 🙇🏻

It looks like the mobile unit tests fails are related to the button block: https://github.com/WordPress/gutenberg/actions/runs/12429816613/job/34704026313?pr=68023#step:6:2906

@shimotmk

Copy link
Copy Markdown
Contributor Author

@ramonjd
Thanks for the reply
I don't know how to fix the mobile unit.
How do I fix it?

@ramonjd

ramonjd commented Dec 22, 2024

Copy link
Copy Markdown
Member

I think the hook needs to be exported for native:

diff --git a/packages/block-editor/src/hooks/index.native.js b/packages/block-editor/src/hooks/index.native.js
index c7f9df868f..0e4c2aa276 100644
--- a/packages/block-editor/src/hooks/index.native.js
+++ b/packages/block-editor/src/hooks/index.native.js
@@ -33,3 +33,4 @@ export { getColorClassesAndStyles, useColorProps } from './use-color-props';
 export { getSpacingClassesAndStyles } from './use-spacing-props';
 export { useCachedTruthy } from './use-cached-truthy';
 export { useEditorWrapperStyles } from './use-editor-wrapper-styles';
+export { getTypographyClassesAndStyles } from './use-typography-props';

Try that, see if it works! 😄

@shimotmk

Copy link
Copy Markdown
Contributor Author

@ramonjd
Thank you!
It looks like the test passed🙌.

@ramonjd

ramonjd commented Dec 23, 2024

Copy link
Copy Markdown
Member

This is nearly ready, thank you for all the work here @shimotmk

There's one thing I noticed with global styles. If you add a writing mode value to the button block in global styles, it's applied to the inner .wp-block-button .wp-block-button__link, which was a similar issue to above.

Screenshot 2024-12-24 at 7 56 26 am

I'm not 100% sure of the remedy. I tried converting button/block.json to use the selectors API:

	"selectors": {
		"root": ".wp-block-button .wp-block-button__link",
		"typography": {
			"writing-mode": ".wp-block-button"
		}
	}

But I think I'm missing something as it doesn't work.

If you don't mind, we might wait to ask @aaronrobertshaw for his opinion, as he knows this stuff better than I.

@ramonjd ramonjd added [Type] Bug An existing feature does not function as intended [Feature] Typography Font and typography-related issues and PRs labels Dec 27, 2024
@ramonjd

ramonjd commented Dec 28, 2024

Copy link
Copy Markdown
Member

I tried converting button/block.json to use the selectors API:

It should be

	"selectors": {
		"root": ".wp-block-button .wp-block-button__link",
		"typography": {
			"writingMode": ".wp-block-button"
		}
	}

And it should work, but I think it's being filtered out via wp_theme_json_get_style_nodes here:

https://github.com/WordPress/gutenberg/blob/trunk/lib/class-wp-theme-json-gutenberg.php#L2667-L2667

The filter is added here:

https://github.com/WordPress/gutenberg/blob/trunk/lib/script-loader.php#L43

If I comment out that line it works!

So maybe something to do with wp_filter_out_block_nodes - I haven't tested that deeply yet.

@ramonjd

ramonjd commented Dec 30, 2024

Copy link
Copy Markdown
Member

So maybe something to do with wp_filter_out_block_nodes - I haven't tested that deeply yet.

Nope 😄 It was because of the block style sheet caching. I cleared my cache and it was 👍🏻

I still think we need to add the following to the block.json file so that the writing mode in global styles is added to the wrapper:

diff --git a/packages/block-library/src/button/block.json b/packages/block-library/src/button/block.json
index 953fcae023..6fcb7aca4c 100644
--- a/packages/block-library/src/button/block.json
+++ b/packages/block-library/src/button/block.json
@@ -132,7 +132,6 @@
 				"width": true
 			}
 		},
-		"__experimentalSelector": ".wp-block-button .wp-block-button__link",
 		"interactivity": {
 			"clientNavigation": true
 		}
@@ -142,5 +141,11 @@
 		{ "name": "outline", "label": "Outline" }
 	],
 	"editorStyle": "wp-block-button-editor",
-	"style": "wp-block-button"
+	"style": "wp-block-button",
+	"selectors": {
+		"root": ".wp-block-button .wp-block-button__link",
+		"typography": {
+			"writingMode": ".wp-block-button"
+		}
+	}
 }

Then I think this PR is good to go.

Thank you!!

@shimotmk

Copy link
Copy Markdown
Contributor Author

@ramonjd

Thanks
It was working fine, so I fixed it with the fix

I still think we need to add the following to the block.json file so that the writing mode in global styles is added to the wrapper:

diff --git a/packages/block-library/src/button/block.json b/packages/block-library/src/button/block.json
index 953fcae023..6fcb7aca4c 100644
--- a/packages/block-library/src/button/block.json
+++ b/packages/block-library/src/button/block.json
@@ -132,7 +132,6 @@
 				"width": true
 			}
 		},
-		"__experimentalSelector": ".wp-block-button .wp-block-button__link",
 		"interactivity": {
 			"clientNavigation": true
 		}
@@ -142,5 +141,11 @@
 		{ "name": "outline", "label": "Outline" }
 	],
 	"editorStyle": "wp-block-button-editor",
-	"style": "wp-block-button"
+	"style": "wp-block-button",
+	"selectors": {
+		"root": ".wp-block-button .wp-block-button__link",
+		"typography": {
+			"writingMode": ".wp-block-button"
+		}
+	}
 }

@ramonjd ramonjd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tested:

✅ Settings styles for style.block['core/button'] in theme.json works as expected
✅ Global styles are applied correctly, with writing mode set on the block wrapper
✅ Block-level styles override global styles

Thank you for all the work on this PR @shimotmk 🙇🏻

@ramonjd ramonjd merged commit e5dca54 into WordPress:trunk Dec 31, 2024
@github-actions github-actions Bot added this to the Gutenberg 20.0 milestone Dec 31, 2024
@shimotmk

Copy link
Copy Markdown
Contributor Author

@ramonjd
Thanks for the review!

@shimotmk shimotmk deleted the fix/button-typography branch December 31, 2024 00:49
Gulamdastgir-Momin pushed a commit to Gulamdastgir-Momin/gutenberg that referenced this pull request Jan 23, 2025
Fixes WordPress#64662. The current typography does not work correctly with the given Core Button block styles. This fix forces inner elements to inherit the styles with the exception of `writingMode` which is set on the block wrapper.

Co-authored-by: shimotmk <shimotomoki@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Typography Font and typography-related issues and PRs [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Button Block: Can't set typography

3 participants