Skip to content
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.

Sign up for GitHub

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

Quote v2: retain selection after transform #39838

Merged
merged 2 commits into from
Mar 30, 2022
Merged

Quote v2: retain selection after transform #39838

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf9909c
Quote v2: retain selection after transform
ellatrix Mar 29, 2022
f317890
Remove the character
ellatrix Mar 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/block-editor/src/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ function RichTextWrapper(
__unstableAllowPrefixTransformations,
formatTypes,
onReplace,
selectionChange,
} ),
useRemoveBrowserShortcuts(),
useShortcuts( keyboardShortcuts ),
Expand Down
40 changes: 37 additions & 3 deletions packages/block-editor/src/components/rich-text/use-input-rules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
/**
* External dependencies
*/
import { findKey } from 'lodash';

/**
* WordPress dependencies
*/
import { useRef } from '@wordpress/element';
import { useRefEffect } from '@wordpress/compose';
import { slice, toHTMLString } from '@wordpress/rich-text';
import { insert, toHTMLString } from '@wordpress/rich-text';
import { getBlockTransforms, findTransform } from '@wordpress/blocks';
import { useDispatch } from '@wordpress/data';

Expand All @@ -13,6 +18,34 @@ import { useDispatch } from '@wordpress/data';
import { store as blockEditorStore } from '../../store';
import { preventEventDiscovery } from './prevent-event-discovery';

// A robust way to retain selection position through various
// transforms is to insert a special character at the position and
// then recover it.
const START_OF_SELECTED_AREA = '\u0086';

function findSelection( blocks ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took advantage of this and abstracted it into a reusable function while removing Lodash in #41806. Would love to get your feedback there when you get a chance @ellatrix.

let i = blocks.length;

while ( i-- ) {
const attributeKey = findKey(
blocks[ i ].attributes,
( v ) =>
typeof v === 'string' &&
v.indexOf( START_OF_SELECTED_AREA ) !== -1
);

if ( attributeKey ) {
return [ blocks[ i ].clientId, attributeKey, 0 ];
Copy link
Contributor

@youknowriad youknowriad Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

identifier is a good name for a block-editor agnostic RichText. attributeKey is good for a block-editor specific RichText :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

const nestedSelection = findSelection( blocks[ i ].innerBlocks );

if ( nestedSelection ) {
return nestedSelection;
}
}
}

export function useInputRules( props ) {
const {
__unstableMarkLastChangeAsPersistent,
Expand All @@ -22,7 +55,7 @@ export function useInputRules( props ) {
propsRef.current = props;
return useRefEffect( ( element ) => {
function inputRule() {
const { value, onReplace } = propsRef.current;
const { value, onReplace, selectionChange } = propsRef.current;

if ( ! onReplace ) {
return;
Expand Down Expand Up @@ -52,11 +85,12 @@ export function useInputRules( props ) {
}

const content = toHTMLString( {
value: slice( value, start, text.length ),
value: insert( value, START_OF_SELECTED_AREA, 0, start ),
} );
const block = transformation.transform( content );

onReplace( [ block ] );
selectionChange( ...findSelection( [ block ] ) );
__unstableMarkAutomaticChange();
}

Expand Down