- Issue created by @wim leers
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
@lauriii, do you agree that does not make sense to support, and the 3 remaining missing
$ref
s in the issue summary would address all needs? - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
@lauriii, also see the issue summary: do you want only (oEmbed) remote video, or also local video URLs?
Added more detail to the oEmbed video proposal.
- ๐ซ๐ฎFinland lauriii Finland
I think we should express the uploaded videos as follows in
schema.json
:{ "video": { "title": "video", "type": "object", "required": ["src"], "properties": { "src": { "title": "Video URL", "type": "string", "format": "uri" }, "poster": { "title": "Poster image URL", "$ref": "json-schema-definitions://experience_builder.module/image-uri" }, "width": { "title": "Video width", "type": "integer" }, "height": { "title": "Video height", "type": "integer" }, "autoplay": { "title": "Autoplay", "type": "boolean" }, "loop": { "title": "Loop", "type": "boolean" }, "muted": { "title": "Muted", "type": "boolean" }, "controls": { "title": "Show controls", "type": "boolean" } } } }
For oEmbed, I'm not sure we need the max width / max height here. It seems something that the component itself should handle? We may need to expose the height / width of the resource, as well as title which would make this shape as follows in
schema.json
:{ "oembed_video": { "title": "oembed_video", "type": "object", "required": ["url"], "properties": { "url": { "title": "oEmbed URL", "type": "string", "format": "uri", "contentMediaType": "application/oembed+json", "x-oembed-type": "video" }, "width": { "title": "Video width", "type": "integer" }, "height": { "title": "Video height", "type": "integer" }, "title": { "title": "Video title", "type": "string" } } } }
For documents, to display a link, we'd need a bit more than just the URI in the
schema.json
:{ "document": { "title": "document", "type": "object", "required": ["src"], "properties": { "src": { "title": "Document URL", "$ref": "json-schema-definitions://experience_builder.module/document-uri" }, "title": { "title": "Document title", "type": "string" }, "description": { "title": "Document description", "type": "string" }, "filename": { "title": "Filename", "type": "string" }, "filesize": { "title": "File size", "type": "integer", "description": "File size in bytes" }, "mimetype": { "title": "MIME type", "type": "string" } }, "document-uri": { "title": "Document URL", "type": "string", "format": "uri-reference", "pattern": "^(/|https?://)?.*\\.(txt|rtf|doc|docx|ppt|pptx|xls|xlsx|pdf|odf|odg|odp|ods|odt|fodt|fods|fodp|fodg|key|numbers|pages)(\\?.*)?(#.*)?$" } }, }
- ๐ฌ๐งUnited Kingdom f.mazeikis Brighton
f.mazeikis โ made their first commit to this issueโs fork.
- ๐ฌ๐งUnited Kingdom f.mazeikis Brighton
@lauriii
In #5 you've proposed shapes for
video
andoembed_video
and I have some questions.- For
video
you've proposedposter
property - as @chirstian pointed out on the draft MR - the commonly used term (and already existing property in Drupal Media API) forposter
would bethumbnail
. This also applies to Youtube, vimeo and others. Is there a good reason we would not usethumbnail
as terminology and the actual Drupal Mediathumbnail
property? - Properties for
height
andwidth
forvideo
are not currently stored for video_file source, we could alter the source and add missing properties viahook_media_source_info_alter
- is this what you would expect? - For
oembed_video
described properties could be fetched from oEmbed resource provider (Youtube or Vimeo) - Media entity itself seems to fetch those at the runtime. Do you expect us to fetch the values and store/own them as prop inputs? Fetch the values and use them, but not store them? Do you want users to be able to provide their owntitle
,width
andheight
, that would override whatever oEmbed provider supplies?
Depending on the answer to 3 it might look quite similar to 2 - just trying to figure out what the expectation is here, before I write a bunch of code.
- For
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
#9:
<video poster>
is not semantically the same as\Drupal\media\Entity\Media::loadThumbnail()
.See https://developer.mozilla.org/en-US/docs/Web/API/HTMLVideoElement/poster and contrast with
core/modules/image/config/install/image.style.thumbnail.yml
.The media thumbnail is a (typically square) image used when browsing the media library. You cannot use this for
<video poster
: the aspect ratio would be wrong, and the quality is very likely far too low.Conclusion: without the ability to extract an image from a video (we definitely can't do that โ Drupal's media system doesn't even do this), we must make do without a
poster
. Which is fine, because as you can see:poster
is optional!just do not populate
poster
by default ๐- Same thing here โ they're both optional. Since core's "Video" media type doesn't support it, I don't see how XB could support it.
โ ๏ธ โ ๏ธ โ ๏ธ OTOH โ what you're dealing with here is that
<video width>
refers to the video display area, not the video's resolution.I bet that this is something @lauriii would like to see supported โ even if you could enter bad values (e.g. specifying width/height not respecting aspect ratio etc.), because it impacts the content authoring experience a lot.
However, given the current reality of XB only allowing to use
StaticPropSource
s to populate SDC/code component props, this technically requires a field type + widget that providessrc
(media library) +width
/height
(separate static inputs). This is whatAdaptedPropSource
was intended for, but XB's UI doesn't support this yet.we don't allow
width
andheight
to be entered either, because we cannot support it yet. Here too, that's possible, because both are optional ๐ -
Do you expect us to fetch the values and store/own them as prop inputs? Fetch the values and use them, but not store them? Do you want users to be able to provide their own title, width and height, that would override whatever oEmbed provider supplies?
Here too, everything I wrote in response to #9.1 and #9.2 applies:
width
andheight
are not the ones you seem to think these are referring to (and would requireAdaptedPropSource
in the UI), andurl
is the only one that is required.
tl;dr: forget about all optional key-value pairs, limit scope here to only the required ones, because they're the only ones that are feasible.
- ๐ฌ๐งUnited Kingdom f.mazeikis Brighton
Implemented feedback from #10 โจ Define JSON Schema $refs for linking/embedding videos and linking documents Active , updated
simple/video
SDC component and added newsimple/ombed_video
SDC component. This was useful for testing via UI, but not quite sure if we want to keep it.
Added comments to MR, moving to needs review. - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
YAYAYAY!
First local video from Drupal's Media Library rendered via a component:
First remote video (about XB of course โ Lee's talk on YouTube!) rendered via a component:
โฆ unfortunately some kind of issue with YouTube refusing things, probably CSP-related, we can figure that out next ๐
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Found the culprit. oEmbed videos are passed as-is:
<iframe src="https://www.youtube.com/watch?v=v747p7xEcgg" width="" height="" title="" data-xb-uuid="a9a2498e-e1a1-4464-b2ea-db72daf7ea60"></iframe>
โฆ which is incorrect.
This should actually render something like
<iframe src="https://example.com/media/oembed?url=https%3A//www.youtube.com/watch%3Fv%3Dv747p7xEcgg&max_width=0&max_height=0&hash=2kCbWsOQ7urG-UZLzC9KUBuT1B-7jnlX66TsoG_WL7g" width="" height="" title="" data-xb-uuid="a9a2498e-e1a1-4464-b2ea-db72daf7ea60"></iframe>
Why? Because that's how core renders remote videos successfully. See for yourself by doing:
- Go to
/admin/config/media/media-settings
and enable stand-alone media URLs - Create a video media entity for some YouTube video
- Navigate to
/media/N
- Video appears!
- Put a breakpoint in
OEmbedFormatter::viewElements()
, observe the different steps, specifically how$url
and$resource_url
exist. This results in:
array ( '#type' => 'html_tag', '#tag' => 'iframe', '#attributes' => array ( 'src' => 'http://core.test/media/oembed?url=https%3A//www.youtube.com/watch%3Fv%3Dv747p7xEcgg&max_width=0&max_height=0&hash=2kCbWsOQ7urG-UZLzC9KUBuT1B-7jnlX66TsoG_WL7g', 'scrolling' => false, 'width' => 200, 'height' => 113, 'class' => array ( 0 => 'media-oembed-content', ), 'loading' => 'lazy', ), '#attached' => array ( 'library' => array ( 0 => 'media/oembed.formatter', ), ), )
I think this MR needs to perform that
https://www.youtube.com/watch?v=v747p7xEcgg
๐http://core.test/media/oembed?url=https%3A//www.youtube.com/watch%3Fv%3Dv747p7xEcgg&max_width=0&max_height=0&hash=2kCbWsOQ7urG-UZLzC9KUBuT1B-7jnlX66TsoG_WL7g
transformation, by adding a new computed property on the field definitions for the oEmbed-using
MediaType
bundles of theMedia
entity type. That can be done usinghook_entity_bundle_field_info_alter()
. The MR already needs that for something else too: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...(The
hash
is coming from\Drupal\media\IFrameUrlHelper::getHash()
, the rest from\Drupal\media\OEmbed\UrlResolver::getResourceUrl()
.)This would also result in
\hook_oembed_resource_url_alter()
etc firing (see the example implementation: it rewrites all YouTube URLs to the cookieless variant!) - Go to
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Making @f.mazeikis' observation easier to find: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1....
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
This needs work for:
- multiple remarks on the MR
- but most importantly because it's not yet resolving an author-provided URL to the corresponding oEmbed URL, which is why remote videos refuse to render
Note: I'd be totally fine with splitting this across multiple MRs, so you could already land the "local video" MR immediately.
Finally: this is not yet handling document links.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
StaticPropSource
matching was already working ๐DynamicPropSource
matching was not yet working, in part due to core bugs. While this also doesn't work in HEAD for multi-bundle image media references, it's important that the shapes we define are known to be matchable to avoid painting ourselves into a corner by the time we work onContentTemplates
in the near (post-beta1) future. So, fixed that.
There are still remaining caveats though, but that's definitely a pre-existing problem, and the most critical part (the video URL) is working ๐ Follow-up: ๐ Add `IntegerSemanticsConstraint` Active .- The approach @f.mazeikis took for oEmbed videos was based on a suggestion by @lauriii: add a new Twig function. That's pragmatic, and makes things work, but โฆ it also makes the subsequent issue
๐
[PP] FE implementation of Video Prop
Active
impossible. So the solution is to do what we've done for the
file_uri
field type'surl
property or better yet: the wholly newurl
property we just added to thelink
field type in โจ [PP-1] Make link widget autocomplete work (for uri and uri-reference props) Postponed .
The latter is what I'm currently working on, and is very unlikely to get finished in the next 1.5 hour (before the next and final sprint before beta1 starts).
So, being pragmatic: creating an MR to land just local video shape matching support, to unblock ๐ [PP] FE implementation of Video Prop Active with at least local videos, because as of finishing point 2 above, that's completely working. Here it is in action in both the
all-props
SDC and the newvideo
SDC: - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Still had one major concern about the new
experience_builder:video
SDC, because it conflated:- intrinsic width: width of the video itself โ a fact
- display width: the width the content author would like the video to be displayed at โ a subjective decision
So, made that a reality:
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
The change I made in #21 introduces a new prop:
display_width
. It usestype: integer, minimum: 1
.This caused 2 test failures:
Drupal\Tests\experience_builder\Kernel\Config\ComponentTest::testComponentAutoUpdate Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for experience_builder.component.sdc.experience_builder.video with the following errors: 0 [active_version] The version da2ffec495563dd6 does not match the hash of the settings for this version, expected facbb275ecb9699a.
+
Drupal\Tests\experience_builder\Kernel\MediaLibraryHookStoragePropAlterTest::testUniquePropSchemaDiscovery Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for experience_builder.component.sdc.experience_builder.video with the following errors: 0 [active_version] The version 44d35b0af1837caf does not match the hash of the settings for this version, expected f828aa2fb0fe4c76.
Note that
''
is literally the default value formax
in\Drupal\Core\Field\Plugin\Field\FieldType\IntegerItem::defaultFieldSettings()
. So the instance settings we're validating:[ "max": "", "min": 1, ]
Root cause:
Component::save()
eventually calls\Drupal\Core\Config\Config::save(has_trusted_data: FALSE)
, which eventually calls\Drupal\Core\Config\StorableConfigBase::castValue(key: 'versioned_properties.active.settings.prop_field_definitions.display_width.field_instance_settings.max', value: '')
- which contains this beauty:
// Special handling for integers and floats since the configuration // system is primarily concerned with saving values from the Form API // we have to special case the meaning of an empty string for numeric // types. In PHP this would be casted to a 0 but for the purposes of // configuration we need to treat this as a NULL. $empty_value = $value === '' && ($element instanceof IntegerInterface || $element instanceof FloatInterface); if ($value === NULL || $empty_value) { $value = NULL; }
๐ฌ๐ฌ๐ฌ๐ฌ๐ฌ๐ฌ๐ฌ๐ฌ
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
This should do it: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
-
wim leers โ
committed 2932feb8 on 0.x
Issue #3524130 by f.mazeikis, wim leers, lauriii: Define JSON Schema $...
-
wim leers โ
committed 2932feb8 on 0.x