[PP] FE implementation of Video Prop

Created on 4 June 2025, about 2 months ago

Overview

Postponed on Define JSON Schema $refs for linking/embedding videos and linking documents Active

Once that lands, we should ensure the video prop is properly editable in the component instance form and add tests that prove as such.

It's possible that this will "just work" already, but the tests will still be necessary.

Proposed resolution

User interface changes

📌 Task
Status

Active

Version

0.0

Component

Redux-integrated field widgets

Created by

🇺🇸United States bnjmnm Ann Arbor, MI

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @bnjmnm
  • 🇺🇸United States effulgentsia

    "alpha blocker" was a typo. This is a blocker for beta.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    This issue's title/summary is misleading: Define JSON Schema $refs for linking/embedding videos and linking documents Active introduces not ONE "video" shape, but 2: one for local, one for remote.

    The local one is about to land: [#3524130#21], the remote one won't land today for sure. oEmbed is hard.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • Merge request !1227#3528396 F.E. video prop → (Merged) created by bnjmnm
  • 🇫🇮Finland lauriii Finland

    Note: the scope of this issue should be limited to local videos, i.e. we should not add support for OEmbed (Youtube / Vimeo) here.

  • 🇺🇸United States effulgentsia

    "FE implementation" is ambiguous, so re-titling this issue to clarify its scope. Add a Video prop type to the Code Component editor Active is a separate FE issue for the video prop that's a completely separate scope.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    This MR looks pretty far along, and the new ui/tests/e2e/media-video-prop.cy.js is passing! 🥳

    1. ✅ e2e tests run fine locally and fulfill the purpose. (The default media thumbnail is not loading, but that's fine — it does load on a "real" site, just tested that explicitly.)
    2. One small change would make the new test module more widely useful: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
    3. Question: what's the source for the 2 videos? Did you create them? Asking for licensing purposes: they're going to be licensed as GPL2+ upon commit (and in principle they already are by having them in the MR). Just hoping to confirm 🤞
    4. ✅ I don't quite like xb_test_video_fixture_install(): much of that could be just exported config instead; which would make this usable in recipes, too (and which would then benefit Playwright tests in the future).

      But since the Media and File entities must be created this way, I think it's reasonable and pragmatic 👍

    So: RTBC. If you can address 2+3 and get CI to green, let's merge 👍

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    #9.2

    Question: what's the source for the 2 videos? Did you create them? Asking for licensing purposes: they're going to be licensed as GPL2+ upon commit (and in principle they already are by having them in the MR). Just hoping to confirm

    I grabbed these videos from Pexels, which specifies they are free to use (although I should point out no specific license is mentioned). Also note I reduced the dimensions and frame rate of the videos before adding to provide a smaller file size.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    WFM 👍

  • 🇺🇸United States bnjmnm Ann Arbor, MI
  • Pipeline finished with Running
    18 days ago
    #542335
  • 🇺🇸United States bnjmnm Ann Arbor, MI

    Not 100% sure on this, but it seems like the mechanism that accounts for an image not yet having a value hasn't been applied to video. I added an intentionally failing e2e test that reproduces the steps.

    To Reproduce: Add video component to layout, then update the Display Width field before adding a video
    (This doesn't have to be the display width field - it can be any field in a component instance form with a video prop)

    • We get the Component failed to render, check logs for more detail. in the preview
    • The logged error is
      An exception has  been thrown during the rendering of a template 
          ("[experience_builder:video/video.src] NULL value found, but a  
          string is required. This may be because the property is empty    
         instead of having data present.
    • Here's the model being sent in the bad request

    There's one scenario where this won't happen: If the prop is optional and a video has been added then removed. In this instance, the empty video will not result in an error because of the way removed media items are handled on optional props. The request no longer attempts to send the default value and instead sends empty ones

    (The above is with a temporarily altered Video component so the video prop is not required)

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    Unassigning myself so a B.E. dev can have a look at #13

  • Pipeline finished with Failed
    18 days ago
    #542342
  • 🇺🇸United States effulgentsia

    I think @larowlan would have some insight into #13. Seems like our handling of "use this example value that's already the prop shape but isn't a Media reference" for images should also work for videos, but apparently something about it isn't.

  • First commit to issue fork.
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Got to the bottom of this and it was quite tricky

    The difference between image and video is shown here in the shape matching \Drupal\experience_builder\Hook\ShapeMatchingHooks::getFieldTypeProps

    return match ($media_source_class) {
          Image::class => [
            'src' => new ReferenceFieldTypePropExpression(
              new FieldTypePropExpression('entity_reference', 'entity'),
              new ReferenceFieldPropExpression(
                new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'entity'),
                new FieldPropExpression(BetterEntityDataDefinition::create('file'), 'uri', \NULL, 'url')
              )
            ),
            'alt' => new ReferenceFieldTypePropExpression(new FieldTypePropExpression('entity_reference', 'entity'), new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'alt')),
            'width' => new ReferenceFieldTypePropExpression(new FieldTypePropExpression('entity_reference', 'entity'), new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'width')),
            'height' => new ReferenceFieldTypePropExpression(new FieldTypePropExpression('entity_reference', 'entity'), new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'height')),
            // TRICKY: Additional computed property on image fields added by Experience Builder.
            // @see \Drupal\experience_builder\Plugin\Field\FieldTypeOverride\ImageItemOverride
            'srcSetCandidateTemplate' => new ReferenceFieldTypePropExpression(new FieldTypePropExpression('entity_reference', 'entity'), new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'srcset_candidate_uri_template')),
          ],
          VideoFile::class => [
            'src' => new ReferenceFieldTypePropExpression(
              new FieldTypePropExpression('entity_reference', 'entity'),
              new ReferenceFieldPropExpression(
                new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'entity'),
                new FieldPropExpression(BetterEntityDataDefinition::create('file'), 'uri', \NULL, 'url'),
                TRUE,
              )
            ),
          ],
          default => [],
        };
    

    If you look at the src expression on both of them, you will see that its largely identical.
    So why doesn't our fallback to default values work for video?

    Well its purely by change that the Image also has alt, width and height

    When we try to evaluate these expressions in \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::clientModelToInput because there's no entity found, we end up in the catch block that falls back to the default value.

    But this doesn't happen for Video. But why?

    Because Image shape match contains three ReferenceFieldTypePropExpressions that wrap FieldPropExpressions but Video only contains one that wraps ReferenceFieldPropExpression

    And if we look into the evaluator it has this code

    if ($entity_or_field === NULL) {
          // Entity is optional for reference fields: the reference may point to
          // something or not.
          if ($expr instanceof ReferenceFieldPropExpression) {
            return NULL;
          }
          throw new \OutOfRangeException('No data provided to evaluate expression ' . (string) $expr);
        }
    

    so the $expr instanceof ReferenceFieldPropExpression code here means that for Video, the OutOfRangeException is never thrown and hence the default value handling doesn't work.

    To fix this, I added a new option to ReferenceFieldPropExpression, an isRequired flag. I would like @wim leers to confirm my approach here as I'm not totally sure about USV (although I'm not really making use of USV to track the required flag).

    Once I add that and check for it, it works as expected. You can find that change in this commit

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    👏 to @larowlan for tracking that down. Assigning to @wim-leers here to reflect the existing assignment in the MR to get a +1 on that backend solution.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Apparently it is a deep back-end/shape matching bug that @bnjmnm's e2e test coverage uncovered here 🤯

    While the purpose was ensuring the worked as expected, it actually largely did (which I already expected given how I didn't have to change a single thing to video-selection-from-Media-Library work — see #5), except that infrastructure broke it. 👻

    EDIT: cross-posted with @bnjmnm 😅

  • Pipeline finished with Failed
    17 days ago
    #543087
  • Pipeline finished with Running
    17 days ago
    #543137
  • Pipeline finished with Success
    17 days ago
    Total: 656s
    #543248
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Found one more scenario that hits a bug that causes the sdc.experience_builder.video component to fail to render:

    Twig\Error\RuntimeError occurred during rendering of component 8e5cdb12-a631-49b0-afa0-e69887dfdf86 in Content two (-), field field_xb_demo: An exception has been thrown during the rendering of a template ("[experience_builder:video/display_width] Must have a minimum value greater than or equal to 1. The provided value is: "0".") in "experience_builder:video" at line 1.
    

    Steps to reproduce:

    1. 🟢 Insert XB's Video component
    2. ℹ️ Observe that the client-side returned by /xb/api/v0/config/component for this component contains:
          "propSources": {
            "video": {
              "required": true,
              "jsonSchema": {
                "title": "video",
                "type": "object",
                "required": [
                  "src"
                ],
                "properties": {
                  "src": {
                    "title": "Video URL",
                    "type": "string",
                    "format": "uri-reference",
                    "pattern": "^(/|https?://)?.*\\.([Mm][Pp]4)(\\?.*)?(#.*)?$"
                  },
                  "poster": {
                    "title": "Image URL",
                    "type": "string",
                    "format": "uri-reference",
                    "pattern": "^(/|https?://)?.*\\.([Pp][Nn][Gg]|[Gg][Ii][Ff]|[Jj][Pp][Gg]|[Jj][Pp][Ee][Gg]|[Ww][Ee][Bb][Pp]|[Aa][Vv][Ii][Ff])(\\?.*)?(#.*)?$"
                  }
                }
              },
              "sourceType": "static:field_item:entity_reference",
              "expression": "ℹ︎entity_reference␟{src↝entity␜␜entity:media:video␝field_media_video_file␞␟entity␜*␜entity:file␝uri␞␟url}",
              "sourceTypeSettings": {
                "storage": {
                  "target_type": "media"
                },
                "instance": {
                  "handler": "default:media",
                  "handler_settings": {
                    "target_bundles": {
                      "video": "video"
                    }
                  }
                }
              },
              "default_values": {
                "source": [
                  
                ],
                "resolved": {
                  "src": "https://media.istockphoto.com/id/1340051874/video/aerial-top-down-view-of-a-container-cargo-ship.mp4?s=mp4-640x640-is&k=20&c=5qPpYI7TOJiOYzKq9V2myBvUno6Fq2XM3ITPGFE8Cd8=",
                  "poster": "https://example.com/600x400.png"
                }
              }
            },
            "display_width": {
              "required": false,
              "jsonSchema": {
                "type": "integer",
                "minimum": 1
              },
              "sourceType": "static:field_item:integer",
              "expression": "ℹ︎integer␟value",
              "sourceTypeSettings": {
                "instance": {
                  "min": 1,
                  "max": null
                }
              }
            }
          },
          "transforms": [
            
          ],
      
    3. 🟢 Select a video (here: the one with target_id=2) but do not enter a display width, the client will send:
      {
          "source": {
              "video": {
                  "expression": "ℹ︎entity_reference␟{src↝entity␜␜entity:media:video␝field_media_video_file␞␟entity␜*␜entity:file␝uri␞␟url}",
                  "sourceType": "static:field_item:entity_reference",
                  "value": "2",
                  "sourceTypeSettings": {
                      "storage": {
                          "target_type": "media"
                      },
                      "instance": {
                          "handler": "default:media",
                          "handler_settings": {
                              "target_bundles": {
                                  "video": "video"
                              }
                          }
                      }
                  }
              },
              "display_width": {
                  "expression": "ℹ︎integer␟value",
                  "sourceType": "static:field_item:integer",
                  "value": [],
                  "sourceTypeSettings": {
                      "instance": {
                          "min": 1,
                          "max": null
                      }
                  }
              }
          },
          "resolved": {
              "video": "2",
              "display_width": []
          }
      }
      

      This renders fine.

    4. 🔴 Then I remove the selected video, which causes:
      {
          "source": {
              "video": {
                  "expression": "ℹ︎entity_reference␟{src↝entity␜␜entity:media:video␝field_media_video_file␞␟entity␜*␜entity:file␝uri␞␟url}",
                  "sourceType": "static:field_item:entity_reference",
                  "value": {
                      "src": "https://media.istockphoto.com/id/1340051874/video/aerial-top-down-view-of-a-container-cargo-ship.mp4?s=mp4-640x640-is&k=20&c=5qPpYI7TOJiOYzKq9V2myBvUno6Fq2XM3ITPGFE8Cd8=",
                      "poster": "https://example.com/600x400.png"
                  },
                  "sourceTypeSettings": {
                      "storage": {
                          "target_type": "media"
                      },
                      "instance": {
                          "handler": "default:media",
                          "handler_settings": {
                              "target_bundles": {
                                  "video": "video"
                              }
                          }
                      }
                  }
              },
              "display_width": {
                  "expression": "ℹ︎integer␟value",
                  "sourceType": "static:field_item:integer",
                  "value": [
                      {
                          "value": ""
                      }
                  ],
                  "sourceTypeSettings": {
                      "instance": {
                          "min": 1,
                          "max": null
                      }
                  }
              }
          },
          "resolved": {
              "video": {
                  "src": "https://media.istockphoto.com/id/1340051874/video/aerial-top-down-view-of-a-container-cargo-ship.mp4?s=mp4-640x640-is&k=20&c=5qPpYI7TOJiOYzKq9V2myBvUno6Fq2XM3ITPGFE8Cd8=",
                  "poster": "https://example.com/600x400.png"
              },
              "display_width": [
                  {
                      "value": ""
                  }
              ]
          }
      }
      

      Note how display_width's raw widget value is passed: value: "", which matches <input data-drupal-selector="edit-xb-component-props-189b4371-3fc7-4b4e-bb98-e5fe9a4c291d-display-width-0-value" data-form-id="component_inputs_form" type="number" id="edit-xb-component-props-189b4371-3fc7-4b4e-bb98-e5fe9a4c291d-display-width-0-value" name="xb_component_props[189b4371-3fc7-4b4e-bb98-e5fe9a4c291d][display_width][0][value]" step="1" min="1" placeholder="" class="_root_1or6m_1 form-number" value>

    … which looks an awful lot like client-side transforms aren't being applied. Which … they aren't, because they're absent from the API response in step 1 — for every component 😅

    Ah … but … it's 🐛 Component transforms need to be per sourceType, not per component prop Active that made it so, and moved that to #attached['xb-transforms'] + respecting that in \Drupal\experience_builder\Render\MainContent\XBTemplateRenderer::renderResponse().

    To be determined where the root cause of that lies, but this is unfortunately reproducibly broken. The e2e test can and should be expanded to remove the video, check that it renders, then re-add it and continue the remainder of the test (where it sets display width 190 etc).

    From a https://en.wikipedia.org/wiki/Robustness_principle POV, the server-side should gracefully handle invalid values received by the client that the field type cannot make sense of (FieldItemListInterface::filterEmptyItems()), so … adding a work-around for the above problem, and triggering a deprecation error, to allow us to figure us out the root cause after beta1 👍

  • Pipeline finished with Failed
    17 days ago
    Total: 685s
    #543346
  • Pipeline finished with Failed
    17 days ago
    Total: 2027s
    #543377
  • Pipeline finished with Failed
    17 days ago
    Total: 855s
    #543395
  • Pipeline finished with Failed
    17 days ago
    Total: 679s
    #543410
  • Pipeline finished with Failed
    17 days ago
    Total: 804s
    #543422
  • Pipeline finished with Canceled
    17 days ago
    Total: 1304s
    #543502
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    So after the crazy #20 detour, time to properly answer @larowlan's #17:

    To fix this, I added a new option to ReferenceFieldPropExpression, an isRequired flag. I would like @wim leers to confirm my approach here as I'm not totally sure about USV (although I'm not really making use of USV to track the required flag).

    1. This was missing the corresponding update to FieldObjectPropsExpression, which itself uses ReferenceFieldPropExpression. This is totally possible to make work: I got it to green.
    2. To keep the expressions consistent, we should also update ReferenceFieldTypePropExpression, even though for data integrity reasons, it's already required that a StaticPropSource is non-empty (see assert($field_item_list->count() === 1); in \Drupal\experience_builder\PropSource\StaticPropSource::isMinimalRepresentation()). This will require widespread changes.
    3. The
            // Entity is optional for reference fields: the reference may point to
            // something or not.
            if ($expr instanceof ReferenceFieldPropExpression) {
              return NULL;
            }
      

      code is positively ancient — I never expected it to survive to this day: it was a prototype to throw away/refactor! 😅 It dates back to >14 months ago, before we even were using the issue queue and MRs.

    4. Finally: it's very easy to make a mistake somewhere along the way in the expression. HEAD contains a bug. Specifically, it's very easy to to make one reference in a chain required. Lee's original commit forgets this too, because it results in:
      ℹ︎entity_reference␟{src↝entity␜␜entity:media:baby_videos|vacation_videos␝field_media_video_file|field_media_video_file_1␞␟entity␜*␜entity:file␝uri␞␟url}
      

      and not
      ℹ︎entity_reference␟{src↝entity␜*␜entity:media:baby_videos|vacation_videos␝field_media_video_file|field_media_video_file_1␞␟entity␜*␜entity:file␝uri␞␟url}

    So … trying a completely different approach: rather than modifying our StructuredDataPropExpressionInterfaces themselves, with not only LOTS of changes throughout the codebase, but also makes every hook_storage_prop_shape_alter() implementation more brittle. Furthermore … it'd require changing that hook, because it currently does not distinguish between required or not! That'd also mean potentially doubling every CandidateStorablePropShape: once for required, once for optional.

    Turns out … that works too 😊 commit + update a few calls I missed = green, and I'll be able to revert lots of (most?) changes.

    💡 All this led to the lightbulb moment that that code I quoted in point 3 above is truly positively ancient and should be generally applying, rather than special-casing ReferenceFieldPropExpression. Just did that, and then spent (too much) time figuring out the 2 failures: it was because of optional props with examples that need DefaultRelativeUrlPropSource … 😬 made that much more explicit.

    That was green, so finally reverted all the prop expression changes: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

    All green, more explicit, less change (if we'd have had to push this to completion, then all version hashes would change too, not to mention the last point), thoroughly manually tested.

    I'm RTBC'ing and merging this as soon as it's green, because it A) makes @bnjmnm's new e2e test pass, B) it's totally fine for @larowlan to want to do a follow-up MR, C) it's totally fine for @bnjmnm to add the test coverage for #20 in a follow-up MR.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • Pipeline finished with Skipped
    17 days ago
    #543533
  • Pipeline finished with Skipped
    17 days ago
    #543534
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • Pipeline finished with Failed
    17 days ago
    Total: 1437s
    #543522
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    @larowlan in chat:

    that sounds much cleaner, thanks

    Yay, that answers — would still be nice to see happen 😊

  • Tested changes on branch 0.x , following scenarios :

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Thanks for the detailed report!

    • Error message : X data must be >= 1
    • On navigating away 0 value is removed and display width is empty
    • on preview on page video width is 30
    • After publish , on page video width is empty , and set according to the page size (902)

    This is a known problem. The preview then does not match (width 30) the entered value (0), which then post-publish indeed results in something the author did not see in the preview. The issue to change that: 📌 Invalid prop errors should be detailed in Component preview Active .

    Same for the other.

Production build 0.71.5 2024