Respect RTL cell layouting order in grid layout (#6232)

Co-authored-by: PgBiel <9021226+PgBiel@users.noreply.github.com>

crates/typst-layout/src/grid/layouter.rs

@@ -11,7 +11,7 @@ use typst_library::layout::{
 use typst_library::text::TextElem;
 use typst_library::visualize::Geometry;
 use typst_syntax::Span;
-use typst_utils::{MaybeReverseIter, Numeric};
+use typst_utils::Numeric;
 
 use super::{
     generate_line_segments, hline_stroke_at_column, layout_cell, vline_stroke_at_row,
@@ -574,7 +574,7 @@ impl<'a> GridLayouter<'a> {
 
             // Reverse with RTL so that later columns start first.
             let mut dx = Abs::zero();
-            for (x, &col) in self.rcols.iter().enumerate().rev_if(self.is_rtl) {
+            for (x, &col) in self.rcols.iter().enumerate() {
                 let mut dy = Abs::zero();
                 for row in rows {
                     // We want to only draw the fill starting at the parent
@@ -643,18 +643,13 @@ impl<'a> GridLayouter<'a> {
                                     .sum()
                             };
                             let width = self.cell_spanned_width(cell, x);
-                            // In the grid, cell colspans expand to the right,
+                            let mut pos = Point::new(dx, dy);
-                            // so we're at the leftmost (lowest 'x') column
+                            if self.is_rtl {
-                            // spanned by the cell. However, in RTL, cells
+                                // In RTL cells expand to the left, thus the
-                            // expand to the left. Therefore, without the
+                                // position must additionally be offset by the
-                            // offset below, cell fills would start at the
+                                // cell's width.
-                            // rightmost visual position of a cell and extend
+                                pos.x = self.width - (dx + width);
-                            // over to unrelated columns to the right in RTL.
+                            }
-                            // We avoid this by ensuring the fill starts at the
-                            // very left of the cell, even with colspan > 1.
-                            let offset =
-                                if self.is_rtl { -width + col } else { Abs::zero() };
-                            let pos = Point::new(dx + offset, dy);
                             let size = Size::new(width, height);
                             let rect = Geometry::Rect(size).filled(fill);
                             fills.push((pos, FrameItem::Shape(rect, self.span)));
@@ -1236,10 +1231,9 @@ impl<'a> GridLayouter<'a> {
         }
 
         let mut output = Frame::soft(Size::new(self.width, height));
-        let mut pos = Point::zero();
+        let mut offset = Point::zero();
 
-        // Reverse the column order when using RTL.
+        for (x, &rcol) in self.rcols.iter().enumerate() {
-        for (x, &rcol) in self.rcols.iter().enumerate().rev_if(self.is_rtl) {
             if let Some(cell) = self.grid.cell(x, y) {
                 // Rowspans have a separate layout step
                 if cell.rowspan.get() == 1 {
@@ -1257,25 +1251,17 @@ impl<'a> GridLayouter<'a> {
                     let frame =
                         layout_cell(cell, engine, disambiguator, self.styles, pod)?
                             .into_frame();
-                    let mut pos = pos;
+                    let mut pos = offset;
                     if self.is_rtl {
-                        // In the grid, cell colspans expand to the right,
+                        // In RTL cells expand to the left, thus the position
-                        // so we're at the leftmost (lowest 'x') column
+                        // must additionally be offset by the cell's width.
-                        // spanned by the cell. However, in RTL, cells
+                        pos.x = self.width - (pos.x + width);
-                        // expand to the left. Therefore, without the
-                        // offset below, the cell's contents would be laid out
-                        // starting at its rightmost visual position and extend
-                        // over to unrelated cells to its right in RTL.
-                        // We avoid this by ensuring the rendered cell starts at
-                        // the very left of the cell, even with colspan > 1.
-                        let offset = -width + rcol;
-                        pos.x += offset;
                     }
                     output.push_frame(pos, frame);
                 }
             }
 
-            pos.x += rcol;
+            offset.x += rcol;
         }
 
         Ok(output)
@@ -1302,8 +1288,8 @@ impl<'a> GridLayouter<'a> {
         pod.backlog = &heights[1..];
 
         // Layout the row.
-        let mut pos = Point::zero();
+        let mut offset = Point::zero();
-        for (x, &rcol) in self.rcols.iter().enumerate().rev_if(self.is_rtl) {
+        for (x, &rcol) in self.rcols.iter().enumerate() {
             if let Some(cell) = self.grid.cell(x, y) {
                 // Rowspans have a separate layout step
                 if cell.rowspan.get() == 1 {
@@ -1314,17 +1300,19 @@ impl<'a> GridLayouter<'a> {
                     let fragment =
                         layout_cell(cell, engine, disambiguator, self.styles, pod)?;
                     for (output, frame) in outputs.iter_mut().zip(fragment) {
-                        let mut pos = pos;
+                        let mut pos = offset;
                         if self.is_rtl {
-                            let offset = -width + rcol;
+                            // In RTL cells expand to the left, thus the
-                            pos.x += offset;
+                            // position must additionally be offset by the
+                            // cell's width.
+                            pos.x = self.width - (offset.x + width);
                         }
                         output.push_frame(pos, frame);
                     }
                 }
             }
 
-            pos.x += rcol;
+            offset.x += rcol;
         }
 
         Ok(Fragment::frames(outputs))

crates/typst-layout/src/grid/rowspans.rs

@@ -3,7 +3,6 @@ use typst_library::engine::Engine;
 use typst_library::foundations::Resolve;
 use typst_library::layout::grid::resolve::Repeatable;
 use typst_library::layout::{Abs, Axes, Frame, Point, Region, Regions, Size, Sizing};
-use typst_utils::MaybeReverseIter;
 
 use super::layouter::{in_last_with_offset, points, Row, RowPiece};
 use super::{layout_cell, Cell, GridLayouter};
@@ -23,6 +22,10 @@ pub struct Rowspan {
     /// specified for the parent cell's `breakable` field.
     pub is_effectively_unbreakable: bool,
     /// The horizontal offset of this rowspan in all regions.
+    ///
+    /// This is the offset from the text direction start, meaning that, on RTL
+    /// grids, this is the offset from the right of the grid, whereas, on LTR
+    /// grids, it is the offset from the left.
     pub dx: Abs,
     /// The vertical offset of this rowspan in the first region.
     pub dy: Abs,
@@ -118,10 +121,11 @@ impl GridLayouter<'_> {
             // Nothing to layout.
             return Ok(());
         };
-        let first_column = self.rcols[x];
         let cell = self.grid.cell(x, y).unwrap();
         let width = self.cell_spanned_width(cell, x);
-        let dx = if self.is_rtl { dx - width + first_column } else { dx };
+        // In RTL cells expand to the left, thus the position
+        // must additionally be offset by the cell's width.
+        let dx = if self.is_rtl { self.width - (dx + width) } else { dx };
 
         // Prepare regions.
         let size = Size::new(width, *first_height);
@@ -185,10 +189,8 @@ impl GridLayouter<'_> {
     /// Checks if a row contains the beginning of one or more rowspan cells.
     /// If so, adds them to the rowspans vector.
     pub fn check_for_rowspans(&mut self, disambiguator: usize, y: usize) {
-        // We will compute the horizontal offset of each rowspan in advance.
+        let offsets = points(self.rcols.iter().copied());
-        // For that reason, we must reverse the column order when using RTL.
+        for (x, dx) in (0..self.rcols.len()).zip(offsets) {
-        let offsets = points(self.rcols.iter().copied().rev_if(self.is_rtl));
-        for (x, dx) in (0..self.rcols.len()).rev_if(self.is_rtl).zip(offsets) {
             let Some(cell) = self.grid.cell(x, y) else {
                 continue;
             };

tests/suite/layout/grid/rtl.typ

@@ -193,3 +193,143 @@
   ),
   ..range(0, 10).map(i => ([\##i], table.cell(stroke: green)[123], table.cell(stroke: blue)[456], [789], [?], table.hline(start: 4, end: 5, stroke: red))).flatten()
 )
+
+--- grid-rtl-counter ---
+// Test interaction between RTL and counters
+#set text(dir: rtl)
+#let test = counter("test")
+#grid(
+  columns: (1fr, 1fr),
+  inset: 5pt,
+  align: center,
+  [
+    a: // should produce 1
+    #test.step()
+    #context test.get().first()
+  ],
+  [
+    b: // should produce 2
+    #test.step()
+    #context test.get().first()
+  ],
+)
+
+--- grid-rtl-rowspan-counter-equal ---
+// Test interaction between RTL and counters
+#set text(dir: rtl)
+#let test = counter("test")
+#grid(
+  columns: (1fr, 1fr),
+  inset: 5pt,
+  align: center,
+  grid.cell(rowspan: 2, [
+    a: // should produce 1
+    #test.step()
+    #context test.get().first()
+  ]),
+  grid.cell(rowspan: 2, [
+    b: // should produce 2
+    #test.step()
+    #context test.get().first()
+  ]),
+)
+
+--- grid-rtl-rowspan-counter-unequal-1 ---
+// Test interaction between RTL and counters
+#set text(dir: rtl)
+#let test = counter("test")
+#grid(
+  columns: (1fr, 1fr),
+  inset: 5pt,
+  align: center,
+  grid.cell(rowspan: 5, [
+    b: // will produce 2
+    #test.step()
+    #context test.get().first()
+  ]),
+  grid.cell(rowspan: 2, [
+    a: // will produce 1
+    #test.step()
+    #context test.get().first()
+  ]),
+  grid.cell(rowspan: 3, [
+    c: // will produce 3
+    #test.step()
+    #context test.get().first()
+  ]),
+)
+
+--- grid-rtl-rowspan-counter-unequal-2 ---
+// Test interaction between RTL and counters
+#set text(dir: rtl)
+#let test = counter("test")
+#grid(
+  columns: (1fr, 1fr),
+  inset: 5pt,
+  align: center,
+  grid.cell(rowspan: 2, [
+    a: // will produce 1
+    #test.step()
+    #context test.get().first()
+  ]),
+  grid.cell(rowspan: 5, [
+    b: // will produce 2
+    #test.step()
+    #context test.get().first()
+  ]),
+  grid.cell(rowspan: 3, [
+    c: // will produce 3
+    #test.step()
+    #context test.get().first()
+  ]),
+)
+
+--- grid-rtl-rowspan-counter-mixed-1 ---
+// Test interaction between RTL and counters
+#set text(dir: rtl)
+#let test = counter("test")
+#grid(
+  columns: (1fr, 1fr),
+  inset: 5pt,
+  align: center,
+  [
+    a: // will produce 1
+    #test.step()
+    #context test.get().first()
+  ],
+  grid.cell(rowspan: 2, [
+    b: // will produce 2
+    #test.step()
+    #context test.get().first()
+  ]),
+  [
+    c: // will produce 3
+    #test.step()
+    #context test.get().first()
+  ],
+)
+
+--- grid-rtl-rowspan-counter-mixed-2 ---
+// Test interaction between RTL and counters
+#set text(dir: rtl)
+#let test = counter("test")
+#grid(
+  columns: (1fr, 1fr),
+  inset: 5pt,
+  align: center,
+  grid.cell(rowspan: 2, [
+    b: // will produce 2
+    #test.step()
+    #context test.get().first()
+  ]),
+  [
+    a: // will produce 1
+    #test.step()
+    #context test.get().first()
+  ],
+  [
+    c: // will produce 3
+    #test.step()
+    #context test.get().first()
+  ]
+)