Skip to content
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

Conversation

BryanValverdeU
Copy link
Contributor

@BryanValverdeU BryanValverdeU commented Feb 18, 2025

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

Copy link

@Copilot Copilot AI left a 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

@@ -29,7 +30,8 @@ export type KnownPasteSourceType =
| 'googleSheets'
| 'wacComponents'
| 'default'
| 'singleImage';
| 'singleImage'
| 'excelNonNativeEvent';
Copy link
Collaborator

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.

Copy link
Contributor Author

@BryanValverdeU BryanValverdeU Feb 18, 2025

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

Copy link
Collaborator

@JiuqingSong JiuqingSong Feb 18, 2025

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.

Copy link
Contributor Author

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

@BryanValverdeU BryanValverdeU merged commit 7beec85 into master Feb 20, 2025
7 checks passed
@BryanValverdeU BryanValverdeU deleted the u/bvalverde/fixExcelPasteWithProgramaticRetrievalOfClipboard branch February 20, 2025 22:11
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