Writing Flow/Rich Text: unify split logic #54543
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ellatrix
changed the title
|
Size Change: +665 B (+0.04%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in f950d2f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8922587588
|
ellatrix
force-pushed
the
try/unify-split
branch
from
e317df8
to
193ba19
Compare
ellatrix
changed the title
ellatrix
force-pushed
the
try/unify-split
branch
from
f4949be
to
92a26b3
Compare
ellatrix
added
[Type] Bug
ellatrix
force-pushed
the
try/unify-split
branch
from
ef85e8d
to
329a41d
Compare
ellatrix
requested review from
fluiddot,
dcalhoun,
kevin940726,
Mamaduka,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
ellatrix
commented
Oct 9, 2023
ellatrix
force-pushed
the
try/unify-split
branch
from
6dab2f8
to
817802d
Compare
richtabor
approved these changes
Oct 10, 2023
Mamaduka
reviewed
Oct 12, 2023
There was a problem hiding this comment.
Maybe we should leave the onSplit callback if the block has more specific needs for this logic.
A good example would be the button block. Currently, it copies all attributes on the split action, including URLs. Which I think is a bug; only styling attributes and text included on the split action should be copied.
|
@Mamaduka Right, that's a good point. I don't like keeping onSplit though, maybe you should be able to opt-out of attributes in the block supports key? Or opt-in? What do you think? Or maybe a transform? |
|
@ellatrix, I thought we'd have to support the legacy method for backward compatibility. |
|
@Mamaduka I meant, how to incorporate it in the new way? |
|
It seems that due to this PR, when you paste text into a text field inside a block, it is always converted to a Paragraph block. See #61377 |
|
Good catch, @t-hamano! It's odd that the e2e tests missed this; we have an extensive suite for the Embed block. When the issue is fixed, we should probably introduce a new testxed, maybe using the RSS block. |
Mamaduka
reviewed
May 7, 2024
Comment on lines
+35
to
+36
| defaultView.addEventListener( 'copy', onCopy ); | ||
| defaultView.addEventListener( 'cut', onCopy ); |
There was a problem hiding this comment.
I believe this change produces a TypeError: Cannot read properties of null (reading 'addEventListener') error. I'm having trouble reproducing it manually, but it's consistent when running e2e tests.
npm run test:e2e -- -g "can be inserted after refresh"
Screenshot
49 tasks
Merged
3 tasks
Merged
|
This comment is a followup and context for my comment on the WP Dev Blog post that mentions this change, where I expressed confusion about this logic and its settings in My comment, for convenience:
Seeing this comment, @ellatrix asked to see my code to help clarify. Here is my block code:
{
"$schema": "https://schemas.wp.org/trunk/block.json",
"apiVersion": 3,
"name": "baz-qux/page-tagline",
"title": "Page Tagline",
"category": "baz-qux",
"icon": "megaphone",
"description": "A short introduction to the page.",
"keywords": ["intro", "excerpt", "description"],
"supports": {
"anchor": false,
"align": false,
"splitting": false
},
"editorScript": "file:./page-tagline.js"
}
import metadata from './block.json';
const { registerBlockType } = wp.blocks;
const { useBlockProps, RichText } = wp.blockEditor;
registerBlockType( metadata.name, {
attributes: {
content: {
type: 'string',
source: 'text',
selector: 'p'
}
},
edit: (props) => {
return <RichText
{ ...useBlockProps() }
tagName="p"
className="wp-block wp-block-paragraph page-tagline"
value={ props.attributes.content }
placeholder="Add tagline"
onChange={ ( newContent ) => props.setAttributes({ content: newContent }) }
/>;
},
save: (props) => {
return <RichText.Content
{ ...useBlockProps.save() }
tagName="p"
className="page-tagline"
value={ props.attributes.content }
/>;
}
} );Thank you for your help! |
|
Hi! I tried your code, and for me it looks like
Now, you are right that setting <RichText
{ ...useBlockProps() }
identifier="content"This should allow the block to be split into two blocks. We should indeed mention that in documentation. It's also worth noting that if you'd like to allow merging these blocks back together, you should provide a merge( attributes, attributesToMerge ) {
return {
content:
( attributes.content || '' ) +
( attributesToMerge.content || '' ),
};
},
edit: function Edit( props ) {
return (
<RichText
{ ...useBlockProps() }
identifier="content"
onMerge={ props.mergeBlocks } |
|
I've added docs here: #63016 |
|
Thank you for the followup! This is helpful. I am now getting the behavior I expect, and it's nice that hitting Enter at the end of a block that has (Am I correct in assuming that the One more thing: I'm unclear from the discussion on this ticket whether |
|
As far as I know, there's no
|
3 tasks
widoz
added a commit
to widoz/gutenberg
that referenced
this pull request
Jul 22, 2024
widoz
added a commit
to widoz/gutenberg
that referenced
this pull request
Jul 22, 2024
ellatrix
pushed a commit
that referenced
this pull request
Jul 23, 2024
Co-authored-by: widoz <[email protected]>
Open
2 tasks
2 tasks
|
Another regression: It appears that it is no longer possible to paste text from outside WordPress into non-modal classic block. See #64771. |
2 tasks
|
@ellatrix Can we please have some input here? #65274 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What/why?
Fixes #53076: when pasting blocks, the split block should be merged with the pasted blocks.
This PR aims to unify split logic (enter key and paste). Currently we have logic both in the block editor generally and rich text specifically when it contains selection. This PR moves all the logic to the block editor (writing flow), adding a block.json setting that tells us if we can split a block.
This removes the need for the block to pass an
onSplitfunction, the behaviour can be turned on with just a setting. This is also quite convenient for extenders wishing to recreate the core blocks.This will also allow us to solve splitting for paste more easily, which is actually a split operation + merging the ends together.
How?
One implementation details is that change of how rich text listens for key events. Right now in trunk, the event listener is attached to the rich text node, so writing flow (listening on a wrapper node) cannot prevent default behaviour. To fix this, rich text can listen to the document instead. This is how it works for browser behaviour too. Default behaviour is cancelable until the event bubbled all the way through the DOM. Rich text is meant to be a sort of polyfill for the native
contenteditableAPI: actions should be cancelable.Testing Instructions
Copy a few paragraphs and paste them into another paragraph. The result should be:
Testing Instructions for Keyboard
Screenshots or screencast