GoDAM - VideoJet Support - #2051
Conversation
…pup and godam/video block
…r organization is disabled
Add a 'Play on modal' toggle (default false) to the godam/video block, [godam_video] shortcode, Elementor widget, and WPBakery element. When enabled, clicking the inline player opens it in a lightbox modal (inspired by the shoppable video modal) and plays it there.
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds “Play on modal” lightbox playback to the GoDAM video player across block/shortcode/page-builders, introduces extensible guide messages in thumbnail panels, and refines media-library transcoding UI gating for better plugin compatibility.
Changes:
- Added a
playOnModalattribute/control and wired it into frontend player initialization with a new modal manager + styles. - Added optional (sanitized) guide-message support for thumbnail panels in both the block editor and media-library attachment details.
- Updated media-library UI gating so transcoding status/bulk action remain available on
upload.phpeven in additive mode.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| inc/templates/godam-player.php | Emits modal-related class + data-play-on-modal attribute for frontend behavior. |
| inc/classes/wpbakery-elements/class-wpb-godam-video.php | Adds WPBakery control for play-on-modal. |
| inc/classes/shortcodes/class-godam-player.php | Adds shortcode attributes + boolean parsing/mapping for play-on-modal. |
| inc/classes/elementor-widgets/class-godam-video.php | Adds Elementor switcher and passes play-on-modal into render attributes. |
| inc/classes/class-assets.php | Localizes new guide-message strings via filters for editor and media library. |
| assets/src/js/media-library/views/attachment-detail-two-column.js | Renders optional sanitized guide message in attachment details “Video Thumbnails” panel. |
| assets/src/js/media-library/views/attachment-browser.js | Adjusts additive-mode gating to keep retranscode bulk action available on upload.php. |
| assets/src/js/media-library/utility.js | Introduces isMediaTranscodingUIEnabled() helper and exports it. |
| assets/src/js/media-library/index.js | Uses isMediaTranscodingUIEnabled() to decide when to override media-library views. |
| assets/src/js/godam-player/managers/playerManager.js | Registers play-on-modal videos with the new modal manager. |
| assets/src/js/godam-player/managers/modalManager.js | New: implements shared lightbox modal that moves/restores the inline player. |
| assets/src/css/media-library.scss | Styles the new media-library thumbnail guide message. |
| assets/src/css/godam-player.scss | Adds modal overlay/wrapper/close styles and play-on-modal cursor affordance. |
| assets/src/blocks/godam-player/editor.scss | Styles the block-editor thumbnail guide message. |
| assets/src/blocks/godam-player/edit-common-settings.js | Adds play-on-modal toggle control to block inspector playback settings. |
| assets/src/blocks/godam-player/components/ThumbnailPanel.js | Renders optional sanitized guide message in block thumbnail panel. |
| assets/src/blocks/godam-player/block.json | Adds the new playOnModal block attribute. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| register( video ) { | ||
| const figure = video.closest( 'figure' ); | ||
| // Move the outer wrapper (parent of <figure>) because it carries the | ||
| // aspect-ratio / brand-color CSS custom properties. | ||
| const playerRoot = figure?.parentElement || video.closest( '.godam-video-wrapper' ); | ||
| if ( ! playerRoot || playerRoot.dataset.godamModalBound === '1' ) { | ||
| return; | ||
| } | ||
| playerRoot.dataset.godamModalBound = '1'; | ||
|
|
||
| // Intercept clicks in the capture phase — before Video.js handles them — | ||
| // so an inline click opens the modal instead of playing inline. | ||
| playerRoot.addEventListener( | ||
| 'click', | ||
| ( event ) => { | ||
| // Once the player is live inside the modal, let clicks through to | ||
| // the Video.js controls. | ||
| if ( this.activeEntry && this.activeEntry.video === video ) { | ||
| return; | ||
| } | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| this.open( video, playerRoot ); | ||
| }, | ||
| true, | ||
| ); | ||
| } |
| // Leave an anchor so the player returns to its exact inline position. | ||
| const anchor = document.createComment( 'godam-modal-anchor' ); | ||
| playerRoot.parentNode.insertBefore( anchor, playerRoot ); | ||
| modal.content.appendChild( playerRoot ); |
| &:hover, | ||
| &:focus-visible { | ||
| background: rgba(255, 255, 255, 0.2); | ||
| outline: none; | ||
| } |
| width: min(1000px, 92vw); | ||
| max-height: 90dvh; | ||
| opacity: 0; |
| * @since n.e.x.t | ||
| * | ||
| * @param string $message Guide message HTML. Default empty string. |
There was a problem hiding this comment.
Disregard this one: @since n.e.x.t is the intended state during development in this repo. It is a WordPress-standard placeholder that gets resolved to the real version at release time (step 7 of the release checklist), which is exactly what stops a wrong version number from leaking into the docs. Please leave both occurrences as n.e.x.t.
subodhr258
left a comment
subodhr258
left a comment
There was a problem hiding this comment.
Review
Nice, well-commented PR. The "Play on modal" approach (move the already-initialised inline player into a shared lightbox, move it back on close) is a good call: it avoids a second player instance and keeps analytics/layers/engagement on one object. The additive-mode carve-out for transcoding UI is correctly scoped, and the guide-message filters are cleanly documented.
Two issues I would want resolved before merge, plus some accessibility and scoping points.
Verification I ran
Run PHPCS inspectionpasses.Run JS unit testsandRun Plugin Checkare red, but both die atnpm ciwith lockfile drift, which is the known repo-wide breakage ondevelop, not this PR.- ESLint and Stylelint are clean on every file this PR touches (the handful of errors I hit locally all sit on pre-existing lines).
DOMPurifyis already imported inattachment-detail-two-column.js,isomorphic-dompurifymatches the repo convention, andvideojs.getPlayer( element )matches existing usage invideoPlayer.js/analytics.js/playerManager.js. All good.- I checked the additive-mode claim in the
isMediaTranscodingUIEnabled()comment:attachment.js:39does gate drag-to-folder onisFolderOrgDisabled(), so opening upwp.media.view.Attachmentsonupload.phpdoes not leak folder behaviour. Claim holds.
1. Modal overflows the viewport for anything taller than ~4:3 (blocking)
.godam-player-modal-item forces max-width: 100% !important, which discards the height-derived max-width the template computes for Player Height. The player's height then comes only from aspect-ratio on .easydam-player.video-js. max-height: 90dvh on .godam-player-modal-wrapper does nothing here, because the wrapper's overflow is visible and the child is never height-constrained, so the video just overflows past it.
Measured in Chrome at 1470x836, using the PR's CSS verbatim:
| Aspect ratio | Rendered in modal | Overflows viewport by |
|---|---|---|
| 16:9 | 1000 x 563 | 0 |
| 4:3 | 1000 x 750 | 0 |
| 1:1 | 1000 x 1000 | 164px |
| 9:16 | 1000 x 1778 | 942px |
At 9:16 the control bar lands at y ≈ 1780, entirely below the fold, so the video is unplayable and uncloseable except via Escape. Vertical/reel video is a first-class GoDAM use case, so this will get hit.
The fix is to let the height cap drive the width instead of pinning width to 100%:
.godam-player-modal-item {
width: auto !important;
max-width: 100% !important;
max-height: 90dvh;
margin: 0 auto !important;
aspect-ratio: var(--rtgodam-video-aspect-ratio, 16 / 9);
}The custom property is already on the moved root, so aspect-ratio plus max-height plus width: auto will size the box off the height cap for tall videos and off the wrapper width for wide ones. Worth a quick check at 9:16, 1:1 and 16:9 on both a tall and a short viewport.
2. Share modal renders behind the player modal (blocking, if global share is on)
.share-modal-container is z-index: 9999 (godam-player.scss:663) and shareManager.getModalParent() appends it to document.body outside fullscreen. The new overlay is z-index: 100000 and the wrapper 100001, also on document.body, so they compare directly in the root stacking context.
Once the player is live inside the lightbox, clicks pass through to the Video.js controls, so the share button is reachable. Pressing it puts the share UI underneath the lightbox backdrop: the user sees the dark overlay and nothing else, with body scroll locked by both. Reproducible whenever enableGlobalVideoShare is on and the video has the share button.
Either raise .share-modal-container above the lightbox, or have getModalParent() return the modal wrapper when the player is inside it.
3. Close button is unreachable for screen readers
wrapper gets role="dialog" and aria-modal="true", but closeBtn is appended to document.body as a sibling of the wrapper, not inside it (modalManager.js:92). aria-modal="true" hides everything outside the dialog from the accessibility tree, so AT users have no Close control at all; Escape is the only way out.
Move the button inside wrapper and position it with CSS. While you are there, the dialog has no accessible name: aria-label={ __( 'Video player', 'godam' ) } or aria-labelledby pointing at the video title would fix it.
4. No focus trap
Focus is moved into the wrapper on open and restored on close, which is the right shape, but Tab still walks straight out of the dialog into the page behind it. Given aria-modal="true" is already claiming the dialog is modal, a Tab/Shift+Tab cycle in handleKeydown would make that true.
Worth discussing
Overlay inner blocks get swallowed. The capture-phase preventDefault() / stopPropagation() is bound to the whole player root, which contains .godam-video-overlay-container and its author-placed inner blocks. With Play on modal enabled, a "Buy now" button or link in the video overlay silently stops working and opens the lightbox instead. If that is intended, it is worth saying so in the toggle's help text. If not, skip the intercept when event.target.closest( '.godam-video-overlay-container' ) matches.
Autoplay + Play on modal is unresolved. With both on, the video autoplays inline (muted) and a click then re-plays it in the lightbox. The template already resolves the analogous conflict one line up (if ( $godam_autoplay ) { $godam_hover_select = 'none'; }), so there is a precedent for picking a winner here rather than leaving both active.
No tests. modalManager.js is 184 lines of DOM move-and-restore with an obvious failure mode: the player not landing back in its original position. The repo already runs JS unit tests next door (assets/src/js/godam-player/analytics-helpers.test.js, utils/layerActions.test.js), so an open then close then "DOM is byte-identical to before" assertion in jsdom would be cheap insurance.
Missing E2E hooks. The toggle correctly got data-test-id="godam-video-control-play-on-modal", but the modal DOM it drives (overlay, wrapper, close button) has none, so there is nothing stable to assert against.
iOS QA. Re-parenting a live <video> can reset playback state in Safari/iOS. The open path orders the move before play() and the close path pauses before moving, which is the right order, but this is worth an explicit check on a real device.
Nits
isMediaTranscodingUIEnabled()also gateswp.media.view.Attachments(column-width logic and the customAttachmentView), which is wider than "transcoding UI". The behaviour is right, the name just undersells the scope.rtgodam_video_block_video_thumbnails_guide_messageandrtgodam_media_library_video_thumbnails_guide_messageare near-identical. One filter with a$contextargument would be easier to document, though two is defensible if you expect the copy to diverge.- The guide message's only sanitisation is client-side. A
wp_kses_post()on the filter output would mean the trust boundary does not rest entirely on JS. Low risk since only code can set it, but it is one line. - The two
stylelint-disable-next-linecomments are reasonable, butno-duplicate-selectorsfiring on.godam-video-wrapper(three separate blocks now, at 1747, 2324 and 2515) reads more like the file wanting a consolidation pass than a rule wanting a silence.
| /* The moved player root carries its own inline max-width (from Player Height); | ||
| * neutralise it so the player fills the modal width instead. The class is added | ||
| * to the player root by modalManager.js while it lives inside the modal. */ | ||
| .godam-player-modal-item { |
There was a problem hiding this comment.
Blocking: max-width: 100% !important here discards the height-derived max-width the template computes, so the player's height comes only from aspect-ratio on .easydam-player.video-js. The max-height: 90dvh on the wrapper cannot rein it in, because the wrapper is overflow: visible and the child is never height-constrained.
Measured in Chrome at 1470x836 with this CSS verbatim: 16:9 renders 1000x563 (fine), 4:3 renders 1000x750 (fine), 1:1 renders 1000x1000 and overflows the viewport by 164px, 9:16 renders 1000x1778 and overflows by 942px with the control bar at y ~1780, entirely below the fold.
Letting the height cap drive the width fixes it:
.godam-player-modal-item {
width: auto !important;
max-width: 100% !important;
max-height: 90dvh;
margin: 0 auto !important;
aspect-ratio: var(--rtgodam-video-aspect-ratio, 16 / 9);
}The custom property is already on the moved root, so tall videos size off max-height and wide ones off the wrapper width.
| .godam-player-modal-overlay { | ||
| position: fixed; | ||
| inset: 0; | ||
| z-index: 100000; |
There was a problem hiding this comment.
Blocking (when global share is on): .share-modal-container is z-index: 9999 (line 663) and shareManager.getModalParent() appends it to document.body outside fullscreen, same as this overlay. At 100000 the lightbox backdrop sits on top of it.
Once the player is live in the modal, clicks pass through to the Video.js controls, so the share button is reachable. Pressing it shows the user a dark overlay and nothing else, with body scroll locked twice over.
Either raise .share-modal-container above the lightbox, or have getModalParent() return the modal wrapper when the player is inside it.
| wrapper.appendChild( content ); | ||
|
|
||
| document.body.appendChild( overlay ); | ||
| document.body.appendChild( closeBtn ); |
There was a problem hiding this comment.
The close button is appended to document.body as a sibling of wrapper, but wrapper carries aria-modal="true" (line 84). That attribute hides everything outside the dialog from the accessibility tree, so screen reader users have no Close control at all and Escape becomes the only exit.
Moving closeBtn inside wrapper and positioning it with CSS keeps the same visual result and puts it back in the a11y tree. The dialog is also unnamed, so aria-label={ __( 'Video player', 'godam' ) } (or aria-labelledby pointing at the video title) would be worth adding on the same line.
| if ( this.activeEntry && this.activeEntry.video === video ) { | ||
| return; | ||
| } | ||
| event.preventDefault(); |
There was a problem hiding this comment.
This intercepts every click anywhere in the player root, which includes .godam-video-overlay-container and its author-placed inner blocks. With Play on modal enabled, a link or button an author dropped into the video overlay silently stops working and opens the lightbox instead.
If that is the intended behaviour it is worth stating in the toggle's help text. If not, an early bail on event.target.closest( '.godam-video-overlay-container' ) would keep the overlay interactive.
| * | ||
| * @param {KeyboardEvent} event - Keydown event. | ||
| */ | ||
| handleKeydown( event ) { |
There was a problem hiding this comment.
Escape closes and focus is restored on close, which is the right shape, but Tab still walks straight out of the dialog into the page behind it. Since aria-modal="true" is already asserting the dialog is modal, a Tab / Shift+Tab cycle here would make that assertion true.
This pull request introduces a new "Play on modal" feature for the GoDAM video player, allowing videos to open in a lightbox modal when clicked, and improves the media library's transcoding UI logic for better compatibility with other plugins. It also adds guide message support to the thumbnail panel and refines related UI styling.
GoDAM Player: "Play on modal" feature
playOnModalblock attribute and toggle control in the editor, enabling videos to open in a centered modal/lightbox overlay when clicked (block.json,edit-common-settings.js,godam-player.scss,playerManager.js,modalManager.js). [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]UI Improvements: Thumbnail panel guide message
Media Library: Transcoding UI logic
upload.php), even when folder organization is disabled (additive mode), while keeping other custom UI minimal for compatibility with other DAM plugins. [1] [2] [3] [4] [5] [6]Styling and Accessibility
Other minor improvements
1. Custom thumbnail guide message
2. Re-add the support transcoding status and re-transcode media action
Screen.Recording.2026-07-29.at.7.31.23.PM.mov
3. Add Lightbox support for GoDAM Video block, shortcode, as well as elementor and wpbakery blocks
Screen.Recording.2026-07-29.at.5.33.39.PM.mp4