feat: add support for vertical writing mode#1013
Conversation
|
Wow! That's a serious change. I'm no Unicode/Text Layout expert, so I would assume it's better. Does it actually fixes #890 ? Also, 16MB for SourceHanSansHWSC-Bold.otf is too much. Getting it below 1MB would be nice. Also, can you post new test file results before and the patch? Just for the reference. I want to see how exactly thing have changed. Thanks! |
Sorry to keep you waiting. The setup of fonttools has puzzled me for quite a while。 |
|
Thanks! Looks good to me. |
| let mut vertical_sub = 0; | ||
|
|
||
| for i in 0..output.len() { | ||
| for i in 0..output_horizontal.len() { |
There was a problem hiding this comment.
Some concerns by Claude: Ignore, if this is noise:
Cross-indexing the two shaping passes can panic / mismatch glyphs.
The loop is bounded by output_horizontal.len(), but in vertical mode positions/infos come from output_vertical (and sub_positions/sub_infos are the other pass). This assumes both passes return the same glyph count and a 1:1 positional correspondence. rustybuzz/HarfBuzz guarantees neither across different directions and feature sets (vert/vrtr/vhal substitutions, ligatures, cluster regrouping). If the other pass yields fewer glyphs, positions[i] / sub_positions[i] is an out-of-bounds panic — and because sub_* is indexed unconditionally, this can fire on horizontal text too, not just vertical.
Suggest bounding by min(len) (or .get(i)) and, better, looking up the backup glyph by cluster index rather than positional i. A property test over mixed Latin+CJK+ligature runs would cover this.
Related: sub_text[start..end] below slices using start = info.cluster from one pass and end from another — if cluster order isn't monotonic for the chosen pass, start > end panics. Worth a guard.
| let output_horizontal = rustybuzz::shape(&rb_font, &features, buffer_horizontal); | ||
| let output_vertical = rustybuzz::shape(&rb_font, &features, buffer_vertical); |
There was a problem hiding this comment.
Some concerns by Claude: Ignore, if this is noise:
Both shaping passes run unconditionally — perf regression on the common path.
output_vertical is always computed, but in horizontal mode it's never read (the if writing_mode == TopToBottom branches that consume sub_* are skipped). This roughly doubles shaping cost for all existing horizontal text, and this function is on the hot path for every <text> in every SVG. Please gate the second pass (and its buffer) behind writing_mode == WritingMode::TopToBottom.
| } else if matches!( | ||
| orientation, | ||
| // Characters which are displayed sideways, rotated 90 degrees clockwise compared to the code charts. | ||
| unicode_vo::Orientation::TransformedOrRotated | _ | ||
| ) { |
There was a problem hiding this comment.
Some concerns by Claude: Ignore, if this is noise:
This matches! is always true — the else if is just an else.
The | _ wildcard matches every variant, so naming TransformedOrRotated is misleading and it trips Clippy's wildcard_in_or_patterns. Make it a plain else, or restrict to the orientation(s) you actually mean.
| } else if matches!( | |
| orientation, | |
| // Characters which are displayed sideways, rotated 90 degrees clockwise compared to the code charts. | |
| unicode_vo::Orientation::TransformedOrRotated | _ | |
| ) { | |
| } else { |
| if glyph.text_orientation == unicode_vo::Orientation::Rotated { | ||
| glyph.width as f32 * sx // Use original width for rotated characters | ||
| } else { | ||
| glyph.dx.abs() as f32 * 2.0 * sx // Vertical advance for upright characters |
There was a problem hiding this comment.
Some concerns by Claude: Ignore, if this is noise:
Comment/code mismatch + undocumented assumption.
The comment says "vertical advance (dy * 2)" but the code uses dx. This looks like it's reconstructing the advance from HarfBuzz's vertical-centering x-offset (≈ −width/2) — clever but brittle and undocumented. Please fix the comment to match, and explain why dx*2 recovers the advance (or read the actual vertical advance directly).
| for glyph in glyphs { | ||
| let sx = glyph.font.scale(font_size); | ||
|
|
||
| glyphs_orientation = glyph.text_orientation; |
There was a problem hiding this comment.
Some concerns by Claude: Ignore, if this is noise:
Cluster orientation reflects only the last glyph.
glyphs_orientation/glyphs_replaced are overwritten each iteration, so a multi-glyph cluster ends up with whatever the final glyph had. The comment states the "same orientation" assumption but nothing enforces it — a mixed cluster would silently mis-rotate. Consider asserting it (debug) or deriving from the first glyph deliberately.
There was a problem hiding this comment.
Some concerns by Claude: Ignore, if this is noise:
This swap may defeat the test's purpose.
This test (per the linked "your code displays Japanese wrong") exists to show that xml:lang=ja vs zh-HANT select different glyphs via language-aware fallback. Pinning a single subset font risks the three lines rendering identically, removing the behavior under test. Same concern applies to the Amiri→Source Han Sans HW SC swaps in mixed-languages-with-tb-and-underline.svg and tb-with-rotate*.svg, where the text still contains Arabic. If the goal is just deterministic CI rendering, prefer keeping the original fonts and adding new vertical-mode tests — or explain in the PR why each swap preserves the original intent.
Also: these files dropped their trailing newline and were fully re-indented (4→6 spaces, attributes rejoined), which buries the one real change. Please revert the reformatting and change only font-family.
3 participants
vhalto the characterẤ, theharfbuzzrendering differs from that of Chrome, resulting in reduced spacing.SourceHanSansHWSC-Regular.subset.ttfto support tests.vhalhalt) to accommodate CJK text.