Skip to content

fix(player): don't fire layers during hover Start video preview on hover - #2108

Open
KMchaudhary wants to merge 1 commit into
developfrom
fix/video-preview-layers
Open

fix(player): don't fire layers during hover Start video preview on hover#2108
KMchaudhary wants to merge 1 commit into
developfrom
fix/video-preview-layers

Conversation

@KMchaudhary

Copy link
Copy Markdown
Collaborator

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, and GodamVideoPlayer classes. The most important changes are grouped below:

Hover preview state tracking:

  • Added an isPreviewPlaying flag and an isPreviewActive() method to HoverManager to track when a hover preview is running and expose this state. [1] [2] [3]
  • Ensured isPreviewPlaying is 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:

  • Added a suppression mechanism to LayersManager, including a suppressionCheck predicate, setSuppressionCheck(), and areLayersSuppressed() methods. These prevent layer firing (including time-based and event-driven triggers) while suppression is active. [1] [2] [3] [4] [5]

Integration with player:

  • Registered the suppression check in GodamVideoPlayer so that all layers are suppressed while a hover preview is running.

Demo

Screen.Recording.2026-09-03.at.3.43.56.PM.mov

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.

🟡 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 later play event occurs outside the video. This makes suppression outlive the preview and drops unrelated layer processing in the intervening paused state (for example, a seek-generated timeupdate). Keep the flag set through the preview-generated pause dispatch, 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.

Comment on lines +76 to +80
this.player.on( 'play', () => {
if ( ! this.isHovered ) {
this.isPreviewPlaying = false;
}
} );
Comment on lines 193 to +196
handleCustomLayersTimeUpdate( currentTime ) {
if ( this.areLayersSuppressed() ) {
return;
}
@github-actions

github-actions Bot commented Sep 3, 2026

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/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

@subodhr258

Copy link
Copy Markdown
Collaborator

Review

Solid, well-scoped fix, and the reasoning in the JSDoc is genuinely helpful. A few things I checked and liked:

  • Suppression is registered lazily (() => this.hoverManager?.isPreviewActive() === true), so manager construction order does not matter.
  • Every reveal path is gated through areLayersSuppressed(): the timeupdate triggers (timestamp, watch_depth), the on_pause and end_of_video event handlers, hotspots, and custom add-on layers.
  • No regressions in the sibling modes: autoplay bails out of init(), show-player-controls never sets the flag, and the block-editor preview (isPreviewEnabled) path is untouched. handlePlay() is not gated but is a no-op unless a layer is already visible, so that is fine.

Two related observations (same root cause) and one test suggestion.


1. Fast hover-flick can briefly lift suppression before the preview's own pause (medium)

The new play listener clears isPreviewPlaying whenever !this.isHovered. On a quick flick this re-opens the exact on_pause window the "do not clear in stopPreview()" comment was written to prevent:

  1. mouseenter runs startPreview(): sets isPreviewPlaying = true, then player.play() (the play event is queued).
  2. mouseleave runs immediately: stopPreview() calls player.pause() (queues pause) and sets isHovered = false, deliberately leaving isPreviewPlaying = true.
  3. The still-pending play task fires first (media events are FIFO, and play was queued before pause). It now sees !isHovered and sets isPreviewPlaying = false.
  4. The pause task then fires, finds areLayersSuppressed() === false, and handlePause() reveals the on_pause CTA over the preview the viewer just left.

I confirmed handlePause() has no time guard: after stopPreview() resets currentTime(0) the player is paused-not-ended, so an on_pause layer (show, !triggered, hidden) is revealed and marked triggered. It needs a video with an on_pause CTA plus the play dispatch landing after mouseleave (likely while the video is still buffering), so it is intermittent rather than guaranteed, but it defeats the stated invariant.

2. Playback committed while the pointer stays on the video leaves layers suppressed (low)

The mirror case. isPreviewPlaying is only cleared by a click on the <video> element (handleVideoClick) or by a play event when !isHovered. If real playback starts while the pointer is still over the video and no click reaches the element (pressing space to resume via the global keyboard handler in playerManager.js, or a programmatic player.play() through the exposed player.hoverManager), neither path runs and every layer stays suppressed for the whole session until the next mouseleave or video click. Lower impact, since the video is still muted with controls hidden in that state and it self-heals, but it is the same underlying issue.

Both cases come from inferring "committed to real playback" from isHovered or a click target. Deriving it from an explicit signal instead would close both windows: for example clear isPreviewPlaying only when the player is actually in real, unmuted playback (gate on isVideoClicked or on the unmute already done in handleVideoClick), rather than on the raw !isHovered condition.

3. Consider a small unit test for the flag lifecycle (low)

The new logic is pure and easy to pin down: areLayersSuppressed() uses a deliberate strict === true, and the fix depends on isPreviewPlaying being set in startPreview(), cleared in the right places, and specifically not cleared in stopPreview(). A short spec (there is precedent at assets/src/js/godam-player/utils/layerActions.test.js) would stop a future refactor from silently reintroducing layers-during-preview or the stuck-suppression case above.

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 handleTimeUpdate. The current WooCommerce manager is safe on that basis, but since the layer registry exists for third-party add-ons, passing a suppression accessor (or documenting the contract) would make the preview invariant enforceable rather than conventional.

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