Fix Arabic ligature bidi reordering in textkit

packages/textkit/src/layout/bidiReordering.ts

@@ -31,12 +31,12 @@ const getReorderedIndices = (string: string, segments) => {
   return indices;
 };
 
-const getItemAtIndex = (runs: Run[], objectName: string, index: number) => {
+const getRunItemAtIndex = (runs: Run[], index: number) => {
   for (let i = 0; i < runs.length; i += 1) {
     const run = runs[i];
-    const updatedIndex = run.stringIndices[index - run.start];
     if (index >= run.start && index < run.end) {
-      return run[objectName][updatedIndex];
+      const glyphIndex = run.stringIndices?.[index - run.start];
+      if (glyphIndex !== undefined) return { run, glyphIndex };
     }
   }
 
@@ -60,34 +60,60 @@ const reorderLine = (line: AttributedString) => {
 
   const updatedString = bidi.getReorderedString(line.string, embeddingLevels);
 
-  const updatedRuns = line.runs.map((run) => {
-    const selectedIndices = indices.slice(run.start, run.end);
-    const updatedGlyphs = [];
-    const updatedPositions = [];
-
-    const addedGlyphs = new Set();
-
-    for (let i = 0; i < selectedIndices.length; i += 1) {
-      const index = selectedIndices[i];
-
-      const glyph = getItemAtIndex(line.runs, 'glyphs', index);
-
-      if (addedGlyphs.has(glyph.id)) continue;
+  const updatedRuns: Run[] = [];
+  let currentRun: Run | null = null;
+  let currentStart = 0;
+  let currentGlyphs: NonNullable<Run['glyphs']> = [];
+  let currentGlyphIndices: number[] = [];
+  let currentPositions: NonNullable<Run['positions']> = [];
+  let currentStringIndices: number[] = [];
+  let currentStringIndexByGlyphIndex = new Map<number, number>();
+
+  const flushRun = (end: number) => {
+    if (!currentRun) return;
+
+    updatedRuns.push({
+      ...currentRun,
+      start: currentStart,
+      end,
+      glyphs: currentGlyphs,
+      glyphIndices: currentGlyphIndices,
+      positions: currentPositions,
+      stringIndices: currentStringIndices,
+    });
+  };
+
+  for (let visualIndex = 0; visualIndex < indices.length; visualIndex += 1) {
+    const { run, glyphIndex } = getRunItemAtIndex(
+      line.runs,
+      indices[visualIndex],
+    );
+
+    if (run !== currentRun) {
+      flushRun(visualIndex);
+      currentRun = run;
+      currentStart = visualIndex;
+      currentGlyphs = [];
+      currentGlyphIndices = [];
+      currentPositions = [];
+      currentStringIndices = [];
+      currentStringIndexByGlyphIndex = new Map<number, number>();
+    }
 
-      updatedGlyphs.push(glyph);
-      updatedPositions.push(getItemAtIndex(line.runs, 'positions', index));
+    let reorderedGlyphIndex = currentStringIndexByGlyphIndex.get(glyphIndex);
 
-      if (glyph.isLigature) {
-        addedGlyphs.add(glyph.id);
-      }
+    if (reorderedGlyphIndex === undefined) {
+      reorderedGlyphIndex = currentGlyphs.length;
+      currentStringIndexByGlyphIndex.set(glyphIndex, reorderedGlyphIndex);
+      currentGlyphs.push(run.glyphs![glyphIndex]);
+      currentGlyphIndices.push(visualIndex - currentStart);
+      currentPositions.push(run.positions![glyphIndex]);
     }
 
-    return {
-      ...run,
-      glyphs: updatedGlyphs,
-      positions: updatedPositions,
-    };
-  });
+    currentStringIndices.push(reorderedGlyphIndex);
+  }
+
+  flushRun(indices.length);
 
   return {
     box: line.box,

packages/textkit/tests/layout/bidiReordering.test.ts

@@ -13,6 +13,14 @@ const initializeToIndex = (size: number) => {
   return arr;
 };
 
+const createPositions = (size: number) =>
+  new Array(size).fill(null).map(() => ({
+    xAdvance: 0,
+    yAdvance: 0,
+    xOffset: 0,
+    yOffset: 0,
+  }));
+
 describe('bidiReordering', () => {
   test('should return reversed string', () => {
     const word = 'Lorem';
@@ -146,4 +154,124 @@ describe('bidiReordering', () => {
 
     expect(result[0][0].string).toBe('eroL');
   });
+
+  test('should keep repeated ligature glyph ids from different clusters', () => {
+    const string = {
+      string: 'abcdef',
+      runs: [
+        {
+          attributes: {
+            direction: 'rtl' as const,
+            bidiLevel: 1,
+          },
+          start: 0,
+          end: 6,
+          glyphs: [
+            { id: 1, advanceWidth: 10, codePoints: [0x61] },
+            {
+              id: 9,
+              advanceWidth: 10,
+              codePoints: [0x62, 0x63],
+              isLigature: true,
+            },
+            { id: 2, advanceWidth: 10, codePoints: [0x64] },
+            {
+              id: 9,
+              advanceWidth: 10,
+              codePoints: [0x65, 0x66],
+              isLigature: true,
+            },
+          ] as Glyph[],
+          positions: createPositions(4),
+          stringIndices: [0, 1, 1, 2, 3, 3],
+          glyphIndices: [0, 1, 3, 4],
+        },
+      ],
+    };
+
+    const result = bidiReorderingInstance([[string]]);
+    const run = result[0][0].runs[0];
+
+    expect(result[0][0].string).toBe('fedcba');
+    expect(run.glyphs!.map((glyph) => glyph.id)).toEqual([9, 2, 9, 1]);
+    expect(run.stringIndices).toEqual([0, 0, 1, 2, 2, 3]);
+    expect(run.glyphIndices).toEqual([0, 2, 3, 5]);
+  });
+
+  test('should keep ligature clusters in their owning run after mixed bidi reordering', () => {
+    const string = {
+      string: 'الشركة (saudi)',
+      runs: [
+        {
+          attributes: {
+            direction: 'rtl' as const,
+            bidiLevel: 1,
+          },
+          start: 0,
+          end: 8,
+          glyphs: [
+            { id: 1, advanceWidth: 10, codePoints: [0x627] },
+            { id: 2, advanceWidth: 10, codePoints: [0x644] },
+            {
+              id: 29,
+              advanceWidth: 10,
+              codePoints: [0x634, 0x631],
+              isLigature: true,
+            },
+            { id: 4, advanceWidth: 10, codePoints: [0x643] },
+            { id: 5, advanceWidth: 10, codePoints: [0x629] },
+            { id: 6, advanceWidth: 10, codePoints: [0x20] },
+            { id: 7, advanceWidth: 10, codePoints: [0x28] },
+          ] as Glyph[],
+          positions: createPositions(7),
+          stringIndices: [0, 1, 2, 2, 3, 4, 5, 6],
+          glyphIndices: [0, 1, 2, 4, 5, 6, 7],
+        },
+        {
+          attributes: {
+            direction: 'rtl' as const,
+            bidiLevel: 2,
+          },
+          start: 8,
+          end: 13,
+          glyphs: [
+            { id: 8, advanceWidth: 10, codePoints: [0x73] },
+            { id: 9, advanceWidth: 10, codePoints: [0x61] },
+            { id: 10, advanceWidth: 10, codePoints: [0x75] },
+            { id: 11, advanceWidth: 10, codePoints: [0x64] },
+            { id: 12, advanceWidth: 10, codePoints: [0x69] },
+          ] as Glyph[],
+          positions: createPositions(5),
+          stringIndices: initializeToIndex(5),
+          glyphIndices: initializeToIndex(5),
+        },
+        {
+          attributes: {
+            direction: 'rtl' as const,
+            bidiLevel: 1,
+          },
+          start: 13,
+          end: 14,
+          glyphs: [
+            { id: 13, advanceWidth: 10, codePoints: [0x29] },
+          ] as Glyph[],
+          positions: createPositions(1),
+          stringIndices: [0],
+          glyphIndices: [0],
+        },
+      ],
+    };
+
+    const result = bidiReorderingInstance([[string]]);
+    const line = result[0][0];
+
+    expect(line.string).toBe('(saudi) ةكرشلا');
+    expect(
+      line.runs.map((run) => line.string.slice(run.start, run.end)),
+    ).toEqual(['(', 'saudi', ') ةكرشلا']);
+    expect(line.runs[2].glyphs!.map((glyph) => glyph.id)).toEqual([
+      7, 6, 5, 4, 29, 2, 1,
+    ]);
+    expect(line.runs[2].stringIndices).toEqual([0, 1, 2, 3, 4, 4, 5, 6]);
+  });
 });