Skip to content

Video editor: add progress stripe, front-end controls preview, fix missing skip buttons - #2070

Open
KMchaudhary wants to merge 1 commit into
developfrom
fix/ve-player-preview
Open

Video editor: add progress stripe, front-end controls preview, fix missing skip buttons#2070
KMchaudhary wants to merge 1 commit into
developfrom
fix/ve-player-preview

Conversation

@KMchaudhary

Copy link
Copy Markdown
Collaborator

Fixes: #2069

This pull request makes significant improvements to the video player editor, focusing on accurately previewing the front-end control bar, synchronizing player controls with saved settings, and cleaning up legacy or redundant code. The changes ensure that the editor's player preview matches the real front-end experience, especially regarding skip buttons, volume panel, branding, and appearance settings.

Player control bar synchronization and preview improvements:

  • The editor now builds the control bar preview (previewControlBar) from saved settings, ensuring skip buttons and other controls appear as they will on the front end. This replaces the previous fixed control list and addresses cases where skip buttons were missing for older videos. [1] [2] [3]
  • The VideoJSPlayer component synchronizes control bar elements (volume panel, skip buttons, branding icon, play button position, and vertical layout) with the latest settings using dedicated helper functions. This ensures the preview always reflects the current configuration. [1] [2]
  • Appearance settings like colors and zoom level are now applied via CSS variables, matching the front-end stylesheet for a consistent look.

Code cleanup and maintainability:

  • Legacy and redundant code for manually toggling control visibility, color, and button placement has been removed or replaced with more robust and maintainable logic.
  • The SCSS file removes the rule that always hid the native control bar, delegating control visibility to the new logic and styles.

UI/UX consistency:

  • The custom play button image logic is now isolated and only runs when a custom image is actually set, preventing unnecessary DOM manipulation and ensuring the play button behaves as expected.
  • The player preview and progress stripe are shown or hidden based on the current editor tab and layer selection, providing a clearer and more accurate editing experience. [1] [2]

These changes collectively provide a more accurate, maintainable, and user-friendly video editing interface.

Demo

Screen.Recording.2026-08-10.at.9.04.42.AM.mov

Copilot AI balanced review requested due to automatic review settings August 10, 2026 03:37
@github-actions

Copy link
Copy Markdown

🔍 WordPress Plugin Check Report

⚠️ Status: Passed with warnings

📊 Report

🎯 Total Issues ❌ Errors ⚠️ Warnings
16 0 16

⚠️ Warnings (16)

📁 composer.json (1 warning)
📍 Line 🔖 Check 💬 Message
0 missing_composer_json_file The "/vendor" directory using composer exists, but "composer.json" file is missing.
📁 readme.txt (2 warnings)
📍 Line 🔖 Check 💬 Message
0 mismatched_plugin_name Plugin name "GoDAM - Organize WordPress Media Library & File Manager with Unlimited Folders for Images, Videos & more" is different from the name declared in plugin header "GoDAM".
0 trademarked_term The plugin name includes a restricted term. Your chosen plugin name - "GoDAM - Organize WordPress Media Library & File Manager with Unlimited Folders for Images, Videos & more" - contains the restricted term "wordpress" which cannot be used at all in your plugin name.
📁 assets/build/blocks/godam-gallery-v2/render.php (2 warnings)
📍 Line 🔖 Check 💬 Message
15 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$inner_block_video_ids".
23 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$inner_block_video_ids".
📁 assets/build/css/main.css (1 warning)
📍 Line 🔖 Check 💬 Message
0 EnqueuedStylesScope This style is being loaded in all contexts.
📁 assets/src/libs/analytics.min.js (5 warnings)
📍 Line 🔖 Check 💬 Message
0 EnqueuedScriptsScope This script is being loaded in all frontend contexts.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880 (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/2026/08/10/hello-world/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/sample-page/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-attachment-post/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
📁 assets/build/js/main.min.js (5 warnings)
📍 Line 🔖 Check 💬 Message
0 EnqueuedScriptsScope This script is being loaded in all frontend contexts.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880 (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/2026/08/10/hello-world/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/sample-page/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-attachment-post/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.

🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

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.

Improves the video editor’s stage preview so it more accurately reflects the front-end player (control bar composition, styling via CSS variables, and missing skip buttons), while introducing a minimal progress affordance when the control bar is hidden.

Changes:

  • Added a reel-style progress stripe component and styles for stage playback feedback when controls are hidden.
  • Reworked control bar preview/composition and synchronized it with saved settings (volume panel, skip buttons, branding, play button position, captions visibility).
  • Added defaults/fallbacks for skip buttons (Redux initial state and PHP template) and consolidated control-bar visibility styling into a dedicated SCSS module.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pages/video-editor/video-editor.scss Loads the new _player-controls.scss styles.
pages/video-editor/style.scss Removes local control-bar visibility rule; defers to _player-controls.scss.
pages/video-editor/redux/slice/videoSlice.js Adds default skipButtons values to match UI-default behavior.
pages/video-editor/components/player/PlayerProgressStripe.js Introduces the progress stripe component driven by Video.js playback state.
pages/video-editor/components/appearance/Appearance.js Removes direct DOM control-bar manipulation; relies on state + VideoJSPlayer sync.
pages/video-editor/_player-controls.scss Centralizes stage control-bar visibility + progress stripe styling.
pages/video-editor/_editor-shell.scss Removes legacy rule that always hid the control bar; points to _player-controls.scss.
pages/video-editor/VideoJSPlayer.js Adds helpers to sync the control bar with saved settings; adds stripe + CSS variables for appearance.
pages/video-editor/VideoEditor.js Seeds stage control bar preview from saved settings instead of a fixed list.
inc/templates/godam-player.php Restores default skip buttons for legacy videos missing stored skip duration.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +224 to 237
// Vertical control bar (legacy option, not exposed in the Settings tab).
if ( 'vertical' === controlBarSettings.controlBarPosition ) {
controlBar.addClass( 'vjs-control-bar-vertical' );
for ( const control of controlBar.el().querySelectorAll( '.vjs-control' ) ) {
control.classList.add( 'vjs-control-vertical' );
if ( control.classList.contains( 'vjs-volume-panel' ) ) {
control.classList.add( 'vjs-volume-panel-vertical' );
control.classList.remove( 'vjs-volume-panel-horizontal' );
}

if ( control.classList.contains( 'vjs-volume-horizontal' ) ) {
control.classList.add( 'vjs-volume-vertical' );
}
}
}
Comment on lines +80 to +93
const read = () => {
if ( player.isDisposed() ) {
return;
}
const total = player.duration();
setDuration( Number.isFinite( total ) && total > 0 ? total : 0 );
setCurrentTime( player.currentTime() || 0 );
setBufferedEnd( getBufferedEnd( player ) );
};

const tick = () => {
read();
rafRef.current = requestAnimationFrame( tick );
};
Comment on lines +104 to +118
&__range {
position: absolute;
top: -15px;
left: 0;
width: 100%;
height: 18px;
min-height: 0;
margin: 0;
padding: 0;
cursor: pointer;
background: transparent;
border: 0;
opacity: 0;
appearance: none;
}
Comment on lines +516 to +521
if ( empty( $godam_control_bar_settings['skipButtons']['forward'] ) ) {
$godam_video_setup['controlBar']['skipButtons'] = array(
'forward' => 10,
'backward' => 10,
);
}
@subodhr258

Copy link
Copy Markdown
Collaborator

Code review

Reviewed the full diff against a checkout of the branch, plus the surrounding front-end player code so I could check the "preview matches the front end" claims directly.

Overall: this is a solid, well-reasoned refactor and the core approach is right. Building the preview control bar from saved settings, driving appearance through the shared --rtgodam-* CSS variables, and replacing the old imperative DOM poking with videojs component APIs are all improvements. I confirmed the appearance parity actually works: App.js imports the front-end stylesheet (assets/src/css/godam-player.scss), and its .vjs-control-bar / .vjs-control:hover rules are global and !important, so the variables set in VideoJSPlayer genuinely restyle the preview. The React lifecycle also holds up: the rAF loop in PlayerProgressStripe starts/stops correctly, unbinds guarded by isDisposed(), and the single-init guard prevents double player creation.

Two things I'd fix before merge, plus a few nits.

Worth addressing before merge

1. Custom brand logo renders unconstrained in the preview (parity bug).
syncBrandingIcon appends a bare <img id="branding-icon"> directly to the control-bar element (VideoJSPlayer.js:129-133). On the front end, setupBrandingButton builds it as a real control-bar child whose element carries vjs-custom-play-button, with the image as <img id="branding-icon" class="branding-icon"> inside it (assets/src/js/godam-player/managers/controlsManager.js:397-414). The front-end sizing rule is scoped to that wrapper:

.vjs-control.vjs-custom-play-button #branding-icon { height: 100%; width: auto; object-fit: fill; }

The preview image is not a descendant of .vjs-custom-play-button and never gets the branding-icon class, so that rule cannot match it. The only editor rule for it is #branding-icon { order: 9 } (pages/video-editor/style.scss:166), which is position only. A 512x512 custom logo therefore renders at its intrinsic size in the preview while the front end clamps it to the control-bar height. Since custom branding is exactly one of the things this preview is meant to show faithfully, this is the one control guaranteed to look wrong.
Fix: build the branding as a real control-bar child mirroring setupBrandingButton (also fixes the order: 9 vs front-end order: 8 difference), or at minimum add the vjs-custom-play-button wrapper + branding-icon class, or add a preview-scoped rule that sizes a bare #branding-icon to the bar height.

2. Keyboard focus on the progress-stripe slider is invisible (WCAG 2.4.7).
The <input type="range"> is opacity: 0 with appearance: none; border: 0, and the visible handle only becomes opaque under &:hover, &.is-scrubbing (pages/video-editor/_player-controls.scss). is-scrubbing is set only on onPointerDown, and there is no :focus / :focus-visible / :focus-within rule. A keyboard user who tabs onto the slider (which is arrow-key operable) gets no handle and no focus ring, and opacity: 0 on the element also suppresses the UA outline. Fix:

.godam-ve-stripe:focus-within .godam-ve-stripe__handle { opacity: 1; }

plus a visible outline on .godam-ve-stripe__range:focus-visible.

Nits and polish

3. Captions button previews for videos with no text tracks. With subsCapsButton: true always seeded and the .is-captions-visible .vjs-subs-caps-button.vjs-hidden { display: flex !important } rule, turning "Display captions" on shows the CC button in the preview even when the video has no tracks, where the front end would not build it. The comment says this is deliberate so the toggle has a visible effect on not-yet-transcribed videos, which is reasonable, but it does make the preview claim slightly more than the front end delivers. Worth a quick designer confirm, or a line in the help text noting the button only appears on the front end once the video has captions.

4. forward: 0 preview vs front-end divergence (latent). The preview uses Number(...forward) || 0 then skipDuration ? {...} : false, so a stored forward of 0/'0' shows no skip buttons, while the PHP guard empty($...['skipButtons']['forward']) restores {forward:10, backward:10} and renders them. This is not reachable through the current Settings UI (only 5/10/30, and handleSkipDuration guards < 1), so it is latent, but the two guards reading the same stored value differently is a trap worth closing now while it is cheap. Pick one semantic for "0 means no buttons" and apply it on both sides.

5. New seek control has no data-test-id. Every other control in the editor carries one (e.g. godam-video-editor-control-volume), but the stripe's range input only has an aria-label. Per the team's E2E convention, a Playwright scrub test would have to fall back to the .godam-ve-stripe__range styling class. Suggest adding data-test-id="godam-video-editor-control-progress".

6. Progress slider announces raw seconds to screen readers. The range exposes value={currentTime} in seconds with no aria-valuetext, so AT announces "45" while sighted users get formatClock ("0:45") in the hover bubble. Add aria-valuetext={ formatClock( currentTime ) }.

7. No unit tests for the new pure logic. getBufferedEnd (max over TimeRanges, throw-safe) and the progress/buffer clamp math are pure and the repo already runs wp-scripts test-unit-js (see pages/video-editor/utils/index.test.js). Extracting those into utils/ with a few Jest cases (multi-range buffered, duration = 0 guard, clamp at 0 and 100) would guard against silent regressions.

Notes

  • CI is green except the POT check, which is expected during development.
  • Behavior change to call out in the release notes: legacy videos whose stored controlBar lacked a skipButtons key rendered no skip buttons on the front end and will now render 10s buttons (this is the intended Video Editor: show player controls on the video editor stage preview #2069 fix, and it restores parity with the Settings tab, which always displayed "10 seconds"). Flagging it only so it is a documented change rather than a surprise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants