-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Excel non-native paste event handling and related utilities #2950
Implement Excel non-native paste event handling and related utilities #2950
Conversation
…trievalOfClipboard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 19 changed files in this pull request and generated 2 comments.
Files not reviewed (14)
- packages/roosterjs-content-model-plugins/lib/paste/Excel/processPastedContentFromExcel.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/powerPoint/processPastedContentFromPowerPointTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/word/processPastedContentFromWacTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/utils/getStyleMetadataTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/excel/processPastedContentFromExcelTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/utils/linkParserTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/word/processPastedContentFromWordDesktopTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/excel/validateExcelFragmentTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/utils/deprecatedColorParserTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/lib/paste/PastePlugin.ts: Evaluated as low risk
- demo/scripts/controlsV2/demoButtons/pasteButton.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/lib/paste/pasteSourceValidations/getPasteSource.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/plugin/ContentModelPastePluginTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/lib/paste/Excel/handleExcelContentFromNotNativeEvent.ts: Evaluated as low risk
...roosterjs-content-model-plugins/test/paste/excel/handleExcelContentFromNotNativeEventTest.ts
Outdated
Show resolved
Hide resolved
packages/roosterjs-content-model-plugins/test/paste/createBeforePasteEventMock.ts
Outdated
Show resolved
Hide resolved
…board' of https://github.com/microsoft/roosterjs into u/bvalverde/fixExcelPasteWithProgramaticRetrievalOfClipboard
…iveEvent parameter
@@ -29,7 +30,8 @@ export type KnownPasteSourceType = | |||
| 'googleSheets' | |||
| 'wacComponents' | |||
| 'default' | |||
| 'singleImage'; | |||
| 'singleImage' | |||
| 'excelNonNativeEvent'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this exist in all cases when copy from Excel? If so, do we still need to keep 'excelDesktop'? Maybe we just need to replace 'excelDesktop' and always use the new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the shadow workbook type exists when copying from Excel. Should be available from ctrl + v and programatically pasting
The main difference is that when pasting wIth Ctrl + V we need the current approach, (clipboard fragment does not have a table element, so we do an special case).
But when getting the clipboard programatically, the fragment does contain a table but we need to add borders to the table.
Another option is to merge both logic in excel handling, but if fragment contains a table we do not need to build the table from the rawHtml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then let's keep the change in this PR to keep it simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, just modified the logic. Now we just keep all the excel handling in a single block of the switch. I think it looks cleaner. Let me know if it is okay
…trievalOfClipboard
…add new HTML template for non-native Excel content
…board' of https://github.com/microsoft/roosterjs into u/bvalverde/fixExcelPasteWithProgramaticRetrievalOfClipboard
…trievalOfClipboard
…trievalOfClipboard
When pasting from Excel but retrieving the clipboard content programatically, such as in the Top Ribbon Paste button, The native APIs are returning the content with a table and removes all the attributes we use to determine whether the content is from Excel.
This causes an inconsistency when pasting the content from Ctrl + V and the paste button, as when using Ctrl + V we add the border colors, if the tables. But since with the paste button we don't have the border colors, we need to handle this case.
This PR adds a new paste source for excel non native so we can add parsers to add the borders to the tables when pasting excel content and not using ctrl + v
Fix 272712