-
-
Notifications
You must be signed in to change notification settings - Fork 18
BL-15721 show bubbles on top of videos #7597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1642,21 +1642,34 @@ svg.bloom-videoControl { | |
| } | ||
| } | ||
|
|
||
| // Detector above the canvas so we can detect hover and clicks of playback controls | ||
| .bloom-videoMouseDetector { | ||
| 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 { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6535,7 +6535,8 @@ export class CanvasElementManager { | |
| .find(".bloom-ui") | ||
| .filter( | ||
| (_, x) => | ||
| !x.classList.contains("bloom-videoControlContainer"), | ||
| !x.classList.contains("bloom-videoControlContainer") && | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
|
||
devin-ai-integration[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||||
|
|
@@ -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", | ||||||||
| ); | ||||||||
|
|
@@ -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; | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -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) && | ||||||||
|
|
@@ -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")) { | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Potential null dereference on The Root CauseBefore this PR, const video = (ev.currentTarget as HTMLElement)
?.closest(".bloom-videoContainer")
?.getElementsByTagName("video")[0];The optional chaining ( if (!video.closest(".drag-activity-play")) {In normal operation this is unlikely to crash because the mouseDetector is only created when a Impact: A crash in the click handler would prevent any further click processing on that video container, silently breaking play/pause behavior.
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||
|
|
||||||||
There was a problem hiding this comment.
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"