Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions src/BloomBrowserUI/bookEdit/css/editMode.less
Original file line number Diff line number Diff line change
Expand Up @@ -1642,21 +1642,34 @@ svg.bloom-videoControl {
}
}

// Detector above the canvas so we can detect hover and clicks of playback controls
.bloom-videoMouseDetector {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"

height: 100%;
width: 100%;
position: absolute;
top: 0;
z-index: @canvasElementCanvasZIndex + 1;
}

// Show pause and replay when playing and hovered.
.bloom-videoContainer.playing:hover {
.bloom-videoControlContainer {
&.bloom-videoPauseIcon,
&.bloom-videoReplayIcon {
display: flex;
.bloom-videoContainer.playing {
.bloom-videoMouseDetector:hover {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

.bloom-videoControlContainer {
&.bloom-videoPauseIcon,
&.bloom-videoReplayIcon {
display: flex;
}
}
}
}

// Show play, centered, when hovering a video that is not paused or playing
.bloom-videoContainer:not(.paused):not(.playing):hover {
.bloom-videoPlayIcon {
display: flex;
left: calc(50% - @videoIconSize / 2);
.bloom-videoContainer:not(.paused):not(.playing) {
.bloom-videoMouseDetector:hover {
.bloom-videoPlayIcon {
display: flex;
left: calc(50% - @videoIconSize / 2);
}
}
}

Expand All @@ -1674,6 +1687,13 @@ svg.bloom-videoControl {
}
}

// TODO copilot did this and it works but I'm not sure it's the right fix. Let's see if HandlePlayClick gets fixed in BL-15936
// For draggable videos in Play Mode, disable pointer events on the mouse detector
// so clicks can pass through to the drag system which handles playback
.drag-activity-play [data-draggable-id] .bloom-videoMouseDetector {
pointer-events: none;
}
Comment on lines +1690 to +1695

Choose a reason for hiding this comment

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

🚩 TODO comment left in production code suggesting uncertainty about approach

Lines 1690-1694 contain a TODO comment: // TODO copilot did this and it works but I'm not sure it's the right fix. along with a CSS rule that disables pointer events on the mouse detector for draggable videos in Play Mode. The comment suggests this rule was generated by Copilot and the author isn't confident it's correct. This may need a follow-up investigation per BL-15936 to ensure draggable video playback works properly without this workaround.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


// If the background image hasn't been set in a game, we don't want to show the placeholder image.
.bloom-page[data-tool-id="game"]:not([data-activity="simple-dom-choice"]) {
.bloom-canvas
Expand Down
3 changes: 2 additions & 1 deletion src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6535,7 +6535,8 @@ export class CanvasElementManager {
.find(".bloom-ui")
.filter(
(_, x) =>
!x.classList.contains("bloom-videoControlContainer"),
!x.classList.contains("bloom-videoControlContainer") &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

!x.classList.contains("bloom-videoMouseDetector"),
)
.remove();
thisCanvasElement.find(".bloom-dragHandleTOP").remove(); // BL-7903 remove any left over drag handles (this was the class used in 4.7 alpha)
Expand Down
26 changes: 17 additions & 9 deletions src/BloomBrowserUI/bookEdit/js/bloomVideo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,14 @@ export function SetupVideoEditing(container) {
// debugging, and just might prevent a problem in normal operation.
videoElement.parentElement?.classList.remove("playing");
videoElement.parentElement?.classList.remove("paused");
videoElement.addEventListener("click", handleVideoClick);
const mouseDetector =
videoElement.ownerDocument.createElement("div");
mouseDetector.classList.add("bloom-videoMouseDetector");
mouseDetector.classList.add("bloom-ui"); // don't save as part of document
mouseDetector.addEventListener("click", handleVideoClick);
videoElement.parentElement?.appendChild(mouseDetector);
const playButton = wrapVideoIcon(
videoElement,
mouseDetector,
// Alternatively, we could import the Material UI icon, make this file a TSX, and use
// ReactDom.render to render the icon into the div. But just creating the SVG
// ourselves (as these methods do) seems more natural to me. We would not be using
Expand All @@ -44,13 +49,13 @@ export function SetupVideoEditing(container) {
);
playButton.addEventListener("click", handlePlayClick);
const pauseButton = wrapVideoIcon(
videoElement,
mouseDetector,
getPauseIcon("#ffffff", videoElement),
"bloom-videoPauseIcon",
);
pauseButton.addEventListener("click", handlePauseClick);
const replayButton = wrapVideoIcon(
videoElement,
mouseDetector,
getReplayIcon("#ffffff", videoElement),
"bloom-videoReplayIcon",
);
Expand Down Expand Up @@ -302,17 +307,17 @@ export function updateVideoInContainer(container: Element, url: string): void {
// configure one of the icons we display over videos. We put a div around it and apply
// various classes and append it to the parent of the video.
function wrapVideoIcon(
videoElement: HTMLVideoElement,
parent: HTMLElement,
icon: HTMLElement,
iconClass: string,
): HTMLElement {
const wrapper = videoElement.ownerDocument.createElement("div");
const wrapper = parent.ownerDocument.createElement("div");
wrapper.classList.add("bloom-videoControlContainer");
wrapper.classList.add("bloom-ui"); // don't save as part of document
wrapper.appendChild(icon);
wrapper.classList.add(iconClass);
icon.classList.add("bloom-videoControl");
videoElement.parentElement?.appendChild(wrapper);
parent.appendChild(wrapper);
return icon;
}

Expand All @@ -336,6 +341,7 @@ export function handlePlayClick(ev: MouseEvent, forcePlay?: boolean) {
// becuse the click might be a drag on the canvas element. We'll let CanvasElementManager
// decide and call playVideo if appropriate. That is, if we're not in Play mode,
// where dragging is not applicable, or being called FROM the CanvasElementManager.
// TODO The above comment is out of date; there
if (
!forcePlay &&
video.closest(kCanvasElementSelector) &&
Expand Down Expand Up @@ -391,12 +397,14 @@ function handlePauseClick(ev: MouseEvent) {
// be a natural bit of code to put in dragActivityRuntime.ts, except we don't need
// it there, because BloomPlayer has this behavior for all videos, not just in Games.)
const handleVideoClick = (ev: MouseEvent) => {
const video = ev.currentTarget as HTMLVideoElement;
const video = (ev.currentTarget as HTMLElement)
?.closest(".bloom-videoContainer")
?.getElementsByTagName("video")[0];

// If we're not in Play mode, we don't need these behaviors.
// At least I don't think so. Outside Play mode, clicking on canvas elements is mainly about moving
// them, but we have a visible Play button in case you want to play one. In BP (and Play mode), you
// can't move them (unless one day we make them something you can drag to a target), so it
// can't move them if they aren't draggable, so it
// makes sense that a click anywhere on the video would play it; there's nothing else useful
// to do in response.
if (!video.closest(".drag-activity-play")) {

Choose a reason for hiding this comment

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

🟡 Potential null dereference on video in handleVideoClick after optional chaining

The handleVideoClick handler obtains video via optional chaining (lines 400-402), meaning video can be undefined. However, line 410 immediately accesses video.closest(...) without a null check, which would throw TypeError: Cannot read properties of undefined (reading 'closest'). Lines 414 and 417 also access video.paused / pass ev to handlers that dereference video without guarding.

Root Cause

Before this PR, video was assigned as ev.currentTarget as HTMLVideoElement (bloomVideo.ts:399 old code), which was always a valid HTMLVideoElement because the click listener was attached directly to the <video> element. After the change, the listener is on the bloom-videoMouseDetector div, and video is obtained through DOM traversal:

const video = (ev.currentTarget as HTMLElement)
    ?.closest(".bloom-videoContainer")
    ?.getElementsByTagName("video")[0];

The optional chaining (?.) acknowledges that the result can be undefined, but the very next usage at line 410 does not guard against it:

if (!video.closest(".drag-activity-play")) {

In normal operation this is unlikely to crash because the mouseDetector is only created when a <video> element exists inside the container. However, if the video element is removed after setup (e.g., by another editing operation), or if the DOM structure is unexpected, this will throw at runtime.

Impact: A crash in the click handler would prevent any further click processing on that video container, silently breaking play/pause behavior.

Suggested change
if (!video.closest(".drag-activity-play")) {
if (!video || !video.closest(".drag-activity-play")) {
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Expand Down
3 changes: 0 additions & 3 deletions src/content/bookLayout/basePage.less
Original file line number Diff line number Diff line change
Expand Up @@ -336,9 +336,6 @@ books may contain divs with box-header-off, so we need a rule to hide them.*/
background-size: contain;
}

// above canvas so we can click playback controls
z-index: @canvasElementCanvasZIndex + 1;

video {
// I don't know exactly why this works, but it makes the video shrink to fit,
// keep its aspect ratio, and center in whatever direction is not filled.
Expand Down