diff --git a/src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts b/src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts index b3d8ed3dd1aa..d27bb9af8415 100644 --- a/src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts +++ b/src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts @@ -81,7 +81,8 @@ import { get, postData, postJson, postString } from "../../utils/bloomApi"; import AudioRecording from "../toolbox/talkingBook/audioRecording"; import PlaceholderProvider from "./PlaceholderProvider"; import { getExactClientSize } from "../../utils/elementUtils"; -import { copyContentToTarget, getTarget } from "bloom-player"; +import { getTarget } from "bloom-player"; +import { copyContentToTargetAndCleanup } from "./dragActivityRuntimeUtils"; import { showRequiresSubscriptionDialogInEditView } from "../../react_components/requiresSubscription"; import { FeatureStatus } from "../../react_components/featureStatus"; import $ from "jquery"; @@ -2905,7 +2906,7 @@ export class CanvasElementManager { this.doAfterNewImageAdjusted(); this.doAfterNewImageAdjusted = undefined; } - copyContentToTarget(canvasElement); + copyContentToTargetAndCleanup(canvasElement); } // When the image is changed in a canvas element (e.g., choose or paste image), diff --git a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts index efa216df4d1a..d29a666f097c 100644 --- a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts +++ b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts @@ -9,7 +9,10 @@ import { SetupResizableElement, SetupImagesInContainer, } from "./bloomImages"; -import { SetupVideoEditing } from "./bloomVideo"; +import { + removeTransientVideoTimestampParams, + SetupVideoEditing, +} from "./bloomVideo"; import { SetupWidgetEditing } from "./bloomWidgets"; import { setupOrigami, cleanupOrigami } from "./origami"; import theOneLocalizationManager from "../../lib/localizationManager/localizationManager"; @@ -1215,6 +1218,7 @@ function removeEditingDebris() { for (let i = 0; i < textLabels.length; i++) { textLabels[i].remove(); } + removeTransientVideoTimestampParams(document.body); cleanupNiceScroll(); // don't leave the nicescroll debris around } diff --git a/src/BloomBrowserUI/bookEdit/js/bloomVideo.spec.ts b/src/BloomBrowserUI/bookEdit/js/bloomVideo.spec.ts new file mode 100644 index 000000000000..a8afaba01433 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/bloomVideo.spec.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from "vitest"; +import { + removeTransientVideoTimestampParams, + stripTransientVideoTimestampParam, +} from "./bloomVideo"; + +describe("bloomVideo transient video timestamp cleanup", () => { + it("removes only the transient video timestamp param", () => { + const cleaned = stripTransientVideoTimestampParam( + "video.mp4?persist=true&bloomVideoTransientTimestamp=123#t=1,2", + ); + + expect(cleaned).toBe("video.mp4?persist=true#t=1,2"); + }); + + it("leaves urls without the transient param unchanged", () => { + const cleaned = stripTransientVideoTimestampParam( + "video.mp4?persist=true#t=1,2", + ); + + expect(cleaned).toBe("video.mp4?persist=true#t=1,2"); + }); + + it("cleans transient params from video sources in the page", () => { + document.body.innerHTML = + '
'; + + removeTransientVideoTimestampParams(document.body); + + const videos = Array.from(document.body.querySelectorAll("video")); + expect(videos[0].getAttribute("src")).toBe("movie.mp4"); + expect(videos[1].querySelector("source")?.getAttribute("src")).toBe( + "clip.mp4?keep=1#t=2,4", + ); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/js/bloomVideo.ts b/src/BloomBrowserUI/bookEdit/js/bloomVideo.ts index b4681e325378..e1ad92a48be5 100644 --- a/src/BloomBrowserUI/bookEdit/js/bloomVideo.ts +++ b/src/BloomBrowserUI/bookEdit/js/bloomVideo.ts @@ -18,45 +18,47 @@ import { getReplayIcon } from "../img/replayIcon"; import { kCanvasElementSelector } from "../toolbox/canvas/canvasElementUtils"; import $ from "jquery"; -export function SetupVideoEditing(container) { +const kBloomVideoTransientTimestampParam = "bloomVideoTransientTimestamp"; + +export function SetupVideoEditing(container: HTMLElement) { $(container) .find(".bloom-videoContainer") .each((index, vc) => { SetupVideoContainer(vc); }); - Array.from(container.getElementsByTagName("video")).forEach( - (videoElement: HTMLVideoElement) => { - videoElement.removeAttribute("controls"); - // I don't think we need to do this in normal operation, but it's useful when - // 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 playButton = wrapVideoIcon( - videoElement, - // 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 - // React for anything except to make use of an image which unfortunately is only - // available by default as a component. - getPlayIcon("#ffffff", videoElement), - "bloom-videoPlayIcon", - ); - playButton.addEventListener("click", handlePlayClick); - const pauseButton = wrapVideoIcon( - videoElement, - getPauseIcon("#ffffff", videoElement), - "bloom-videoPauseIcon", - ); - pauseButton.addEventListener("click", handlePauseClick); - const replayButton = wrapVideoIcon( - videoElement, - getReplayIcon("#ffffff", videoElement), - "bloom-videoReplayIcon", - ); - replayButton.addEventListener("click", handleReplayClick); - }, - ); + Array.from( + container.getElementsByTagName("video"), + ).forEach((videoElement: HTMLVideoElement) => { + videoElement.removeAttribute("controls"); + // I don't think we need to do this in normal operation, but it's useful when + // 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 playButton = wrapVideoIcon( + videoElement, + // 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 + // React for anything except to make use of an image which unfortunately is only + // available by default as a component. + getPlayIcon("#ffffff", videoElement), + "bloom-videoPlayIcon", + ); + playButton.addEventListener("click", handlePlayClick); + const pauseButton = wrapVideoIcon( + videoElement, + getPauseIcon("#ffffff", videoElement), + "bloom-videoPauseIcon", + ); + pauseButton.addEventListener("click", handlePauseClick); + const replayButton = wrapVideoIcon( + videoElement, + getReplayIcon("#ffffff", videoElement), + "bloom-videoReplayIcon", + ); + replayButton.addEventListener("click", handleReplayClick); + }); } function SetupVideoContainer(videoContainerDiv: Element) { @@ -92,7 +94,7 @@ function videoPlayingEventHandler(e: Event) { currentVideoElement = video; let end: number = getVideoEndSeconds(video); const untrimmedEndPoint: number = 0.0; - if (end == untrimmedEndPoint) { + if (end === untrimmedEndPoint) { // We can't just set the endpoint to equal the duration of the video here, because // we will be testing that the current playback time is greater than the endpoint. // Since we test for the end of the video every 1/10th of a second, set the endpoint @@ -292,13 +294,59 @@ export function updateVideoInContainer(container: Element, url: string): void { video.appendChild(source); } if (source) { - source.setAttribute("src", url); + source.setAttribute("src", addTransientVideoTimestampParam(url)); + video.load(); // Transparent background videos allow the placeholder to show. See BL-13918. container.classList.remove("bloom-noVideoSelected"); } } } +function addTransientVideoTimestampParam(url: string): string { + const hashIndex = url.indexOf("#"); + const hash = hashIndex >= 0 ? url.substring(hashIndex) : ""; + const beforeHash = hashIndex >= 0 ? url.substring(0, hashIndex) : url; + const queryIndex = beforeHash.indexOf("?"); + const baseUrl = + queryIndex >= 0 ? beforeHash.substring(0, queryIndex) : beforeHash; + const query = queryIndex >= 0 ? beforeHash.substring(queryIndex + 1) : ""; + const searchParams = new URLSearchParams(query); + searchParams.set(kBloomVideoTransientTimestampParam, Date.now().toString()); + const search = searchParams.toString(); + + return `${baseUrl}${search ? `?${search}` : ""}${hash}`; +} + +export function stripTransientVideoTimestampParam(url: string): string { + const hashIndex = url.indexOf("#"); + const hash = hashIndex >= 0 ? url.substring(hashIndex) : ""; + const beforeHash = hashIndex >= 0 ? url.substring(0, hashIndex) : url; + const queryIndex = beforeHash.indexOf("?"); + if (queryIndex < 0) { + return url; + } + + const baseUrl = beforeHash.substring(0, queryIndex); + const query = beforeHash.substring(queryIndex + 1); + const searchParams = new URLSearchParams(query); + searchParams.delete(kBloomVideoTransientTimestampParam); + const search = searchParams.toString(); + + return `${baseUrl}${search ? `?${search}` : ""}${hash}`; +} + +export function removeTransientVideoTimestampParams(root: ParentNode): void { + for (const element of Array.from( + root.querySelectorAll("video[src], video source[src]"), + )) { + const src = element.getAttribute("src"); + if (!src) { + continue; + } + element.setAttribute("src", stripTransientVideoTimestampParam(src)); + } +} + // 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( diff --git a/src/BloomBrowserUI/bookEdit/js/dragActivityRuntimeUtils.ts b/src/BloomBrowserUI/bookEdit/js/dragActivityRuntimeUtils.ts new file mode 100644 index 000000000000..e29400c10de5 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/dragActivityRuntimeUtils.ts @@ -0,0 +1,17 @@ +import { copyContentToTarget, getTarget } from "bloom-player"; + +// Keep UI-only selection state out of target shadow content. +export function copyContentToTargetAndCleanup( + draggableElement: HTMLElement, +): void { + copyContentToTarget(draggableElement); + + const target = getTarget(draggableElement); + if (!target) { + return; + } + + target.querySelectorAll(".bloom-selected").forEach((el) => { + el.classList.remove("bloom-selected"); + }); +} diff --git a/src/BloomBrowserUI/bookEdit/js/videoUtils.ts b/src/BloomBrowserUI/bookEdit/js/videoUtils.ts index 59dcf0f4cfee..0f388d94eea4 100644 --- a/src/BloomBrowserUI/bookEdit/js/videoUtils.ts +++ b/src/BloomBrowserUI/bookEdit/js/videoUtils.ts @@ -9,6 +9,39 @@ import { export const kVideoContainerClass = "bloom-videoContainer"; +function resolveSelectableVideoContainer( + videoContainer: Element | undefined | null, + body: HTMLElement | null, +): Element | undefined { + if (!videoContainer) { + return undefined; + } + + const containingTarget = videoContainer.closest("[data-target-of]"); + if (!containingTarget) { + return videoContainer; + } + + const draggableId = containingTarget.getAttribute("data-target-of"); + if (!draggableId || !body) { + return undefined; + } + + // Keep this local to avoid importing CanvasElementManager (which imports this file). + // We can have copied target content that still contains data-draggable-id, so make + // sure we resolve to a real draggable (not anything inside [data-target-of]). + const draggable = Array.from( + body.querySelectorAll(`[data-draggable-id='${draggableId}']`), + ).find((candidate) => !candidate.closest("[data-target-of]")) as + | HTMLElement + | undefined; + if (!draggable) { + return undefined; + } + + return draggable.querySelector(`.${kVideoContainerClass}`) ?? undefined; +} + // Set the attribute which makes a canvas element active for the sign language tool. // Make sure nothing else has it. // If it's in a canvas element, make that canvas element active. If not, make sure no canvas element is active. @@ -17,14 +50,19 @@ export function selectVideoContainer( videoContainer: Element | undefined | null, notifyCanvasElementManager = true, ) { - const body = getPageIframeBody(); + const body = + getPageIframeBody() ?? videoContainer?.ownerDocument?.body ?? null; + const selectableVideoContainer = resolveSelectableVideoContainer( + videoContainer, + body, + ); if (body) { - Array.from(body.getElementsByClassName("bloom-selected")) - .filter((e) => e !== videoContainer) - .forEach((e) => e.classList.remove("bloom-selected")); + Array.from(body.getElementsByClassName("bloom-selected")).forEach((e) => + e.classList.remove("bloom-selected"), + ); } - videoContainer?.classList.add("bloom-selected"); - const canvasElement = videoContainer?.closest( + selectableVideoContainer?.classList.add("bloom-selected"); + const canvasElement = selectableVideoContainer?.closest( kCanvasElementSelector, ) as HTMLElement; // If it's in a canvas element, make that canvas element active. If not, make sure no canvas element is active. @@ -40,9 +78,9 @@ export function selectVideoContainer( // since the yellow box is hidden in canvas elements, and we would like the same one (that is our active // canvas element) to be selected in the sign langauge tool if we switch back.) export function deselectVideoContainers() { - const videoContainers: HTMLElement[] = Array.from( - document.getElementsByClassName(kVideoContainerClass) as any, - ); + const videoContainers = Array.from( + document.getElementsByClassName(kVideoContainerClass), + ) as HTMLElement[]; videoContainers .filter((x) => !x.closest(kCanvasElementSelector)) .forEach((container) => { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx index ec8dfefc1ab3..0a2a90d8d553 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx @@ -638,7 +638,12 @@ export const ToolboxRoot: React.FunctionComponent = () => { window.toolboxReactAdapter = { isEnabled: () => true, setActiveToolByToolId: (toolId: string) => { - setExpandedSectionId(normalizeToolId(toolId)); + const normalizedToolId = normalizeToolId(toolId); + setExpandedSectionId(normalizedToolId); + const callbackToolId = toToolboxToolId(normalizedToolId); + activeToolChangedCallbacks.current.forEach((callback) => { + callback(callbackToolId); + }); }, getActiveToolId: () => { if (!expandedSectionId) { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/games/GamePromptDialog.tsx b/src/BloomBrowserUI/bookEdit/toolbox/games/GamePromptDialog.tsx index fe8ffc0073f1..626285e376f6 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/games/GamePromptDialog.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/games/GamePromptDialog.tsx @@ -10,11 +10,11 @@ interface IGamePromptDialogProps { import { useRef } from "react"; import { adjustDraggablesForLanguage, - copyContentToTarget, getTarget, shuffle, isTheTextInDraggablesTheSame, } from "bloom-player"; +import { copyContentToTargetAndCleanup } from "../../js/dragActivityRuntimeUtils"; import { setGeneratedDraggableId } from "../canvas/CanvasElementItem"; import { adjustTarget, makeTargetForDraggable } from "../games/GameTool"; import * as ReactDOM from "react-dom"; @@ -403,7 +403,7 @@ const initializeDialog = (prompt: HTMLElement, tg: HTMLElement | null) => { "bloom-unused-in-lang", i >= letters.length, ); - copyContentToTarget(draggables[i]); + copyContentToTargetAndCleanup(draggables[i]); } const shuffledDraggables = draggables.slice(); shuffledDraggables.splice(letters.length); // don't want any invisible ones taking up space diff --git a/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx index 170d4d524671..6185b8844e7f 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx @@ -26,7 +26,6 @@ import { ToolBox } from "../toolbox"; import { adjustDraggablesForLanguage, classSetter, - copyContentToTarget, getTarget, playInitialElements, prepareActivity, @@ -58,6 +57,7 @@ import { kBackgroundImageClass, kDraggableIdAttribute, } from "../../js/CanvasElementManager"; +import { copyContentToTargetAndCleanup } from "../../js/dragActivityRuntimeUtils"; import { getCanvasElementManager, kCanvasElementSelector, @@ -935,7 +935,7 @@ const DragActivityControls: React.FunctionComponent<{ currentCanvasElement === getCanvasElementManager()?.getActiveElement() ) { - copyContentToTarget(currentCanvasElement); + copyContentToTargetAndCleanup(currentCanvasElement); } }); draggableToTargetObserver.current.observe(currentCanvasElement, { @@ -1891,6 +1891,18 @@ export function getActiveDragActivityTab(): number { return window.top!["dragActivityPage"] ?? 0; } +function removeBloomSelectedFromTargets(page: HTMLElement): void { + page.querySelectorAll("[data-target-of] .bloom-selected").forEach((el) => { + el.classList.remove("bloom-selected"); + }); +} + +function scheduleRemoveBloomSelectedFromTargets(page: HTMLElement): void { + // Some drag-activity runtime operations can copy content asynchronously after + // tab setup, so run cleanup again on the next tick. + setTimeout(() => removeBloomSelectedFromTargets(page), 0); +} + // The top-level function to get everything into the right state for the specified tab // (Start, Correct, Wrong, Play). // Note: the games code is currently pulled into the page bundle, but this function @@ -1995,6 +2007,11 @@ export function setActiveDragActivityTab(tab: number) { disableDraggingTargets(page); hideGamePromptDialog(page); } + + // Defensive cleanup: copies of draggables in targets should never carry video + // selection state, even if copied by external runtime code paths. + removeBloomSelectedFromTargets(page); + scheduleRemoveBloomSelectedFromTargets(page); } // Replace the origami control with the Game tab control if the page is a game. diff --git a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx index d0907cef1866..d9a370d50ee3 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx @@ -10,7 +10,6 @@ import { post, postDataWithConfig, postJson, - postThatMightNavigate, } from "../../../utils/bloomApi"; import { ToolBottomHelpLink } from "../../../react_components/ToolBottomHelpLink"; import { UrlUtils } from "../../../utils/urlUtils"; @@ -20,6 +19,7 @@ import calculateAspectRatio from "calculate-aspect-ratio"; import VideoTrimSlider from "../../../react_components/videoTrimSlider"; import { updateVideoInContainer } from "../../js/bloomVideo"; import { selectVideoContainer } from "../../js/videoUtils"; +import { getCanvasElementManager } from "../canvas/canvasElementUtils"; import $ from "jquery"; // The recording process can be in one of these states: @@ -518,13 +518,34 @@ export class SignLanguageToolControls extends React.Component< updateVideoInContainer(container, url); } + public leaveCurrentVideoContext(onComplete?: () => void) { + document.removeEventListener("keydown", this.onKeyPress); + window.clearTimeout(this.timerId); + SignLanguageTool.removeVideoOverlay(); + this.setState( + { + recording: false, + stateClass: "idle", + minutesRecorded: "", + secondsRecorded: "", + haveRecording: false, + videoStatistics: emptyVideoStatistics, + videoInfoLoaded: false, + }, + onComplete, + ); + } + + private refreshAfterVideoChange() { + this.leaveCurrentVideoContext(() => { + this.getVideoStatsFromFile(); + }); + } + private importRecording() { post("signLanguage/importVideo", (result) => { this.updateVideo(result.data); - // Makes sure the page gets saved with a reference to the new video, - // and incidentally that everything gets updated to be consistent with the - // new state of things. - postThatMightNavigate("common/saveChangesAndRethinkPageEvent"); + this.refreshAfterVideoChange(); }); } @@ -546,12 +567,7 @@ export class SignLanguageToolControls extends React.Component< if (video && video.parentElement) video.parentElement.removeChild(video); } - // Makes sure the page gets saved without a reference to the deleted video, - // and incidentally that everything gets updated to be consistent with the - // new state of things. - postThatMightNavigate( - "common/saveChangesAndRethinkPageEvent", - ); + this.refreshAfterVideoChange(); } }, ); @@ -727,12 +743,7 @@ export class SignLanguageToolControls extends React.Component< }, (result) => { this.updateVideo(result.data); - // Makes sure the page gets saved with a reference to the new video, - // and incidentally that everything gets updated to be consistent with the - // new state of things. - postThatMightNavigate( - "common/saveChangesAndRethinkPageEvent", - ); + this.refreshAfterVideoChange(); }, ); // Don't know why this is necessary, but for some reason, the stream we have is no @@ -787,6 +798,18 @@ export class SignLanguageToolControls extends React.Component< export class SignLanguageTool extends ToolboxToolReactAdaptor { private reactControls: SignLanguageToolControls; + private static getVideoContainersFromPage( + page: HTMLElement, + selected?: boolean, + ): HTMLElement[] { + const selector = selected + ? ".bloom-videoContainer.bloom-selected" + : ".bloom-videoContainer"; + return Array.from(page.querySelectorAll(selector)).filter( + (container) => !container.closest("[data-target-of]"), + ) as HTMLElement[]; + } + public makeRootElement(): HTMLDivElement { const root = document.createElement("div"); root.setAttribute("class", "signLanguageBody"); @@ -811,14 +834,11 @@ export class SignLanguageTool extends ToolboxToolReactAdaptor { // Specify 'true' to get only containers marked as selected public static getVideoContainers(selected?: boolean): HTMLElement[] { - let classes = "bloom-videoContainer"; - if (selected) { - classes += " bloom-selected"; - } const page = ToolBox.getPage(); - return ( - page ? Array.from(page.getElementsByClassName(classes)) : [] - ) as HTMLElement[]; + if (!page) { + return []; + } + return this.getVideoContainersFromPage(page, selected); } public static getSelectedVideoPathAndTiming(): string | null { @@ -839,7 +859,7 @@ export class SignLanguageTool extends ToolboxToolReactAdaptor { } public static getSelectedVideoPath(): string | null { - // strip off the ?now= param we sometimes use to prevent use of cached old versions, + // strip off the temporary cache-busting query param we sometimes use to prevent use of cached old versions, // and the #t= fragment used to trim videos. return UrlUtils.extractPathComponent( SignLanguageTool.getSelectedVideoPathAndTiming() as string, @@ -847,7 +867,7 @@ export class SignLanguageTool extends ToolboxToolReactAdaptor { } public detachFromPage() { - this.reactControls.setNoVideo(false); + this.reactControls.leaveCurrentVideoContext(); // Decided NOT to remove bloom-selected here. It's harmless (only the edit stylesheet // does anything with it) and leaving it allows us to keep the same one selected // when we come back to the page. This is especially important when refreshing the @@ -910,6 +930,84 @@ export class SignLanguageTool extends ToolboxToolReactAdaptor { } }; + public showTool() { + this.syncSelectionFromCurrentPage(); + } + + private syncSelectionFromCurrentPage() { + const pageBody = ToolBox.getPage(); + if (!pageBody) { + // Tool activation can race with page readiness. + window.setTimeout(() => this.syncSelectionFromCurrentPage(), 100); + return; + } + + const preselectedVideo = pageBody.querySelector( + ".bloom-videoContainer.bloom-selected", + ) as HTMLElement | null; + if (preselectedVideo) { + // Normalize any existing selection through videoUtils logic so target copies + // cannot remain selected. + selectVideoContainer(preselectedVideo); + } + + let containers = SignLanguageTool.getVideoContainersFromPage( + pageBody, + false, + ); + if (containers.length === 0) { + const activeCanvasElement = + getCanvasElementManager()?.getActiveElement(); + const activeVideoContainer = activeCanvasElement?.querySelector( + ".bloom-videoContainer", + ) as HTMLElement | null; + if ( + activeVideoContainer && + !activeVideoContainer.closest("[data-target-of]") + ) { + selectVideoContainer(activeVideoContainer); + containers = [activeVideoContainer]; + } + } + + if (containers.length === 0) { + if (this.reactControls.state.enabled) { + this.reactControls.turnOffVideo(); + this.reactControls.setState({ + enabled: false, + }); + } + return; + } + + // We want one video container to be selected, so pick the first. + // If one is already marked selected, presumably from a previous use of this page, + // we'll leave that one active. + const selectedVideos = SignLanguageTool.getVideoContainersFromPage( + pageBody, + true, + ); + const videoToUse = + selectedVideos.length > 0 ? selectedVideos[0] : containers[0]; + selectVideoContainer(videoToUse); + this.updateStateForSelected(videoToUse); + + for (let i = 0; i < containers.length; i++) { + const container = containers[i]; + // UpdateMarkup is called fairlyfrequently. Not sure what effect having + // the same listener attached multiple times might have, so play safe by + // removing it before adding. + container.removeEventListener("click", this.containerClickListener); + container.addEventListener("click", this.containerClickListener); + } + // we turn it off when we leave a page, so even if we already have enabled:true, + // we need to turn it on for this page now we know there is something to record. + this.reactControls.turnOnVideo(); + if (!this.reactControls.state.enabled) { + this.reactControls.setState({ enabled: true }); + } + } + public newPageReady() { // among other things, this clears us out of "processing" // when the page is refreshed with the new video @@ -921,45 +1019,8 @@ export class SignLanguageTool extends ToolboxToolReactAdaptor { haveRecording: false, videoInfoLoaded: false, }); - const containers = SignLanguageTool.getVideoContainers(false); - if (!containers || containers.length === 0) { - if (this.reactControls.state.enabled) { - this.reactControls.turnOffVideo(); - this.reactControls.setState({ - enabled: false, - }); - } - } else { - // We want one video container to be selected, so pick the first. - // If one is already marked selected, presumably from a previous use of this page, - // we'll leave that one active. - const selectedVideos = SignLanguageTool.getVideoContainers(true); - const videoToUse = - selectedVideos.length > 0 ? selectedVideos[0] : containers[0]; - selectVideoContainer(videoToUse); - this.updateStateForSelected(videoToUse); - for (let i = 0; i < containers.length; i++) { - const container = containers[i]; - // UpdateMarkup is called fairlyfrequently. Not sure what effect having - // the same listener attached multiple times might have, so play safe by - // removing it before adding. - container.removeEventListener( - "click", - this.containerClickListener, - ); - container.addEventListener( - "click", - this.containerClickListener, - ); - } - // we turn it off when we leave a page, so even if we already have enabled:true, - // we need to turn it on for this page now we know there is something to record. - this.reactControls.turnOnVideo(); - if (!this.reactControls.state.enabled) { - this.reactControls.setState({ enabled: true }); - } - } + this.syncSelectionFromCurrentPage(); } // This (param) container has just been selected as the current one by either: diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts index 13df95efd46f..f9a64fa8793e 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -637,8 +637,15 @@ export class ToolBox { // if it was an actual "input" element, we would just check for "checked", // but it's actually a div with possibly a checkmark character inside, // so just check string length. + // An earlier version of this code simulated a click on the checkbox, + // but that failed when it was already active, toggling it to inactive, + // where here we want to ensure it is active AND selected. if (checkBox.innerText.length === 0) { - checkBox.click(); // will also activate + turnOnToolFromCheckbox( + checkBox, + ToolBox.addToolToString(toolId), + true, + ); } else { setCurrentTool(toolId); } @@ -742,24 +749,41 @@ export function showOrHideTool_click(chkbox) { const tool = $(chkbox).data("tool"); const turnOn = chkbox.innerHTML === ""; if (turnOn) { - chkbox.innerHTML = checkMarkString; - postString( - "editView/saveToolboxSetting", - "active\t" + chkbox.id + "\t1", - ); + turnOnToolFromCheckbox(chkbox, tool, true); } else { - chkbox.innerHTML = ""; - postString( - "editView/saveToolboxSetting", - "active\t" + chkbox.id + "\t0", - ); + setToolCheckboxEnabledState(chkbox, false); + showOrHideTool(chkbox.id, tool, false); } - showOrHideTool(chkbox.id, tool, turnOn); } -function showOrHideTool(chkboxId: string, tool: string, turnOn: boolean) { +function setToolCheckboxEnabledState( + chkbox: HTMLDivElement, + turnOn: boolean, +): void { + chkbox.innerHTML = turnOn ? checkMarkString : ""; + postString( + "editView/saveToolboxSetting", + "active\t" + chkbox.id + "\t" + (turnOn ? "1" : "0"), + ); +} + +function turnOnToolFromCheckbox( + chkbox: HTMLDivElement, + tool: string, + openTool: boolean, +): void { + setToolCheckboxEnabledState(chkbox, true); + showOrHideTool(chkbox.id, tool, true, openTool); +} + +function showOrHideTool( + chkboxId: string, + tool: string, + turnOn: boolean, + openTool: boolean = true, +) { if (turnOn) { - beginAddTool(chkboxId, tool, true); + beginAddTool(chkboxId, tool, openTool); } else { $("*[data-toolId]") .filter(function () { @@ -1826,7 +1850,7 @@ function loadToolboxTool( } // if requested, open the tool that was just inserted - if (openTool && toolbox.toolboxIsShowing()) { + if (openTool) { const adapter = getToolboxReactAdapter(); if (adapter) { const toolId = header.attr("data-toolId");