Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion src/tablemap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,48 @@ export class TableMap {
top: topB,
bottom: bottomB,
} = this.findCell(b);
return {
let rect = {
left: Math.min(leftA, leftB),
top: Math.min(topA, topB),
right: Math.max(rightA, rightB),
bottom: Math.max(bottomA, bottomB),
};

// Expand the rectangle to ensure it's truly rectangular and includes
// all cells that span across its boundaries
let expanded = true;
while (expanded) {
expanded = false;

// Check all cells in the current rectangle
for (let row = rect.top; row < rect.bottom; row++) {
for (let col = rect.left; col < rect.right; col++) {
const index = row * this.width + col;
const cellPos = this.map[index];
const cellRect = this.findCell(cellPos);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is room for performance improvement:

  1. We don't need to check all cells in the current rectangle. We only need to check the cells at the four edges. This can reduce the check numbers from $\text{width} \times \text{height}$ to $O(\text{width} + \text{height})$.
  2. this.findCell() call is slow because it's an $O(\text{width} \times \text{height})$ operation. We should avoid unnecessary this.findCell() calls. One method is to use a seen object to cache all checked points like this example.


// If this cell extends beyond the current rectangle, expand it
if (cellRect.left < rect.left) {
rect.left = cellRect.left;
expanded = true;
}
if (cellRect.right > rect.right) {
rect.right = cellRect.right;
expanded = true;
}
if (cellRect.top < rect.top) {
rect.top = cellRect.top;
expanded = true;
}
if (cellRect.bottom > rect.bottom) {
rect.bottom = cellRect.bottom;
expanded = true;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remove all four expanded = true here, I can still pass all tests. Could you construct a test case that will fail once expanded = true lines are removed?

You can use the following case as an example:

CleanShot.2025-12-27.at.04.13.19.mp4
CleanShot.2025-12-27.at.04.14.20.mp4

}
}
}

return rect;
}

// Return the position of all cells that have the top left corner in
Expand Down
89 changes: 89 additions & 0 deletions test/cellselection-rect.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import ist from 'ist';
import { describe, it } from 'vitest';

import { TableMap } from '../src';

import { table, tr, td, p } from './build';

describe('CellSelection rectangular constraint', () => {
it('should expand selection to include full rowspan cells', () => {
// Table structure:
// | A | B (rowspan=2) | C |
// | D | B | E |
const tableNode = table(
tr(
td(p('A')), // pos 1
td({ rowspan: 2 }, p('B')), // pos 6
td(p('C')), // pos 11
),
Comment on lines +13 to +17
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear what pos 1 means here. I recommend using the comment shown here in the test file.

tr(
td(p('D')), // pos 18
// B continues here
td(p('E')), // pos 23
),
);

const map = TableMap.get(tableNode);

// Select from A (pos=1) to C (pos=11)
// Because B has rowspan=2, the selection should expand to a rectangle
// that includes D and E
const rect = map.rectBetween(1, 11);

console.log('Map:', map.map);
console.log('Rect:', rect);
console.log('Cells in rect:', map.cellsInRect(rect));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove these console.log in the test file


// Expected: selection should include all cells A, B, C, D, E
const cells = map.cellsInRect(rect);

// Verify the selection is rectangular (should include the second row)
ist(rect.top, 0);
ist(rect.bottom, 2); // Should be 2, not 1
ist(rect.left, 0);
ist(rect.right, 3);

// Should include all 5 cells
ist(cells.length, 5);
});

it('should expand selection to include full colspan cells', () => {
// Table structure:
// | A | B | C |
// | D (colspan=2) | E |
// When selecting from A to E, crossing D (colspan=2),
// the selection should be a complete rectangle
const tableNode = table(
tr(
td(p('A')), // pos 1
td(p('B')), // pos 6
td(p('C')), // pos 11
),
tr(
td({ colspan: 2 }, p('D')), // pos 18
td(p('E')), // pos 23
),
);

const map = TableMap.get(tableNode);

// Select from A (pos=1) to E (pos=23)
// Should form a complete rectangle
const rect = map.rectBetween(1, 23);

console.log('Map:', map.map);
console.log('Rect:', rect);
console.log('Cells in rect:', map.cellsInRect(rect));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove these console.log in the test file


const cells = map.cellsInRect(rect);

// Verify the selection is rectangular
ist(rect.top, 0);
ist(rect.bottom, 2);
ist(rect.left, 0);
ist(rect.right, 3);

// Should include all cells
ist(cells.length, 5);
});
});
26 changes: 20 additions & 6 deletions test/cellselection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,16 +108,20 @@ describe('CellSelection.content', () => {
eq,
));

it('cuts off cells sticking out horizontally', () =>
it('expands selection to include cells spanning horizontally', () =>
// With rectangular selection constraint, when a cell spans across the boundary,
// the selection expands to include the entire table
ist(
selectionFor(
table(tr(c11, cAnchor, c(2, 1)), tr(c(4, 1)), tr(c(2, 1), cHead, c11)),
).content(),
slice(table(tr(c11, c11), tr(td({ colspan: 2 }, p())), tr(cEmpty, c11))),
slice(table(tr(c11, c11, c(2, 1)), tr(c(4, 1)), tr(c(2, 1), c11, c11))),
eq,
));

it('cuts off cells sticking out vertically', () =>
it('expands selection to include cells spanning vertically', () =>
// With rectangular selection constraint, the selection expands to include
// cells that span across the selection boundaries
ist(
selectionFor(
table(
Expand All @@ -127,11 +131,15 @@ describe('CellSelection.content', () => {
tr(c11),
),
).content(),
slice(table(tr(c11, td({ rowspan: 2 }, p()), cEmpty), tr(c11, c11))),
slice(
table(tr(c11, c(1, 4), c(1, 2)), tr(c11), tr(c(1, 2), c11), tr(c11)),
),
eq,
));

it('preserves column widths', () =>
it('expands selection to preserve column widths', () =>
// With rectangular selection constraint, when a colspan cell spans the selection,
// the entire row must be included
ist(
selectionFor(
table(
Expand All @@ -140,7 +148,13 @@ describe('CellSelection.content', () => {
tr(c11, cHead, c11),
),
).content(),
slice(table(tr(c11), tr(td({ colwidth: [200] }, p())), tr(c11))),
slice(
table(
tr(c11, c11, c11),
tr(td({ colspan: 3, colwidth: [100, 200, 300] }, p('x'))),
tr(c11, c11, c11),
),
),
eq,
));
});
Expand Down
30 changes: 27 additions & 3 deletions test/commands.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -501,8 +501,26 @@ describe('mergeCells', () => {
it("doesn't do anything when only one cell is selected", () =>
test(table(tr(cAnchor, c11)), mergeCells, null));

it("doesn't do anything when the selection cuts across spanning cells", () =>
test(table(tr(cAnchor, c(2, 1)), tr(c11, cHead, c11)), mergeCells, null));
it('expands selection when it includes spanning cells', () =>
// With rectangular selection constraint, the selection from cAnchor to cHead
// will expand to include the entire table because c(2,1) spans across
test(
table(tr(cAnchor, c(2, 1)), tr(c11, cHead, c11)),
mergeCells,
table(
tr(
td(
{ colspan: 3, rowspan: 2 },
p('x'),
p('x'),
p('x'),
p('x'),
p('x'),
),
),
tr(),
),
));

it('can merge two cells in a column', () =>
test(
Expand Down Expand Up @@ -667,10 +685,16 @@ describe('setCellAttr', () => {
test(table(tr(cCursor, c11)), setCellAttr('test', 'default'), null));

it('will set attributes on all cells covered by a cell selection', () =>
// With rectangular selection constraint, selection expands to include
// the entire first two rows because c(2,1) spans columns 0-1
test(
table(tr(c11, cAnchor, c11), tr(c(2, 1), cHead), tr(c11, c11, c11)),
setCellAttr('test', 'value'),
table(tr(c11, cAttr, cAttr), tr(c(2, 1), cAttr), tr(c11, c11, c11)),
table(
tr(cAttr, cAttr, cAttr),
tr(td({ colspan: 2, test: 'value' }, p('x')), cAttr),
tr(c11, c11, c11),
),
));
});

Expand Down
6 changes: 4 additions & 2 deletions test/tablemap.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,15 @@ describe('TableMap', () => {
});

it('can find the rectangle between two cells', () => {
ist(map.cellsInRect(map.rectBetween(1, 6)).join(', '), '1, 6, 18, 25');
// With rectangular selection constraint, selections must expand to include
// all cells that span across the boundaries
ist(map.cellsInRect(map.rectBetween(1, 6)).join(', '), '1, 6, 11, 18, 25');
ist(map.cellsInRect(map.rectBetween(1, 25)).join(', '), '1, 6, 11, 18, 25');
ist(map.cellsInRect(map.rectBetween(1, 1)).join(', '), '1');
ist(map.cellsInRect(map.rectBetween(6, 25)).join(', '), '6, 11, 18, 25');
ist(map.cellsInRect(map.rectBetween(6, 11)).join(', '), '6, 11, 18');
ist(map.cellsInRect(map.rectBetween(11, 6)).join(', '), '6, 11, 18');
ist(map.cellsInRect(map.rectBetween(18, 25)).join(', '), '18, 25');
ist(map.cellsInRect(map.rectBetween(18, 25)).join(', '), '6, 11, 18, 25');
ist(map.cellsInRect(map.rectBetween(6, 18)).join(', '), '6, 18');
});

Expand Down
Loading