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
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
Comment #2
anybodyI think these issues are somehow related.
Comment #3
_shyComment #4
grevil commentedInstead 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.
Comment #5
anybodyYes, 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.
Comment #6
anybodyComment #7
anybodyComment #8
anybody@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...
Comment #9
anybody@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
@todofor later refactoring to use the SDC.Once 2.1 is implemented, we should decide if 2.2 makes sense as follow-up.
Comment #10
anybodyComment #11
grevil commentedSee discuission here: https://git.drupalcode.org/project/drupal/-/merge_requests/16540#note_17...
For an example implementation.
Comment #12
grevil commentedI 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.
Comment #14
anybodyUpdate: 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.
Comment #15
grevil commentedApparently its now completed: https://github.com/videojs/v10/issues/2384.
Comment #17
grevil commentedOK 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.
Comment #18
grevil commentedComment #19
anybodyOk @grevil will finish and test this as soon as beta33 is out with the required library sources!
Watching github.com/videojs/v10/issues/2526
Comment #20
grevil commentedUnfortunately, we still can't continue here.....
see https://github.com/videojs/v10/issues/2526#issuecomment-5760545155
Comment #21
grevil commentedThe dialog works now, but the presentation is off. @thomas.frobieter, maybe you can do something about that?
Comment #22
anybodyOkay 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.
Comment #23
grevil commented@anybody agreed!
Comment #25
thomas.frobieterI'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.
Comment #26
thomas.frobieterDone, 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.
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.
Comment #27
anybodyComment #32
grevil commentedAlright, 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.
Comment #33
grevil commentedOk, just talked with @thomas.frobieter, the bug only appears for olivero. Not for any other themes, so good to go!
Comment #35
grevil commented