[RNMobile] [Embed block] Detect when an embeddable URL is pasted into an empty paragraph #35204
Conversation
jd-alexander
added
[Block] Embed
|
Size Change: -3 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
packages/block-editor/src/components/rich-text/embed-handler-picker.native.js
Show resolved
Hide resolved
jd-alexander
requested review from
cameronvoell,
ellatrix and
guarani
as code owners
There was a problem hiding this comment.
Paste URL on different blocks
I've done some tests pasting a URL on different blocks and I found out that the embed block could be also created from the following blocks:
- Cover block
- Group block
- List block
Cover block
Works as expected ✅, although I noticed that the embed block extends the cover frame when previewed on mobile mode, however, I think this is a rendering issue that comes from WordPress.
Click to see screenshots
| Editor | Preview |
|---|---|
 |
 |
Group block
Works as expected ✅ .
Click to see screenshots
| Editor | Preview |
|---|---|
 |
 |
List block
Works as expected ✅ .
Click to see screenshots
| Before pasting URL | After pasting URL |
|---|---|
 |
 |
Handle non-empty paragraph block
I tried to reproduce the test case No embed or link as described in the PR description and in my case, instead of pasting the URL as plain text, it's automatically creating an embed block (see attached video).
Click to see video
embed-block-paste-url-native.mp4
I'm wondering whether is expected that the picker shows up on a non-empty paragraph when a URL is pasted. I checked this behavior in the web version and in that case, an embed block is created independently of the content of the Paragraph.
Click to see video
embed-block-paste-url.mp4
Nevertheless, the task states:
Should not to trigger when pasting in the middle of a text
From my POV, I think it makes more sense to only create an embed block on empty paragraphs but I'd double-check with the rest of the team if we're ok diverging from the web version's flow. If the embed block creation should only be applied to empty paragraph blocks, it would be great that we review the logic to prevent creating them automatically.
Undo operations
I did a quick test on the web version and the undo operation doesn't work as expected either. Looks like the undo/redo operations are not working properly in the embed block in general, I'd open a new issue as a follow-up but I wouldn't block this PR due to this.
Click to see video
embed-block-paste-url-undo.mp4
| function onPickerSelect( value ) { | ||
| const selectedItem = pickerOptions.find( | ||
| ( item ) => item.value === value | ||
| ); | ||
| selectedItem.onSelect(); | ||
| } |
There was a problem hiding this comment.
I'm wondering if wrapping this callback with useCallback would prevent potential re-renders 🤔 .
There was a problem hiding this comment.
Good idea! Just to verify, once this is done, I would need to utilize some form of tracing mechanism to verify the number of re-renders taking place. What tool do you utilize to do this?
There was a problem hiding this comment.
@fluiddot I did this operation here be43155 579def2
There was a problem hiding this comment.
Good idea! Just to verify, once this is done, I would need to utilize some form of tracing mechanism to verify the number of re-renders taking place. What tool do you utilize to do this?
I completely missed this comment, sorry. I usually measure renders by adding logs but I think you could use Flipper and the React devtools that are already included for this.
There was a problem hiding this comment.
I did a quick test and found out that doesn't prevent re-renders of the Picker component because the parent is the one that forces the render. The only way I managed to prevent extra renders is by memoizing the component (see attached diff patch). Nevertheless, it's a good practice to wrap callbacks passed in props with useCallback, so let's stick with this change 👍 .
In the following screenshots, you can see that independently on using useCallback it renders the Picker component three times:
Without useCallback |
With useCallback |
|---|---|
 |
 |
Click here to see the diff patch
diff --git a/packages/block-editor/src/components/rich-text/embed-handler-picker.native.js b/packages/block-editor/src/components/rich-text/embed-handler-picker.native.js
index 686e55b241a2aca54102fd5ec8974f585e0e8fa9..c4ffc0894e38a74b17effbe6710ae145c04329bd 100644
--- a/packages/block-editor/src/components/rich-text/embed-handler-picker.native.js
+++ b/packages/block-editor/src/components/rich-text/embed-handler-picker.native.js
@@ -10,6 +10,7 @@ import {
forwardRef,
useRef,
useImperativeHandle,
+ memo,
useCallback,
} from '@wordpress/element';
import { Picker } from '@wordpress/components';
@@ -30,7 +31,7 @@ const DEFAULT_PICKER_OPTIONS = [
},
];
-export default forwardRef( ( {}, ref ) => {
+const EmbedHandlerPicker = forwardRef( ( {}, ref ) => {
const pickerRef = useRef();
const pickerOptions = useRef( DEFAULT_PICKER_OPTIONS ).current;
@@ -62,3 +63,4 @@ export default forwardRef( ( {}, ref ) => {
/>
);
} );
+export default memo( EmbedHandlerPicker );
There was a problem hiding this comment.
I did a quick test and found out that doesn't prevent re-renders of the Picker component because the parent is the one that forces the render. The only way I managed to prevent extra renders is by memoizing the component (see attached diff patch). Nevertheless, it's a good practice to wrap callbacks passed in props with useCallback, so let's stick with this change 👍 .
Understood 🙇🏾
In the following screenshots, you can see that independently on using useCallback it renders the Picker component three times:
Indeed! I was able to fire up DevTools and observe the profiler in action.
|
👋 Hi @jd-alexander, let me know if you'd like a review from me. I think I was requested automatically here as code owner, but not sure if you need my review here since @fluiddot is also reviewing. No problem either way, just seeking clarification. |
Hey @guarani @fluiddot is covering the reviews here so we are good. Thanks for checking in 😎 |
|
Thanks for the thorough tests and feedback @fluidot 💯 Handle non-empty paragraph block
Interesting. When I try to reproduce this I still end up getting the link as a text. Even on the web, I get the text sometimes but other times an Embed block is created.
Yeah, now looking at the issue/specs I am thinking that may be the best solution as well. I will take a look at the logic that is causing the Undo operations
I am going to check if there are any existing issues because this bug seems pretty critical to me. But yes, I agree that it should be a blocker since it happening there as well. We should leave a note when the functionality is being tested so no one is caught off guard with the behavior if they attempt to make undo or redo operations. |
|
There was a problem hiding this comment.
Awesome work @jd-alexander 💯 .
Embed block automatically created on some blocks 🟢
I confirm that this issue is no longer present 🎊 . However, I noticed a new one related to pasting an URL copied from non-paragraph blocks.
Paste an URL copied from a non-paragraph block ❌
When pasting an URL copied from a non-paragraph block like the Heading block, the picker is shown but when selecting the "Create embed" option it creates the block of the copied URL instead of an embed block.
You can reproduce the issue with the following steps:
- Have an embeddable URL in the clipboard
- Add a Heading block
- Paste the URL
- Observe that the URL is pasted as plain text
- Select and copy the URL from the Heading block
- Add a Paragraph block
- Paste the URL
- Observe that the embed handler picker is shown
- Tap on the "Create embed" option
- Observe that a Heading block with URL is created instead an embed block
Click here to display the video
embed-block-paste-url-non-paragraph-block.mp4
I think we would need to refactor the onPaste function to handle this case. I checked how this behavior works in the web editor, looks like it's expected that we paste the block of the copied URL no the embed block, so we have to figure out a way to prevent displaying the embed handler picker in this case.
|
@fluiddot I added the fixes here. I will do another run-through of tests later to confirm that nothing was broken. |
| const derivedBlock = content[ 0 ]; | ||
| const { name } = derivedBlock; | ||
|
|
||
| // When an URL is pasted in an empty paragraph then the EmbedHandlerPicker should showcase options allowing the transformation of that URL | ||
| // into either an Embed block or a link within the target paragraph. If the paragraph is non-empty, the URL is pasted as text. | ||
| if ( __unstableEmbedURLOnPaste && name === 'core/embed' ) { |
There was a problem hiding this comment.
On Android, this logic fails because it is not resolving the pasted URL as a block. The content that is parsed by the PasteHandler is a string. Further details on the differences in the clipboard behavior as expounded on this comment #35204 (comment) refactoring to accommodate this case.
There was a problem hiding this comment.
I think we should handle the else case of this if block, probably we could call the same as in line 504:
onReplace( content, content.length - 1, -1 );
This way if we copy, for example, a Heading block that contains an URL, it will paste that block.
There was a problem hiding this comment.
@carlosgprim we may have to tag-team on resolving the final Heading block issue with the Embed Paste URL. This is what is taking place.
There are cases where copying a URL from a Heading block results in a string when pasted instead of being pasted as a block when it’s processed by the PasteHandler therefore the bottom sheet is still shown. I am not sure how we can figure out the source of the copy/paste if only a string is returned. Still investigating.
| return; | ||
| } | ||
|
|
||
| mode = 'BLOCKS'; |
There was a problem hiding this comment.
I think we should preserve this line to be executed (if the conditions of line 448 are satisfied) because it's used in the pasteHandler call below. Maybe this has to do with the issue you encountered in this comment.
There was a problem hiding this comment.
Oh yes. Good eye!
| const derivedBlock = content[ 0 ]; | ||
| const { name } = derivedBlock; | ||
|
|
||
| // When an URL is pasted in an empty paragraph then the EmbedHandlerPicker should showcase options allowing the transformation of that URL | ||
| // into either an Embed block or a link within the target paragraph. If the paragraph is non-empty, the URL is pasted as text. | ||
| if ( __unstableEmbedURLOnPaste && name === 'core/embed' ) { |
There was a problem hiding this comment.
I think we should handle the else case of this if block, probably we could call the same as in line 504:
onReplace( content, content.length - 1, -1 );
This way if we copy, for example, a Heading block that contains an URL, it will paste that block.
…/gutenberg into rnmobile/embed-block-paste-url # Conflicts: # packages/block-editor/src/components/rich-text/index.native.js
|
While testing I identified the following test cases that would be great to include and run them with these changes: Paste URL with HTML tagsPreparation:
At this point, the clipboard content will be: Empty paragraph
Non-empty paragraph
Paste URL plain textPreparation:
At this point, the clipboard content will be: Empty paragraph
Non-empty paragraph
Paste URL from blockPreparation:
At this point, the clipboard content will be: Empty paragraph
Non-empty paragraph
|
|
@jd-alexander heads up that I pushed a small refactor in this commit related to the paste logic so it covers the test cases I mentioned in the previous comment 🎊 . |
|
Thanks for the collaboration to get this to the finish line @fluiddot I will run your test cases along with the previous test cases I had as well. |
|
@fluiddot as you will see in the latest commit, I had to remove the |
|
Testing round on iPhone 11 & Google Pixel 4 XL, here are the results: Paragraph block
Paste URL on other blocks
Embed block is not created on the blocks belowThe following blocks are derivatives of the
|
I'm wondering if it's really necessary to remove the gutenberg/packages/block-editor/src/components/rich-text/use-paste-handler.js Lines 199 to 205 in 94d75b2 I think the culprit here is the |
@jd-alexander I confirm that it was caused by that condition because after removing it, the issue couldn't be reproduced. I'd like to suggest making the following modifications, I tested them locally and all the test cases succeed 🟢 : diff --git a/packages/block-editor/src/components/rich-text/index.native.js b/packages/block-editor/src/components/rich-text/index.native.js
index be05f24b2b62cdc948626f1161edc3ee778edb47..bdd49831331212f83aa07f29e92ec47dfe505ffd 100644
--- a/packages/block-editor/src/components/rich-text/index.native.js
+++ b/packages/block-editor/src/components/rich-text/index.native.js
@@ -451,7 +451,11 @@ function RichTextWrapper(
createLinkInParagraph( plainText.trim(), onReplace ),
} );
- if ( isPastedURL ) {
+ if (
+ __unstableEmbedURLOnPaste &&
+ isEmpty( value ) &&
+ isPastedURL
+ ) {
mode = 'BLOCKS';
}
@@ -463,7 +467,7 @@ function RichTextWrapper(
preserveWhiteSpace,
} );
- if ( typeof content === 'string' && ! isPastedURL ) {
+ if ( typeof content === 'string' ) {
let valueToInsert = create( { html: content } );
addActiveFormats( valueToInsert, activeFormats );
Note that I added the same conditions of the web version for setting the |
Ah, that makes sense! I will give it a test locally so that we can merge this 🙇🏾 |
There was a problem hiding this comment.
LGTM 🎊 !
I tested the following test cases:
Paragraph block
- Create Embed ✅
- Create link ✅
- No embed or link ✅
Paste URL on other blocks
- Media & Text block ✅
- Cover block 🟡 - (known rendering issues with WordPress)
- Group block ✅
- Column block ✅
Embed block is not created on the blocks below
- Heading block ✅
- Quote block ✅
- List block ✅
- Preformatted block ✅
- Verse ✅
- Search ✅
- File block ✅
Paste URL with HTML tags
- Empty paragraph ✅
- Non-empty paragraph ✅
Paste URL plain text
- Empty paragraph ✅
- Non-empty paragraph ✅
Paste URL from block
- Empty paragraph ✅
- Non-empty paragraph ✅
Tested on Simulator - iPhone 12 Pro Max (iOS 14.5) and Samsung Galaxy S20 FE 5G (Android 10).
|
Thanks for testing @fluiddot I also did a round of tests to verify that the paste functionality was not broken across the editor. I tested the following test cases: Paragraph block
Paste URL on other blocks
Embed block is not created on the blocks below
Paste URL with HTML tags
Paste URL plain text
Paste URL from block
Tested on Simulator - iPhone 11 (iOS 14.4) and Google Pixel 4 XL (Android 10). |
|
@jd-alexander heads up that I updated the PR's description to expand the info related to which blocks this functionality is also available. New description:
|
gutenberg-mobile PRwordpress-mobile/gutenberg-mobile#4048Description
This PR implements functionality that allows the
Paragraphblock to detect when an embeddable URL is pasted to trigger the opening of an Embed-related bottom sheet. The options are as follows.This functionality is supported by the Paragraph block and the following blocks, that contain a paragraph:
How has this been tested?
Paragraph block
Create Embed ✅
Click to see screenshots
Create link ✅
Click to see screenshots
No embed or link ✅
Click to see screenshots
Create Embed - Undo operations ❌
Click to see screenshots
Media & Text block
Create Embed ✅
Click to see screenshots
Create link ✅
Click to see screenshots
Types of changes
rich-textutils now contain a function that will create a link within a paragraph.__unstableEmbedURLOnPasteprop was passed to thenativeParagraph block so that the underlyingrich-textcomponent can processed embed related URLs on paste.EmbedHandlerPickercomponent was created that intercepts pasted embed URLs in theonPasteof therich-textcomponent and allows the user to create an embed or convert the URL to a link within the paragraph.Checklist:
*.native.jsfiles for terms that need renaming or removal).