Skip to content

User/rashmithakur/rowselection on cellediting poc#7

Open
rashmi-thakurr wants to merge 10 commits intoPOCForTableViewGapsfrom
user/rashmithakur/rowselection-on-cellediting-poc
Open

User/rashmithakur/rowselection on cellediting poc#7
rashmi-thakurr wants to merge 10 commits intoPOCForTableViewGapsfrom
user/rashmithakur/rowselection-on-cellediting-poc

Conversation

@rashmi-thakurr
Copy link
Copy Markdown
Collaborator

@rashmi-thakurr rashmi-thakurr commented Apr 6, 2026

Scenarios Validated

  1. Single tap unselected cell - Selects the cell/row, no editing

  2. Single tap already selected cell - Selected cell enters edit mode

  3. Double tap unselected cell - Cell enters edit mode directly

  4. Double tap read only cell/ Single tap read only cell - cell/row is selected but cell doesn't enters edit mode

  5. Rapid triple tap - Cell enters edit mode once on double-tap. No duplicate editing, no flicker, no crash. Third tap interacts with the edit control (TextBox) normally.

  6. Enter edit mode --> double tap any editable cell - Row is highlighted

  7. Escape --> while editing, press escape - cell exits editing mode but row and cell is selected and highlighted

  8. Enter --> Double tap to edit, type something and then press enter content inside the cell changes and cell/row - selection moves to the cell below in the next row and that cells enters edit mode

  9. Click outside table --> double tap to edit cell and then click on empty space outside the table - cell exits edit mode but row/cell remains selected and highlighted.

  10. Tab while editing - editing and highlight moves to the cell below in the next row

  11. shift+tab while editing - editing and highlight moves to the cell above in the previous row

  12. arrow keys while editing - arrow keys don't work; because the arrow keys are captured by the edit control textbox for cursor movement within the text

  13. Tab past the last row --> what happens at the boundary? - editing and highlight moves to the cell in the next column in the first row

  14. Edit a row, scroll it off-screen, scroll back - Cell still in edit mode and row is still highlighted

  15. Tab to a row that's off-screen - Table auto scroll to show the new cell/row and they are selected and highlighted

  16. Rapidly scroll while editing - No random rows getting highlighted. No visual glitches. Only the editing row is highlighted

  17. Start editing and then filter the column - cell exits editing mode, highlighting is removed, filtered table comes up

  18. Start editing, then sort by clicking a header - cell exits editing mode, highlighting is removed and sorting happens

  19. Filter rows - such that edited row disappears - seems to be working fine

  20. Remove filter to bring row back- seems to be working as expected

  21. Multi select mode (ctrl + click) - Editing highlight and multi-row selection coexist cleanly — editing highlights only the active row without interfering with other selected rows, and all visual states clear properly on escape, filter, or
    sort.

@hiteshkrmsft
Copy link
Copy Markdown

can youu check these?


🔴 Critical Issues

  1. Virtualization bug — stale _isEditing on recycled containers (TableViewRow.cs)

_editingHighlightRow in TableView.cs holds a direct reference to a TableViewRow container. In a virtualized list, containers are recycled when rows scroll out of view. When a container is recycled for a
different row index, it carries the _isEditing = true state from the old row. This causes EnsureAlternateColors() to be silently skipped on that new row:

// EnsureAlternateColors guard — will silently skip if _isEditing is stale
if (TableView is null || RowPresenter is null || _isEditing) return;

Result: A recycled container's alternating row color is never updated, causing visual corruption. The highlight state needs to be reset in the row's PrepareContainerForItemOverride/OnApplyTemplate cycle,
or tracked by row index rather than container reference.


🟠 Significant Issues

  1. Navigation semantics change is too broad (TableView.cs, line 280)

-newSlot = GetNextSlot(newSlot, shiftKey, e.Key is VirtualKey.Enter);
+newSlot = GetNextSlot(newSlot, shiftKey, e.Key is VirtualKey.Enter || SelectionUnit is TableViewSelectionUnit.Row);

Passing true for the Enter-like flag when SelectionUnit is Row means all navigation keys (Tab, Arrow) always advance by row instead of by cell. If the intent is only to change Tab/Enter behavior, this is
overly broad and will break Arrow key cell navigation within row-mode editing.

  1. Potential async reentrancy in OnTapped/OnDoubleTapped (TableViewRow.cs)

Both handlers are now async void. The !TableView.IsEditing guard is checked synchronously, but BeginCellEditing yields the UI thread during the await. On a double-tap, the event sequence is: Tap1 → Tap2 →
DoubleTap. If BeginCellEditing from OnTapped (Tap2) has not yet set IsEditing = true before OnDoubleTapped fires (which is possible at an await yield), both attempts could invoke BeginCellEditing
simultaneously. Add a local bool guard set before the await to prevent this.

  1. ContainerFromIndex returns null for off-screen rows (TableView.cs, HandleNavigations)

if (ContainerFromIndex(newSlot.Row) is TableViewRow newRow)
{
_editingHighlightRow = newRow;
newRow.ApplyEditingHighlight(true);
}

If the target row is not currently in the viewport, ContainerFromIndex returns null. The old row's highlight is cleared, but the new row gets no highlight and _editingHighlightRow remains null. There's no
recovery path when the row eventually scrolls into view.

  1. CellOrRow selection unit is inconsistent

SetIsEditing applies the editing highlight only when SelectionUnit is TableViewSelectionUnit.Row, but OnTapped in the original code handled both Row and CellOrRow. If CellOrRow is a valid unit, the
highlight logic should cover it too — or there should be an explicit comment explaining why it's excluded.


🟡 Minor Issues

  1. ApplyEditingHighlight doesn't explicitly reset background on Windows

On Windows, the highlight sets RowPresenter.Background = _itemPresenter?.PointerOverBackground. If _itemPresenter is null, the background gets set to null — this may be a visible flicker or incorrect
background on some theme states. There's no fallback.

  1. _isEditing field name collision risk

TableViewRow already has a field _isEditing = false. TableView.cs also tracks editing via its own IsEditing property. The naming is not wrong, but a more specific name like _hasEditingHighlight would make
the intent clearer and reduce cognitive overhead.

  1. Tests are smoke tests, not behavior tests (TableViewRowEditingTests.cs)

SetIsEditing_TogglesInRowMode doesn't add any rows/data to the TableView, so ContainerFromIndex will always return null in the highlight path — the highlight code path is never actually exercised. The test
only verifies the IsEditing boolean flips correctly.

ApplyEditingHighlight_Toggles verifies "should not crash" but doesn't assert any visual state. Given the highlight logic is the core of this PR, tests for at least the _isEditing flag state and
EnsureAlternateColors guard behavior would be more valuable.

  1. PR title contains "poc" — should not target a long-lived branch without cleanup

The branch (user/rashmithakur/rowselection-on-cellediting-poc) and title both say POC. Before this is merged even into POCForTableViewGaps, the code should be cleaned up: remove POC-specific comments,
ensure no TODO markers are left, and confirm the approach is committed to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants