shaping: (wrapping) fix space aware advance for bidi

When wrapping lines containing bidi runs, a run that progresses in the opposite direction from the overall paragraph will have its trailing whitespace in the middle of the line, not at the end. As such, it isn't valid to ignore the advance of trailing whitespace for such a run. We can only safely ignore trailing whitespace when the run is the same progression as the paragraph. Signed-off-by: Chris Waldon <[email protected]>
go-text · Dec 17, 2024 · e1733d2 · e1733d2
1 parent 958a5bf
commit e1733d2
Show file tree

Hide file tree

Showing 3 changed files with 168 additions and 3 deletions.
diff --git a/shaping/output.go b/shaping/output.go
@@ -184,11 +184,14 @@ func (o *Output) RecomputeAdvance() {
 // advanceSpaceAware adjust the value in [Advance]
 // if a white space character ends the run.
 // Any end letter spacing (on the last glyph) is also removed
+// The paragraphDir is the text direction of the overall paragraph containing o.
+// If the paragraphDir is different then o's Direction, this method has no effect
+// because the trailing space in this run will always be internal to the paragraph.
 //
 // TODO: should we take into account multiple spaces ?
-func (o *Output) advanceSpaceAware() fixed.Int26_6 {
+func (o *Output) advanceSpaceAware(paragraphDir di.Direction) fixed.Int26_6 {
 	L := len(o.Glyphs)
-	if L == 0 {
+	if L == 0 || paragraphDir != o.Direction {
 		return o.Advance
 	}
 

diff --git a/shaping/output_test.go b/shaping/output_test.go
@@ -305,3 +305,165 @@ func TestLine_AdjustBaseline(t *testing.T) {
 		}
 	}
 }
+
+func TestAdvanceSpaceAware(t *testing.T) {
+	type testcase struct {
+		name         string
+		paragraphDir di.Direction
+		run          Output
+		expected     fixed.Int26_6
+	}
+	for _, tc := range []testcase{
+		{
+			name:         "matching ltr no whitespace",
+			paragraphDir: di.DirectionLTR,
+			expected:     10,
+			run: Output{
+				Advance: 10,
+				Glyphs: []Glyph{
+					{
+						Width:      10,
+						XAdvance:   10,
+						RuneCount:  1,
+						GlyphCount: 1,
+					},
+				},
+				Direction: di.DirectionLTR,
+				Runes:     Range{Count: 1},
+			},
+		},
+		{
+			name:         "matching ltr with whitespace",
+			expected:     0,
+			paragraphDir: di.DirectionLTR,
+			run: Output{
+				Advance: 10,
+				Glyphs: []Glyph{
+					{
+						Width:      0,
+						XAdvance:   10,
+						RuneCount:  1,
+						GlyphCount: 1,
+					},
+				},
+				Direction: di.DirectionLTR,
+				Runes:     Range{Count: 1},
+			},
+		},
+		{
+			name:         "matching rtl no whitespace",
+			expected:     10,
+			paragraphDir: di.DirectionRTL,
+			run: Output{
+				Advance: 10,
+				Glyphs: []Glyph{
+					{
+						Width:      10,
+						XAdvance:   10,
+						RuneCount:  1,
+						GlyphCount: 1,
+					},
+				},
+				Direction: di.DirectionRTL,
+				Runes:     Range{Count: 1},
+			},
+		},
+		{
+			name:         "matching rtl with whitespace",
+			expected:     0,
+			paragraphDir: di.DirectionRTL,
+			run: Output{
+				Advance: 10,
+				Glyphs: []Glyph{
+					{
+						Width:      0,
+						XAdvance:   10,
+						RuneCount:  1,
+						GlyphCount: 1,
+					},
+				},
+				Direction: di.DirectionRTL,
+				Runes:     Range{Count: 1},
+			},
+		},
+		{
+			name:         "mismatched ltr no whitespace",
+			expected:     10,
+			paragraphDir: di.DirectionLTR,
+			run: Output{
+				Advance: 10,
+				Glyphs: []Glyph{
+					{
+						Width:      10,
+						XAdvance:   10,
+						RuneCount:  1,
+						GlyphCount: 1,
+					},
+				},
+				Direction: di.DirectionRTL,
+				Runes:     Range{Count: 1},
+			},
+		},
+		{
+			name:         "mismatched ltr with whitespace",
+			expected:     10,
+			paragraphDir: di.DirectionLTR,
+			run: Output{
+				Advance: 10,
+				Glyphs: []Glyph{
+					{
+						Width:      0,
+						XAdvance:   10,
+						RuneCount:  1,
+						GlyphCount: 1,
+					},
+				},
+				Direction: di.DirectionRTL,
+				Runes:     Range{Count: 1},
+			},
+		},
+		{
+			name:         "mismatched rtl no whitespace",
+			expected:     10,
+			paragraphDir: di.DirectionRTL,
+			run: Output{
+				Advance: 10,
+				Glyphs: []Glyph{
+					{
+						Width:      10,
+						XAdvance:   10,
+						RuneCount:  1,
+						GlyphCount: 1,
+					},
+				},
+				Direction: di.DirectionLTR,
+				Runes:     Range{Count: 1},
+			},
+		},
+		{
+			name:         "mismatched rtl with whitespace",
+			expected:     10,
+			paragraphDir: di.DirectionRTL,
+			run: Output{
+				Advance: 10,
+				Glyphs: []Glyph{
+					{
+						Width:      0,
+						XAdvance:   10,
+						RuneCount:  1,
+						GlyphCount: 1,
+					},
+				},
+				Direction: di.DirectionLTR,
+				Runes:     Range{Count: 1},
+			},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			actual := tc.run.advanceSpaceAware(tc.paragraphDir)
+			if actual != tc.expected {
+				t.Errorf("expected advance %d, got %d", tc.expected, actual)
+			}
+		})
+	}
+}
diff --git a/shaping/wrapping.go b/shaping/wrapping.go
@@ -1165,7 +1165,7 @@ func (l *LineWrapper) processBreakOption(option breakOption, config lineConfig)
 	}
 	isFirstInLine := l.scratch.candidateLen() == 0
 	candidateRun := cutRun(run, l.mapper.mapping, l.lineStartRune, option.breakAtRune, isFirstInLine)
-	candidateLineWidth := (candidateRun.advanceSpaceAware() + l.scratch.candidateAdvance()).Ceil()
+	candidateLineWidth := (candidateRun.advanceSpaceAware(l.config.Direction) + l.scratch.candidateAdvance()).Ceil()
 	if candidateLineWidth > config.maxWidth {
 		// The run doesn't fit on the line.
 		if !l.scratch.hasBest() {