Add Line Indent support. #73114
Open
Add Line Indent support. #73114
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mtias
added
[Status] In Progress
mtias
added
the
Global Styles
mtias
added
the
[Feature] Design Tools
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
2 similar comments
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
Size Change: +997 B (+0.04%) Total Size: 2.58 MB
ℹ️ View Unchanged
|
Contributor
|
Neat PR! This is cool to see. In testing this, I found two issues:
Here's a video showing both of these, including when it works automatically and when it suddenly doesn't: line.indent.mov |
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @ericmacknight. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
aaronrobertshaw
force-pushed
the
add/line-indent-support
branch
from
9839b49 to
53806f6
Compare
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for putting this PR together. It's always nice to see long awaited block supports becoming available 👍
For the most part this has been testing pretty well for me.
✅ Global Styles were applied as expected for both text element and paragraph block
✅ Block instance styles overrode Global Styles as expected
✅ TextIndentControl worked well (after quick tweak below)
I hope you don't mind but I took the liberty to push a few minor tweaks:
- Set an initial position for the TextIndentControl slider to match the unset state better
- Made text indent display by default in Global Styles panel as all controls are supposed to be shown there (at least this has been the long standing position)
- Noted the new block support in
global-styles-and-settings.md. More docs will need updating if we land this as a stabilized block support - Addressed linting errors
- I also gave this a quick rebase to avoid a build error
Before I dive back in to this for a final review, there are a few questions I'd like to float:
- Should we land this as a fully stabilized block support?
- For recent block support additions, the thinking has been to avoid
__experimentalsupports due to difficulties in cleanly deprecating these. - There's already an issue to stabilize all typography supports. A PR had even been merged before being reverted towards this stabilization.
- If we stabilize this new support before landing the feature, we will also need to update some other docs e.g.
style-versions.md&block-supports.mdetc.
- For recent block support additions, the thinking has been to avoid
- As the TextIndentControl is displayed as full width within the typography panel, it's order seems clunky.
- It makes sense that the Line Indent field follows the Line Height field but this means the flow between single column and full width controls is off. See below.
-
- Perhaps the Line Height and Line Indent could be rearranged after the text decoration/letter case type fields?
- It makes sense that the Line Indent field follows the Line Height field but this means the flow between single column and full width controls is off. See below.
- Is there a plan to make this support use presets?
- The proposed width/height block supports have been flagged as potentially benefitting from using presets, the line indent support could also reuse the spacing presets
- If so, all these new supports and the current ones for spacing and border radius, could benefit from a single shared component to render a preset slider and custom value toggle + input.
I'm happy to help out further here if I can. If not, I'll circle back and review again with fresh eyes tomorrow.
Member
Author
Of course not :)
Yeah, I think that'd be fine.
This could work
I think it could make sense as a follow up, yes. |
aaronrobertshaw
force-pushed
the
add/line-indent-support
branch
from
53806f6 to
d17f895
Compare
Contributor
|
I've given this a rebase to fix the conflicts. I've also:
That works for me. Edit: Issue for using presets for line indent support can be found here: #73604 |
Contributor
|
This PR should be good for some further testing. I'm out of time on this today but will sort out the backport PR tonight or Monday, then address any feedback across both. |
Member
Author
|
This is looking pretty good, thanks for the tweaks! |
aaronrobertshaw
force-pushed
the
add/line-indent-support
branch
from
80c90c2 to
5c5b271
Compare
aaronrobertshaw
force-pushed
the
add/line-indent-support
branch
from
5c5b271 to
54892f3
Compare
Contributor
|
This PR should be close but the failing e2e looks suspicious. Locally, I'm getting the same error on trunk as this branch but it seems only this PR is failing on GitHub. The test is attempting to locate elements via the I'm probably missing something simple so will look with fresher eyes tomorrow. |
ramonjd
reviewed
Dec 4, 2025
| } | ||
| } ); | ||
|
|
||
| // Text indent needs explicit handling since it may not be in parent settings. |
Member
There was a problem hiding this comment.
I have no idea why this PR is causing the e2e fail.
The fail occurs in the should apply custom colors and font sizes in a variation test, and the fontSize value wasn't coming through to the control.
Here's what my agent is telling me:
if the PR added code that initializes or ensures styles.elements.text exists in the base (e.g., when textIndent is supported), then the shallow merge would drop it when a variation is applied.
So, doing a deep merge in the variation seems to fix it:
diff --git a/packages/global-styles-ui/src/variations/variation.tsx b/packages/global-styles-ui/src/variations/variation.tsx
index ace9f1d47fd..4f0263a804c 100644
--- a/packages/global-styles-ui/src/variations/variation.tsx
+++ b/packages/global-styles-ui/src/variations/variation.tsx
@@ -10,7 +10,10 @@ import { Tooltip } from '@wordpress/components';
import { useMemo, useContext, useState } from '@wordpress/element';
import { ENTER } from '@wordpress/keycodes';
import { _x, sprintf } from '@wordpress/i18n';
-import { areGlobalStylesEqual } from '@wordpress/global-styles-engine';
+import {
+ areGlobalStylesEqual,
+ mergeGlobalStyles,
+} from '@wordpress/global-styles-engine';
/**
* Internal dependencies
@@ -41,7 +44,7 @@ export default function Variation( {
} = useContext( GlobalStylesContext );
const context = useMemo( () => {
- let merged = { ...base, ...variation };
+ let merged = mergeGlobalStyles( base, variation );
if ( properties ) {
merged = filterObjectByProperties( merged, properties );
}I suppose because there are nested objects that need to be merged there, e.g., base.styles.elements.text with variation.styles.elements.text
Member
There was a problem hiding this comment.
Oh wait, now it doesn't pass again. Sorry for the red herring. I swear it passed on this change!
Member
There was a problem hiding this comment.
Okay it's because I had another suggested fix applied, one that does, I hope, work:
diff --git a/packages/global-styles-engine/src/settings/get-style.ts b/packages/global-styles-engine/src/settings/get-style.ts
index 411888e023a..b2ef29dc5a5 100644
--- a/packages/global-styles-engine/src/settings/get-style.ts
+++ b/packages/global-styles-engine/src/settings/get-style.ts
@@ -11,13 +11,58 @@ export function getStyle< T = any >(
blockName?: string,
shouldDecodeEncode = true
): T | undefined {
+ if ( ! globalStyles ) {
+ return undefined;
+ }
+
+ // When querying element paths like 'elements.text', also check root-level styles
+ // as root-level styles apply to the text element
+ if ( path && path.startsWith( 'elements.' ) ) {
+ const elementPath = path.split( '.' );
+ const elementName = elementPath[ 1 ];
+
+ // First try the element-specific path
+ const elementValue = getValueFromObjectPath( globalStyles, [
+ 'styles',
+ ...elementPath,
+ ] );
+ if ( elementValue !== undefined ) {
+ return (
+ shouldDecodeEncode
+ ? getValueFromVariable(
+ globalStyles,
+ blockName,
+ elementValue as UnresolvedValue
+ )
+ : elementValue
+ ) as T | undefined;
+ }
+
+ // Fallback to root-level styles for text element
+ if ( elementName === 'text' ) {
+ const rootValue = getValueFromObjectPath( globalStyles, [
+ 'styles',
+ ] );
+ if ( rootValue !== undefined ) {
+ return (
+ shouldDecodeEncode
+ ? getValueFromVariable(
+ globalStyles,
+ blockName,
+ rootValue as UnresolvedValue
+ )
+ : rootValue
+ ) as T | undefined;
+ }
+ }
+
+ return undefined;
+ }
+
const appendedPath = path ? '.' + path : '';
const finalPath = ! blockName
? `styles${ appendedPath }`
: `styles.blocks.${ blockName }${ appendedPath }`;
- if ( ! globalStyles ) {
- return undefined;
- }
const rawResult = getValueFromObjectPath( globalStyles, finalPath ) as
| string
Contributor
There was a problem hiding this comment.
I came to the same conclusion, i.e. the move of text element styles from styles.typography etc to styles.elements.text.typography was the culprit.
What I can't answer just yet was why that chance was made in this PR.
The fallback approach should work but I'm interested to know if we really need to move the styles at all and increase complexity here as a result.
Member
There was a problem hiding this comment.
yeah it would be nice to know why that was the choice. there must be a architectural reason.
i can't get much out of the pr description.
cc @mtias if you can provide more info
Contributor
There was a problem hiding this comment.
My best guess at the moment is it was to allow generic styling of p tags via the text element. Prior to this PR all those text styles are applied to the body tag.
A lot of themes out there will be applying generic typography styles via styles.typography so that should be maintained for backward compatiblity. Making the text element target p wlil be a breaking change.
Unless another solution is found it would also prevent users from updating those generic typography styles, previously applied to body.
Do we need to introduce a new element? One to split generic p and body typographic styles?
While testing I think I spotted another bug potentially due to the recent refactoring of the Global Styles into their own package. When you edit Global Styles via the Editor > Styles sidebar, the style changes are reflected in real time but if you save and then click into the editor, the recent styles are lost.
Member
There was a problem hiding this comment.
Do we need to introduce a new element? One to split generic p and body typographic styles?
I haven't thought deeply about this, but prima facie that sounds like a logical approach.
Member
Author
There was a problem hiding this comment.
The way to handle p + p in both editor and frontend ended up being slightly more convoluted than I hoped. It'd be nice to review if there is a more straightforward way to handle this that may feel more idiomatic.
Contributor
There was a problem hiding this comment.
I've started exploring some of the suggested options here. Starting with a paragraphs element. It might be easier to discuss with a working PoC.
It's rough and incomplete at this stage but a draft has been added via: 70e7e07
aaronrobertshaw
force-pushed
the
add/line-indent-support
branch
from
54892f3 to
dd7c782
Compare
8 tasks
|
I am reading this stuff with much the same level of comprehension as I would read cricket scores in Swahili, but it is clear to me how tricky this business is and how hard you guys are working to make it happen, and I just want to thank you all. |
Member
2 for 188 with 12 overs to go I heard |
Introduces a new typography tool to apply first-line indent to paragraphs. It can be applied locally or globally through GS. In global styles it works by skipping the first paragraph and indenting every subsequent paragraph instead. It resets across containers (like quotes).
aaronrobertshaw
force-pushed
the
add/line-indent-support
branch
from
dd7c782 to
857164b
Compare
aaronrobertshaw
force-pushed
the
add/line-indent-support
branch
from
857164b to
70e7e07
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #37462.
Introduces a new typography tool to apply first-line indent to paragraphs. It can be applied locally or globally through GS. In global styles it works by skipping the first paragraph and indenting every subsequent paragraph instead. It resets across containers (like quotes).