Rich text: formats: allow format to filter value when changing tag name #35516
Conversation
ellatrix
added
[Type] Bug
|
Size Change: +86 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
fullofcaffeine
force-pushed
the
fix/rich-text-format-upgrade
branch
from
905195a to
0dd8577
Compare
| __unstableFilterAttributeValue( key, value ) { | ||
| if ( key !== 'style' ) return value; | ||
| if ( value && ! value.includes( 'background-color' ) ) return value; | ||
| const addedCSS = [ 'background-color', 'rgba(0, 0, 0, 0)' ].join( ':' ); |
There was a problem hiding this comment.
Nit and curious: why the join( ':' ) and not just a single string?
There was a problem hiding this comment.
It's probably keeping with the style used in ./inline.js. But personally I'd avoid it in such a situation, since I don't know how smart the VM is.
| @@ -29,7 +29,7 @@ import { __ } from '@wordpress/i18n'; | |||
| */ | |||
| import { textColor as settings } from './index'; | |||
|
|
|||
| function parseCSS( css = '' ) { | |||
| export function parseCSS( css = '' ) { | |||
There was a problem hiding this comment.
Is this export really needed? I couldn't find usage of this function outside of this module.
There was a problem hiding this comment.
Maybe a dev artefact? I would remove it too.
|
|
||
| for ( const key in formatType.attributes ) { | ||
| const name = formatType.attributes[ key ]; | ||
| registeredAttributes[ key ] = _attributes[ name ]; |
There was a problem hiding this comment.
What are registered and unregistered attributes in this context?
There was a problem hiding this comment.
unregistered attributes are attributes that the format has but not registered to it. It basically ensures that, if you e.g. add a style or data attribute to a strong tag, it will be preserved.
packages/rich-text/src/create.js
Outdated
| registeredAttributes[ key ] = attributes[ name ]; | ||
| } else { | ||
| unregisteredAttributes[ name ] = attributes[ name ]; | ||
| if ( ! _attributes[ name ] ) continue; |
There was a problem hiding this comment.
Without this line, registeredAttributes would have the following shape in some circumstances:
id: undefined
target: undefined
type: undefined
url: "https://wordpress.org/gutenberg"
[[Prototype]]: Object
Notice the id, target, and type keys. They're all undefined. This caused an exception (I don't have the backtrace handy at the moment and I don't recall exactly where) that would make the inputted character to not to be registered in the editor (hence not part of the block markup).
Not sure how serious this was, but I guess this could cause more data loss in certain scenarios as shown by the previously failing tests. This is what was causing some tests to fail the snapshot comparison due to differences in the markup (missing a certain char). Let me know what you think about the fix!
There was a problem hiding this comment.
The continue solution fixed the undefined issue, but caused the loop to be skipped in some cases which meant it wouldn't fix the background-color because _unstableFilterAttributeValue wouldn't be called. I've changed it to e02f694, which seems to fix the undefined value problem depicted above and the original mark background-color issue, too.
| @@ -127,7 +127,8 @@ export const textColor = { | |||
| }, | |||
| __unstableFilterAttributeValue( key, value ) { | |||
| if ( key !== 'style' ) return value; | |||
| if ( value && ! value.includes( 'background-color' ) ) return value; | |||
There was a problem hiding this comment.
It seems the logic was inverted here... AFAIU we shouldn't be negating the second expression, right? 🤔
| __unstableFilterAttributeValue( key, value ) { | ||
| if ( key !== 'style' ) return value; | ||
| if ( value && ! value.includes( 'background-color' ) ) return value; | ||
| const addedCSS = [ 'background-color', 'rgba(0, 0, 0, 0)' ].join( ':' ); |
There was a problem hiding this comment.
It's probably keeping with the style used in ./inline.js. But personally I'd avoid it in such a situation, since I don't know how smart the VM is.
| @@ -29,7 +29,7 @@ import { __ } from '@wordpress/i18n'; | |||
| */ | |||
| import { textColor as settings } from './index'; | |||
|
|
|||
| function parseCSS( css = '' ) { | |||
| export function parseCSS( css = '' ) { | |||
There was a problem hiding this comment.
Maybe a dev artefact? I would remove it too.
fullofcaffeine
force-pushed
the
fix/rich-text-format-upgrade
branch
from
478c73c to
5443df2
Compare
|
Do not merge yet! We found another issue while testing. If a post is saved pre-11.7 with a selection with a custom or preset color, it will end up looking as expected in the editor when opened in an instance with this fix applied. However, the actual block markup is not updated with the fixes applied at runtime by the logic here. Since it doesn't get persisted, the actual selection will (probably) look wrong in the frontend. |
|
BTW can we add test fixtures for the problematic block markup to cover the issue that this is fixing? 🤔 |
|
Size Change: +174 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
fullofcaffeine
force-pushed
the
fix/rich-text-format-upgrade
branch
from
9bc623a to
a5ff3bb
Compare
Existing CSS rules already end with a semicolon, and joining it with the `background-color` rule would result in something like: ``` color:#e511be;;background-color:rgba(0, 0, 0, 0) ``` Which is benign AFAIK, but doesn't look right. Best to prepend the `background-color` rule, since it doesn't include a `;`, but the `join` will take care of adding it when joining it with the other existing rules, if any.
fullofcaffeine
force-pushed
the
fix/rich-text-format-upgrade
branch
from
d347e62 to
077d753
Compare
|
This is testing well for me - content added in 11.6 gets highlighted when viewed in the editor on 11.7.0, and isn't properly converted to Once this patch is applied, however, the editor's I also tested to confirm that any background colors added while on 11.7.0 are not effected by this patch. For both provided presets and custom colors, the transparent background was not applied, and existing backgrounds were respected after the patch was applied. |
|
My gut feeling is that some of these changes are happening at a lower level than they should be -- especially the However, I don't have any evidence to back up that gut feeling. It's really just a feeling that there must be e.g. some sort of allowlist somewhere to which we can add, say, I hope that another @WordPress/gutenberg-core member with more of a clue about RichText will review this and maybe have some suggestions 😅 |
Thanks for chiming in Bernie! And no problem, I totally understand. I'm also aware this might not be an ideal / permanent solution, but I hope it paves the way to one. Perhaps it could also be used as an interim solution depending on how things go.
I'll try to test that assumption! |
A quick test doesn't seem to validate the allowlist assumption. I reverted the changes in this changeset and changed from Another aspect of this is that, without the fix here, |
There was a problem hiding this comment.
Hi @ellatrix, @fullofcaffeine, @mcsf, @ockham, thank you all for the work in fixing this issue!
I pasted created a post with the following markup:
<!-- wp:paragraph -->
<p><mark style="background-color:rgba(0, 0, 0, 0)" class="has-inline-color has-purple-color">dfgdf</mark></p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p><mark style="background-color:rgba(0, 0, 0, 0);color:#dd2727" class="has-inline-color">dfdfd</mark></p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p><mark style="background-color:#EEEADD" class="has-inline-color">dfdfd</mark></p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p><mark style="background-color:#EEEADD" class="has-inline-color">dfd</mark></p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>fg<mark style="background-color:#eb0505" class="has-inline-color">f</mark></p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>xzfd <span class="has-inline-color has-green-color">f</span><span class="has-inline-color has-red-color">ghfgh</span></p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p><span style="color:#08a5e9" class="has-inline-color">gfhfg</span></p>
<!-- /wp:paragraph -->
The last two paragraphs are old text color markup on trunk they appear on the editor in yellow. In this branch both appear correctly but there is something strange <p><span style="color:#08a5e9" class="has-inline-color">gfhfg</span></p>get migrated to a mark markup while <p>xzfd <span class="has-inline-color has-green-color">f</span><span class="has-inline-color has-red-color">ghfgh</span></p>keeps the span tag. I think we may have a bug in the migration logic where preset colors are not migrated to marks?
But given that it has no visual impact and the issue is affecting the users, I think we can merge this PR as a temporary solution given that for the users, everything seems to work well.
In the future, I think we should explore other solutions. Namely, when we apply a text color (that may even be applied using a class), we should not add an inline style every time just to make the background transparent. I think we should add the following style on editor and frontend:
.has-inline-color:not([class*=background-color]) { background-color:rgba(0, 0, 0, 0) }
That rule will avoid adding repeated transparent color inline styles and will make the markup cleaner. We will also remove the need to, during the init phase, apply a background color because the mark is not yellow anymore.
If later we find out we need a programmatic format change on the init, I think a possible higher-level solution going in the direction of what @ockham said would be to on the Format component implementation packages/format-library/src/text-color/index.js, TextColorEdit component have an useEffect call that inspects the value prop. If the useeffect logic finds we need a programmatic change, the useEffect would call onChange with the changes we need.
I found other issues not related to this PR:
- When we apply a preset background color we are not applying the color using a class as happens on the text color. If the preset colors later change, we will have text colors being updated, but the background will not be updated, which may cause design inconsistencies.
- On block colors, when there is a text color, we add has-text-color class. When there is a background color, we add a has-background class. On the format, we add has-inline-color for background or text color. Should we have two distinct classes to be consistent with what happens at the block level?
…me (#35516) * Rich text: formats: allow format to filter value when changing tag name * Do not add a transparent background-color if already set * Fix undefined values in the list of attributes, which would cause the rich-text component to break * Do not skip the entire loop iteration if key does not exist in _attributes * Format library: text-color: Explain use of __unstableFilterAttributeValue * Remove `export` for `parseCSS` * Make sure to delete undefined registeredAttributes * Handle format attributes on rich-text init * Only handle changes for `text-color` formats upon init * Be a bit more defensive when accessing record.current.formats * Prepend the `addedCSS` to prevent double `;` Existing CSS rules already end with a semicolon, and joining it with the `background-color` rule would result in something like: ``` color:#e511be;;background-color:rgba(0, 0, 0, 0) ``` Which is benign AFAIK, but doesn't look right. Best to prepend the `background-color` rule, since it doesn't include a `;`, but the `join` will take care of adding it when joining it with the other existing rules, if any. Co-authored-by: Marcelo Serpa <[email protected]> Co-authored-by: Miguel Fonseca <[email protected]>
|
This has been cherry-picked into the |
|
We're adding tests for this in #35847. |
…me (#35516) * Rich text: formats: allow format to filter value when changing tag name * Do not add a transparent background-color if already set * Fix undefined values in the list of attributes, which would cause the rich-text component to break * Do not skip the entire loop iteration if key does not exist in _attributes * Format library: text-color: Explain use of __unstableFilterAttributeValue * Remove `export` for `parseCSS` * Make sure to delete undefined registeredAttributes * Handle format attributes on rich-text init * Only handle changes for `text-color` formats upon init * Be a bit more defensive when accessing record.current.formats * Prepend the `addedCSS` to prevent double `;` Existing CSS rules already end with a semicolon, and joining it with the `background-color` rule would result in something like: ``` color:#e511be;;background-color:rgba(0, 0, 0, 0) ``` Which is benign AFAIK, but doesn't look right. Best to prepend the `background-color` rule, since it doesn't include a `;`, but the `join` will take care of adding it when joining it with the other existing rules, if any. Co-authored-by: Marcelo Serpa <[email protected]> Co-authored-by: Miguel Fonseca <[email protected]>
| record.current?.formats[ 0 ]?.[ 0 ]?.type === 'core/text-color'; | ||
|
|
||
| if ( hasRelevantInitFormat ) { | ||
| handleChangesUponInit( record.current ); |
There was a problem hiding this comment.
@fullofcaffeine Why is this needed? We shouldn't dirty on init.
There was a problem hiding this comment.
When we upgrade a span to a mark, it's only visible in the editor. The front-end will still be a span and that's fine. Only when the user changes something in the rich text field will the change be made.
There was a problem hiding this comment.
When we upgrade a span to a mark, it's only visible in the editor. The front-end will still be a span and that's fine. Only when the user changes something in the rich text field will the change be made.
There was a problem with the blocks in the editor not being persisted to the block editor's HTML markup. For instance, the programmatically added background-color: rgba(0, 0, 0 ,0) rule wasn't being persisted to the code markup at all even after editing something else/saving the post and this fixed it*. Follow these steps to reproduce:
- Checkout
v11.6.0and build; - Start a new post, go to the code editor, add two paragraphs: "Custom by 11.6" and "Preset by 11.6". With the "Text color" tool, set a custom color for the first one and a preset for the other. Save and Publish.
- Checkout
v11.7.0and build; - Open the same post, add two paragraphs below the others: "Custom by 11.7" and "Preset by 11.7". With the "Highlight" tool, set a custom color for the first one and a preset for the other. Save and Publish.
- Checkout
cbcdfd67f6(a commit before I added thehandleChangesUponInit), build; - When you load the same post, you'll see something like this:
Peek.2021-10-22.10-55.mp4
At first glance, it all looks good in the editor. However, the second paragraph still has a yellow mark on the frontend. By looking at the code markup in the code editor, you'll see that the transparent background color is not there. If you edit the post*, it will still not sync up the markup, it will still look broken in the frontend.
*It does sync up if I edit the same paragraph, but that's less than ideal, as it'd require the user to type in each one of them to make the onChange to run. That wouldn't scale well in a long post with hundreds or maybe thousands of them.
I'm aware that the handleChangesUponInit change is hacky and less than ideal, but it seemed to work well as an interim solution. Feel free to change it as you see fit, I'll also be following up to pick up your brain on it :)
There was a problem hiding this comment.
That's what it's meant to do. It's not meant to sync up until the user edits the same paragraph. Sure, the old colours that were added will not update to mark, but that's fine. All that matters is that they look visually correct in the editor.
There was a problem hiding this comment.
That's what it's meant to do. It's not meant to sync up until the user edits the same paragraph. Sure, the old colours that were added will not update to mark, but that's fine. All that matters is that they look visually correct in the editor.
That makes sense! I think the scale of the issue (i.e in number of paragraphs affected) wasn't as big as I thought then. I think it'd be possible to use the solution without syncing upon init, but instruct users to click each affected paragraph, then 🤔 - it'd require more work from users, though. I completely agree that for the long term this shouldn't be here, thanks for creating the follow-up!
Description
Fixes #34680 (comment).
Problem: when changing the tag name from
spantomarkfor the highlight format the transparent background color is not added for old highlights that have just a text colour and no background color. The transparent background color is needed to reset the default browser style.Solution: allow a format type to filter the value to add a transparent background color if none is present.
I'm not quite sure about the future of this API, so I marked it as unstable.
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).