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.

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

Pasting WordPress.com site link turns into Embed block with "This block has encountered an error" #39935

Closed
kriskarkoski opened this issue Mar 6, 2020 · 42 comments
Assignees
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg [Type] Bug

Comments

@kriskarkoski
Copy link
Contributor

Steps to reproduce

  1. Starting at URL: In the Block Editor paste a link to a WordPress.com site (tested using an Atomic site)
  2. The link will be converted to an embed block that shows the error "This block has encountered an error and cannot be previewed"

What I expected

The link to appear as an embed (which I think we currently block) or a normal hyperlink

What happened instead

The embed shows an error on the back end and nothing on the frontend.

Browser / OS version

I don't believe this is affecting a specific combination, but I am using Chrome 79.0.3945.13 on OSX 10.

Screenshot / Video

Screen Shot 2020-03-06 at 9 54 34 AM

Context / Source

#manual-testing, #user-report

@kriskarkoski kriskarkoski added [Type] Bug [Goal] Gutenberg Working towards full integration with Gutenberg labels Mar 6, 2020
@simison
Copy link
Member

simison commented Mar 7, 2020

I could replicate this with Gutenberg 7.3.0 but not on 7.4.0, so I suspect this is fixed upstream already and will get fixed on .com once we upgrade.

@millerf
Copy link
Contributor

millerf commented Mar 8, 2020

Confirmed...

Screenshot 2020-03-08 at 18 16 03

@kriskarkoski
Copy link
Contributor Author

Nice, thanks for testing @simison @millerf , I forgot about us being out of sync

@millerf
Copy link
Contributor

millerf commented Mar 13, 2020

@lancewillett This issue can be closed. Not sure how to request it,...

@kriskarkoski
Copy link
Contributor Author

I'll close it now that the new version is close to rolling out on WordPress.com that should fix this.

@lancewillett
Copy link
Contributor

I'm able to reproduce the error with a non WordPress.com URL:

Trying:

Neither work when pasted into the block editor.

Screen Shot 2020-03-29 at 17 09 26

This WordPress.com URL does work as expected:

@lancewillett lancewillett reopened this Mar 30, 2020
@simison
Copy link
Member

simison commented Mar 30, 2020

@lancewillett did you replicate with simple or atomic site? If atomic, what's the Gutenberg version?

@millerf
Copy link
Contributor

millerf commented May 11, 2020

@lancewillett I had no problem with those links...

Screenshot 2020-05-11 at 17 18 59

@simison
Copy link
Member

simison commented May 11, 2020

We've since upgraded to Gutenberg v8.0 so that might've fixed this. I think we were on v7.7.1 around March 30th and before that on v7.3.0.

@millerf
Copy link
Contributor

millerf commented May 11, 2020

I think it should be same to close it, then...

@simison
Copy link
Member

simison commented May 11, 2020

Seeing this still happen with Gutenberg 8.0.0, directly via site's WP Admin (/wp-admin/post-new.php):

image

I think if you copy-paste two links, you'll get a list like in your screenshot (#39935 (comment)), and if you copy-paste them individually, you'll get "embedding" prompt which fails.

@millerf
Copy link
Contributor

millerf commented May 11, 2020

On my local env:

Screenshot 2020-05-11 at 17 36 37

On Wordpress:

Screenshot 2020-05-11 at 17 38 02

I am not sure what is the difference with the wp-admin editor.

@millerf
Copy link
Contributor

millerf commented May 11, 2020

Screenshot 2020-05-11 at 17 43 25

I don't see it on wp-admin...

@sirreal sirreal self-assigned this May 13, 2020
@sirreal
Copy link
Member

sirreal commented May 13, 2020

Pasting the URLs

Both links error on Simple sites (running Gutenberg 8.0.0, iframed and WP Admin).

I tested with a Gutenberg development build. Both links work fine:

Screen Shot 2020-05-13 at 23 46 57

I tested on Atomic and both links work fine. The issue is something specific to Simple sites.

@simison
Copy link
Member

simison commented May 14, 2020

@sirreal did you test with Calypso iframe or directly in wp-admin? I see the errors happen with iframe on Simple site.

@sirreal
Copy link
Member

sirreal commented May 14, 2020

@sirreal did you test with Calypso iframe or directly in wp-admin? I see the errors happen with iframe on Simple site.

My comment was unclear, I always observe this issue on Simple sites. It appears to be something specific to them.

@Addison-Stavlo
Copy link
Contributor

Testing on simple, ephemeral JN, and local Gutenberg:

For simple, the url ends up in text in a paragraph block:
Screen Shot 2020-05-14 at 4 55 15 PM

on the ephemeral JN, the embed block appears with its failure message:
Screen Shot 2020-05-14 at 4 57 17 PM

Locally the link embeds as expected:
Screen Shot 2020-05-14 at 4 55 26 PM

behaviors seem consistent between Iframe and wp-admin as well. I have not encountered the "this block as encountered an error and cannot be previewed" result as noted above though.

@sirreal sirreal removed their assignment Jun 9, 2020
@yansern
Copy link
Contributor

yansern commented Aug 5, 2020

Seems like the issue is that core-embed/wordpress block is being unregistered after it was registered.

image

This leads back to unregisterWordPressEmbedBlock() in gutenberg-wpcom-embed.js.

image

@yansern
Copy link
Contributor

yansern commented Aug 5, 2020

The unregisterWordPressEmbedBlock() can be traced back to r201315-wpcom.

@mmtr What was the reason behind unregistering the WordPress embed block? If we do not need to do that anymore, then we could remove that function.

Here's a phab patch to remove it - D47668-code. If not, just abandon the patch.

@yansern yansern self-assigned this Aug 5, 2020
@mmtr
Copy link
Member

mmtr commented Aug 6, 2020

The unregisterWordPressEmbedBlock() can be traced back to r201315-wpcom.

That was mainly a refactor that moved the function to another file. It was first introduced in D25893-code.

@mmtr What was the reason behind unregistering the WordPress embed block?

The rationale was that at that point we were not able to embed WordPress content in WordPress.com simple sites (#28326 (comment)).

If that is no longer an issue I agree we can remove the function.

@ockham
Copy link
Contributor

ockham commented Aug 6, 2020

Thanks @yansen, and @mmtr for the pointer!

I think #28326 is key here. It was a deliberate decision to remove the WordPress embed block, since we don't support non-WP.com WordPress embeds on WP.com out of security concerns. (As #28326 explains, this was the case even before the block editor.)

I think that the problem is now that if a user inserts an WordPress site URL, we still trigger the embedding behavior that tries to turn that URL into the corresponding block -- which is no longer there. The fix should be to "unregister" the embed transform for WordPress site URLs.

@ockham
Copy link
Contributor

ockham commented Aug 6, 2020

IIUC, embeds work vaguely as follows in Gutenberg:

  • User enters URL
  • Gutenberg creates a generic core/embed block with a preview (really just an <a href={ url }>{ caption || url }</a>) of the referenced URL (see)
  • Gutenberg looks at that URL and decides whether it can turn the generic core/embed one into a more specific one (which will have a better preview of the URL, e.g. the familiar layout of a tweet or an Instagram post).
  • Gutenberg also looks at the preview HTML to determine whether it contains class="wp-embedded-content" to determine whether the URL points to a WordPress page (since those cannot be simply determined by looking at the URL). If it is, it uses the core-embed/wordpress block.

The relevant code seems to be here: https://github.com/WordPress/gutenberg/blob/af53147b3adf1951aeac7bdbbf3b7a574d8bc610/packages/block-library/src/embed/util.js#L113-L128

If the core-embed/wordpress block is unregistered, that latter part fails.

@ockham
Copy link
Contributor

ockham commented Aug 6, 2020

I think that the correct fix needs to be made against Gutenberg (in https://github.com/WordPress/gutenberg/blob/af53147b3adf1951aeac7bdbbf3b7a574d8bc610/packages/block-library/src/embed/util.js): If a "specialized" embed block isn't available, don't attempt to transform the "generic" core/embed block into it. (I think this fix is necessary, but probably not sufficient.)

@simison
Copy link
Member

simison commented Aug 6, 2020 via email

@ockham
Copy link
Contributor

ockham commented Aug 6, 2020

I think there's some work going on in core to turn all those specific embed
blocks into variations of that base block.

Would they help?

I'm not sure TBH. That work might be largely orthogonal -- I assume there will still be logic to turn a "generic" embed block into a WordPress embed block, so we'll still need to do something about that.

@ockham
Copy link
Contributor

ockham commented Aug 6, 2020

I think there's some work going on in core to turn all those specific embed
blocks into variations of that base block.

Found the relevant PR I think: WordPress/gutenberg#24090

Edit: This is what the relevant part looks like in that PR: https://github.com/WordPress/gutenberg/pull/24090/files#diff-56fcc4a2c51b3f80f9ae35c2b3b94b34R122-R133

@ntsekouras
Copy link
Member

I assume there will still be logic to turn a "generic" embed block into a WordPress embed block

The above PR was just merged and the logic for WordPress embed block discovery hasn't changed.

@ockham
Copy link
Contributor

ockham commented Aug 10, 2020

I assume there will still be logic to turn a "generic" embed block into a WordPress embed block

The above PR was just merged and the logic for WordPress embed block discovery hasn't changed.

Actually, things are a bit harder now 🤔 Before, we could unregister the core-embed/wordpress block (and I would've needed to add logic to packages/block-library/src/embed/util.js to check whether that block is registered, before attempting to transform into it).

With WordPress/gutenberg#24090, we can no longer unregister the core-embed/wordpress block, since it has become a block variation of the core/embed; and I don't think there's an API to unregister a singular variation of a block (yet).

@ockham
Copy link
Contributor

ockham commented Aug 10, 2020

I don't think there's an API to unregister a singular variation of a block (yet).

I stand corrected: https://developer.wordpress.org/block-editor/data/data-core-blocks/#removeBlockVariations

@ockham
Copy link
Contributor

ockham commented Aug 10, 2020

Okay, I can work with this. I'll file an upstream fix.

@ntsekouras
Copy link
Member

@ockham since you're making changes with embed please keep in mind that the logic for detecting a possible better embed match has changed and is using providerNameSlug field: https://github.com/WordPress/gutenberg/blob/8474a2edcf302b7a383839801156d077cf7810c9/packages/block-library/src/embed/util.js#L108.

There is proper backwards compatibility with core but I've noticed in .com the above field providerNameSlug is not filled by some middleware you have there, I guess. This will cause problems with older embed blocks as it compares them with current version and the providerNameSlug adds css classes based on provider, that makes them invalid.

So you'll have to fill this property properly. If you need any help please let me know!

@ockham
Copy link
Contributor

ockham commented Aug 13, 2020

WIP fix here: WordPress/gutenberg#24559

@lsl
Copy link
Contributor

lsl commented Aug 21, 2020

More in

  • D47668-code
  • D48438-code

@ockham
Copy link
Contributor

ockham commented Aug 21, 2020

This has been addressed by WordPress/gutenberg#24559 (which will probably be released as part of the next Gutenberg release, v8.9). Once that release is installed on WP.com, and once we land D48267-code, the issue should be fixed 🤞

@david-szabo97
Copy link
Contributor

@ockham I find it interesting that WordPress embed has already stopped working with Gutenberg Edge (8.9-rc1).
Since D48267-code is not merged (and I'm not on sandbox), it shouldn't work. I guess.

I tested it on Simple site + Gutenberg edge sticker.
image

As you can see I embedded multiple WordPress links (links from this discussion) and it's not showing an error, but it's not embedding it as a WordPress embed variation either. Since D48267-code isn't deployed, it should embed it as a WP embed variation. Am I missing something?

@david-szabo97
Copy link
Contributor

david-szabo97 commented Sep 3, 2020

EDIT:
TLDR: We need this patch: D48267-code
TLDR: oAuth embed auto discovery embeds the iframe

Figured it out. It's actually being embedded:
image
But you can see an inline style there on the iframe: position: absolute;clip: rect(1px, 1px, 1px, 1px); which is basically hiding the embedded content.
image

wp-embed.js is supposed to remove that inline style, but since we don't have that on WP.com, it's not happening.

@david-szabo97
Copy link
Contributor

A fix to get rid of autoembedding:

remove_filter( 'the_content', array( $GLOBALS['wp_embed'], 'autoembed' ), 8 );
remove_filter( 'widget_text_content', array( $GLOBALS['wp_embed'], 'autoembed' ), 8 );

Since many sites are probably using bare links (which are being replaced by autoembedding), this is a really bad idea to do right now. I'm not sure if we should even think about removing autoembed feature of WP from WPcom.

cc @ockham

@david-szabo97
Copy link
Contributor

Actually we can close this issue (fixed in Gutenberg v8.9) and create a new if this is something that we should tackle. 😄

@ockham
Copy link
Contributor

ockham commented Sep 15, 2020

Thanks a lot for your research, @david-szabo97 !

I also think we can close this issue for now. I agree that we probably don't want to get rid of the auto-embedding -- that behavior likely predates Gutenberg, so it's probably established at this point.

I'll file an issue for this part though since this seems wrong: #39935 (comment)

@ockham ockham closed this as completed Sep 15, 2020
@ockham
Copy link
Contributor

ockham commented Sep 15, 2020

I'll file an issue for this part though since this seems wrong: #39935 (comment)

I can repro this in a vanilla Gutenberg install, so I think we'll need to report this in the GB repo.

@ockham
Copy link
Contributor

ockham commented Sep 15, 2020

I can repro this in a vanilla Gutenberg install, so I think we'll need to report this in the GB repo.

WordPress/gutenberg#25344

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg [Type] Bug
Projects
None yet
Development

No branches or pull requests