Implement text-on-path support with SVG 2 compliance#4055
Implement text-on-path support with SVG 2 compliance#4055jsjgdh wants to merge 1 commit intoGraphiteEditor:masterfrom
Conversation
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Code Review
This pull request implements SVG 2 text-on-path layout rules by adding a new text_on_path node and integrating the kurbo library for path calculations. The FontCache was refactored to use Arc for better data sharing, and the PathBuilder was updated to handle glyph drawing for paths. Review feedback identifies several critical issues, including fragile string-based font mapping in SVGs, incorrect glyph horizontal shifting due to absolute coordinate reuse, and a bug in path reversal for complex geometries. Improvements were also suggested for the efficiency of arc length sampling and the correction of a double-radian conversion in font synthesis.
| let svg = svg.replace("font-family=\"sans-serif\"", "font-family=\"Source Sans Pro\""); | ||
| let svg = svg.replace("font-family='sans-serif'", "font-family='Source Sans Pro'"); |
There was a problem hiding this comment.
Performing string replacements on the SVG XML to handle font families is fragile. It doesn't account for variations in whitespace, single vs. double quotes, or font-family lists. It also fails to handle fonts specified via CSS styles. A more robust approach would be to configure usvg to map generic font families to the desired local font via its font database options.
| let final_transform = | ||
| DAffine2::from_translation(DVec2::new(point.x, point.y)) * DAffine2::from_angle(angle) * DAffine2::from_translation(DVec2::new(glyph.x as f64, -glyph.y as f64)); |
There was a problem hiding this comment.
The glyph transformation uses glyph.x as a translation after placing the glyph on the path. In parley, glyph.x is the absolute horizontal position within the run. Since the path is already sampled at a specific distance (mid), applying glyph.x again will cause incorrect horizontal shifting along the path. The translation should only account for the offset relative to the sampled point (e.g., centering the glyph horizontally).
| let final_transform = | |
| DAffine2::from_translation(DVec2::new(point.x, point.y)) * DAffine2::from_angle(angle) * DAffine2::from_translation(DVec2::new(glyph.x as f64, -glyph.y as f64)); | |
| let final_transform = | |
| DAffine2::from_translation(DVec2::new(point.x, point.y)) * DAffine2::from_angle(angle) * DAffine2::from_translation(DVec2::new(-glyph.advance as f64 / 2.0, -glyph.y as f64)); |
| insert_index: usize, | ||
| graphite_gradient_stops: &HashMap<String, GradientStops>, | ||
| ) { | ||
| log::error!("DIAGNOSTIC: Visiting node root: {:?}", node); |
| for i in 1..=samples_per_segment { | ||
| let t = i as f64 / samples_per_segment as f64; | ||
| let sub_len = seg.subsegment(0.0..t).arclen(accuracy); | ||
| lengths.push(cumulative + sub_len); |
There was a problem hiding this comment.
Calling seg.subsegment(0.0..t).arclen(accuracy) inside a loop is inefficient because calculating the arc length of a Bezier curve involves numerical integration. This results in
| line.items().for_each(|item| { | ||
| if let PositionedLayoutItem::GlyphRun(glyph_run) = item { | ||
| let run = glyph_run.run(); | ||
| let style_skew = run.synthesis().skew().map(|angle| DAffine2::from_cols_array(&[1., 0., -angle.to_radians().tan() as f64, 1., 0., 0.])); |
There was a problem hiding this comment.
In parley 0.6, RunSynthesis::skew returns the angle in radians. Calling to_radians() on a value that is already in radians will result in an incorrect skew angle.
| let style_skew = run.synthesis().skew().map(|angle| DAffine2::from_cols_array(&[1., 0., -angle.to_radians().tan() as f64, 1., 0., 0.])); | |
| let style_skew = run.synthesis().skew().map(|angle| DAffine2::from_cols_array(&[1., 0., -(angle as f64).tan(), 1., 0., 0.])); |
|
!build (Run ID 24950870909) |
There was a problem hiding this comment.
10 issues found across 13 files
Confidence score: 3/5
- There is moderate merge risk because several medium-severity, high-confidence text rendering/import issues could affect user-visible output, especially around fallback font selection and text-on-path placement behavior.
- The most severe functional concerns are in
node-graph/nodes/text/src/text_on_path.rsandeditor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, where anchoring math and chunk assignment can misplace or corrupt multi-line/multi-chunk SVG text layout. - Additional correctness regressions in
node-graph/nodes/text/src/path_builder.rsandnode-graph/nodes/text/src/font_cache.rs(synthetic skew handling, nondeterministic fallback key choice, and style-name changes for 950 weight) increase the chance of visual inconsistencies; the PR title convention issue appears process-related rather than runtime-breaking. - Pay close attention to
node-graph/nodes/text/src/text_on_path.rs,editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs,node-graph/nodes/text/src/font_cache.rs, andnode-graph/nodes/text/src/path_builder.rs- these contain the highest-impact rendering and import behavior risks.
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/font_cache.rs">
<violation number="1" location="node-graph/nodes/text/src/font_cache.rs:53">
P2: Removing the explicit 950 font-weight name changes generated style strings and can break exact style-name matching for 950 fonts.</violation>
<violation number="2" location="node-graph/nodes/text/src/font_cache.rs:126">
P2: Missing-font fallback chooses an arbitrary `HashMap` key, making font resolution nondeterministic when default font is unavailable.</violation>
</file>
<file name="node-graph/nodes/text/src/text_context.rs">
<violation number="1" location="node-graph/nodes/text/src/text_context.rs:64">
P2: `layout_text` was made fully public, unintentionally expanding the external API and exposing backend-specific `Layout` details; `pub(crate)` is sufficient for the new internal caller.</violation>
</file>
<file name="node-graph/nodes/text/src/text_on_path.rs">
<violation number="1" location="node-graph/nodes/text/src/text_on_path.rs:125">
P2: `reverse_bezpath` collects all segments across subpaths and reverses them as a single continuous path. Any `MoveTo` elements between distinct subpaths are lost, causing separate subpaths to be incorrectly merged into one. For paths with multiple subpaths, this produces incorrect geometry with unexpected connecting lines.</violation>
<violation number="2" location="node-graph/nodes/text/src/text_on_path.rs:207">
P2: Multiline text-on-path anchoring uses layout block width (`full_width`) while placement advances cursor across all lines, causing middle/end anchor misalignment and visibility errors.</violation>
</file>
<file name="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:555">
P2: Unconditional error-level diagnostic log in normal SVG import path causes noisy false error reports.</violation>
<violation number="2" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:715">
P2: Text-on-path import incorrectly applies all chunk text to the first path chunk, breaking multi-chunk/mixed-flow SVG text layout.</violation>
</file>
<file name="node-graph/nodes/text/src/path_builder.rs">
<violation number="1" location="node-graph/nodes/text/src/path_builder.rs:38">
P2: `origin` is dead state: it is assigned but never read, and no longer affects point generation, making transform state misleading.</violation>
<violation number="2" location="node-graph/nodes/text/src/path_builder.rs:79">
P2: `render_glyph_run` drops font synthesis skew by tying `style_skew` to `tilt` instead of `run.synthesis().skew()`, which can remove synthetic italic/slant styling in rendered output.</violation>
</file>
<file name="node-graph/nodes/gstd/src/text.rs">
<violation number="1" location="node-graph/nodes/gstd/src/text.rs:81">
P2: Custom agent: **PR title enforcement**
PR title is not in imperative mood and lacks a leading action verb as required by PR title conventions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| .font_file_data | ||
| .keys() | ||
| .find(|font| font.font_family == core_types::consts::DEFAULT_FONT_FAMILY && font.font_style == core_types::consts::DEFAULT_FONT_STYLE) | ||
| .or_else(|| self.font_file_data.keys().next()); |
There was a problem hiding this comment.
P2: Missing-font fallback chooses an arbitrary HashMap key, making font resolution nondeterministic when default font is unavailable.
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/font_cache.rs, line 126:
<comment>Missing-font fallback chooses an arbitrary `HashMap` key, making font resolution nondeterministic when default font is unavailable.</comment>
<file context>
@@ -51,81 +50,97 @@ impl Font {
+ .font_file_data
.keys()
.find(|font| font.font_family == core_types::consts::DEFAULT_FONT_FAMILY && font.font_style == core_types::consts::DEFAULT_FONT_STYLE)
+ .or_else(|| self.font_file_data.keys().next());
+ fallback
}
</file context>
| @@ -1,6 +1,7 @@ | |||
| use core_types::{Ctx, table::Table}; | |||
There was a problem hiding this comment.
P2: Custom agent: PR title enforcement
PR title is not in imperative mood and lacks a leading action verb as required by PR title conventions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gstd/src/text.rs, line 81:
<comment>PR title is not in imperative mood and lacks a leading action verb as required by PR title conventions.</comment>
<file context>
@@ -74,3 +75,48 @@ fn text<'i: 'n>(
+
+/// Flows text glyphs along a vector path following the SVG 2 text-on-path layout rules (§11.8).
+#[node_macro::node(category("Text"))]
+fn text_on_path<'i: 'n>(
+ _: impl Ctx,
+ #[scope("editor-api")] editor_resources: &'i WasmEditorApi,
</file context>
There was a problem hiding this comment.
@jsjgdh Based on the project's PR title conventions and the core changes in this PR, here are a few options that use the required imperative mood:
- Implement text-on-path support with SVG 2 compliance (Best overall)
- Add text-on-path node and high-fidelity SVG export (Specific to technical additions)
- Enable flowing text along vector paths (Focuses on the user-facing feature)
I flagged the original title "Text on path" because it functions as a noun phrase rather than a command. These suggestions lead with an action verb (Implement, Add, Enable) to match the team's preference.
For more details on these rules, you can check the learnings page.
Wasm: 23.10 MB — JS: 0.43 MB — CSS: 0.09 MB — Fonts: 0.30 MB — Images: 0.09 MB — All Assets: 24.01 MB |
f07c79b to
76938eb
Compare
ef54a07 to
2c13faa
Compare
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
6 issues found across 18 files
Confidence score: 2/5
- There are concrete runtime-failure risks:
node-graph/nodes/text/src/text_on_path.rscan panic on invalid font parsing, andeditor/src/messages/portfolio/document/graph_operation/utility_types.rscan panic when Text On Path node registration is missing, which makes this high risk to merge as-is. editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rsappears to parse TextPath attributes from the wrong SVG scope and collides anonymous<text>elements on an empty key, so layout can be applied to the wrong node (clear user-facing regression risk).node-graph/nodes/text/src/path_builder.rsbakes per-glyph placement into geometry instead ofTableRow.transform, which can break downstream assumptions about row-level transforms; the duplication note ineditor/src/messages/portfolio/document/graph_operation/utility_types.rsis lower severity but increases future drift risk.- Pay close attention to
node-graph/nodes/text/src/text_on_path.rs,editor/src/messages/portfolio/document/graph_operation/utility_types.rs,editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, andnode-graph/nodes/text/src/path_builder.rs- they contain panic paths and text-on-path mapping/transform behaviors most likely to cause regressions.
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/text_on_path.rs">
<violation number="1" location="node-graph/nodes/text/src/text_on_path.rs:294">
P1: Unwrap on font parsing can panic on invalid font data/index, crashing text-on-path rendering instead of skipping the affected run.</violation>
</file>
<file name="editor/src/messages/portfolio/document/graph_operation/utility_types.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/utility_types.rs:293">
P3: Custom agent: **PR title enforcement**
PR title is a noun phrase instead of an imperative command.</violation>
<violation number="2" location="editor/src/messages/portfolio/document/graph_operation/utility_types.rs:312">
P2: Panics if Text On Path node registration is missing instead of failing gracefully.</violation>
<violation number="3" location="editor/src/messages/portfolio/document/graph_operation/utility_types.rs:354">
P3: This new method duplicates the existing transform/stroke/fill chain setup logic, which increases the risk of the different insertion paths drifting apart over time.</violation>
</file>
<file name="node-graph/nodes/text/src/path_builder.rs">
<violation number="1" location="node-graph/nodes/text/src/path_builder.rs:58">
P2: Per-glyph placement is baked into geometry instead of preserved in `TableRow.transform`, breaking the row-level transform contract for downstream consumers.</violation>
</file>
<file name="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:554">
P2: TextPath attrs are parsed out of the wrong SVG scope and anonymous `<text>` elements collide on the empty-string key, so text-on-path layout can be applied to the wrong node.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| let font = run.font(); | ||
| let font_size = run.font_size(); | ||
| let normalized_coords = run.normalized_coords().iter().map(|coord| skrifa::instance::NormalizedCoord::from_bits(*coord)).collect::<Vec<_>>(); | ||
| let outlines = ReadFontsRef::from_index(font.data.as_ref(), font.index).unwrap().outline_glyphs(); |
There was a problem hiding this comment.
P1: Unwrap on font parsing can panic on invalid font data/index, crashing text-on-path rendering instead of skipping the affected run.
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/text_on_path.rs, line 294:
<comment>Unwrap on font parsing can panic on invalid font data/index, crashing text-on-path rendering instead of skipping the affected run.</comment>
<file context>
@@ -0,0 +1,346 @@
+ let font = run.font();
+ let font_size = run.font_size();
+ let normalized_coords = run.normalized_coords().iter().map(|coord| skrifa::instance::NormalizedCoord::from_bits(*coord)).collect::<Vec<_>>();
+ let outlines = ReadFontsRef::from_index(font.data.as_ref(), font.index).unwrap().outline_glyphs();
+
+ glyph_run.glyphs().for_each(|glyph| {
</file context>
| ) { | ||
| let path_vector = Table::new_from_element(Vector::from_subpaths(path_subpaths, true)); | ||
| let text_on_path_node = resolve_proto_node_type(graphene_std::text::text_on_path::IDENTIFIER) | ||
| .expect("Text On Path node does not exist") |
There was a problem hiding this comment.
P2: Panics if Text On Path node registration is missing instead of failing gracefully.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/utility_types.rs, line 312:
<comment>Panics if Text On Path node registration is missing instead of failing gracefully.</comment>
<file context>
@@ -289,6 +290,82 @@ impl<'a> ModifyInputsContext<'a> {
+ ) {
+ let path_vector = Table::new_from_element(Vector::from_subpaths(path_subpaths, true));
+ let text_on_path_node = resolve_proto_node_type(graphene_std::text::text_on_path::IDENTIFIER)
+ .expect("Text On Path node does not exist")
+ .node_template_input_override([
+ Some(NodeInput::scope("editor-api")),
</file context>
| ..Default::default() | ||
| }); | ||
| let mut vector = Vector::from_subpaths(subpaths, false); | ||
| vector.transform(transform); |
There was a problem hiding this comment.
P2: Per-glyph placement is baked into geometry instead of preserved in TableRow.transform, breaking the row-level transform contract for downstream consumers.
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 58:
<comment>Per-glyph placement is baked into geometry instead of preserved in `TableRow.transform`, breaking the row-level transform contract for downstream consumers.</comment>
<file context>
@@ -1,158 +1,149 @@
- ..Default::default()
- });
+ let mut vector = Vector::from_subpaths(subpaths, false);
+ vector.transform(transform);
+ self.vector_table.push(TableRow::new_from_element(vector));
} else {
</file context>
| @@ -12,11 +12,10 @@ use graph_craft::document::value::TaggedValue; | |||
| use graph_craft::document::{NodeId, NodeInput}; | |||
There was a problem hiding this comment.
P2: TextPath attrs are parsed out of the wrong SVG scope and anonymous <text> elements collide on the empty-string key, so text-on-path layout can be applied to the wrong node.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, line 554:
<comment>TextPath attrs are parsed out of the wrong SVG scope and anonymous `<text>` elements collide on the empty-string key, so text-on-path layout can be applied to the wrong node.</comment>
<file context>
@@ -532,6 +542,55 @@ fn parse_hex_stop_color(hex: &str, opacity: f32) -> Option<Color> {
+ pub length_adjust: Option<String>,
+}
+
+fn pre_parse_textpath_attrs(svg: &str) -> std::collections::HashMap<String, TextPathAttrs> {
+ fn get_attr<'a>(attrs_str: &'a str, name: &str) -> Option<&'a str> {
+ ['"', '\''].into_iter().find_map(|q| {
</file context>
| @@ -3,7 +3,7 @@ use crate::messages::portfolio::document::node_graph::document_node_definitions: | |||
| use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier; | |||
There was a problem hiding this comment.
P3: This new method duplicates the existing transform/stroke/fill chain setup logic, which increases the risk of the different insertion paths drifting apart over time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/utility_types.rs, line 354:
<comment>This new method duplicates the existing transform/stroke/fill chain setup logic, which increases the risk of the different insertion paths drifting apart over time.</comment>
<file context>
@@ -289,6 +290,82 @@ impl<'a> ModifyInputsContext<'a> {
+ self.network_interface.insert_node(transform_id, transform_node, &[]);
+ self.network_interface.move_node_to_chain_start(&transform_id, layer, &[], self.import);
+
+ let stroke = resolve_proto_node_type(graphene_std::vector_nodes::stroke::IDENTIFIER)
+ .expect("Stroke node does not exist")
+ .default_node_template();
</file context>
| @@ -3,7 +3,7 @@ use crate::messages::portfolio::document::node_graph::document_node_definitions: | |||
| use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier; | |||
There was a problem hiding this comment.
P3: Custom agent: PR title enforcement
PR title is a noun phrase instead of an imperative command.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/utility_types.rs, line 293:
<comment>PR title is a noun phrase instead of an imperative command.</comment>
<file context>
@@ -289,6 +290,82 @@ impl<'a> ModifyInputsContext<'a> {
self.network_interface.move_node_to_chain_start(&fill_id, layer, &[], self.import);
}
+ pub fn insert_text_on_path(
+ &mut self,
+ text: String,
</file context>
There was a problem hiding this comment.
1 issue found across 10 files
Confidence score: 5/5
- This PR is low risk to merge because the only reported issue is a title-convention mismatch, not a code behavior defect.
- The most severe item is in
node-graph/nodes/text/src/text_on_path.rsat 3/10 severity and points to PR title phrasing (noun phrase vs imperative), so impact is process/compliance rather than user-facing functionality. - Pay close attention to
node-graph/nodes/text/src/text_on_path.rs- ensure the associated PR title is updated to match the required imperative convention.
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/text_on_path.rs">
<violation number="1" location="node-graph/nodes/text/src/text_on_path.rs:1">
P3: Custom agent: **PR title enforcement**
PR title is a noun phrase rather than an imperative command, so it does not satisfy the required title convention.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,346 @@ | |||
| use core_types::table::Table; | |||
There was a problem hiding this comment.
P3: Custom agent: PR title enforcement
PR title is a noun phrase rather than an imperative command, so it does not satisfy the required title convention.
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/text_on_path.rs:
<comment>PR title is a noun phrase rather than an imperative command, so it does not satisfy the required title convention.</comment>
<file context>
@@ -0,0 +1,346 @@
+use core_types::table::Table;
+use dyn_any::DynAny;
+use glam::{DAffine2, DVec2};
+use kurbo::{BezPath, ParamCurve, ParamCurveArclen, ParamCurveDeriv, PathEl, PathSeg};
+use parley::PositionedLayoutItem;
+use skrifa::MetadataProvider;
+use skrifa::raw::FontRef as ReadFontsRef;
+use std::sync::Arc;
+use vector_types::{TextOnPathMetadata, Vector};
</file context>
Not to be merged, only for testing.
Known Issue: Font