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

Saving a Video block with a file URL displays placeholder text in the editor #42515

Closed
dcalhoun opened this issue Jul 18, 2022 · 4 comments
Closed
Assignees
Labels
[Block] Video Affects the Video Block Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended

Comments

@dcalhoun
Copy link
Member

dcalhoun commented Jul 18, 2022

Description

Saving a Video block that points to a raw file URL (e.g. a MP4 file, not a YouTube embed), the block's preview is removed as if the URL was not set. However, the file URL remains within the post's HTML content.

Step-by-step reproduction instructions

  1. Create a new post in the native editor.
  2. Add a Video block.
  3. Tap Insert from URL.
  4. Set the URL to an MP4 file.
  5. Dismiss the block settings bottom sheet.
  6. Note the preview video player displays for the block.
  7. Tap the three-dot menu in the top-right of the editor.
  8. Tap Save as Draft.

Expected behaviour

The Video block displays a preview of the video URL.

Actual behaviour

The Video block appears with a placeholder as if no URL is set.

Screenshots or screen recording (optional)

RPReplay_Final1658172366.MP4

WordPress information

  • WordPress version: 6.0
  • Gutenberg version: 13.6.0
  • Are all plugins except Gutenberg deactivated? Yes
  • Are you using a default theme (e.g. Twenty Twenty-One)? Geologist

Device information

  • Device: Samsung Galaxy S20, iPhone SE
  • Operating system: Android 12, iOS 11
  • WordPress app version: WP 20.3
@dcalhoun dcalhoun added [Type] Bug An existing feature does not function as intended Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Block] Video Affects the Video Block labels Jul 18, 2022
@dcalhoun dcalhoun changed the title Saving a Video block with a raw file URL displays placeholder text in the editor Saving a Video block with a file URL displays placeholder text in the editor Jul 18, 2022
@derekblank
Copy link
Member

I took a look at this one, and believe the issue is due to this early return block before the render function that displays the MediaPlaceholder when no id is present:

if ( ! id ) {
    return (
        <View style={ { flex: 1 } }>
            <MediaPlaceholder
                allowedTypes={ [ MEDIA_TYPE_VIDEO ] }
                onSelect={ this.onSelectMediaUploadOption }
                onSelectURL={ this.onSelectURL }
                icon={ this.getIcon( ICON_TYPE.PLACEHOLDER ) }
                onFocus={ this.props.onFocus }
                autoOpenMediaUpload={
                    isSelected && wasBlockJustInserted
                }
            />
        </View>
    );
}

Although the edit.native.js Video block file is setting the id attribute to be the new video URL here, the web equivalent edit.js file does not set the id as the URL. In fact, it sets the id and poster attributes as undefined:

// edit.native.js
onSelectURL( url ) {
    [...]
    setAttributes( { id: url, src: url } );
}

// edit.js
onSelectURL( newSrc ) {
    [...]
    setAttributes( { src: newSrc, id: undefined, poster: undefined } );
}

Although the native file sets the id and this works locally, this param is not returned in the list of attributes when navigating away and returning to the post (like when creating a draft). Example log of a video's attributes in edit.native.js:

 LOG  attributes {"caption": "", "controls": true, "fileForImmediateUpload": null, "isVideoPressExample": false, "maxWidth": "100%", "preload": "metadata", "seekbarColor": "", "seekbarLoadingColor": "", "seekbarPlayedColor": "", "src": "http://commondatastorage.googleapis.com/gtv-videos-bucket/sample/BigBuckBunny.mp4", "useAverageColor": true, "videoPressTracks": []}

The lack of an id results in the display of the early return function in the native file, and perceived content loss for the user. I am not entirely sure why the web file is setting id and poster as undefined. This is likely something to track down, and also likely the cause of several other thumbnail issues linked on #42443. One possible explanation is the id is undefined to leverage modern browser's inbuilt capability to display poster images and thumbnails from video files, and the native early return function mentioned above may just need to be updated.

To preserve the web behavior and also eliminate the perceived content loss on mobile, one solution could be to modify the early return function to check for a src attribute as well. This change to the early return function logic appears to fix the issue:

if ( ! id && ! src ) {
    return (
      <MediaPlaceholder ... />
    );
}

I'll continue to investigate what else (if anything) this potential change might affect within edit.native.js.

@dcalhoun
Copy link
Member Author

dcalhoun commented Jul 20, 2022

Great investigative work, @derekblank.

It appears the web editor's conditional was refactored in #19162. Subsequently, the same logic in the native editor was refactored in #16331. The refactors are slightly different in regards to which attribute they rely upon — id or src.

if ( ! src ) {
return (
<div { ...blockProps }>
<MediaPlaceholder

if ( ! id ) {
return (
<View style={ { flex: 1 } }>
<MediaPlaceholder

I wonder if we are able to align the conditionals so that they function the same upon each platform. I.e. both platforms check src, and not id. We would likely need to test various video selection types on native to ensure this approach works for all of them, e.g. URL, upload, media library. It is possible that this approach would not work for native if there are circumstances where an id is present, but a src is not.

WDYT?

@derekblank
Copy link
Member

I wonder if we are able to align the conditionals so that they function the same upon each platform. I.e. both platforms check src, and not id.

Thanks for the further investigation, @dcalhoun. I agree that aligning the conditionals to check for if ( ! src ) would be the best option to start with. It would not affect the current web behavior, and it would certainly fix the issue in the case of using the Insert from URL option on mobile. I'll do some further testing on how this might affect the other media options, and open a PR if the result is favorable.

@fluiddot fluiddot added the [Priority] High Used to indicate top priority items that need quick attention label Jul 22, 2022
@derekblank derekblank self-assigned this Jul 25, 2022
@derekblank
Copy link
Member

This issue is resolved via #49858.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Video Affects the Video Block Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants