Retain text color when pasting #27399

grzim · 2020-12-01T14:56:09Z

Description

Not ready for merging yet!

According to issue #27212 there is a problem that when text is copied and pasted it is losing text colouring.
The problem is - as far as I investigated it looks like it was no mechanics to handle it from the very beginning, therefore it is not a regression bug but this functionality was missing from the beggining.
Every letter is wrapped in a <span> and later altered to html tag which refers to span style (strong, i, etc.) or unwrapped.
The problem lays in the architectural design of this mechanism. Not all styles can be represented as a corresponding HTML element and this is why colouring is not working. My solution was to add a special tag 'coloured' that is only used for wrapping in order to preserve text color, and then in the final html code, every is altered to with a color style applied.
All in all, I don't like the mechanism that is here as it is not future proof. Adding any new feature, like text highlight, different text sizes etc or changing the existing ones will need to be done separately in this part of code, which will lead to problems in the future for sure.
Also in the current state, sub and sup are not working when copy-pasted. The same when we edit something as html and will add some styling - they will be lost when copy-pasting. The source of all of those problems is the same.

Another issue connected to the same problem:
#27345

How has this been tested?

Tested visualy - e2e tests are not passed. I can fix them but it is a good idea to discuss the solution first.
This mechanics is used in many other parts of code (not only copy-pasting) so it is vital to work out a wise solution.

Screenshots

Types of changes

New feature

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

…d color in order not to paste text with the selection highlight

adamziel · 2020-12-01T16:16:24Z

packages/dom/src/phrasing-content.js

+		const openingTagRegExp = new RegExp( `<${ tag }`, 'g' );
+		const closingTagRegExp = new RegExp( `</${ tag }`, 'g' );
+
+		const withOriginalOpeningTag = HTML.replace(


I would walk the tree instead of relying on regexps - they may have unintended consequences.

All manually placed special chars like < have escape characters so this should not be dangerous, nevertheless, indeed tree traversing is just safe, so it is better to approach, thanks.
I am just not convinced with the entire idea - as I've mentioned in the desc - this is not scalable and there would be needed a huge amount of logic to cover all cases (colors are just a tip of an iceberg). I think it will be better to change the mechanics of unfolding letters from their corresponding spans and the way spans are translated to native html tags.

adamziel · 2020-12-01T16:21:28Z

packages/blocks/src/api/raw-handling/phrasing-content-reducer.js

 		} = node.style;

+		// adds color to the wrapper
+		if ( color && ! defaultColor.includes( color ) ) {
+			const wrapper = doc.createElement( 'coloured' );


My memory is hazy, but I remember using custom tags had some surprising effects. I'm not completely opposed to custom tags as long as we can be sure they works as intended in all the browsers. Maybe using one of the allowed tags with a custom attribute like data-has-color="true" would be easier?

Or maybe just checking for style.color later on would suffice?

This custom tag is not placed in html. I don't like the idea to put any custom tag directly to html unless it is a component. I use them only in the internal mechanics not to lose it, as every letter is stripped out of a span tag. Therefore, every custom tag is transformed back to span at the very end of all transformations

We should not insert custom tags or data attributes. We should just preserve the color style property, and add the has-inline-color class.

adamziel · 2020-12-02T16:49:07Z

All in all, I don't like the mechanism that is here as it is not future proof. Adding any new feature, like text highlight, different text sizes etc or changing the existing ones will need to be done separately in this part of code, which will lead to problems in the future for sure.

How else would you approach this? The overall approach here is allowlisting a specific subset of html. My understanding is that regardless of the implementation details we will always have to explicitly add/allow new attributes and ways of formatting.

Also CCing @ellatrix @noisysocks as authors of the code.

grzim · 2020-12-02T23:39:41Z

Explicitly adding new attributes is not the right choice, as a user can use any attrs they want - they can edit the text as HTML. If the attr is not set in contentSchema then it will be lost - not only when pasted but also when edited (#27345) . I am not sure why there was a need to introduce contentSchemas, I will be happy if authors could put some light on it.
The proposed solution is to get rid of schemas that define which attrs can be used to which elements. Instead, preserve all attributes of an element wrapping a letter (it can be span or any other tag). As the list of all atrrs (especially style) will be long, I would remove all attributes that are there by default.
If this diff (element/defaultElement) is empty then a letter is unwrapped from a span. If the diff is not empty then transform it to the corresponding HTML tag if there is such (like <strong>), if there is no corresponding tag, leave it as it is. Thanks to this we don't have to define all possible attributes in advance.

ellatrix

Hi!

You're right that this is not a bug, it's just not implemented yet.

The raw handler strips all HTML except for what we'd explicitly like to preserve. In the case of text colouring, it seems there are some classes and style properties that need to be preserved: has-inline-color, has-...-color (in case of a named colour, and style="color:#...".

A fix should just preserve these attributes, nothing more I think.

grzim · 2020-12-03T15:41:02Z

Preserving this content will work for colouring but this will not help in a general issue of loosing additional data. User can edit text as html and add other formatting. This will be lost.

I will make another PR describing the entire mechanics as focusing only on colours will not help in solving the general problem

ellatrix · 2020-12-06T10:43:07Z

What additional data are you referring to?

grzim · 2020-12-07T23:46:52Z

By additional data, I mean all the styles and attributes that a user can add but are not listed in schemas. As users can edit the text as html, they have a huge amount of possibilities. Listing them all is not possible. We need te to be secure, so I would suggest using third party sanitization instead of hardcoded schemas

…d - 1. get default styling (black text) 2. diff the default styling with styles of pasted element to get all nondefault styles applied to element 3. Sanitize output 4. paste the output

grzim · 2020-12-14T00:05:35Z

@ellatrix how about adding to the schema styles we want to preserve?

Also - schemas define the form of HTML sanitization, shouldn't it be done on the server-side?

ellatrix · 2020-12-15T10:47:34Z

You mean that they can add a style attribute in HTML mode and put anything in there? This is already possible right? No need to sanitise it. At most we can generally sanitise the attribute value, but no need to sanitise colors or style attributes.

grzim · 2020-12-15T15:47:47Z

As far as I understood schemas are defined in order to describe what kind of attributes elements can have. Otherwise, they are erased. What is the purpose of it if not to sanitize?
Adding 'style' attr to schema would prevent from removing color style, which is added when a user adds text coloring.

ellatrix · 2021-02-25T15:24:23Z

Does #27967 resolve this issue? For internally pasted content we should not run all the paste filters. For externally pasted content we should and we should not allow color formatting because there can be a lot of unwanted styling present.

grzim · 2021-03-02T22:15:59Z

Looks like #27967 has nothing to do with retaining text color when pasting. Tested on master and the issue still appears.

grzim · 2021-03-02T22:34:41Z

Closing this PR as this issue should be fixed in a more general way - to handle different custom styles which users can add to their text and are lost on pasting.

grzegorz_marzencki added 2 commits November 30, 2020 22:10

adding span to schema in order to handle text color. Remove backgorun…

b1c9e8f

…d color in order not to paste text with the selection highlight

adding possibility to paste coloured text

abc73fc

grzim requested a review from ellatrix as a code owner December 1, 2020 14:56

adding custom html tag in order to preserve information about text color

48001b9

grzim requested review from adamziel and draganescu December 1, 2020 14:59

grzim changed the title ~~Copy paste is losing information about text color~~ Retain text color when pasting Dec 1, 2020

fix in destructuring

7d2825e

adamziel reviewed Dec 1, 2020

View reviewed changes

ellatrix requested changes Dec 3, 2020

View reviewed changes

grzegorz_marzencki added 2 commits December 8, 2020 02:47

file refactor

bbdfa3b

sketch - instead of manualy writing schemas, another solution propose…

7d37efe

…d - 1. get default styling (black text) 2. diff the default styling with styles of pasted element to get all nondefault styles applied to element 3. Sanitize output 4. paste the output

annezazu added the [Feature] Paste label Jan 13, 2021

draganescu added the Needs Technical Feedback Needs testing from a developer perspective. label Feb 17, 2021

Base automatically changed from master to trunk March 1, 2021 15:44

grzim closed this Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retain text color when pasting #27399

Retain text color when pasting #27399

grzim commented Dec 1, 2020 •

edited

Loading

adamziel Dec 1, 2020

grzim Dec 1, 2020 •

edited

Loading

adamziel Dec 1, 2020 •

edited

Loading

adamziel Dec 1, 2020

grzim Dec 1, 2020 •

edited

Loading

ellatrix Dec 3, 2020

adamziel commented Dec 2, 2020 •

edited

Loading

grzim commented Dec 2, 2020 •

edited

Loading

ellatrix left a comment

grzim commented Dec 3, 2020 •

edited

Loading

ellatrix commented Dec 6, 2020

grzim commented Dec 7, 2020

grzim commented Dec 14, 2020 •

edited

Loading

ellatrix commented Dec 15, 2020

grzim commented Dec 15, 2020

ellatrix commented Feb 25, 2021

grzim commented Mar 2, 2021

grzim commented Mar 2, 2021 •

edited

Loading

Retain text color when pasting #27399

Retain text color when pasting #27399

Conversation

grzim commented Dec 1, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

adamziel Dec 1, 2020

Choose a reason for hiding this comment

grzim Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

adamziel Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

adamziel Dec 1, 2020

Choose a reason for hiding this comment

grzim Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

ellatrix Dec 3, 2020

Choose a reason for hiding this comment

adamziel commented Dec 2, 2020 • edited Loading

grzim commented Dec 2, 2020 • edited Loading

ellatrix left a comment

Choose a reason for hiding this comment

grzim commented Dec 3, 2020 • edited Loading

ellatrix commented Dec 6, 2020

grzim commented Dec 7, 2020

grzim commented Dec 14, 2020 • edited Loading

ellatrix commented Dec 15, 2020

grzim commented Dec 15, 2020

ellatrix commented Feb 25, 2021

grzim commented Mar 2, 2021

grzim commented Mar 2, 2021 • edited Loading

grzim commented Dec 1, 2020 •

edited

Loading

grzim Dec 1, 2020 •

edited

Loading

adamziel Dec 1, 2020 •

edited

Loading

grzim Dec 1, 2020 •

edited

Loading

adamziel commented Dec 2, 2020 •

edited

Loading

grzim commented Dec 2, 2020 •

edited

Loading

grzim commented Dec 3, 2020 •

edited

Loading

grzim commented Dec 14, 2020 •

edited

Loading

grzim commented Mar 2, 2021 •

edited

Loading