Add a Video prop type to the Code Component editor

📌 Change video prop shape: remove width and height, and make src optional Active landed 👍

1. Updated proposed resolution based on 📌 Change video prop shape: remove width and height, and make src optional Active
2. The config schema should already allow this: ✨ Define JSON Schema $refs for linking/embedding videos and linking documents Active already did:
   

             Choice:
               - 'json-schema-definitions://experience_builder.module/image'
   +           - 'json-schema-definitions://experience_builder.module/video'
   

   If you're getting 422 responses (validation errors) from the server upon creating these, assign to me: I'd be happy to assist/debug.

Approved because looks good from a shape matching POV 👍 The code looks nice too!

But it's not yet passing tests:

 ✖  code-editor-component-data-props.cy      641ms        1        -        1        -        - │
  │    .jsx                                                                                        │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✖  code-editor-component-data-slots.cy      531ms        1        -        1        -        - │
  │    .jsx     

@wim-leers

Getting 422 Error from the backend when you do Add to components because the poster values we use fail the validation because it looks like the validation checks for image file name extensions:

https://placehold.co/1080x1920?text=Vertical
https://placehold.co/1920x1080?text=Widescreen

I then worked around the validation error by adding .png in the URL so I think the frontend can just keep these as the poster URLs (MR updated), and the backend doesn't need to change its validation.

👍 Whew! 😅

But I wonder if that is what you were warning me about and this needs to be fixed now:

Yes!

That's even being thrown by \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\JsComponent::rewriteExampleUrl().

🐞

/modules/contrib/experience_builder/ui/assets/videos/mountain_wide.mp4

This would need to change, because it's different on every site 😅

For now I've assumed experience_builder/ui/assets/videos/mountain_wide.mp4.

🏓 Back to you!

I want to confirm something with the src url later but the frontend part is pretty much done ready for a review.

Per #13.

Manually tested this with this component

const Video = ({video, display_width = 400}) => {
  return (<video controls poster={video.poster} width={display_width}>
    <source src={video.src}/>
  </video>)
}
export default Video;

Worked a treat - nice one!

Going to test without xb_vite

Confirmed the default video doesn't work without xb_vite - which is likely #12

Preview in code editor suffers same issue if Vite isn't enabled

I made some changes to make this work with and without Vite. Hopefully they're ok @hooroomoo @wim leers.
I also tried to make it as portable as possible. In my earlier testing I found that if I had vite enabled the resolved example URL included https://localhost:5173 which wasn't portable. In the code I added to \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\JsComponent::rewriteExampleUrl I think that should survive both the module path changing AND the basepath.

Tested as follows both with and without vite and it is working:

* default value in code editor
* default value in XB preview
* default value in published node

Here's my JS component:

uuid: 3deb2c1e-2bfc-40af-a86b-650caca71b18
langcode: en
status: true
dependencies: {  }
machineName: video
name: Video
required:
  - video
  - caption
props:
  video:
    title: Video
    type: object
    examples:
      ### 👈️  This includes the location on my machine
       ### 👉️ But the code I've added should transpose that
      -
        src: /modules/experience_builder/ui/assets/videos/bird_vertical.mp4 

        poster: 'https://placehold.co/1080x1920.png?text=Vertical'
    $ref: 'json-schema-definitions://experience_builder.module/video'
  displayWidth:
    title: 'Display width'
    type: integer
    examples:
      - 400
    enum:
      - 200
      - 300
      - 400
      - 500
  caption:
    title: Caption
    type: string
    examples:
      - 'A video'
slots: {  }
js:
  original: |-
    const Video = ({video, caption, display_width = 400}) => {
      return (<figure><video controls poster={video.poster} width={display_width}>
        <source src={video.src}/>
      </video>
        <caption>{caption}</caption>
      </figure>)
    }
    export default Video;
  compiled: |
    import { jsx as _jsx, jsxs as _jsxs } from "react/jsx-runtime";
    const Video = ({ video, caption, display_width = 400 })=>{
        return /*#__PURE__*/ _jsxs("figure", {
            children: [
                /*#__PURE__*/ _jsx("video", {
                    controls: true,
                    poster: video.poster,
                    width: display_width,
                    children: /*#__PURE__*/ _jsx("source", {
                        src: video.src
                    })
                }),
                /*#__PURE__*/ _jsx("caption", {
                    children: caption
                })
            ]
        });
    };
    export default Video;
css:
  original: ''
  compiled: ''

Feel free to back any of this out if it doesn't suit

I agree with the intent of #19:

I think that should survive both the module path changing AND the basepath.

but there's one more thing I want/intended with what I described and did in #12, but described poorly (sorry! 😬): the generated config representation must be deterministic independent of the environment: the same exact examples entry should be present regardless of:

• site served from root (https://example.com/) or subdirectory (https://example.com/subdir/)
• XB module installation location (modules/experience_builder, modules/contrib/experience_builder, sites/all/modules/experience_builder, etc.)

1. I committed @larowlan's #19 to xb_test_code_components to ensure this works today, and any changes to any related infrastructure are both immediately obvious: the example videos, the representation of them in the code component config entities, and their automatic rewriting to resolvable URLs.
2. Added explicit test coverage to test all requirements I listed above: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
3. Tightened the logic to only allow 2 specific paths (again, see #20 for why): https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
4. Update the test code component accordingly: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
5. Made the generated URL not absolute but root-relative: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

What remains: updating the front-end, because that appears to currently assume that the literal values used in the preview are identical to the ones stored int the JavaScriptComponent config entity. They can't, precisely due to the URL rewriting. Because code components have now started using DefaultRelativeUrlPropSource. We got away with not supporting that so far for image code components because the default image is an absolute URL.
Even if this issue wouldn't force that distinction of "stored example" vs "stored example rewritten to work on the current environment", then 📌 `JavaScriptComponent` config entities must support default images: `File` content entity dependency, base64-encoded upon export Active will for sure force the same: there you'd upload a default image, name it bunny.jpg, and that'd then have to be rewritten to either a data: URI for use in the code editor preview, or to a /sites/default/files/xb/code-component-assets/bunny.jpg-esque URL.

So, pushed a commit outlining what needs to change on the FE: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... — @balintbrews, could you tackle that? 🙏

Approved BE pieces, now just needs the FE piece at the end of #21. 🙏

End of #21 needs to be done in our serializeProps() and deserializeProps() functions that live in ui/src/features/code-editor/utils.ts, and tests should be extended in tests/unit/code-editor-utils.cy.jsx.

Once critical piece left, see my comment on \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\JsComponent::rewriteExampleUrl.

Assigning to Wim to take care of #25.

Added back the ::setAbsolute(TRUE) and added a test to prove that you can call rewriteExampleUrl multiple times in that scenario.

balintbrews → credited mglaman → .

balintbrews → credited phenaproxima → .

Updating status based on Wim's latest comment. Besides that we'll need to re-test and validate from the frontend's perspective the actual part where the error was thrown before.

One more thing I wanted to add is what @mglaman and @phenaproxima helped me figured out after I was hitting the following 422 HTTP error:
Experience Builder does not know of a field type/widget to allow populating the <code>video prop, with the shape {\"type\":\"object\",\"$ref\":\"json-schema-definitions://experience_builder.module/video\"}.

The error happens as you're trying to save a JavaScriptComponent entity and you have no media type that supports local videos. (I didn't test what happens in case this prop shape occurs in an SDC — we are probably gracefully handling it by disabling the component, but it's worth checking.)

We discussed conditionally removing the prop type from the code editor when we detect that an appropriate media type is missing. Then there's the use case of a media type being removed after a prop like this was created, but maybe that's lower priority.

@lauriii, this issue could use your input.

I've got fixes for all 4 of my concerns (1 relating to #27, 3 relating to bugs in the sample experience_builder.js_component.xb_test_code_components_captioned_video.yml), but git.drupal.org is currently down so can't push 😬🤷‍♂️

EDIT: cross-posted with @balintbrews.

The 422 you quote, @balintbrews, occurs when using the Minimal install profile; the Standard install profile include a "local video" media type. This is why @bnjmnm added the xb_test_video_fixture() test-only module in 📌 [PP] FE implementation of Video Prop Active : to allow testing it in our e2e tests, which do use Minimal, not Standard.

We discussed conditionally removing the prop type from the code editor when we detect that an appropriate media type is missing.

That'd indeed be necessary. This would require the back-end to be aware of all prop shapes the front-end might create and providing a "go/no-go" flag. Similar to 📌 Pass current user's XB permissions to the XB UI Active 's drupalSettings.xb.permissions, most likely (which is a list of string: bool key-value pairs).

Actually … a simpler solution than the back-end informing the front-end of which shapes are allowed would be:

1. continue to allow any code component to be created
2. … but add error response handling to the button
3. Which would then clearly tell the code component developer what's happening:

#31: Right, I'm aware that Standard provides that, but most projects don't use Standard, or make changes when they do, so it would be easy to run into this problem.

#32: I think that's not a great experience. It's only slightly better than hitting the error we have now. A code component author might put in all the effort to build a polished component only to find out they can't use it.

I think the available props should be fed by backend because modules may want to add prop types as well. I think it would be fine to open a follow-up issue to resolve that.

Why would the video field not be able to resolve to a file field? It seems that if you had a file field which only allowed uploading videos, you should be able to map that to a component with the video prop.

Why would the video field not be able to resolve to a file field? It seems that if you had a file field which only allowed uploading videos, you should be able to map that to a component with the video prop.

That's a fair point! We simply overlooked that. I sure did 🙈🙈🙈

IMHO this is ready to be merged wrt the 4 concerns I raised and mentioned in #31.

But that still leaves @lauriii's "let's use the file field type and FileWidget as the fallback when no VideoFile-based MediaType exists" proposal. I implemented that.

But what's not yet working: the widget for that.

This is what it currently looks like:

This looks like a pure back-end problem, so investigating…

Also: this now means we need somebody (@bnjmnm?) to add the necessary e2e test coverage to prove this actually works, just like we did in 📌 [PP] FE implementation of Video Prop Active , which blocked this issue 😅.

Re-assigning to him for that, which can get started while I dig into #36. I don't know yet how far I'll get, because I'll need to call it a day very soon.

Root cause found. Added explicit check for it and now throws a helpful exception. Result: many failures, because 0.x was somehow not negatively affected by this bug.

The fix is simple: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

Debugging this, I'm not seeing a POST/PATCH request back to the back end after the AJAX is processed for attaching a file.
Looking into it, the fids hidden element is missing the data-ajax and data-form-id attributes.
They're being added to the managed_file element but they don't filter down to the inner elements.

Will dig deeper.

Ben indicated he was pulled onto something else, so updating assignment field to flag that I'm working on this.

I've started a branch here https://git.drupalcode.org/issue/experience_builder-3534601/-/merge_requ... that tries an alternate approach.

Looks like it has less fails than the current branch, but is messing up how media widgets work based on the fails.

There is also some issues with how the file generic widget works in XB vs in the node edit form.

Once a file is added, in the node edit form the widget no longer shows the <input type="file"> element. But in XB it does.
And I can see that is coming back from the AJAX response. There's multiple layers of process callbacks happening between file widget and managed file element but I can't see any of them that relate to showing/hiding that element.

I've also looked into preprocess functions etc.

.

And of course after commenting here in exasperation I found it \Drupal\file\Element\ManagedFile::preRenderManagedFile which is being clobbered by our ::prerender callback, fixing

Ok, this has beaten me today.

Where I got to:

• Work is in this branch 3534601-add-a-video-larowlan
• File generic is working except for falling back to the default value when you remove a previously uploaded file. From what I can see we're back in the same territory we were last week with expressions evaluating to NULL like we had for the video SDC. The difference here is that we're getting ['src' => NULL] instead of a plain NULL and this is preventing it using the default url prop source - see screenshot from debugging -
  
• If we can get past that, we still need to resolve e2e tests for the functionality and address the other fails the changes to filtering empty items yield here. Perhaps we can hard-code that filtering to file items as a temporary workaround

larowlan → changed the visibility of the branch 3534601-add-a-video-larowlan to hidden.

As is often the case, the path forward here came to me apropos of nothing whilst I was not thinking about it anymore.

I've added an alternate implementation of FileItem::isEmpty that allows for $this->target_id === '' to represent an empty value.

With that change and some code in prop value parsing in the front-end for required values where the prop is required, equal to '' but expecting an object that makes it consistent with changes we made for optional images being removed we have a green test-suite 🤯

I've gone through and marked all the open threads resolved as appropriate.

I've also written an e2e test for the expected behavior for both required and optional video props using @bnjmnm's example components. I modified the optional one slightly so that it worked without a video value.

I think this is ready again. I've commented the changes since the last review with ℹ️ to make it easier for reviewers.

Glad to come out the other side of this one 😌

I really, really, really wanted this to land.

But I also really, really, really want to understand this.

The changes described in #46 I understand conceptually.

However, undoing what my analysis at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... concluded and switching back to using absolute URLs is AFAICT still coating over the root cause of the problem. So … I brought back root-relative URLs for example URLs in code components. Not out of spite, not because I want to be right, not because I want my code.
But because I fear that we're creating more unstability. See my comment at #3535806-12: Default image is lost from optional images → , which I believe is another symptom of this bug.

Yes, fixing that bug is kind of out of scope here, but this issue is the first one to have surfaced it, and whose debugging led me to get closer to a solution.

The key fix is https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... — which resulted in a single failure, convincing me this is viable.

I hope to wrap this up tonight, but if not … @larowlan: 👆 this is why I didn't simply RTBC & merge — please forgive me. 😇

Green. See my ℹ️ comments for pointers on the key changes.

I think the changes this makes in GeneratedFieldExplicitInputUxComponentSourceBase probably would be more appropriate on 🐛 Default image is not loaded when adding a pattern Active , but I already said above why it happened here → .
In any case: no more reliance on throwing and catching exceptions and arcane comparison to make relative-URLs-as-examples in SDCs+code components work! 🥳

Cherry on the pie: my perhaps naïve hope to massively simplify turns out to not have been successful 🍰

If anything, that hopefully means 🐛 Default image is not loaded when adding a pattern Active can either be closed easily or repurposed for adding expanding test coverage and/or tackling what's below.

I think this bit in GeneratedFieldExplicitInputUxComponentSourceBase::inputToClientModel() still needs attention/reconsidering in 🐛 Default image is not loaded when adding a pattern Active (or elsewhere):

      // @see getPropsValues() in formUtil.ts for the equivalent empty string
      // comparison.
      if ((\is_scalar($value) && (string) $value === '')) {
        // Don't send empty values. This is consistent with
        // syncPropSourcesToResolvedValues in the type-script code.
        unset($model['source'][$prop_name], $model['resolved'][$prop_name]);
        continue;
      }

along with this bit in getPropsValues() in ui/src/components/form/formUtil.ts

    // @todo: this means that if an optional field has format requirements, it's
    //   not truly optional as the empty value will not be stored.

… because '' is not the only way a value can be empty.

concluded and switching back to using absolute URLs is AFAICT still coating over the root cause of the problem

Oh! I agree, I thought this was gone.

Will review the changes today, thanks for working on this

Removed the xbModuleUrl setting and made one minor change to support 🐛 Default image is not loaded when adding a pattern Active but for videos - ie. an optional video once removed should show the default.

Leaving at RTBC for Wim to confirm that fix

Thanks!

LGTM! 🚢

#48's is being addressed in 🐛 Hero component failed to render after removing text Active ! 🥳

Automatically closed - issue fixed for 2 weeks with no activity.