Fix visual order of nested bidi embeddings

bidi/bidi.go

@@ -47,7 +47,7 @@ type Paragraph struct {
 	runsEnd []int
 }
 
-// Run is a slice of text with a constant direction.
+// Run is a slice of text with a constant embedding level and direction.
 type Run struct {
 	// Start and End indicate the subslice of the input text : text[Start:End]
 	Start, End int
@@ -142,18 +142,11 @@ func (p *Paragraph) segment(defaultDirection Direction) Runs {
 
 // depends only on [resultLevels]
 func (p *Paragraph) buildRuns() Runs {
-	var isRTL bool
-
-	// lvl = 0,2,4,...: left to right
-	// lvl = 1,3,5,...: right to left
+	// Even levels are left to right and odd levels are right to left.
+	// Keep the exact levels. Reordering also depends on embedding depth.
 	for i, lvl := range p.resultLevels {
-		curIsRTL := lvl%2 != 0
-		if i == 0 {
-			isRTL = curIsRTL
-		} else if curIsRTL != isRTL {
-			// close the current run
+		if i != 0 && lvl != p.resultLevels[i-1] {
 			p.runsEnd = append(p.runsEnd, i)
-			isRTL = curIsRTL
 		}
 	}
 	// close the last run

bidi/bidi_test.go

@@ -580,3 +580,17 @@ func BenchmarkPathological(b *testing.B) {
 		}
 	}
 }
+
+func TestRunsPreserveEmbeddingLevels(t *testing.T) {
+	var p Paragraph
+	runs := p.SegmentString("a\u202abא\u202cב", LeftToRight)
+	want := []Run{{0, 2, 0}, {2, 3, 2}, {3, 5, 3}, {5, 6, 1}}
+	if runs.NumRuns() != len(want) {
+		t.Fatalf("got %d runs, want %d", runs.NumRuns(), len(want))
+	}
+	for i, expected := range want {
+		if got := runs.Run(i); got != expected {
+			t.Errorf("run %d: got %+v, want %+v", i, got, expected)
+		}
+	}
+}

shaping/wrapping.go

@@ -901,13 +901,15 @@ type WrappedLine struct {
 	TrimmedTrailingWhitespace fixed.Int26_6
 }
 
-// swapVisualOrder inverts the visual index of runs in [subline],
-// by swapping pairs of visual indices across the midpoint of the slice.
-func swapVisualOrder(subline Line) {
-	L := len(subline)
-	for i := range subline[0 : L/2] {
-		j := (L - i) - 1
-		subline[i].VisualIndex, subline[j].VisualIndex = subline[j].VisualIndex, subline[i].VisualIndex
+// reverseVisualOrder reflects the visual indices of line[start:end].
+// Reversals at higher levels only permute within a sequence, so the runs
+// in the range hold the visual positions start..end-1 in some order and a
+// reflection reverses them whatever that order is. Swapping the indices
+// of line[i] and line[end-1-i] instead is wrong once an inner level has
+// moved them.
+func reverseVisualOrder(line Line, start, end int) {
+	for i := start; i < end; i++ {
+		line[i].VisualIndex = int32(start+end-1) - line[i].VisualIndex
 	}
 }
 
@@ -942,13 +944,13 @@ func computeBidiOrdering(finalLine Line) {
 					contiguousStart = i
 				} // else : add to contiguous
 			} else if contiguousStart != -1 { // we are just past the sequence
-				swapVisualOrder(finalLine[contiguousStart:i]) // swap ...
-				contiguousStart = -1                          // ... and reset
+				reverseVisualOrder(finalLine, contiguousStart, i)
+				contiguousStart = -1
 			}
 		}
 		// handle the sequence if required
 		if contiguousStart != -1 {
-			swapVisualOrder(finalLine[contiguousStart:])
+			reverseVisualOrder(finalLine, contiguousStart, len(finalLine))
 		}
 	}
 }

shaping/wrapping_test.go

@@ -10,6 +10,7 @@ import (
 	"testing"
 	"testing/quick"
 
+	"github.com/go-text/typesetting/bidi"
 	"github.com/go-text/typesetting/di"
 	"github.com/go-text/typesetting/font"
 	"github.com/go-text/typesetting/language"
@@ -3509,7 +3510,7 @@ func TestComputeBidiOrdering(t *testing.T) {
 				{Level: 1},
 				{Level: 0},
 			},
-			expectedVisualOrder: []int{0, 4, 5, 3, 2, 1, 6},
+			expectedVisualOrder: []int{0, 5, 4, 3, 1, 2, 6},
 		},
 		{
 			name: "bidi example 3",
@@ -3543,7 +3544,7 @@ func TestComputeBidiOrdering(t *testing.T) {
 				{Level: 1}, // 10
 				{Level: 1}, // 11
 			},
-			expectedVisualOrder: []int{11, 10, 2, 3, 7, 8, 6, 5, 4, 9, 1, 0},
+			expectedVisualOrder: []int{11, 10, 2, 3, 8, 7, 6, 4, 5, 9, 1, 0},
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
@@ -3684,3 +3685,26 @@ func TestWrapping_oneLine_overflow_bug(t *testing.T) {
 	_, done := l.WrapNextLine(maxWidth)
 	tu.Assert(t, done)
 }
+
+func TestComputeBidiOrderingLevels(t *testing.T) {
+	for _, tc := range []struct {
+		levels []bidi.Level
+		want   []int32
+	}{
+		{[]bidi.Level{0, 1, 2, 2, 1, 1, 0}, []int32{0, 5, 3, 4, 2, 1, 6}},
+		{[]bidi.Level{0, 2, 2, 0}, []int32{0, 1, 2, 3}},
+		{[]bidi.Level{1, 3, 3, 1}, []int32{3, 2, 1, 0}},
+		{[]bidi.Level{2, 2, 2}, []int32{0, 1, 2}},
+	} {
+		line := make(Line, len(tc.levels))
+		for i, level := range tc.levels {
+			line[i].Level = level
+		}
+		computeBidiOrdering(line)
+		for i, run := range line {
+			if run.VisualIndex != tc.want[i] {
+				t.Errorf("levels %v: logical run %d has visual index %d, want %d", tc.levels, i, run.VisualIndex, tc.want[i])
+			}
+		}
+	}
+}