Quote v2: retain selection after transform #39838
Conversation
ellatrix
added
[Feature] Rich Text
| // then recover it. | ||
| const START_OF_SELECTED_AREA = '\u0086'; | ||
|
|
||
| function findSelection( blocks ) { |
There was a problem hiding this comment.
While this is the only use case for nested blocks, there may be value in reusing this function in other places where we use the START_OF_SELECTED_AREA trick.
|
Size Change: +103 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
| ); | ||
|
|
||
| if ( attributeKey ) { | ||
| return [ blocks[ i ].clientId, attributeKey, 0 ]; |
There was a problem hiding this comment.
If I'm not wrong this returns the attribute name as "identifier". AFAIK, this has never been enforced anyway. A block can use an "identifier" in a RichText that is completely different than the RichText attribute name. In fact, I think assuming there's a 1-1 relationship between attributes and RichText is a wrong assumptions. One could use multiple RichText in a single attribute... or transform RichText values...
Maybe a better solution here would be to only update the selection (and focus the RichText) when the RichText rerenders internally and detects that the invisible character is in the received value prop. It's also a solution that is block-independent (can be bundled inside the raw RichText).
There was a problem hiding this comment.
AFAIK, this has never been enforced anyway.
Right, it's hard to enforce, but it should be the same as the attribute key. If you use a merge function in your block (or an input rule with this change), it has to be that way.
In fact, I think assuming there's a 1-1 relationship between attributes and RichText is a wrong assumptions. One could use multiple RichText in a single attribute... or transform RichText values...
Sure, you can do that. But if you're using merge functions you can't :)
Maybe a better solution here would be to only update the selection (and focus the RichText) when the RichText rerenders internally and detects that the invisible character is in the received value prop. It's also a solution that is block-independent (can be bundled inside the raw RichText).
My fear with that is that the character ends up in the block editor store. So far we've always taken it out before syncing to the store.
There was a problem hiding this comment.
@youknowriad We can also just return blocks[ i ].clientId and it will still work. The editor will still place focus on the first focusable element it will find (in theory less correct, but in this case it's the same thing).
There was a problem hiding this comment.
If you use a merge function in your block (or an input rule with this change), it has to be that way.
Just looked at the "merge" functions and it seem they make that assumption too but it seems wrong there as well. I think ideally even merge functions should just put the character in the right position instead of making that assumptions.
It seems to me we have two options:
1- Be opinionated and assume "merging" is only for full text blocks and enforce that the rule "identifier <=> attribute name" is enforced somehow (document, warn block authors...)
2- Be non-opinionated and let block authors define the desired behavior, basically there's two options here:
a- Use an invisible character that can end up in attributes...
b- Allow "merge" functions (and similar functions like transforms) return both "blocks" and a "selection" object.
My idealist mind think 2.B is the best option here conceptually but the pragmatist in me knows that it might be a big change with some backward compatibility challenges. So I'd be ok with 1 for now (this PR basically) but I think we need to be clear and explicit about the requirements here. The block author need to be aware of these more clearly and not something that is assumed.
There was a problem hiding this comment.
@youknowriad We can also just return blocks[ i ].clientId and it will still work. The editor will still place focus on the first focusable element it will find (in theory less correct, but in this case it's the same thing).
But then if a block has two RichText this breaks. If we're sticking with the opinionated approach, enforcing the attribute <=> identifier seems better but we should document and fail properly if not.
There was a problem hiding this comment.
Also, if we do keep the opinionated approach, why do we have merge functions as a block API in the first place? we can just have an attribute "name" in most/all cases.
There was a problem hiding this comment.
Yeah, agreed about failing properly. What I like about (1) is that its works right now and these merge/transform functions remain simple.
I'm wondering if there could be a way for RichText (the block-editor one with context) to know what attribute it's saving to without needing the identifier prop.
I also don't like the name identifier. attributeKey is a bit clearer.
There was a problem hiding this comment.
identifier is a good name for a block-editor agnostic RichText. attributeKey is good for a block-editor specific RichText :)
There was a problem hiding this comment.
Right, this is a block-editor specific RichText :) We have two: the one in the rich text package is block agnostic and the one in the block-editor package (used by block) has a lot of block context for merging and splitting and syncing all state to the block editor.
There was a problem hiding this comment.
This works nicely for both quote v2 and list v2. Code-wise it makes sense to me as well.
I know there's an ongoing conversation and I don't want to presume that I've got all the details right, so I'd like that to settle. This is what I know:
- This approach doesn't add any external APIs so it doesn't force us to commit to this forever
- I'm not able to see how this can potentially break either: it sounds like assuming a 1:1 relationship for a block attribute and a richtext would be more important an issue for the merge situation than for this one (that only udpates block selection).
So, all in all, I'd be in favor of shipping this as it is and change later if needed/wanted.
What?
Similar to howe we retains focus through other block transforms and merges, we can use the selection character trick to find out where focus should move to. Previously the input rule returned just one blocks so it was easy for
replaceBlocksto just place focus on the first block. With nested blocks this is not so simple. The selection character is a robust way to recover selection from any kind of structure the block implementor may insert with an input rule.Why?
When creating a quote with
>, it is important to be able to keep typing.How?
Testing Instructions
Screenshots or screencast