Pasting is keeping background-color even with `doNotAdjustEditorColor: true

[adrienWeiss]

Describe the bug

Hello,

something is behaving weirdly and I'm pretty sure this was not the case at least a few months ago 🤔
a simple copy / paste of a word from and to the editor is inlining a background-color attribute, even if it's white on an already-white editor background (so the user has no way to detect this).
The consequence is pretty dramatic in a mail client usage because the output of the HTML sent will show the pasted words on white, while the rest of the text will be shown on whatever background color the recipient's mail client is set with.

I suspected the background-color style attribute added on the editor contentDiv, which is not set if using doNotAdjustEditorColor: true contructor option, but that doesn't change anything. I'm still convinced it's something that was done in order to correctly render in darkMode.

To Reproduce
Steps to reproduce the behavior:

1. Go to Rooster Demo (or Outlook)
2. type anything
3. select a word -> copy
4. paste it in the same draft
5. inspect that pasted word in the devtools : it has a background-color definition, whereas the copied word has not.

Expected behavior
Pasted text should not declare a background-color style attribute as long as it is the same color than its computedStyle

Screenshots
If applicable, add screenshots to help explain your problem.

Device Information

• OS: [e.g. iOS] : macOS
• Browser [e.g. chrome, safari] : chrome
• Version [e.g. 22] : current demo 8.51.0 — at least since 8.44.1

Additional context
Add any other context about the problem here.

[JiuqingSong]

It is browser's behavior that background color is copied. Currently we still rely on that behavior. But I think we may be able to change it with Content Model Copy Paste. @BryanValverdeU what do you think?

[adrienWeiss]

@JiuqingSong that's what I thought at first but a native contenteditable is not behaving like this.
Here is a quick demo https://codepen.io/adrienweiss/pen/mdQppPa
Using the toggle button you can check against a grey background.
Copying this and pasting it in the Rooster demo will add a background.

If it's indeed the browser like you say, then it's something in the parent context that is making the browser keep this attribute

[adrienWeiss]

Ok @JiuqingSong I think I now understand a lot better what is happening. I don't think Rooster is relying on browser behavior, because inherited styles are actually removed here

roosterjs/packages/roosterjs-editor-dom/lib/clipboard/sanitizePasteContent.ts

Line 17 in 067dc5c

sanitizer.sanitize(fragment, position ? getInheritableStyles(position.element) : undefined);

It is based on a re-implementation of what we expect the browser to do in this function

roosterjs/packages/roosterjs-editor-dom/lib/htmlSanitizer/getInheritableStyles.ts

Lines 17 to 25 in 067dc5c

	export default function getInheritableStyles(element: HTMLElement | null): StringMap {
	let win = element && element.ownerDocument && element.ownerDocument.defaultView;
	let styles = win && element && win.getComputedStyle(element);
	let result: StringMap = {};
	INHERITABLE_PROPERTIES.forEach(
	name => (result[name] = (styles && styles.getPropertyValue(name)) || '')
	);
	return result;
	}

and INHERITABLE_PROPERTIES does not include background-color because it is, indeed, a property that isn't inherited on children elements. Therefore getComputedStyle on the position element will return rgba(0, 0, 0, 0) (transparent) instead of its background-color context (default white).

However, a browser in a native contenteditable will successfully remove the background-color property if it matches the background-color "inherited" at the caret position. I don't know how, but it works.
You can easily check on the demo running locally by just disabled the Rooster init & adding contenteditable to the .editor div.

I do have a solution to make it work using a bit of a hack, by modifying 2 things :

• Adding background-color to the list of inheritable properties

diff --git a/packages/roosterjs-editor-dom/lib/htmlSanitizer/getInheritableStyles.ts b/packages/roosterjs-editor-dom/lib/htmlSanitizer/getInheritableStyles.ts
index aa7e05ab96..bbcf90b9cb 100644
--- a/packages/roosterjs-editor-dom/lib/htmlSanitizer/getInheritableStyles.ts
+++ b/packages/roosterjs-editor-dom/lib/htmlSanitizer/getInheritableStyles.ts
@@ -3,7 +3,7 @@ import { StringMap } from 'roosterjs-editor-types';
 // Inheritable CSS properties
 // Ref: https://www.w3.org/TR/CSS21/propidx.html
 const INHERITABLE_PROPERTIES = (
-    'border-spacing,caption-side,color,' +
+    'border-spacing,caption-side,color,background-color,' +
     'cursor,direction,empty-cells,font-family,font-size,font-style,font-variant,font-weight,' +
     'font,letter-spacing,line-height,list-style-image,list-style-position,list-style-type,' +
     'list-style,orphans,quotes,text-align,text-indent,text-transform,visibility,white-space,' +

• and making sure all elements in the .editor have background-color: inherit. This apparently makes the getComputedStyle correctly identify the inherited background-color property.

diff --git a/demo/scripts/controls/MainPane.scss b/demo/scripts/controls/MainPane.scss
index 50952d324b..e3c6aafc51 100644
--- a/demo/scripts/controls/MainPane.scss
+++ b/demo/scripts/controls/MainPane.scss
@@ -47,6 +47,10 @@
     bottom: 0;
 }
 
+.editor * {
+    background-color: inherit;
+}
+
 .resizer {
     flex-grow: 0;
     flex-shrink: 0;

I would understand that you may not want to do such a thing because it's a bit hacky, but if you don't have another solution to be closer to the native browser "cleanup" of inline styles on pasted content, may I ask that you at least add background-color to INHERITABLE_PROPERTIES, as it would be harmless without the background-color: inherit css property on all children, and would allow users like me to add the hack or not.

Another solution would be to recursively parse the position element parents until a non-transparent getPropertyValue('background-color') is returned, but it does seem costly.

[JiuqingSong]

For the case that copy from same editor then paste, I prefer to solve it when copy since the original text does not have background, the copied content should not have it as well. But what if I copy some other text that really has background color, should we keep it? Sometimes we may want to keep and sometimes not. So it is hard to say if we should always remove it.

I think to solve your original problem, you can create your own plugin to handle BeforePasteEvent, it will give you a HTML fragment before it is pasted into doc, so you can do any change you want there. You can check the background color and remove it if you want.

[adrienWeiss]

That's exactly what I end up doing, by combining a BeforePasteEvent, retrieving the position from there ( is this.editor.getFocusedPosition() the best method ?) and calling the HtmlSanitizer.sanitize method with my own detection of background-color I get it to work. The caveat is that we end up parsing/sanitizing twice the same fragment

However, to answer your first paragrpah, I think it's not the best approach to try to hijack what is being copied, if this is what you intend. There should be no specific treatment depending on where the content is copied from. My point is that, ideally, Rooster behaves exactly like the browser does. What you are describing with "Sometimes we may want to keep and sometimes not" is something the browser already knows how to do in a contenteditable, at least from my tests. If I paste my "white background copied text" at a caret position with a white background, the property is not applied. If I paste it at a caret position with a red background (event inherited from a grand-parent div), the browser will keep the white background property. If you try to play all these scenarios in a simple contenteditable div you should see that the merging is done properly.

From my understandings, getInheritableStyles / sanitize are trying to replicate a native browser behavior, but fails to cover all cases.
It's a bit naive I know but if Rooster had somehow let the pasting happen, then removed the newly pasted content to do every other complex stuff (including handling Word/Excel/etc) on that pasted content, there wouldn't be any need to computed inherited styles to remove as it would already be done.
I seem to understand there are such hacks upon copying text, using a temporary div that is immediately removed from the DOM.

[BryanValverdeU]

We move the content that is going to be copied to a tempDiv, In order to allow other Plugins to do changes to the tempDiv if needed. This TempDiv background color is white color by default.

In one of your plugins you can update the clonedRoot div background color to meet your needs and when the copy is done, the color of the copied content should also perserve the inherited background color.

        if (this.editor && event.eventType === PluginEventType.BeforeCutCopy) {
            event.clonedRoot.style.backgroundColor = 'red'; // Any Color
        }

Eg using the above code to set the background color of the cloned root to red

[JiuqingSong]

For long term, we can do copy fully on top of content model and we can get rid of temp div. In that way we can control what is copied using our code.

For short term, I think a plugin to remove such style should be a easiest way.

[JiuqingSong]

And for the paste side, content model paste should be able to easily detect such background and remove it if it is not necessary.

[adrienWeiss]

@BryanValverdeU thank you for you answer
nonetheless this only expose the case of copying from Rooster editor (without added logic to handle).
The point I'm trying to make is more global, I'll try to be concise :

• Rooster is trying to replicate how a browser natively behaves when pasting HTML data anywhere inside a contenteditable div, by removing all styles found in the clipboard HTML that would already be declared in the element we're pasting into. Rooster does that by using getComputedStyles
• In reality — a simple contenteditable , the browser is indeed removing those properties that are already inherited, but is also able to compute the local background-color, to effectively remove any redundant declaration (based on my observations at least). So getComputedStyle is not enough to replicate the browser behavior.

In the case of a mail client (which is an obvious use case for Rooster), this is really problematic, because a user is often trying to copy something from a received email into a new draft. Copying from somewhere with a white background, into a draft with white background. If that draft had been a simple contenteditable, the browser would have removed the background-color: rgb(255, 255, 255) inline property from the pasted text automatically. RoosterJS does not, therefore the user will be sending their email with a piece of text that would look white if displayed on anything else than white background (— i'm not talking about dark mode manipulations here to keep things simple).

@JiuqingSong : I hope contentModel can solve this in a better fashion than my hack, because yeah having to declare a global selector so that all my elements inside the editor have background-color: inherit declared as fallback is not really ideal. I'm still keeping this until I can safely migrate to contentModel in production, and would happily share my code in detail to anyone reading this trying to fix the same thing

and thank you again to the team for your work 🙏