-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Only use rich text value internally to the RichText component #10439
Conversation
@youknowriad This may be of interest to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does feel nicer from the block's author pov to only have to deal with html
strings. (I proposed this a long time ago :P) But what about performance? Having to serialize/parse html for RichText on each render seems overkill?
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
export { matcher as node } from './node'; | ||
export { matcher as children } from './children'; | ||
|
||
export function richText( selector, multilineTag ) { | ||
export function html( selector, multilineTag ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the regular html
matcher from hpq
is not sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this one would accept multilineTag
as an argument for multiline
RichText values. Otherwise we'd have to go back to the query
matcher and map the values to and from RichText
. I don't mind doing that with strings though. Before this was an issue because it would create a new array or object every time, but with a string we can easily compare.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining, I'm not too concerned about that. multiline
seems fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit strange to me as well, since it's something of a preprocessing filter that doesn't serve much purpose outside the RichText use-case.
Curious: What happens if we don't have this at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would include e.g. the cite. As I said, the alternative would be to revert to using query and map the values on each render. I like that solution less, but since it's just a string now I don't care as much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're looking at this too much through the lens of Rich Text. If we need the matcher to support filtering, that's one thing. But on that basis alone, "multiline" doesn't make much sense to me. I'd rather some general support of aggregates, a property which more accurately reflects the child selector (e.g. #6716 (comment)), or reverting to query as you mention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where will this discussion continue? Are we going to launch 4.0 with the multiline
property for html
attribute sources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally thought it was a good compromise as opposed to the "artificial" key we had to use when using the query
matcher. And I thought this "multiline" option was supposed to fix that.
Honestly I think it is negligible, but we can do some testing. The only addition would be serialising to HTML on input and this should be very fast. Additionally this only happens per RichText instance, not the whole block or whole editor content. |
0fed26a
to
8ed4ae2
Compare
8ed4ae2
to
4e1599c
Compare
I'm still hesitant between this and #10370. Regardless, I think we should do something in The last hesitation for me would be the performance, so we do when do we do this parsing/serialization:
How does this differ from having the attribute as the internal representation directly? Also pinging @aduth for a second opinion. |
I pushed a commit on this branch making the current templates work:
|
Last thing here, I'd like to add is to remove the "children" deprecations for now. My idea is the following:
|
68bba9d
to
9ec19f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many subtle differences in the output html produced for snapshots in unit tests which raises questions why it has changed.
@@ -63,7 +63,7 @@ describe( 'splitting and merging blocks', () => { | |||
await page.keyboard.press( 'Backspace' ); | |||
|
|||
// Replace contents of first paragraph with "Bar". | |||
await pressTimes( 'Backspace', 3 ); | |||
await pressTimes( 'Backspace', 4 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's surprising, any ideas why it needs to be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caret is now placed outside the inline boundary so it requires one more backspace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that its strictly an issue here, but caret placement in and around inline boundaries has been historically problematic and inconsistent and we should be careful to avoid worsening this. e.g. in some cases depending on internal state of TinyMCE, caret will still move inside inline boundary when backspacing (e.g. if Enter + Backspace when at end of a paragraph inside inline boundary).
Related: #9040 (internal state not an issue if its not preserved)
exports[`adding blocks should clean TinyMCE content 1`] = `""`; | ||
exports[`adding blocks should clean TinyMCE content 1`] = ` | ||
"<!-- wp:paragraph --> | ||
<p></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why it's different?
@@ -10,7 +10,7 @@ exports[`templates Using a CPT with a predefined template Should add a custom po | |||
<!-- /wp:paragraph --> | |||
|
|||
<!-- wp:quote --> | |||
<blockquote class=\\"wp-block-quote\\"><p></p></blockquote> | |||
<blockquote class=\\"wp-block-quote\\"></blockquote> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why it no longer prints <p />
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the initial content of quote will be undefined. This is how it was before the rich text value merge:
@@ -179,7 +178,7 @@ export class RichText extends Component { | |||
} ) ); | |||
|
|||
// Save back to HTML from React tree | |||
const html = '<' + tagName + '>' + toHTMLString( value ) + '</' + tagName + '>'; | |||
const html = '<' + tagName + '>' + value + '</' + tagName + '>'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hypest can you help testing if this still works with mobile app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested against the current mobile app master (wordpress-mobile/gutenberg-mobile@6f5e350) and works fine 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️ to both of you.
value: '', | ||
}; | ||
|
||
RichTextContainer.getTextContent = ( html ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like RichText
component should be moved to @wordpress/rich-text
package to avoid this duality where you can have two version of each of the public methods:
import { join } from '@wordpress/rich-text';
import { RichText } from '@wordpress/editor';
join === RichText.join; // false
It feels like with the changes proposed join
should remain private, and RichText.join would be the only public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has changed now.
}, | ||
}, | ||
], | ||
|
||
merge( attributes, attributesToMerge ) { | ||
return { | ||
content: concat( attributes.content, attributesToMerge.content ), | ||
content: RichText.concat( attributes.content, attributesToMerge.content ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between this and attributes.content + attributesToMerge.content
?
Will that difference be obvious to someone who thinks that they are working with strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually changing this right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
export { matcher as node } from './node'; | ||
export { matcher as children } from './children'; | ||
|
||
export function richText( selector, multilineTag ) { | ||
export function html( selector, multilineTag ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit strange to me as well, since it's something of a preprocessing filter that doesn't serve much purpose outside the RichText use-case.
Curious: What happens if we don't have this at all?
@@ -191,15 +191,13 @@ export function matcherFromSource( sourceConfig ) { | |||
|
|||
return matcher; | |||
case 'html': | |||
return html( sourceConfig.selector ); | |||
return html( sourceConfig.selector, sourceConfig.multiline ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious what purpose multiline
serves when considering html
in isolation outside the RichText use-case. We're kinda muddying the distinction between these sources.
} | ||
|
||
const content = <RawHTML>{ html }</RawHTML>; | ||
|
||
if ( Tag ) { | ||
return <Tag { ...props }>{ content }</Tag>; | ||
return <Tag { ...omit( props, [ 'format', 'multiline' ] ) }>{ content }</Tag>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these considered deprecated? Will they ever be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove multiline as it was added after the last release.
…nsure we have the correct API
cef3ef9
to
643133b
Compare
Previously: #5380 (Some "gotchas" which might be relevant here) |
Reiterating Riad's point, this is certainly an improvement from the developer experience angle over what we'd be introducing with Trying to understand the performance impact, I think the impact is an acceptable compromise. It makes me wonder what options we might have for future optimization: If I press a single character, shouldn't this be able to be applied to the internal representation easily without re-parsing/filtering the element in its entirety? Not saying that needs to be done here or even immediately after, but if that's an idea which has been considered to be pursued, I could see how it would offset (or even produce net benefit to) the performance cost paid here. Re: #5380 and with regard to other ongoing work with annotations, do we plan to support the use case where developers can interact with the rich text value if they need to? I suppose that is a matter of using the Interestingly one of the issues with #5380 which led to its closing was the need for a custom serializer, which we now have (twice over). In all, I think we're in a much better position than we were in #5380 with our separations ( |
docs/reference/deprecated.md
Outdated
- `wp.blocks.children.fromDOM` has been removed. Please use `wp.richTextValue.create` instead. | ||
- `wp.blocks.children.concat` has been removed. Please use `wp.richTextValue.concat` instead. | ||
- `wp.blocks.children.getChildrenArray` has been removed. Please use `wp.richTextValue.create` instead. | ||
Gutenberg's deprecation policy is intended to support backwards-compatibility for two minor releases, when possible. The current deprecations are listed below and are grouped by _the version at which they will be removed completely_. If your plugin depends on these behaviors, you must update to the recommended alternative before the noted version.s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a trailing 's' here :)
Yes, I've been thinking about this too. Some browsers pass extra info to the input event which we could use to apply it immediately to the internal state.
Yes, through the formatting API (and perhaps and additional future ones) you'll be able to interact with the internal rich text values with helpers. Nothing changes here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- New post
- Enter title
- Click writing prompt
- Cmd+B to bold
- Click "Save draft"
- Reload page
Expected: One of either:
- Paragraph block with empty strong tag
- No post content (per consistent with Editor: Consider single unmodified default block as empty content #9808)
Actual: Empty paragraph, no strong
.
Related to writing flow E2E change.
@@ -203,7 +203,7 @@ export const settings = { | |||
|
|||
return ( href && | |||
<div> | |||
{ ! isEmpty( fileName ) && | |||
{ ! RichText.isEmpty( fileName ) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fileName
is a string. Why should we need utility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could get rid of the is empty utility as well now. I'm fine either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering them as strings, I see no need for the utility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- New post
- Click writing prompt
- Cmd+B to bold
- Type something
- Press Enter
- Press Backspace
- Press Enter
Expected: Typing in second paragraph.
Actual: Second paragraph created, but caret remains in first.
Works as expected in 3.9.0.
Should have E2E case (could extend "should merge into inline boundary position")
Thanks for the tests @aduth, addressing them now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
I acknowledge that there's still a small bug when merging blocks. Let's create an issue for it and address it separately.
Some issues address, the other will be addressed separately.
When a block attribute was for storing rich text value I used
Does that mean any old blocks that stored the rich text value as an array (in block comments, not using
but when the block was previously created by a template for example:
it will be stored as an array
Is that block going to be invalid somewhere down the line? What is the alternative way to create templates now? How do I convert this to new format for example:
Or can I keep using this format? I'm so confused about these rich text changes, 4.0 broke everything even though I thought it was going to be backwards compatible. (Every block got |
@ktmn Sorry for the breakage. It was meant to be backward compatible but we probably missed something there, a bug. Can I suggest that you you try Gutenberg RC's before their release. We did these RC periods specifically to gather feedback about things we missed? That said, can you create a separate issue with a small block example and the corresponding template that broke so we could take a look? |
@youknowriad Yeah it's my bad for missing the RC, never followed the WordPress blog to see it's announcement. I'm just trying to figure out what the attribute type for rich text needs to be and how to do the new templates.
For example this plugin with Gutenberg 4.0, create an "Advanced button", add some text to it, save and refresh. Oh, and old templates "break" if the rich text attribute is |
So here's the best path forward in term of blocks:
We did manage to try to transform array in templates automatically to strings when the source is How's that look for you? |
What I am wondering is why your blocks without any change broke the templates as we tried to keep the |
In short, this is how my block looked like: {
attributes: {
content: {
type: 'array',
},
},
edit: ({attributes, setAttributes}) => {
return (
<div className='custom-block'>
<wp.editor.RichText
value={attributes.content}
onChange={value => setAttributes({content: value})}
/>
</div>
)
},
save: ({attributes}) => {
return (
<div className='custom-block'>
{attributes.content}
</div>
)
},
} After 4.0.0 that produced So I changed it to string: {
attributes: {
content: {
type: 'string',
},
},
...
} and ran into issues with html being escaped, such as bold text or multiline. {
attributes: {
content: {
type: 'array',
},
},
edit: ({attributes, setAttributes}) => {
return (
<div className='custom-block'>
<wp.editor.RichText
value={attributes.content}
onChange={value => setAttributes({content: value})}
/>
</div>
)
},
save: ({attributes}) => {
return (
<div className='custom-block'>
<wp.editor.RichText.Content value={attributes.content} />
</div>
)
},
} Then I have a block that is basically a preset of blocks, so it looked something like this: {
attributes: {},
edit: ({attributes, setAttributes}) => {
return (
<div className='my-other-block'>
<wp.editor.InnerBlocks
template={[
['custom/block', {
content: ['Content'],
}],
]}
/>
</div>
)
},
save: ({attributes}) => {
return (
<div className='my-other-block'>
<wp.editor.InnerBlocks.Content />
</div>
)
},
} Notice the content in the template is type So to solve that I removed the type from {
attributes: {
content: {},
},
...
} Now it works. It can use string from I'm not saying anything is bugged, this is just how it went down for me. Now I'm just wondering:
|
It's correct but I expect the first issue is that the block was initially doing something wrong: You're rendering the content in html and at the same time saving it to the block's wrapping comment (because there's no source). You could try adding |
I do that because the selectors are not capable handling more complex blocks. For example if I have a button: <div class="button">
Content
</div> it can work easily with this attribute: {
content: {
source: 'html', // not 'children' anymore, right?
selector: '.button',
},
} but when there's a block control that allows to add an icon for example, making the output like this: <div class="button">
Content
<i class="icon banana"></i>
</div> I can no longer fetch only the content without having to wrap it first in a Now maybe the button also has a toggle control allowing the button to only show the icon, resulting in this render: <div class="button">
<i class="icon banana"></i>
</div> then the content would be gone, even if the user just tried to test what this option looks like. Of course I use the For string rich text |
Gotcha, thanks for the explanation. There's something we need to check separately. An issue with this block would be good to check why the template is not working.
Separately, I'd advocate to transform everything into string including the template content. |
In the attributes, The source of `children` is no longer supported and should be changed to `html` along with the type changing to `string`. See PR comment here: WordPress/gutenberg#10439 (comment)
* Add the block.json schema to each block.json file Added schea defintion to each file, this allows validation and auto complete in supported browers. "$schema": "https://json.schemastore.org/block.json" Required fixing the textDomain => textdomain Highlighted an issue with `source: children` not documented. * Switch type/source to string/children In the attributes, The source of `children` is no longer supported and should be changed to `html` along with the type changing to `string`. See PR comment here: WordPress/gutenberg#10439 (comment)
* Add the block.json schema to each block.json file Added schea defintion to each file, this allows validation and auto complete in supported browers. "$schema": "https://json.schemastore.org/block.json" Required fixing the textDomain => textdomain Highlighted an issue with `source: children` not documented. * Switch type/source to string/children In the attributes, The source of `children` is no longer supported and should be changed to `html` along with the type changing to `string`. See PR comment here: WordPress/gutenberg#10439 (comment)
Description
This is a proposal to use the string format on the RichText component by default.
Benefits:
createBlock
to create templates.Cons:
How has this been tested?
Screenshots
Types of changes
Checklist: