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

Converting a VideoPress block to regular video causes the block to crash #39417

Open
mrfoxtalbot opened this issue Sep 17, 2024 · 13 comments · May be fixed by WordPress/gutenberg#69152
Open

Converting a VideoPress block to regular video causes the block to crash #39417

mrfoxtalbot opened this issue Sep 17, 2024 · 13 comments · May be fixed by WordPress/gutenberg#69152
Assignees
Labels
[Feature] VideoPress A feature to help you upload and insert videos on your site. [Platform] Atomic [Platform] Simple [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. [Pri] Normal Triaged [Type] Bug When a feature is broken and / or not performing as intended

Comments

@mrfoxtalbot
Copy link

Impacted plugin

VideoPress

Quick summary

When a VideoPress block is converted to regular video, the block crashes, showing this message:

This block has encountered an error and cannot be previewed.

Kapture.2024-09-17.at.10.24.41.mp4

Steps to reproduce

  1. Upload a video as a VideoPress block or convert a regular video block to VideoPress
  2. Convert the block back to regular video
  3. Notice how the block breaks and shows this message This block has encountered an error and cannot be previewed.

A clear and concise description of what you expected to happen.

It should be possible to go back and forth between the to blocks. If this is not possible for technical reasons, The VideoPress block should not offer the option to convert back to regular video to avoid this error.

What actually happened

The block crashes.

Impact

All

Available workarounds?

Yes, easy to implement

If the above answer is "Yes...", outline the workaround.

The crashed video block CAN be converted back into a VideoPress block but the scenarios is still confusing to users.

Platform (Simple and/or Atomic)

Simple, Atomic

Logs or notes

This is happening on Simple, Atomic, and self-hosted.

@mrfoxtalbot mrfoxtalbot added [Type] Bug When a feature is broken and / or not performing as intended [Feature] VideoPress A feature to help you upload and insert videos on your site. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Needs triage Ticket needs to be triaged labels Sep 17, 2024
@github-actions github-actions bot added [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. [Platform] Atomic [Platform] Simple [Pri] Normal labels Sep 17, 2024
@jartes jartes added Triaged and removed Needs triage Ticket needs to be triaged labels Sep 17, 2024
@jartes jartes moved this from Needs Triage to Triaged in Automattic Prioritization: The One Board ™ Sep 17, 2024
@epeicher epeicher self-assigned this Feb 10, 2025
@epeicher
Copy link
Contributor

A quick note on this to add additional context, when saving the Draft and refreshing the browser, the video is displayed correctly

CleanShot.2025-02-10.at.16.29.20.mp4

@epeicher
Copy link
Contributor

When adding a console.log to this Gutenberg component,

componentDidCatch( error, info ) {
	console.log( error, info );

this is the error printed:

TypeError: Cannot read properties of undefined (reading 'ownerDocument')
    at getComputedCSS (spacing-visualizer.js:75:17)
    at computeStyle (spacing-visualizer.js:86:17)
    at spacing-visualizer.js:22:3
    at updateReducer (react-dom.js?ver=18:15855:24)
    at Object.useReducer (react-dom.js?ver=18:17089:18)
    at useReducer (react.js?ver=18:1616:23)
    at SpacingVisualizer (spacing-visualizer.js:21:43)
    at renderWithHooks (react-dom.js?ver=18:15496:20)
    at updateFunctionComponent (react-dom.js?ver=18:19627:22)

@epeicher
Copy link
Contributor

The error seems to be in Gutenberg editor, I have created this Draft PR to start iterating over it WordPress/gutenberg#69152

@epeicher
Copy link
Contributor

epeicher commented Feb 12, 2025

I have also tried to remove the transforms from the VideoPress block here and here, but I still see the video option when selecting to transform the block

Image

I will progress with the linked PR as I think is the correct way to go.

@epeicher
Copy link
Contributor

I have moved the linked PR WordPress/gutenberg#69152 to Ready for review as I think is the best way to solve this.

@epeicher
Copy link
Contributor

epeicher commented Feb 17, 2025

I have also tried to remove the transforms from the VideoPress block here and here, but I still see the video option when selecting to transform the block

After some investigation, I have found that this is done when applying this filter (that functionality was added as part of #26799). That code is called by this Gutenberg code. So if we remove the core/video in the to field of the transforms field here, the video option will be removed from the menu. Although, as commented, I don't think this is a better experience because allowing to change from VideoPress to video is a helpful option.

@epeicher
Copy link
Contributor

epeicher commented Feb 19, 2025

A new occurrence of the same error has been reproduced when selecting the video block and uploading an item from the VideoPress media library.

Error logged in Developer Console
react-dom.min.js?ver=18:1 TypeError: Cannot read properties of undefined (reading 'ownerDocument')
    at bm (wp-block-editor-20.0.0-hotfix.min.js?ver=8bc2927f2c02c2cae6c4:36:19625)
    at computeStyle (wp-block-editor-20.0.0-hotfix.min.js?ver=8bc2927f2c02c2cae6c4:36:20211)
    at wp-block-editor-20.0.0-hotfix.min.js?ver=8bc2927f2c02c2cae6c4:36:19119
    at Object.wt [as useReducer] (react-dom.min.js?ver=18:1:46943)
    at e.useReducer (react.min.js?ver=18:1:9954)
CleanShot.2025-02-19.at.17.14.44.mp4

@t-hamano
Copy link

I'm not familiar with Jetpack's codebase, but the error appears to occur when generating the preview.

I may be missing something, but is it because the component generating the preview doesn't have useBlockProps?

@epeicher
Copy link
Contributor

Hi @t-hamano, thanks for your suggestion, it makes sense so I have gone ahead and tested it. I have wrapped this component to use useBlockProps and I have even wrapped the whole generated component to use useBlockProps but that didn't fix the issue, I still have an undefined gutenberg block that triggers the error, unless applying this fix WordPress/gutenberg#69152 that handles the undefined block.

@t-hamano
Copy link

t-hamano commented Feb 21, 2025

It's a shame the issue didn't get fixed, but we really need to determine if the problem is with the Gutenberg plugin.

WordPress/gutenberg#69152 addresses the scenario of an undefined blockElement, i.e. when clientId is undefined. But rather than fixing the symptom, I think we should determine why 'clientId' is undefined in the first place.

I think Jetpack overrides the Edit component of the core Video block. I think we need to identify where it is that is making the clientId undefined. Perhaps this file?

https://github.com/t-hamano/jetpack/blob/7a3559d2f7bd0cd8915902a4d0f315167793e5cd/projects/packages/videopress/src/client/block-editor/extend/core-video/index.js#L179-L185

One thing that bothers me is that there is a conditional statement in the JetpackCoreVideoDeprecation component that does not return the BlockListBlock component.

@epeicher
Copy link
Contributor

we really need to determine if the problem is with the Gutenberg plugin.

I agree, there seems to be something in this code causing the blockElement to be undefined in this code

I think Jetpack overrides the Edit component of the core Video block. I think we need to identify where it is that is making the clientId undefined.

Yes, after the latest investigations, I think the problem is caused by the overrides. A quick note on that is that the clientId is not undefined, it is the block element returned by useBlockElement here what returns undefined in initial calls and then it returns the proper core/video block in subsequent calls.

I will investigate your suggestions 👀

@epeicher
Copy link
Contributor

@t-hamano, I have found the code that is causing the blockElement to be undefined is this line of code that enhances the attributes. For testing purposes I have commented that line and confirmed that the error is not raised.

However, it seems to me that the issue in Gutenberg is a race condition in the update of blockElement here caused by changing the attributes, so I would say that the suggested change WordPress/gutenberg#69152 will make the Gutenberg code more robust. What do you think?

@t-hamano
Copy link

the suggested change WordPress/gutenberg#69152 will make the Gutenberg code more robust

I'm concerned that this will suppress errors that should be notified to developers, which may increase the possibility that users will extend core blocks in incorrect ways.

I would recommend further investigation into the following points:

  • What happens if you replace the JetpackCoreVideoDeprecation component here with BlockListBlock? If the problem doesn't occur, then we know the problem is in the JetpackCoreVideoDeprecation component.
  • Could you tell which attributes in attributesDefinition are causing the problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] VideoPress A feature to help you upload and insert videos on your site. [Platform] Atomic [Platform] Simple [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. [Pri] Normal Triaged [Type] Bug When a feature is broken and / or not performing as intended
Projects
Development

Successfully merging a pull request may close this issue.

4 participants