Support 'preload' attribute for Video Block #7929

Soean · 2018-07-12T11:42:18Z

Description

This PR adds the preload attribute to the video block.
As in the Classic editor the default is "metadata" and no attribute is set.

If not set, its default value is browser-defined (i.e. each browser may have its own default value). The spec advises it to be set to metadata.
(https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video#attr-preload).

auto sets preload="auto" and none sets preload="none"

The design is the same as the preload in Audio, see #7590

Gutenberg

Classic Editor

related #7501

jorgefilipecosta · 2018-07-12T16:54:11Z

core-blocks/video/edit.js

@@ -123,6 +124,17 @@ class VideoEdit extends Component {
 							onChange={ this.toggleAttribute( 'controls' ) }
 							checked={ controls }
 						/>
+						<SelectControl
+							label={ __( 'Preload' ) }
+							value={ undefined !== preload ? preload : 'metadata' }


Would setting a default value for the attribute definition solve the need for this undefined checks?
On the save we would only set the attribute if it was different from the default.

Thanks for your feedback, thats a better solution. I committed the change.

Soean · 2018-07-19T13:21:04Z

@jorgefilipecosta I think it's fine now, can you review it again?

jorgefilipecosta · 2018-07-19T20:19:47Z

core-blocks/video/index.js

 						loop={ loop }
 						muted={ muted }
+						preload={ preload !== 'metadata' ? preload : null }


I think we should also apply the property to video element used on edit. So the user may be able to notice some difference (if he had not played the video before).

If the user adds a video, it preloads by default. When the user changes the preload value, nothing changes because the video is already loaded. If the User selects none and reloads the page, there is a visible difference.
I think we can add muted and controls in a different PR to edit, but preload in edit is not very useful for the user because there is no visible change.

I agree with @Soean, I don't think we should add the preload to the edit output.

Also, we don't use preload in edit on the audio block, so best to be consistent and not use it here.

jorgefilipecosta · 2018-07-19T20:21:28Z

Thank you for the changes @Soean things tested well in my test 👍 I left a minor comment that we may consider addressing before the merge.

Soean · 2018-07-26T10:42:14Z

@jorgefilipecosta Thanks for your feedback. I don't think we should add it to edit, see #7929 (comment)

tofumatt

This is all done slightly differently in the Audio block, does it make sense to replicate that approach?

tofumatt · 2018-08-08T15:11:47Z

core-blocks/video/index.js

 						loop={ loop }
 						muted={ muted }
+						preload={ preload !== 'metadata' ? preload : null }


I agree with @Soean, I don't think we should add the preload to the edit output.

tofumatt · 2018-08-08T15:14:44Z

core-blocks/video/index.js

 						loop={ loop }
 						muted={ muted }
+						preload={ preload !== 'metadata' ? preload : null }


Also, we don't use preload in edit on the audio block, so best to be consistent and not use it here.

tofumatt

I think this is good and I pushed a little update that disables editor interaction with the tag and brings it in line with the audio component's implementation. I think this is good to go.

tofumatt

Scratch that, I'm seeing some block validation fails from earlier blocks, looking into it now.

Copons · 2018-08-22T16:56:45Z

@tofumatt May I ask you more info regarding the decision of disabling the player controls?

While working on #8055 I got used to play the audio file to make sure it was uploaded correctly, so now it feels a bit weird that the player controls don't work anymore.
But more importantly though, imho the simple presence of controls gives to the users the expectation that they can interact with them.

As it is right now, there are no visual clues that those controls don't work (e.g. they are not "grayed out"), and so the player feels broken even if it isn't.

tofumatt · 2018-08-22T17:10:19Z

Right now clicking on the block in any way activates the controls, which is very jarring when the user just means to click on the block to edit it. I ran into this while testing the block in this branch, and we've used a similar pattern elsewhere for things like server-side rendered blocks with HTML comments that, when clicked, will activate a link: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/latest-comments/edit.js

I can see the argument for testing the content, though if it was uploaded and working you'll see a preview frame and length the in the players. I think testing the content is better done in the preview screen, leaving the editing screen to edit/interact with the content in an editing context.

If we were to add controls I think they'd be better in the inspector or the toolbar anyway so they're:

not triggered by the user clicking on the block
able to be used when playback controls are disabled

If you wanted to see that implemented–feel free to file a bug and we could discuss it further. 😄

EDIT: Oh, and please cc me on the bug. 😅

Copons · 2018-08-22T17:21:51Z

@tofumatt Thanks for the answer!
I understand the reasoning now but I'm not entirely sure I'm sold on the solution 😛

I guess disabled controls are ok per se, but I'd like to know that they are disabled (for example with some disabled style) before clicking on them, panicking, and eventually maybe realize they are just for show. 🙂

tofumatt · 2018-08-22T17:26:09Z

I'd like to know that they are disabled

That's fair! The <Disabled> component we use to disable interaction with rendered editor elements doesn't really apply default styles to the audio/video controls. We could at least do this for those blocks; could you file an issue for that and cc me on it? I'll do my best to take a look! 😄

tofumatt · 2018-08-22T17:32:26Z

(If you haven't made the issue yet, I can do it actually, sorry!)

Copons · 2018-08-22T17:37:48Z

@tofumatt No worries! I'm off for the day but I'll open it first thing tomorrow.

tofumatt · 2018-08-22T17:41:27Z

Too late! 😉 #9252

Soean added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks [Feature] Media Anything that impacts the experience of managing media labels Jul 12, 2018

benoitchantre mentioned this pull request Jul 12, 2018

Video block: missing features compared to classic editor #7501

Closed

jorgefilipecosta reviewed Jul 12, 2018

View reviewed changes

jorgefilipecosta reviewed Jul 19, 2018

View reviewed changes

Soean added this to the 3.4 milestone Jul 20, 2018

pento modified the milestones: 3.4, 3.5 Jul 30, 2018

gziolo modified the milestones: 3.5, 3.6 Aug 8, 2018

gziolo requested a review from a team August 8, 2018 11:12

tofumatt reviewed Aug 8, 2018

View reviewed changes

youknowriad modified the milestones: 3.6, 3.7 Aug 15, 2018

tofumatt force-pushed the add/preload-video branch from 0f10fa3 to 63cce64 Compare August 16, 2018 22:48

tofumatt self-assigned this Aug 16, 2018

Soean added 3 commits August 20, 2018 18:42

Add preload for the video block

a040312

Set metadata as default

a4a9cb9

Add preload to tests

21384cc

tofumatt force-pushed the add/preload-video branch from 63cce64 to 21384cc Compare August 20, 2018 17:42

feat: Disable editor interaction with video/audio blocks

f36835b

tofumatt approved these changes Aug 20, 2018

View reviewed changes

fix: allow preload=none

127e5d8

tofumatt requested changes Aug 20, 2018

View reviewed changes

fix validation issues

78d8d77

tofumatt approved these changes Aug 21, 2018

View reviewed changes

tofumatt merged commit 8755ebf into master Aug 21, 2018

tofumatt deleted the add/preload-video branch August 21, 2018 14:34

tofumatt mentioned this pull request Aug 22, 2018

Audio and Video controls should look disabled #9252

Closed

mmtr mentioned this pull request Oct 30, 2019

VideoPress: Newly-converted Video blocks are not playable. Automattic/wp-calypso#36836

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support 'preload' attribute for Video Block #7929

Support 'preload' attribute for Video Block #7929

Soean commented Jul 12, 2018

jorgefilipecosta Jul 12, 2018

Soean Jul 12, 2018

Soean commented Jul 19, 2018

jorgefilipecosta Jul 19, 2018

Soean Jul 19, 2018

tofumatt Aug 8, 2018

tofumatt Aug 8, 2018

jorgefilipecosta commented Jul 19, 2018

Soean commented Jul 26, 2018

tofumatt left a comment

tofumatt Aug 8, 2018

tofumatt Aug 8, 2018

tofumatt left a comment

tofumatt left a comment

Copons commented Aug 22, 2018

tofumatt commented Aug 22, 2018 •

edited

Loading

Copons commented Aug 22, 2018

tofumatt commented Aug 22, 2018

tofumatt commented Aug 22, 2018

Copons commented Aug 22, 2018

tofumatt commented Aug 22, 2018

Support 'preload' attribute for Video Block #7929

Support 'preload' attribute for Video Block #7929

Conversation

Soean commented Jul 12, 2018

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Soean commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Jul 19, 2018

Soean commented Jul 26, 2018

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

Copons commented Aug 22, 2018

tofumatt commented Aug 22, 2018 • edited Loading

Copons commented Aug 22, 2018

tofumatt commented Aug 22, 2018

tofumatt commented Aug 22, 2018

Copons commented Aug 22, 2018

tofumatt commented Aug 22, 2018

tofumatt commented Aug 22, 2018 •

edited

Loading