Feat(Text Vertical Align): Added a new feature to enable the vertical…#3908
Feat(Text Vertical Align): Added a new feature to enable the vertical…#3908ziad-ashraf7 wants to merge 2 commits intoGraphiteEditor:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances text layout capabilities by introducing comprehensive support for vertical text alignment. It addresses a previous limitation by allowing users to precisely control text positioning with Top, Center, and Bottom options. The changes span the core rendering engine, node graph, and user interface, providing a seamless experience while ensuring existing documents are correctly migrated. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully adds vertical text alignment functionality, with changes across the backend rendering pipeline, frontend UI, and document migration. The implementation is solid and aligns with the description. I have a couple of suggestions to improve code maintainability by refactoring a calculation and removing some duplicated code.
| /// Vertical alignment of text within its bounding box. | ||
| #[repr(C)] | ||
| #[cfg_attr(feature = "wasm", derive(tsify::Tsify))] | ||
| #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, serde::Serialize, serde::Deserialize, Hash, DynAny, node_macro::ChoiceType)] | ||
| #[widget(Radio)] | ||
| pub enum VerticalAlign { | ||
| #[default] | ||
| Top, | ||
| Center, | ||
| Bottom, | ||
| } |
There was a problem hiding this comment.
This file appears to be a duplicate of node-graph/libraries/core-types/src/text.rs. To improve maintainability and avoid having to update two files for any change to these types, consider removing this file's contents and instead re-exporting the types from core-types. For example, you could replace the contents of this file with pub use core_types::text::*; if the module structure allows, or whatever is appropriate.
| let vertical_offset = match typesetting.vertical_align { | ||
| VerticalAlign::Top => 0., | ||
| VerticalAlign::Center | VerticalAlign::Bottom => { | ||
| let layout_height = layout.height() as f64; | ||
| let container_height = typesetting.max_height.unwrap_or(layout_height); | ||
| let free_space = (container_height - layout_height).max(0.); | ||
|
|
||
| match typesetting.vertical_align { | ||
| VerticalAlign::Center => free_space / 2., | ||
| VerticalAlign::Bottom => free_space, | ||
| _ => unreachable!(), | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
The logic for calculating vertical_offset is correct, but it can be simplified for better readability and maintainability. The nested match statement is a bit redundant. Consider refactoring to a single match or an if let block that also makes the dependency on max_height more explicit.
let vertical_offset = if let Some(container_height) = typesetting.max_height {
let layout_height = layout.height() as f64;
let free_space = (container_height - layout_height).max(0.);
match typesetting.vertical_align {
VerticalAlign::Top => 0.,
VerticalAlign::Center => free_space / 2.,
VerticalAlign::Bottom => free_space,
}
} else {
0.
};![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/nodes/text/src/path_builder.rs">
<violation number="1" location="node-graph/nodes/text/src/path_builder.rs:108">
P2: Vertical offset is applied before skew in merged-path mode, but skew pivot remains at `run_y`, causing unintended horizontal drift for tilted/synthetic-italic text.</violation>
</file>
<file name="node-graph/libraries/core-types/src/text.rs">
<violation number="1" location="node-graph/libraries/core-types/src/text.rs:58">
P1: New required serde field `vertical_align` breaks backward deserialization of older `TypesettingConfig` payloads missing that field.</violation>
</file>
<file name="frontend/src/components/panels/Document.svelte">
<violation number="1" location="frontend/src/components/panels/Document.svelte:378">
P2: Vertical alignment computes free space from `scrollHeight` after fixing element height, which can collapse free space to zero and break Center/Bottom alignment for short text.</violation>
<violation number="2" location="frontend/src/components/panels/Document.svelte:378">
P2: Vertical alignment padding is measured once and not recomputed after text edits or font updates, causing center/bottom-aligned text overlay drift.</violation>
<violation number="3" location="frontend/src/components/panels/Document.svelte:379">
P2: Vertical alignment in the editable overlay uses a floored container height, but backend rendering uses raw `max_height`, so Center/Bottom text positioning can disagree between edit mode and rendered output.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| for glyph in glyph_run.glyphs() { | ||
| let glyph_offset = DVec2::new((run_x + glyph.x) as f64, (run_y - glyph.y) as f64); | ||
| let glyph_offset = DVec2::new((run_x + glyph.x) as f64, (run_y - glyph.y) as f64 + vertical_offset); |
There was a problem hiding this comment.
P2: Vertical offset is applied before skew in merged-path mode, but skew pivot remains at run_y, causing unintended horizontal drift for tilted/synthetic-italic text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/text/src/path_builder.rs, line 108:
<comment>Vertical offset is applied before skew in merged-path mode, but skew pivot remains at `run_y`, causing unintended horizontal drift for tilted/synthetic-italic text.</comment>
<file context>
@@ -105,7 +105,7 @@ impl<Upstream: Default + 'static> PathBuilder<Upstream> {
for glyph in glyph_run.glyphs() {
- let glyph_offset = DVec2::new((run_x + glyph.x) as f64, (run_y - glyph.y) as f64);
+ let glyph_offset = DVec2::new((run_x + glyph.x) as f64, (run_y - glyph.y) as f64 + vertical_offset);
run_x += glyph.advance;
</file context>
| // Wait for layout to settle so we can measure the content height | ||
| await tick(); | ||
|
|
||
| const contentHeight = textInput.scrollHeight; |
There was a problem hiding this comment.
P2: Vertical alignment padding is measured once and not recomputed after text edits or font updates, causing center/bottom-aligned text overlay drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/panels/Document.svelte, line 378:
<comment>Vertical alignment padding is measured once and not recomputed after text edits or font updates, causing center/bottom-aligned text overlay drift.</comment>
<file context>
@@ -366,6 +366,31 @@
+ // Wait for layout to settle so we can measure the content height
+ await tick();
+
+ const contentHeight = textInput.scrollHeight;
+ const containerHeight = Math.floor(data.maxHeight / (data.lineHeightRatio * data.fontSize)) * (data.lineHeightRatio * data.fontSize);
+ const freeSpace = Math.max(containerHeight - contentHeight, 0);
</file context>
…on, and match frontend container height to backend
1 participant
Fixes: #3883
Before
There was only horizontal align for the text (See the video in the issue).
After
2026-03-17.12-39-14.mp4
Files Changed:
editor/src/messages/frontend/frontend_message.rseditor/src/messages/portfolio/document/graph_operation/utility_types.rseditor/src/messages/portfolio/document/node_graph/document_node_definitions.rseditor/src/messages/portfolio/document/overlays/utility_functions.rseditor/src/messages/portfolio/document/overlays/utility_types_native.rseditor/src/messages/portfolio/document_migration.rseditor/src/messages/tool/common_functionality/graph_modification_utils.rseditor/src/messages/tool/tool_messages/text_tool.rsfrontend/src/components/panels/Document.sveltenode-graph/graph-craft/src/document/value.rsnode-graph/libraries/core-types/src/text.rsnode-graph/nodes/gstd/src/text.rsnode-graph/nodes/text/src/lib.rsnode-graph/nodes/text/src/path_builder.rsnode-graph/nodes/text/src/text_context.rsDescription:
Adds vertical text alignment support with Top, Center, and Bottom options, accessible from both the text tool options bar and the properties panel.
Implementation:
Since Parley has no built-in vertical alignment API, this is implemented as a manual Y-offset applied during glyph rendering.
The offset is calculated as (container_height - layout_height).max(0.) * factor, where the factor is 0 for Top, 0.5 for Center, and 1.0 for Bottom. This only takes effect when a max height is set on the text layer.
Changes:
New enum and config -
VerticalAlign enum(Top/Center/Bottom) added toTypesettingConfigin bothcore-types/src/text.rsandnodes/text/src/lib.rs, matching the existingTextAlignpattern.Text rendering pipeline - Vertical offset calculated in
text_context.rs(to_path()) and applied to glyph Y-coordinates in path_builder.rs (render_glyph_run).Node graph - New vertical_align parameter on the text() proto node with a custom widget override (text_vertical_align) registered in document_node_definitions.rs.
TaggedValue::VerticalAlignvariant added.Text tool UI - Radio button widget with Top/Center/Bottom icons added to the text tool options bar, wired through
TextOptionsandTextOptionsUpdate.Frontend overlay -
Document.svelteapplies matching CSS vertical padding to thecontentEditabletextbox so the editing overlay matches the rendered output.Document migration - Handles old documents by inserting the new vertical_align input at index 12 and shifting separate_glyph_elements from index 12 to 13.