fix: ensure cell selections are always rectangular#330
fix: ensure cell selections are always rectangular#330liwenka1 wants to merge 4 commits intoProseMirror:masterfrom
Conversation
- Modified TableMap.rectBetween to automatically expand selection rectangles - Selections now include all cells that span across the boundary - Prevents T-shaped or L-shaped selections when cells have colspan/rowspan - Added tests to verify rectangular selection constraint - Updated existing tests to reflect the new behavior This change aligns with the behavior of mature table editors like Feishu and Microsoft Office, where table selections are always rectangular.
commit: |
Coverage Report
File Coverage |
| tr( | ||
| td(p('A')), // pos 1 | ||
| td({ rowspan: 2 }, p('B')), // pos 6 | ||
| td(p('C')), // pos 11 | ||
| ), |
There was a problem hiding this comment.
It's unclear what pos 1 means here. I recommend using the comment shown here in the test file.
test/cellselection-rect.test.ts
Outdated
| console.log('Map:', map.map); | ||
| console.log('Rect:', rect); | ||
| console.log('Cells in rect:', map.cellsInRect(rect)); |
There was a problem hiding this comment.
Let's remove these console.log in the test file
test/cellselection-rect.test.ts
Outdated
| console.log('Map:', map.map); | ||
| console.log('Rect:', rect); | ||
| console.log('Cells in rect:', map.cellsInRect(rect)); |
There was a problem hiding this comment.
Let's remove these console.log in the test file
src/tablemap.ts
Outdated
| 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; | ||
| } |
There was a problem hiding this comment.
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
src/tablemap.ts
Outdated
| 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); |
There was a problem hiding this comment.
I believe there is room for performance improvement:
- 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})$ . -
this.findCell()call is slow because it's an$O(\text{width} \times \text{height})$ operation. We should avoid unnecessarythis.findCell()calls. One method is to use aseenobject to cache all checked points like this example.
|
This is a great improvement! I left some review comments. |
- Optimize cell checking from O(width×height) to O(width+height) per iteration - Add seen cache to avoid redundant findCell() calls - Update position comments to use standard /* pos */ format - Remove console.log statements from tests - Add test case for complex rowspan/colspan expansion validation Performance improvement: For a 10×10 table, reduced from 100 cells checked to 40 cells per iteration (60% reduction). The optimization maintains correctness by checking only edge cells, as interior cells cannot cause rectangle expansion.
|
Thank you @ocavue for the thorough review and valuable suggestions! I've addressed all the feedback points |
|
Quick context: This wasn't a bug but a feature. forcing a rectangular selection will make some UX behaviours quite difficult to implement like: "change the background color for an entire row" |
That's a good point. I just checked Excel's behavior, and it does provide different behaviors:
CleanShot.2025-12-28.at.00.13.59.mov |
|
Perhaps we can add a new option to |
|
Thanks for the valuable feedback! I've implemented the Changes:
Behavior:
This matches Excel's behavior where drag selections are rectangular, but row/column header selections can include partial merged cells. Please review when you have time. Thanks! |
3 participants
Problem
When selecting table cells with
colspanorrowspanattributes, the current implementation allows non-rectangular selections (T-shaped, L-shaped, etc.). This behavior is inconsistent with mature table editors and can lead to unexpected results.Example Issue
When selecting from cell A to cell C in the following table:
Before Fix: The selection would be T-shaped, only including cells A, B, and C.
After Fix: The selection automatically expands to include all cells (A, B, C, D, E), forming a complete rectangle.
Before Fix
The selection creates a non-rectangular shape when crossing cells with rowspan/colspan.
After Fix
The selection automatically expands to ensure a rectangular shape, including all affected cells.
Industry Standards
This fix aligns with the behavior of professional table editors:
Feishu (Lark) Table Selection
Feishu enforces rectangular selections in tables.
Microsoft Office Table Selection
Microsoft Word/Excel also maintains rectangular cell selections.
Implementation Details
Core Changes
Modified
TableMap.rectBetween()insrc/tablemap.ts:colspanorrowspan), the rectangle expands to include itAlgorithm
Test Coverage
test/cellselection-rect.test.tswith specific tests for rectangular constraintstest/tablemap.test.tsto reflect newrectBetweenbehaviortest/cellselection.test.ts- selections now expand instead of cutting off cellstest/commands.test.ts- commands now operate on expanded rectangular selectionsAll 181 tests pass ✅
Benefits
Breaking Changes
However, this change makes the behavior more intuitive and consistent with industry standards.
Migration Guide
If you have code that depends on the old behavior:
Before:
After:
No API changes are required - the expansion happens automatically in the
CellSelectionconstructor.Related Issues
Fixes issue where table selections could create non-rectangular shapes when cells have colspan or rowspan attributes.
Checklist: