fix: improve cell selection with fallback strategy for merged cells

[liwenka1]

📋 Summary

Improves cell selection logic for merged cells (rowspan/colspan) by implementing a robust fallback strategy in the cellUnderMouse function.

🐛 Problem

The previous fix (#311) improved cell selection for row-merged cells (rowspan) by preferring mousePos.inside over mousePos.pos. However, this introduced a regression where column-merged cells became harder to select in certain regions, particularly when clicking in the lower half of the merged area.

Root cause: The previous code used a simple either-or approach:

const pos = mousePos.inside >= 0 ? mousePos.inside : mousePos.pos;
return cellAround(view.state.doc.resolve(pos));

If inside >= 0, it would always use inside position. But if cellAround() returns null for that position (which can happen in edge cases), the function would return null without trying the pos position as a fallback.

✅ Solution

Implement a two-step fallback strategy:

1. First attempt: Try mousePos.inside when available (inside >= 0)

   • If cellAround() successfully finds a cell, return it immediately
   • This handles rowspan cases well

2. Fallback: If the first attempt fails or inside < 0, try mousePos.pos

   • This ensures we don't give up too early
   • Handles colspan and edge cases

New code:

if (mousePos.inside >= 0) {
  const $cell = cellAround(view.state.doc.resolve(mousePos.inside));
  if ($cell) return $cell;
}
return cellAround(view.state.doc.resolve(mousePos.pos));

🎯 Benefits

• ✅ Maintains the rowspan fix from fix: improve cell selection logic in merge cells #311
• ✅ Resolves colspan selection issues
• ✅ More robust handling of edge cases
• ✅ Cleaner code with explicit fallback logic
• ✅ No performance impact (same number of operations in worst case)

🧪 Testing

Manually tested with:

• Tables with rowspan cells (vertical merge)
• Tables with colspan cells (horizontal merge)
• Complex tables with both rowspan and colspan
• Clicking in various regions of merged cells (top, middle, bottom, edges)

All selection scenarios now work correctly.

📚 Related

• Builds upon: fix: improve cell selection logic in merge cells #311 (fix: improve cell selection logic in merge cells)
• Fixes regression introduced in the previous fix

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/ProseMirror/prosemirror-tables@323

commit: 0b909bd

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	58.99%	931 / 1578
🔵	Statements	58.89%	1086 / 1844
🔵	Functions	53.03%	105 / 198
🔵	Branches	52.17%	612 / 1173

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
src/input.ts	23.48%	23.85%	35.71%	25.45%	54, 64, 67-71, 73, 75, 86-89, 97-248, 254, 259, 270-296
Unchanged Files
src/cellselection.ts	67.98%	60.44%	56%	69.89%	73, 131, 152-154, 184-203, 253, 263-266, 311-313, 322-324, 330-341, 353, 366-441, 465-468
src/columnresizing.ts	3.42%	0.77%	4%	3.82%	63-385, 398-430
src/commands.ts	78.4%	70.56%	74.35%	80.66%	109, 175, 179, 187, 258, 275, 339, 343, 351, 388, 437, 501, 509, 570, 596, 690, 783-988
src/copypaste.ts	98.36%	91.48%	100%	98.7%	39, 333, 345
src/fixtables.ts	74.44%	70.21%	75%	77.61%	34-49, 73, 97, 109, 116
src/index.ts	63.63%	35.71%	80%	66.66%	112, 114-129
src/schema.ts	46.15%	23.68%	41.66%	47.91%	15-37, 42, 43, 45, 48, 122-130, 165, 182, 194-197
src/tablemap.ts	91.7%	93.22%	87.5%	91.13%	62-72, 140, 150, 238, 355
src/tableview.ts	0%	0%	0%	0%	16-100
src/util.ts	74.28%	79.16%	73.33%	73.68%	38, 47, 80-102, 117, 135-142
src/utils/convert.ts	97.95%	91.66%	100%	97.77%	92
src/utils/get-cells.ts	0%	0%	0%	0%	17-72
src/utils/move-column.ts	0%	0%	0%	0%	35-88
src/utils/move-row-in-array-of-rows.ts	100%	100%	100%	100%
src/utils/move-row.ts	0%	0%	0%	0%	34-80
src/utils/query.ts	0%	0%	0%	0%	13-117
src/utils/selection-range.ts	0%	0%	0%	0%	28-190
src/utils/transpose.ts	100%	100%	100%	100%
test/build.ts	96.29%	91.66%	100%	96.15%	78-80

Generated in workflow #82 for commit 0b909bd by the Vitest Coverage Report Action

[ocavue]

Thanks! Could you please add some tests? I'm particularly interested in the following cases mentioned in your description. Please note that the test should fail without your patch and should pass after your patch.

If inside >= 0, it would always use inside position. But if cellAround() returns null for that position (which can happen in edge cases), the function would return null without trying the pos position as a fallback.

[liwenka1]

Thanks! Could you please add some tests? I'm particularly interested in the following cases mentioned in your description. Please note that the test should fail without your patch and should pass after your patch.

If inside >= 0, it would always use inside position. But if cellAround() returns null for that position (which can happen in edge cases), the function would return null without trying the pos position as a fallback.

Thanks for the review!

You're right - I should provide more context. Let me explain the situation honestly:

Background
In my previous PR (#311), I fixed the cell selection issue for merged cells by preferring mousePos.inside over mousePos.pos. However, I overlooked some edge cases in that fix.

The Problem with Previous Fix
The previous implementation used a simple either-or approach:

const pos = mousePos.inside >= 0 ? mousePos.inside : mousePos.pos;
return cellAround(view.state.doc.resolve(pos));

This worked for rowspan cells, but I later discovered it broke selection in certain regions of colspan cells. The issue is: when mousePos.inside >= 0, we always used inside, but if cellAround(inside) returns null, we didn't fall back to try pos.

The Current Fix
This PR implements a proper fallback strategy:

if (mousePos.inside >= 0) {
  const $cell = cellAround(view.state.doc.resolve(mousePos.inside));
  if ($cell) return $cell;
}
return cellAround(view.state.doc.resolve(mousePos.pos));

Visual Demonstration
I've prepared GIFs showing the bug and the fix:

• Before this fix:
  

• After this fix:
  

About Testing
I understand the importance of tests. However, I'm struggling to create a deterministic unit test for this specific edge case because:

1. It requires mocking view.posAtCoords() to return specific inside vs pos values
2. It depends on the exact coordinate-to-position mapping for merged cells
3. I'm not sure how to set up a scenario where cellAround(inside) fails but cellAround(pos) succeeds in a test environment

[ocavue]

LGTM. The GIFs are enough, I think.