Problem/Motivation

Having a video formatter that shows the video's thumbnail image and opens the video in a dialog is a very typical case.

We then especially need to take care of Vidstack controls to be passed to the modal player and things like auto-stop on modal close.

VideoJS seems to cover that with the following functionality:
https://docs.videojs.com/modaldialog
Edit: Seems not to be available in v10 any more: https://github.com/videojs/v10/discussions/2158
Question: https://github.com/videojs/v10/discussions/2158

Drupal Core will also improve its dialogs, but I think the integrated VideoJs ModalDialog is preferable for interaction:
#2158943: Add a native dialog element using HTMX to deprecate the jQuery UI dialog

See #9 for the next steps here.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork videojs-3616068

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

anybody created an issue. See original summary.

anybody’s picture

I think these issues are somehow related.

_shy’s picture

Version: 3.x-dev » 10.x-dev
grevil’s picture

Title: [10.x] Add a VideoJs Dialog formatter with thumbnail » [10.x] Add a VideoJs Dialog (Modal) formatter with thumbnail

Instead of having this as a seperate formatter, we should use a setting instead. Through #3577815: [10.x] Prepare 10.x branch for Video.js 10 implementation we'll already implement some kind of "thumbnail" behavior, which we'll use to have the following behaviors:
- Open in seperate Tab
- Open in same Tab
- Default behavior (play inline)

We will add another "Open in Modal / Dialog" option, once this gets supported through https://github.com/videojs/v10/discussions/2158. If the maintainers decide to not support this feature out of the box, we'll have to implement it ourselves.

anybody’s picture

Issue summary: View changes

Yes, perfect!

If needed the native implementation will be possible through #2158943: Add a native dialog element using HTMX to deprecate the jQuery UI dialog in core, hopefully sooner than later.
We then especially need to take care of Vidstack controls to be passed to the modal player and things like auto-stop on modal close.

anybody’s picture

Issue summary: View changes
anybody’s picture

Assigned: Unassigned » grevil
Status: Active » Postponed
anybody’s picture

@grevil: In the next weeks please at least implement the non-xhtml version as discussed, shouldn't be too complicated. Before we forget about the details...

anybody’s picture

Issue summary: View changes

@grevil: The issue summary needs some updates, I think:

1. There's no more built-in modal / dialog in VideoJS 10, I think. At least for now. I've opened https://github.com/videojs/v10/discussions/2158 to ask for it. This was only for older versions, not 10.x: https://github.com/videojs/v10/discussions/2158

2. As discussed, we're planning to solve this through the same formatter with an option. And we're planning to probably implement it in two steps.

2.1: Place the player directly into the <dialog> component, without AJAX-loading. The downside might be already loading the player / video when only the thumbnail is needed (yet). But maybe it's no issue with lazy loading at all.

2.2: If 2.1 is an issue or there are other reasons, we might provide a dedicated (AJAX) controller for video dialogs which loads the player in the dialog dynamically.

Until #2158943: Add a native dialog element using HTMX to deprecate the jQuery UI dialog is finished, providing an SDC in Drupal for Dialogs, we might implement 2.1 with a @todo for later refactoring to use the SDC.

Once 2.1 is implemented, we should decide if 2.2 makes sense as follow-up.

anybody’s picture

Issue summary: View changes
grevil’s picture

See discuission here: https://git.drupalcode.org/project/drupal/-/merge_requests/16540#note_17...

For an example implementation.

grevil’s picture

I restored the original implementation I made through #3577815: [10.x] Prepare 10.x branch for Video.js 10 implementation, for the HTMX implementation.

The implementation still needs to be checked and compared with the comment from https://git.drupalcode.org/project/drupal/-/merge_requests/16540#note_17....

But first we should implement the native dialog implementation anyway.

anybody’s picture

Status: Postponed » Active

Update: Now they are planning to include it!
https://github.com/videojs/v10/issues/2384

Currently it's a draft.

Nothing against the Drupal solution plus the VideoJS solution as three alternatives, so the user can chose which one works best.
But let's take the simple path first.

Should we wait for the draft and test the built-in solution?

I'm pretty sure all three will have their pro's and con's.

grevil’s picture

Apparently its now completed: https://github.com/videojs/v10/issues/2384.

grevil’s picture

OK inside beta31, the dialog is not yet included and beta32 doesn't have a built (yet).

We should prefer "3616068-10.x-videojs-dialog" as the first implementation. The gist of its current implementation is good, but far from being final. I basically almost copy pasted the example from https://videojs.org/docs/framework/html/reference/dialog.

But since media-dialog doesn't exist in the current js library yet, we have to wait a bit more.

grevil’s picture

Title: [10.x] Add a VideoJs Dialog (Modal) formatter with thumbnail » [10.x] Add a thumbnail setting to open VideoJs inside a Dialog (Modal)
anybody’s picture

Ok @grevil will finish and test this as soon as beta33 is out with the required library sources!
Watching github.com/videojs/v10/issues/2526

grevil’s picture

Unfortunately, we still can't continue here.....

see https://github.com/videojs/v10/issues/2526#issuecomment-5760545155

grevil’s picture

Assigned: grevil » thomas.frobieter

The dialog works now, but the presentation is off. @thomas.frobieter, maybe you can do something about that?

anybody’s picture

Status: Active » Needs work

Okay let's merge this as-is and @thomas.frobieter or someone else can care for the basic styling then in a fresh follow-up branch here, as soon as it's needed! Better to have it technically at all, doesn't have to be perfect yet. The module is in an alpha state and feedback is good.

grevil’s picture

@anybody agreed!

  • grevil committed f4932af0 on 10.x
    feat: #3616068 [10.x] Add a VideoJs Dialog (Modal) formatter with...
thomas.frobieter’s picture

I've just tested the implementation.

- In the template, we have a title and a description. Neither of these is configurable in the field formatter. There should be a select for both fields providing media entity fields (and maybe custom tokens, as we have for Photoswipe Dynamic Caption). Furthermore the current default value for title seems to be the media entity title, what is not ment to be public on most sites.
- The thumbnail image is wrapped in a button, which makes no sense
- The modal itself is way too small. Seems to be a CSS calc() issue on the library side:
--media-dialog-max-width: calc(var(--media-spacing) * 72);
- The modal overlay does not cover the entire viewport. This is because the module prints the media dialog component right after the image, while the library seems to require it within the document root. They use position: absolute instead of position: fixed. In my opinion, position:fixed is much more typical and offers more flexibility.
- Clicking on the backdrop of the modal does not close it, which is quite typical - at least as an option
- (The close button is way too large, but this is up to the library and can easily be fixed at theme level)

We can easily fix most of the current flaws in the library in the module so that the feature is useful. We should create issues for Video.js on GitHub to fix them there.

thomas.frobieter’s picture

Status: Needs work » Needs review

Done, please review: https://git.drupalcode.org/issue/videojs-3616068/-/compare/10.x...3616068-10.x-videojs-dialog-fixes

It has been tested and works perfectly.

The thumbnail image is wrapped in a button, which makes no sense

I have to backtrack on this point—as weird as it is, it's semantically OK. I've added a CSS reset for the button style.

Some unrelated fixes have been made to the background SDC.

I'll create the library issues on Github later.

anybody’s picture

Assigned: thomas.frobieter » grevil

grevil changed the visibility of the branch 3616068-10.x-videojs-dialog to hidden.

grevil changed the visibility of the branch 3616068-10.x-dialog-htmx-approach to hidden.

grevil changed the visibility of the branch 3616068-10.x-add-a to hidden.

grevil’s picture

Assigned: grevil » thomas.frobieter
Status: Needs review » Needs work

Alright, that should be it now. @thomasfrobieter might quickly introduce a topbar fix for the modal, otherwise this is good to go if the tests suceed.

grevil’s picture

Assigned: thomas.frobieter » Unassigned
Status: Needs work » Reviewed & tested by the community

Ok, just talked with @thomas.frobieter, the bug only appears for olivero. Not for any other themes, so good to go!

  • grevil committed 4846d335 on 10.x
    Follow-up to [#3616068] [10.x] Video.js dialog fixes
    
grevil’s picture

Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.