Editor: Support VideoPress post-editor preview

[mjuhasz]

Closes: #1730

This pull request enables previewing of VideoPress videos in the post editor.

There is currently no VideoPress preview offered in the post editor on WordPress.com. When adding a VideoPress video to a post, only the shortcode text is shown in the editor post content. Only after saving and previewing the post can a user see how the post will look like.

Before:

After:

Testing instructions:

Scenario 1 - adding media from your library:

1. Navigate to the Calypso post editor.
2. Select a site, if prompted.
3. Create a new post.
4. Click on Add Media, select a video from your library, then click Insert.
5. Note that a video player is displayed in the editor.
6. Confirm that you can start playing the video and adjust the volume.
7. Confirm that by having the video player in focus (e.g. after clicking at its top bar) a remove button shows up with which the video can be removed.

Strictly speaking Scenario 1 resolves issue #1730, nevertheless the following use cases should also work:

Scenario 2 - adding media by pasting a shortcode:

Same as Scenario 1, except Step 4: Insert the following shortcode text into the editor: [wpvideo kUJmAcSf]

Scenario 3 - using shortcode attributes:

Same as Scenario 1, except:

Step 4: Insert the following shortcode text into the editor: [wpvideo kUJmAcSf at=90 ]
Step 5: Note that a video player is displayed in the editor and its start time is 1m30s.

Remark: not all shortcode attributes are fully supported at this time for preview.

[mjuhasz]

A few outstanding issues/remarks:

• default attributes are not left out from the url - they do not do any harm but they could be filtered out if we have definite defaults
• video width/height and aspect ratio are given some default value but ideally they would be queried from VideoPress. In Jetpack's VideoPress_Video class there is a remote call to get these attributes. Similar logic could be replicated in the wpview plugin.
• width shortcode attribute is ignored right now because a tinymce css entry overrides it: the VideoPress iframe is a descendant of a div with class wpview-wrap so it gets 100% width. We did not have this issue when the shortcode was rendered because there was an extra iframe around the VideoPress one.
• some attributes are not parsed: permalink - it does not seem to take effect; freedom and flashonly - the player embedded in the VideoPress iframe is used

[aduth]

Using Object.assign (or similarly, _.assign or _.defaults) can be a handy way to clearly identify the default values, while still allowing override if value is known:

return Object.assign( {
    w: defaultWidth,
    h: defaultHeight,
    at: 0,
    defaultLangCode: false
}, {
    videopress_guid: shortcode.attrs.numeric[0],
    w: parseInt( namedAttrs.w, 10 ),
    h: parseInt( namedAttrs.h, 10 ),
    autoplay: namedAttrs.autoplay === 'true',
    hd: namedAttrs.hd === 'true',
    loop: namedAttrs.loop === 'true',
    at: parseInt( namedAttrs.at, 10 ),
    defaultLangCode: namedAttrs.defaultlangcode
} );

Might be a negligible performance difference, though the clarity benefits should outweigh this. What do you think?

[mjuhasz]

Very handy, indeed! It makes the intent of the code much clearer.

[aduth]

Very handy, indeed! It makes the intent of the code much clearer.

Actually, this will not work as expected, as the presence of the key in the second argument will override the default value, regardless of whether the value is truthy.

For example, try entering this in your browser console (assuming your browser supports Object.assign):

Object.assign( { a: 1 }, { a: undefined } );

[mjuhasz]

In the end I used lodash's _.defaults. I also had to substitute NaN for undefined to get the expected behavior (i.e. used the default if one exists for unspecified attributes).

[aduth]

Result of running eslint client/components/tinymce/plugins/wpcom-view/wpvideo-view.jsx:

/Users/andrew/Documents/Code/wp-calypso/client/components/tinymce/plugins/wpcom-view/wpvideo-view.jsx
  43:36  warning  There must be a space inside this paren               space-in-parens
  49:25  warning  Infix operators must be spaced                        space-infix-ops
  50:52  warning  Unexpected space before function parentheses          space-before-function-paren
  51:44  warning  Unexpected space before function parentheses          space-before-function-paren
  52:6   warning  Expected indentation of 4 tab characters but found 5  indent
  53:5   warning  Expected indentation of 3 tab characters but found 4  indent
  55:5   warning  There must be a space inside this paren               space-in-parens
  55:10  warning  There must be a space inside this paren               space-in-parens
  55:18  warning  Unexpected space before function parentheses          space-before-function-paren
  57:5   warning  There must be a space inside this paren               space-in-parens
  57:11  warning  There must be a space inside this paren               space-in-parens
  57:15  warning  There must be a space inside this paren               space-in-parens

✖ 12 problems (0 errors, 12 warnings)

You might consider installing a linting plugin for your editor for these issues to be more evident:

https://github.com/Automattic/wp-calypso/blob/master/docs/coding-guidelines/javascript.md#eslint

[aduth]

Mixed whitespace (this should be tabs, not spaces).

Additionally, you may consider leveraging nesting for the pseudo-class :not selector (reference), i.e.

.wpview-wrap iframe {
    display: block;

    &:not( .wpview-type-video ) {
        width: 100%;
    }
}

Also note spaces between parens (reference).

[mjuhasz]

Fixed.

[aduth]

I wonder if we could reuse some of the embed view logic to simplify this, as the oEmbed endpoint markup already returns the video's known dimensions and includes the resizing script automatically.

Example: https://public-api.wordpress.com/oembed/1.0/?url=https%3A%2F%2Fvideopress.com%2Fv%2FkUJmAcSf%3Fat%3D90&for=wordpress

[aduth]

Many of the variable names in these files are obscure acronyms, which we try to avoid, as they're much more difficult to understand at a glance.

[mjuhasz]

I made the variable names more descriptive.

Attribute names are cryptic because those are the names expected in the query string and I want to avoid mapping them back and forth.

[mjuhasz]

The branch is rebased and test are up to the new standard.

Can you elaborate on how I could reproduce the multiple requests you saw?

If I load a draft with video with debug messages enabled (localStorage.setItem( 'debug', '*' );)

1. render is called in WpVideoView which in turn renders <QueryVideo>.
2. Right after that a wpcom:send-request sendRequest("/videos/kUJmAcSf" ) request is sent out.
3. WpVideoView gets re-rendered after wpcom-proxy-request got 200 status code for URL: "/videos/kUJmAcSf"

No other wpcom request is sent out for "/videos/kUJmAcSf" according to the console log.

[aduth]

@mjuhasz : I should have paid attention to the "Initiator" column in the network tab:

I'm guessing that these requests originate from the VideoPress script. Pretty unfortunate that the additional requests are made, but not sure we can do much about it under the current approach.

[apeatling]

@aduth @mjuhasz This has been a long time now, and seems like a great improvement. Can we get this one in? Otherwise we should close this.

[aduth]

@mjuhasz : Do you recall if there were any blockers to merging this? We'll want to rebase, but I'd be happy to help move this one forward, especially given the issues encountered in #3832 in using the shortcode iframe for rendering.

[mjuhasz]

@aduth VideoPress.js issues multiple requests for the same endpoint when loading a post which includes a video. Your former assessment still holds true:

Pretty unfortunate that the additional requests are made, but not sure we can do much about it under the current approach.

[lancewillett]

Re-opening since it'd be great to get #1730 support for improving our video offering, especially as it's a part of a paid plan.

[aduth]

Closed in favor of #8342