[lexical][@lexical/table] Prevent nested table pastes while preserving table-selection pasting#8082
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@aldoprogrammer Apologies, I might have caused the conflicts in this PR due to #8088, which I did preparation for another change relating to nested tables. That PR doesn't change any behaviour but it restructured For the avoidance of doubt, when |
etrepum
left a comment
There was a problem hiding this comment.
This PR has conflicts with main due to another PR being merged, those conflicts need to be resolved before it can be considered for merge.
a2d9307 to
e3f3823
Compare
67bbff5 to
d5a066a
Compare
|
Please do check again |
…cture and cleaning up unused imports in LexicalTablePluginHelpers and LexicalTableSelectionHelpers.
…ClipboardNodesCommand for improved clipboard node insertion within table selections. Refactor command payload structure for clarity and maintainability.
Yes @randal-atticus , the behavior is preserved. When hasNestedTables: true, mixed-node clipboards return false early, allowing the default paste handler to insert the table into the cell as a nested table rather than pasting over it. |
| type TableSelectionLike = BaseSelection & { | ||
| anchor: RangeSelection['anchor']; | ||
| focus: RangeSelection['focus']; | ||
| tableKey: NodeKey; | ||
| }; | ||
|
|
||
| function isTableSelectionLike( | ||
| selection: BaseSelection, | ||
| ): selection is TableSelectionLike { | ||
| return ( | ||
| $isTableSelection(selection) || | ||
| (typeof selection === 'object' && | ||
| selection !== null && | ||
| 'tableKey' in selection && | ||
| 'anchor' in selection && | ||
| 'focus' in selection) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
In what situation does $isTableSelection not work for this use case?
| $isTableNode(node) || node.getType() === 'table', | ||
| ); | ||
| const hasOnlyTableNodes = | ||
| nodes.length > 0 && | ||
| nodes.every((node) => $isTableNode(node) || node.getType() === 'table'); |
There was a problem hiding this comment.
getType should never be used for this use case, what situation are you trying to handle where $isTableNode doesn't work?
…tead of getType()
|
Thanks for the feedback! Removed the unnecessary |
| const isSelectionInsideOfGrid = | ||
| (isRangeSelection && | ||
| anchorAndFocus !== null && | ||
| $findMatchingParent(selection.anchor.getNode(), (n) => | ||
| $isTableCellNode(n), | ||
| ) !== null && | ||
| $findMatchingParent(selection.focus.getNode(), (n) => | ||
| $isTableCellNode(n), | ||
| ) !== null) || | ||
| isTableSelection; | ||
| const templateGrid = nodes.find((node): node is TableNode => | ||
| $isTableNode(node), | ||
| ); | ||
| const hasOnlyTableNodes = | ||
| nodes.length > 0 && nodes.every((node) => $isTableNode(node)); |
There was a problem hiding this comment.
All of these guards can be used as-is, you don't have to wrap them in another function
| const isSelectionInsideOfGrid = | |
| (isRangeSelection && | |
| anchorAndFocus !== null && | |
| $findMatchingParent(selection.anchor.getNode(), (n) => | |
| $isTableCellNode(n), | |
| ) !== null && | |
| $findMatchingParent(selection.focus.getNode(), (n) => | |
| $isTableCellNode(n), | |
| ) !== null) || | |
| isTableSelection; | |
| const templateGrid = nodes.find((node): node is TableNode => | |
| $isTableNode(node), | |
| ); | |
| const hasOnlyTableNodes = | |
| nodes.length > 0 && nodes.every((node) => $isTableNode(node)); | |
| const isSelectionInsideOfGrid = | |
| (isRangeSelection && | |
| anchorAndFocus !== null && | |
| $findMatchingParent(selection.anchor.getNode(), $isTableCellNode) !== null && | |
| $findMatchingParent(selection.focus.getNode(), $isTableCellNode) !== null) || | |
| isTableSelection; | |
| const templateGrid = nodes.find($isTableNode); | |
| const hasOnlyTableNodes = | |
| nodes.length > 0 && nodes.every($isTableNode); |
| const focusCellNode = $findMatchingParent(focus.getNode(), (n) => | ||
| $isTableCellNode(n), | ||
| ); |
There was a problem hiding this comment.
| const focusCellNode = $findMatchingParent(focus.getNode(), (n) => | |
| $isTableCellNode(n), | |
| ); | |
| const focusCellNode = $findMatchingParent(focus.getNode(), $isTableCellNode); |
| } | ||
|
|
||
| let [interimGridMap] = $computeTableMapSkipCellCheck( | ||
| gridNode.getWritable(), |
There was a problem hiding this comment.
The getWritable here is redundant, all of the accessor methods will already do this as needed. The identity of gridNode or direct access to its properties are not used here
| gridNode.getWritable(), | |
| gridNode, |
| } | ||
|
|
||
| [interimGridMap] = $computeTableMapSkipCellCheck( | ||
| gridNode.getWritable(), |
There was a problem hiding this comment.
| gridNode.getWritable(), | |
| gridNode, |
| const originalChildren = cell.getChildren(); | ||
| templateCell.getChildren().forEach((child) => { | ||
| if ($isTextNode(child)) { | ||
| const paragraphNode = $createParagraphNode(); | ||
| paragraphNode.append(child); | ||
| cell.append(child); | ||
| } else { | ||
| cell.append(child); | ||
| } | ||
| }); | ||
| originalChildren.forEach((n) => n.remove()); | ||
| } |
There was a problem hiding this comment.
This logic is wrong, for example the paragraphNode code does absolutely nothing because the child is moved out of the paragraphNode into the cell so the paragraphNode just becomes garbage. It's probably better to just assume that the template table is already normalized (or the destination will be normalized after the fact) than to try and do some ad-hoc normalization here.
| const originalChildren = cell.getChildren(); | |
| templateCell.getChildren().forEach((child) => { | |
| if ($isTextNode(child)) { | |
| const paragraphNode = $createParagraphNode(); | |
| paragraphNode.append(child); | |
| cell.append(child); | |
| } else { | |
| cell.append(child); | |
| } | |
| }); | |
| originalChildren.forEach((n) => n.remove()); | |
| } | |
| cell.splice(0, cell.getChildrenSize(), templateCell.getChildren()); |
| // reset the table selection in case the anchor or focus cell was | ||
| // removed via merge operations | ||
| const [finalGridMap] = $computeTableMapSkipCellCheck( | ||
| gridNode.getWritable(), |
There was a problem hiding this comment.
| gridNode.getWritable(), | |
| gridNode, |
Summary
Prevent nested tables when pasting into table cells.
Testing
Original issue demo: #8077
After having conflict on this pr, pull latest and merged in main: issue still persist
Demo fix: video
Closes #8077