fix(player): don't fire layers during hover Start video preview on hover - #2108
fix(player): don't fire layers during hover Start video preview on hover#2108KMchaudhary wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate issues can leave layers suppressed during real playback or visible during previews.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Suppresses interactive layers during hover-based video previews.
Changes:
- Tracks hover-preview state.
- Adds centralized layer-suppression checks.
- Connects preview state to layer handling.
File summaries
| File | Review |
|---|---|
assets/src/js/godam-player/videoPlayer.js |
Connects hover state to layer suppression. |
assets/src/js/godam-player/managers/layersManager.js |
Adds suppression guards, but API-created custom layers bypass them. |
assets/src/js/godam-player/managers/hoverManager.js |
Tracks preview state, but control-based playback can incorrectly reactivate preview mode. |
Review details
Suppressed comments (1)
assets/src/js/godam-player/managers/hoverManager.js:181
stopPreview()never clears this flag, so after the pointer leaves,isPreviewActive()continues returning true until some laterplayevent occurs outside the video. This makes suppression outlive the preview and drops unrelated layer processing in the intervening paused state (for example, a seek-generatedtimeupdate). Keep the flag set through the preview-generatedpausedispatch, but clear it once that dispatch has completed.
isPreviewActive() {
return this.isPreviewPlaying;
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| this.player.on( 'play', () => { | ||
| if ( ! this.isHovered ) { | ||
| this.isPreviewPlaying = false; | ||
| } | ||
| } ); |
| handleCustomLayersTimeUpdate( currentTime ) { | ||
| if ( this.areLayersSuppressed() ) { | ||
| return; | ||
| } |
🔍 WordPress Plugin Check Report
📊 Report
|
| 📍 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/09/03/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/09/03/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
ReviewSolid, well-scoped fix, and the reasoning in the JSDoc is genuinely helpful. A few things I checked and liked:
Two related observations (same root cause) and one test suggestion. 1. Fast hover-flick can briefly lift suppression before the preview's own The new
I confirmed 2. Playback committed while the pointer stays on the video leaves layers suppressed (low) The mirror case. Both cases come from inferring "committed to real playback" from 3. Consider a small unit test for the flag lifecycle (low) The new logic is pure and easy to pin down: Nit / future-proofing: custom add-on layer managers are constructed without any handle to the suppression state, so gating holds only as long as add-ons reveal exclusively through |
Fixes: #1847
This pull request introduces a mechanism to suppress interactive video layers (such as forms, CTAs, and hotspots) during uncommitted hover previews, ensuring that previews act as pure playback teasers without distractions. The suppression is coordinated between the
HoverManager,LayersManager, andGodamVideoPlayerclasses. The most important changes are grouped below:Hover preview state tracking:
isPreviewPlayingflag and anisPreviewActive()method toHoverManagerto track when a hover preview is running and expose this state. [1] [2] [3]isPreviewPlayingis set appropriately on preview start, on play events, and when the video is clicked to transition from preview to real playback. [1] [2]Layer suppression logic:
LayersManager, including asuppressionCheckpredicate,setSuppressionCheck(), andareLayersSuppressed()methods. These prevent layer firing (including time-based and event-driven triggers) while suppression is active. [1] [2] [3] [4] [5]Integration with player:
GodamVideoPlayerso that all layers are suppressed while a hover preview is running.Demo
Screen.Recording.2026-09-03.at.3.43.56.PM.mov