Use DOMCreator instead of TrustedHTMLHandler #2898
Conversation
juliaroldi
requested review from
JiuqingSong,
BryanValverdeU,
flyingbee2012,
ianeli1,
vinguyen12 and
Copilot
and removed request for
JiuqingSong
There was a problem hiding this comment.
Copilot reviewed 9 out of 24 changed files in this pull request and generated no suggestions.
Files not reviewed (15)
- packages/roosterjs-content-model-plugins/test/paste/processPastedContentFromExcelTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/lib/paste/WordDesktop/processPastedContentFromWordDesktop.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/lib/paste/Excel/processPastedContentFromExcel.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/ContentModelPastePluginTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-core/test/editor/core/createEditorCoreTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-core/lib/coreApi/restoreUndoSnapshot/restoreSnapshotHTML.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/lib/paste/WordDesktop/getStyleMetadata.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/lib/paste/PowerPoint/processPastedContentFromPowerPoint.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/processPastedContentFromWordDesktopTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/getStyleMetadataTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-core/lib/command/createModelFromHtml/createModelFromHtml.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/test/paste/processPastedContentFromPowerPointTest.ts: Evaluated as low risk
- packages/roosterjs-content-model-core/lib/editor/Editor.ts: Evaluated as low risk
- packages/roosterjs-content-model-plugins/lib/paste/PastePlugin.ts: Evaluated as low risk
- packages/roosterjs-content-model-core/lib/command/paste/paste.ts: Evaluated as low risk
Comments skipped due to low confidence (1)
packages/roosterjs-content-model-types/lib/editor/EditorCore.ts:20
- [nitpick] Consider renaming 'trustedHTMLHandler' to 'legacyTrustedHTMLHandler' for consistency and clarity.
import type { DOMCreator, LegacyTrustedHTMLHandler } from '../parameter/TrustedHTMLHandler';
| @@ -1,4 +1,16 @@ | |||
| /** | |||
| * A handler type to convert HTML string to a trust HTML string | |||
| */ | |||
| export type TrustedHTMLHandler = (html: string) => string; | |||
| export type LegacyTrustedHTMLHandler = (html: string) => string; | |||
There was a problem hiding this comment.
Let's mark this interface as "@deprecated" in comment. So next time we do major version bump, we will remove it.
| @@ -193,12 +193,21 @@ export interface IEditor { | |||
| hasFocus(): boolean; | |||
|
|
|||
| /** | |||
| * @deprecated use getDOMCreator instead | |||
| trustedHTMLHandler: | ||
| options.trustedHTMLHandler && !isDOMCreator(options.trustedHTMLHandler) | ||
| ? options.trustedHTMLHandler | ||
| : defaultTrustHtmlHandler, |
There was a problem hiding this comment.
If options.trustedHTMLHandler is DOMCreator, we still need to create a valid trustedHTMLHandler, to satisfy the following case:
- Pass in a DOMCreator when init editor
- call editor.getTrustedHTMLHandler() and use it to sanitize HTML
You can create a function here to use DOMCreator() to return a DOM tree then get its innerHTML and return
There was a problem hiding this comment.
Once we integrate this into OWA, we can search all places where getTrustedHTMLHandler is used and replace with getDOMCreator(), so this new function should not really be used in OWA but other roosterjs customers may still need it
| /** | ||
| * @internal | ||
| */ | ||
| export function domCreator(trustedHTMLHandler?: TrustedHTMLHandler): DOMCreator { |
There was a problem hiding this comment.
Suggest name it createDOMCreator
| @@ -18,6 +19,8 @@ import type { | |||
| */ | |||
| export function createEditorCore(contentDiv: HTMLDivElement, options: EditorOptions): EditorCore { | |||
| const corePlugins = createEditorCorePlugins(options, contentDiv); | |||
| const domCreator = createDOMCreator(options.trustedHTMLHandler); | |||
| const trustedHTMLHandler = createTrustedHTMLHandler(domCreator); | |||
There was a problem hiding this comment.
Suggest create trustedHTMLHandler from options.trustedHTMLHandler directly. So in case user is passing in a legacy handler function, we can use it directly
Use DOMCreator instead of TrustedHTMLHandler
The current trustedHTMLHandler converts a string into a trusted string, and if necessary, transforms the trusted string into a DOM. To optimize this process, introduce a new DOMCreator that directly converts a string into a trusted DOM. Consequently, the trustedHTMLHandler editor option will accept two types of handlers: one that transforms strings into trusted strings and another that converts strings into Document objects.