Conversation
|
Feels dangerous enough to me that maybe it should target 6.4 instead? What do you think? |
| } | ||
|
|
||
| // Detector above the canvas so we can detect hover and clicks of playback controls | ||
| .bloom-videoMouseDetector { |
There was a problem hiding this comment.
I don't know what to call this. "videoInteractionDetector"? "videoMouseEventDetector"? If we decide not to preserve hover detection (see below) then we could call it "videoClickDetector"
| &.bloom-videoReplayIcon { | ||
| display: flex; | ||
| .bloom-videoContainer.playing { | ||
| .bloom-videoMouseDetector:hover { |
There was a problem hiding this comment.
Looks like we decided not to do hover behavior in BloomPlayer (e.g. BloomBooks/bloom-player@c31f0fc) but kept it in BloomDesktop. Do we still want this?
| .filter( | ||
| (_, x) => | ||
| !x.classList.contains("bloom-videoControlContainer"), | ||
| !x.classList.contains("bloom-videoControlContainer") && |
There was a problem hiding this comment.
Not sure what's going on here, we have a "Not sure about keeping this" comment above, but now there's no point in keeping a bloom-videoControlContainer without a bloom-videoMouseDetector, it won't work
|
Please retarget to master |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 handleVideoClick uses ev.currentTarget as HTMLVideoElement but it's now a div, breaking play/pause toggle
In Game Play mode, clicking anywhere on a video should toggle between play and pause. After this PR, handleVideoClick is attached to the mouseDetector div (line 37) instead of the videoElement. However, the handler at line 393 still casts ev.currentTarget as HTMLVideoElement, which is now a div element.
Root Cause and Impact
On line 393, const video = ev.currentTarget as HTMLVideoElement now gets the mouseDetector div, not the actual <video> element. On line 405, video.paused is checked — but a div element has no .paused property, so it evaluates to undefined (falsy). This means the else branch always executes, calling handlePauseClick(ev) regardless of whether the video is actually playing or paused.
Consequence: In Game Play mode (.drag-activity-play), clicking on a paused video will call handlePauseClick instead of handlePlayClick, so the video can never be resumed by clicking on it. The toggle behavior is completely broken — only pause works, never play.
Note that handlePlayClick and handlePauseClick themselves use ev.target (not ev.currentTarget) and navigate via .closest(".bloom-videoContainer"), so they would correctly find the video element. The bug is specifically in the dispatch logic of handleVideoClick.
(Refers to lines 392-409)
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
This change is