[Table Improvements] Add undoSnapshot when tab on a table cell #3264
Conversation
juliaroldi
requested review from
BryanValverdeU,
JiuqingSong,
Copilot,
flyingbee2012,
ianeli1 and
vinguyen12
There was a problem hiding this comment.
Pull request overview
This pull request enhances table functionality in the editor by adding undo snapshot support when pressing Tab inside table cells and enabling insertion of selected content into table cells during table creation.
Changes:
- Adds undo snapshot creation when Tab key is pressed inside a table cell with new content
- Implements functionality to insert selected paragraphs/blocks into the first column of a newly created table
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| packages/roosterjs-content-model-core/lib/corePlugin/undo/UndoPlugin.ts | Adds Tab key handling to create undo snapshots when tabbing inside table cells with new content, includes helper method to check if cursor is inside a table cell |
| packages/roosterjs-content-model-core/test/corePlugin/undo/UndoPluginTest.ts | Adds comprehensive test coverage for Tab key undo snapshot behavior in various scenarios (inside/outside table, with/without new content, different selection states) |
| packages/roosterjs-content-model-api/lib/publicApi/table/insertTable.ts | Implements insertTableContent function to populate table cells with selected content when inserting a table with active selection |
| packages/roosterjs-content-model-api/test/publicApi/table/insertTableTest.ts | Adds extensive tests for insertTableContent functionality covering various content types (paragraphs, lists, quotes, mixed content) and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| editor.formatContentModel( | ||
| (model, context) => { | ||
| const insertPosition = deleteSelection(model, [], context).insertPoint; | ||
| const copiedModel = cloneModel(model); |
There was a problem hiding this comment.
The model is cloned on line 49 unconditionally, but the cloned model is only used on line 64 if there was a range selection (hasSelection is true). This means the clone operation is wasted when inserting a table without a prior selection. Consider moving the cloneModel call inside the conditional block on line 63 to avoid unnecessary performance overhead when there's no selected content to insert.
| const selection = editor.getDOMSelection(); | ||
| if (this.isInsideTableCell(editor, selection)) { | ||
| this.addUndoSnapshot(); | ||
| } |
There was a problem hiding this comment.
The Tab key handling doesn't update this.state.lastKeyPress after taking a snapshot. This is inconsistent with other key handling patterns in the same function. For example, Backspace/Delete handling updates lastKeyPress (line 159), and cursor moving keys set it to null (line 166). The Tab key should either set lastKeyPress = evt.key or lastKeyPress = null for consistency and to ensure proper behavior in subsequent key events.
| } | |
| } | |
| this.state.lastKeyPress = evt.key; |
| it('KeyDown event, Tab key, has new content, inside table cell', () => { | ||
| const preventDefaultSpy = jasmine.createSpy('preventDefault'); | ||
| const findClosestElementAncestorSpy = jasmine | ||
| .createSpy('findClosestElementAncestor') | ||
| .and.returnValue({} as HTMLElement); | ||
| const state = plugin.getState(); | ||
|
|
||
| state.snapshotsManager.hasNewContent = true; | ||
|
|
||
| (editor as any).getDOMHelper = () => ({ | ||
| findClosestElementAncestor: findClosestElementAncestorSpy, | ||
| }); | ||
|
|
||
| getDOMSelectionSpy.and.returnValue({ | ||
| type: 'range', | ||
| range: { | ||
| collapsed: true, | ||
| startContainer: document.createTextNode('test'), | ||
| }, | ||
| }); | ||
|
|
||
| plugin.onPluginEvent!({ | ||
| eventType: 'keyDown', | ||
| rawEvent: { | ||
| key: 'Tab', | ||
| preventDefault: preventDefaultSpy, | ||
| } as any, | ||
| }); | ||
|
|
||
| expect(findClosestElementAncestorSpy).toHaveBeenCalledWith(jasmine.any(Text), 'td, th'); | ||
| expect(takeSnapshotSpy).toHaveBeenCalledTimes(1); | ||
| expect(preventDefaultSpy).toHaveBeenCalledTimes(0); | ||
| expect(undoSpy).toHaveBeenCalledTimes(0); | ||
| }); |
There was a problem hiding this comment.
The test verifies that takeSnapshotSpy is called when Tab is pressed inside a table cell with new content, but it doesn't verify the state of lastKeyPress after the Tab key handling. This test should also check that plugin.getState().lastKeyPress is set to an appropriate value (either 'Tab' or null) after the event is processed, to ensure the plugin state is correctly maintained.
| } | ||
|
|
||
| private isInsideTableCell = (editor: IEditor, selection: DOMSelection | null) => { | ||
| if (selection?.type == 'range' && selection?.range.collapsed) { |
There was a problem hiding this comment.
The condition selection?.type == 'range' uses optional chaining but then immediately checks the same property again. The second check selection?.range.collapsed is redundant because if selection?.type is already verified to be 'range', then selection is guaranteed to be non-null. The second check should use selection.range.collapsed instead of selection?.range.collapsed.
| if (selection?.type == 'range' && selection?.range.collapsed) { | |
| if (selection?.type == 'range' && selection.range.collapsed) { |
| if (path.length == 1 && block) { | ||
| table.rows[index].cells[0].blocks = [block]; | ||
| index++; | ||
| } else if ( | ||
| block && | ||
| path[0].blockGroupType !== 'TableCell' && | ||
| path[0].blockGroupType !== 'Document' && | ||
| path[0] !== lastBlock | ||
| ) { | ||
| table.rows[index].cells[0].blocks = [path[0]]; | ||
| lastBlock = path[0]; | ||
| index++; |
There was a problem hiding this comment.
The function mutates the blocks array of table cells by directly assigning table.rows[index].cells[0].blocks = [block] or table.rows[index].cells[0].blocks = [path[0]]. This overwrites any existing blocks that may have been created by createTableCell. While this appears intentional, it would be more explicit and maintainable to clear the blocks first or add a comment explaining that the default blocks are being replaced with selected content.
| it('KeyDown event, Tab key, has new content, not inside table cell', () => { | ||
| const preventDefaultSpy = jasmine.createSpy('preventDefault'); | ||
| const findClosestElementAncestorSpy = jasmine | ||
| .createSpy('findClosestElementAncestor') | ||
| .and.returnValue(null); | ||
| const state = plugin.getState(); | ||
|
|
||
| state.snapshotsManager.hasNewContent = true; | ||
|
|
||
| (editor as any).getDOMHelper = () => ({ | ||
| findClosestElementAncestor: findClosestElementAncestorSpy, | ||
| }); | ||
|
|
||
| getDOMSelectionSpy.and.returnValue({ | ||
| type: 'range', | ||
| range: { | ||
| collapsed: true, | ||
| startContainer: document.createTextNode('test'), | ||
| }, | ||
| }); | ||
|
|
||
| plugin.onPluginEvent!({ | ||
| eventType: 'keyDown', | ||
| rawEvent: { | ||
| key: 'Tab', | ||
| preventDefault: preventDefaultSpy, | ||
| } as any, | ||
| }); | ||
|
|
||
| expect(findClosestElementAncestorSpy).toHaveBeenCalledWith(jasmine.any(Text), 'td, th'); | ||
| expect(takeSnapshotSpy).toHaveBeenCalledTimes(0); | ||
| expect(preventDefaultSpy).toHaveBeenCalledTimes(0); | ||
| expect(undoSpy).toHaveBeenCalledTimes(0); | ||
| }); |
There was a problem hiding this comment.
This test verifies that takeSnapshotSpy is not called when Tab is pressed outside a table cell, but it doesn't verify the state of lastKeyPress after the Tab key handling. The test should check that plugin.getState().lastKeyPress is set to an appropriate value after the event is processed, to ensure proper behavior in subsequent key events.
| it('KeyDown event, Tab key, no new content, inside table cell', () => { | ||
| const preventDefaultSpy = jasmine.createSpy('preventDefault'); | ||
| const findClosestElementAncestorSpy = jasmine | ||
| .createSpy('findClosestElementAncestor') | ||
| .and.returnValue({} as HTMLElement); | ||
| const state = plugin.getState(); | ||
|
|
||
| state.snapshotsManager.hasNewContent = false; | ||
|
|
||
| (editor as any).getDOMHelper = () => ({ | ||
| findClosestElementAncestor: findClosestElementAncestorSpy, | ||
| }); | ||
|
|
||
| getDOMSelectionSpy.and.returnValue({ | ||
| type: 'range', | ||
| range: { | ||
| collapsed: true, | ||
| startContainer: document.createTextNode('test'), | ||
| }, | ||
| }); | ||
|
|
||
| plugin.onPluginEvent!({ | ||
| eventType: 'keyDown', | ||
| rawEvent: { | ||
| key: 'Tab', | ||
| preventDefault: preventDefaultSpy, | ||
| } as any, | ||
| }); | ||
|
|
||
| expect(findClosestElementAncestorSpy).not.toHaveBeenCalled(); | ||
| expect(takeSnapshotSpy).toHaveBeenCalledTimes(0); | ||
| expect(preventDefaultSpy).toHaveBeenCalledTimes(0); | ||
| expect(undoSpy).toHaveBeenCalledTimes(0); | ||
| }); |
There was a problem hiding this comment.
This test verifies that snapshot is not taken when there's no new content, but it doesn't verify the state of lastKeyPress after the Tab key handling. The test should check that plugin.getState().lastKeyPress is set to an appropriate value after the event is processed.
| it('KeyDown event, Tab key, has new content, selection is not collapsed', () => { | ||
| const preventDefaultSpy = jasmine.createSpy('preventDefault'); | ||
| const findClosestElementAncestorSpy = jasmine | ||
| .createSpy('findClosestElementAncestor') | ||
| .and.returnValue({} as HTMLElement); | ||
| const state = plugin.getState(); | ||
|
|
||
| state.snapshotsManager.hasNewContent = true; | ||
|
|
||
| (editor as any).getDOMHelper = () => ({ | ||
| findClosestElementAncestor: findClosestElementAncestorSpy, | ||
| }); | ||
|
|
||
| getDOMSelectionSpy.and.returnValue({ | ||
| type: 'range', | ||
| range: { | ||
| collapsed: false, | ||
| startContainer: document.createTextNode('test'), | ||
| }, | ||
| }); | ||
|
|
||
| plugin.onPluginEvent!({ | ||
| eventType: 'keyDown', | ||
| rawEvent: { | ||
| key: 'Tab', | ||
| preventDefault: preventDefaultSpy, | ||
| } as any, | ||
| }); | ||
|
|
||
| expect(findClosestElementAncestorSpy).not.toHaveBeenCalled(); | ||
| expect(takeSnapshotSpy).toHaveBeenCalledTimes(0); | ||
| expect(preventDefaultSpy).toHaveBeenCalledTimes(0); | ||
| expect(undoSpy).toHaveBeenCalledTimes(0); | ||
| }); |
There was a problem hiding this comment.
This test verifies that snapshot is not taken when the selection is not collapsed, but it doesn't verify the state of lastKeyPress after the Tab key handling. The test should check that plugin.getState().lastKeyPress is set to an appropriate value after the event is processed.
| it('KeyDown event, Tab key, has new content, selection is not range type', () => { | ||
| const preventDefaultSpy = jasmine.createSpy('preventDefault'); | ||
| const findClosestElementAncestorSpy = jasmine | ||
| .createSpy('findClosestElementAncestor') | ||
| .and.returnValue({} as HTMLElement); | ||
| const state = plugin.getState(); | ||
|
|
||
| state.snapshotsManager.hasNewContent = true; | ||
|
|
||
| (editor as any).getDOMHelper = () => ({ | ||
| findClosestElementAncestor: findClosestElementAncestorSpy, | ||
| }); | ||
|
|
||
| getDOMSelectionSpy.and.returnValue({ | ||
| type: 'image', | ||
| }); | ||
|
|
||
| plugin.onPluginEvent!({ | ||
| eventType: 'keyDown', | ||
| rawEvent: { | ||
| key: 'Tab', | ||
| preventDefault: preventDefaultSpy, | ||
| } as any, | ||
| }); | ||
|
|
||
| expect(findClosestElementAncestorSpy).not.toHaveBeenCalled(); | ||
| expect(takeSnapshotSpy).toHaveBeenCalledTimes(0); | ||
| expect(preventDefaultSpy).toHaveBeenCalledTimes(0); | ||
| expect(undoSpy).toHaveBeenCalledTimes(0); | ||
| }); |
There was a problem hiding this comment.
This test verifies that snapshot is not taken when the selection type is not 'range', but it doesn't verify the state of lastKeyPress after the Tab key handling. The test should check that plugin.getState().lastKeyPress is set to an appropriate value after the event is processed.
Add undoSnapshot after pressing Tab key in a table that has new content, otherwise if the user type content in a table and press tab to move to another cell and then undo the content, all the typed content will be removed.
Before:
After: