From 1df2350e49c1ef2e8be9b8fa2c0ae7768fb5e6a4 Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Fri, 24 Jul 2026 15:28:48 -0700 Subject: [PATCH 01/19] Fix BL-16608 Cleanup toolbox infrastructure (1/7): remove pure dead code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://issues.bloomlibrary.org/youtrack/issue/BL-16608 Now that all 11 toolbox tools are React (extending ToolboxToolReactAdaptor), this removes code that is provably dead — zero callers or never-assigned state: - toolbox.ts: loadToolboxToolText (old load-HTML-template-from-server path), getToolIfOffered, ensureToolEnabled and its only helper getActiveToolIdFromCurrentToolboxUi, insertLangAttributesIntoToolboxElements plus its verbatim duplicate scan inside initialize() (only worked for non-React tools), and the ITool members hasRestoredSettings (written, never read), isExperimental() (no caller), and finishToolLocalization() (no-op impls only). - Tools: the isExperimental/toolRequiresEnterprise stubs and the beginRestoreSettings overrides that exactly duplicated the base class (canvas, game, motion, signLanguage), plus now-unused jquery imports. - toolboxBootstrap.ts/toolboxGlobals.d.ts: dead applyToolboxStateToPageLegacy and getToolIfOffered declarations. - ToolboxRoot.tsx: the never-assigned legacyToolHtmlSubPath/legacyToolBodyHtml machinery — extractFirstToolContentDivAsHtml, loadLegacyToolBodyHtml, the unreachable dangerouslySetInnerHTML render branch, the no-op disconnect-clearing effect, an effect whose guard became unconditionally true, and the dead branches of the 250ms polling effect (the load-bearing hydrate retry is preserved). No behavior change intended. Verified: pnpm typecheck passes, eslint on changed files 0 errors, full vitest suite 550 passed / 5 skipped, and build/agent-vite.sh confirms the production bundle compiles. Co-Authored-By: Claude Fable 5 --- .../bookEdit/toolbox/ToolboxRoot.tsx | 198 +----------------- .../bookEdit/toolbox/canvas/canvasTool.tsx | 12 -- .../bookEdit/toolbox/games/GameTool.tsx | 16 -- .../imageDescription/imageDescription.tsx | 8 - .../impairmentVisualizer.tsx | 8 - .../bookEdit/toolbox/motion/motionTool.tsx | 9 - .../toolbox/signLanguage/signLanguageTool.tsx | 16 -- .../toolbox/talkingBook/talkingBookTool.tsx | 8 - .../bookEdit/toolbox/toolbox.ts | 181 +--------------- .../bookEdit/toolbox/toolboxBootstrap.ts | 1 - .../bookEdit/toolbox/toolboxGlobals.d.ts | 2 - .../toolbox/toolboxToolReactAdaptor.tsx | 5 - 12 files changed, 10 insertions(+), 454 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx index 87381b0ee7d1..408c9c98537b 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx @@ -26,7 +26,6 @@ import { SubscriptionBadgeWithTooltipAndDialog } from "../../react_components/re // The descriptions and plans below were also generated by Copilot and may or may not be helpful. // - it renders accordion sections in React, // - reuses already-instantiated legacy tool DOM when available, -// - falls back to loading legacy HTML snippets when live DOM is not ready, // - and exposes a small adapter so legacy code can still control active tool state. // // Completed migration target: @@ -37,7 +36,6 @@ import { SubscriptionBadgeWithTooltipAndDialog } from "../../react_components/re // - and toolbox globals are reduced to a minimal compatibility surface (or removed). // // Practical completion checklist for this file: -// - remove legacy HTML fallback loading/injection paths, // - remove existing legacy element adoption paths, // - replace legacy event synchronization with direct React/store updates, // - remove hidden legacy #toolbox root handling, @@ -47,8 +45,6 @@ type ToolboxSection = { id: string; englishLabel: string; l10nKey: string; - legacyToolHtmlSubPath?: string; - legacyToolBodyHtml?: string; liveToolBodyElement?: HTMLDivElement; }; @@ -358,41 +354,6 @@ const LiveToolBodyHost: React.FunctionComponent<{ element: HTMLDivElement }> = ( > ); }; -// Legacy toolbox HTML files contain wrappers and scripts; for React rendering we -// only inject the first meaningful tool-body div. -const extractFirstToolContentDivAsHtml = (rawHtml: string): string => { - const parsedDocument = new DOMParser().parseFromString( - rawHtml, - "text/html", - ); - const topLevelElements = Array.from(parsedDocument.body.children); - const firstTopLevelDiv = topLevelElements.find( - (element) => element.tagName.toLowerCase() === "div", - ); - if (firstTopLevelDiv) { - return firstTopLevelDiv.outerHTML; - } - - const firstAnyDiv = parsedDocument.body.querySelector("div"); - if (firstAnyDiv) { - return firstAnyDiv.outerHTML; - } - - throw new Error("Legacy toolbox html did not contain a div tool body."); -}; - -const loadLegacyToolBodyHtml = async ( - section: ToolboxSection, -): Promise => { - if (!section.legacyToolHtmlSubPath) { - return undefined; - } - - const response = await axios.get( - `/bloom/bookEdit/toolbox/${section.legacyToolHtmlSubPath}`, - ); - return extractFirstToolContentDivAsHtml(response.data); -}; // This component is the root of the whole toolbox sidebar. It is rendered into a dedicated // host element created by the edit-frame page pug. @@ -429,7 +390,7 @@ export const ToolboxRoot: React.FunctionComponent = () => { // Resolve body content for a section. // Prefer a live legacy element to avoid duplicate tool roots; otherwise - // load static legacy HTML as a temporary fallback. + // ask the tool to make its React root. const hydrateToolBody = React.useCallback(async (toolId: string) => { if (hydratedToolIds.current.has(toolId)) { return; @@ -500,10 +461,10 @@ export const ToolboxRoot: React.FunctionComponent = () => { // can be marked hydrated before there is any section to receive its body. React.useEffect(() => { sections.forEach((section) => { - const hasBodyContent = - !!section.legacyToolBodyHtml || !!section.liveToolBodyElement; - - if (hasBodyContent || hydratedToolIds.current.has(section.id)) { + if ( + section.liveToolBodyElement || + hydratedToolIds.current.has(section.id) + ) { return; } @@ -511,84 +472,6 @@ export const ToolboxRoot: React.FunctionComponent = () => { }); }, [sections, hydrateToolBody]); - // Some tool content elements may appear shortly after we build sections. - // Keep trying unresolved live tools until their existing DOM is available. - React.useEffect(() => { - // To avoid unnecessary re-renders, only update sections when we actually find a new live element. - setSections((previousSections) => { - let changed = false; - const nextSections = previousSections.map((section) => { - // If it's not legacy, we don't need to 'hydrate' it (look for the body in the legacy accordion). - // If it already has a live element, we don't need to look for it again. - // So such sections just copy into the new array unchanged. - if ( - !section.legacyToolBodyHtml || - section.liveToolBodyElement - ) { - return section; - } - - // If we still don't have a live element, keep the one that is waiting to be hydrated. - const liveToolBodyElement = getLiveToolBodyElement(section.id); - if (!liveToolBodyElement) { - return section; - } - - changed = true; - hydratedToolIds.current.add(section.id); - return { - ...section, - liveToolBodyElement, - legacyToolBodyHtml: undefined, - }; - }); - - // If we didn't change anything, return the old array so React doesn't re-render unnecessarily. - return changed ? nextSections : previousSections; - }); - }, [sections]); - - // If a previously adopted live node is no longer connected, clear our reference - // so later hydration can look for a replacement node under #toolbox. - // Note: getLiveToolBodyElement() cannot rediscover the same detached element; it - // only finds currently attached nodes in the legacy toolbox container. - React.useEffect(() => { - setSections((previousSections) => { - let changed = false; - const disconnectedToolIds: string[] = []; - // Similar logic so that only if we actually find a newly disconnected element - // do we return a different object and cause a re-render. - const nextSections = previousSections.map((section) => { - // Only clear disconnected live elements for tools we can rebuild from legacy sources. - // React-only tools (like Canvas) may temporarily disconnect during host moves; if we clear - // their only live element reference, they can get stuck on "Loading ...". - const canRehydrateFromLegacySource = - !!section.legacyToolHtmlSubPath || - !!section.legacyToolBodyHtml; - if ( - !section.liveToolBodyElement || - section.liveToolBodyElement.isConnected || - !canRehydrateFromLegacySource - ) { - return section; - } - - changed = true; - disconnectedToolIds.push(section.id); - return { - ...section, - liveToolBodyElement: undefined, - }; - }); - - disconnectedToolIds.forEach((toolId) => { - hydratedToolIds.current.delete(toolId); - }); - - return changed ? nextSections : previousSections; - }); - }, [sections]); - // Keep unresolved sections hydrated over time because legacy tool roots are // not always available immediately when sections are first created (or recreated), // and there may be some possibility that they get removed by non-react code. @@ -602,52 +485,8 @@ export const ToolboxRoot: React.FunctionComponent = () => { const hasConnectedLiveToolBodyElement = !!section.liveToolBodyElement && section.liveToolBodyElement.isConnected; - const hasDisconnectedLiveToolBodyElement = - !!section.liveToolBodyElement && - !section.liveToolBodyElement.isConnected; - const canRehydrateFromLegacySource = - !!section.legacyToolHtmlSubPath || - !!section.legacyToolBodyHtml; - - if ( - canRehydrateFromLegacySource && - hasDisconnectedLiveToolBodyElement && - hydratedToolIds.current.has(section.id) - ) { - hydratedToolIds.current.delete(section.id); - } - - if ( - section.legacyToolBodyHtml && - !hasConnectedLiveToolBodyElement - ) { - const liveToolBodyElement = getLiveToolBodyElement( - section.id, - ); - if (liveToolBodyElement) { - hydratedToolIds.current.add(section.id); - setSections((previousSections) => - previousSections.map((previousSection) => { - if (previousSection.id !== section.id) { - return previousSection; - } - - return { - ...previousSection, - liveToolBodyElement, - legacyToolBodyHtml: undefined, - }; - }), - ); - return; - } - } - const hasBodyContent = - !!section.legacyToolBodyHtml || - hasConnectedLiveToolBodyElement; - - if (hasBodyContent) { + if (hasConnectedLiveToolBodyElement) { return; } @@ -655,19 +494,12 @@ export const ToolboxRoot: React.FunctionComponent = () => { section.id === expandedSectionId && hydratedToolIds.current.has(section.id) ) { + // Forget that we hydrated the tool the user is looking at, so that + // hydrateToolBody() will actually try again to get it a body element. hydratedToolIds.current.delete(section.id); } - const shouldForceHydrateRetryForReactTool = - !section.legacyToolHtmlSubPath && - !section.legacyToolBodyHtml; - - if ( - shouldForceHydrateRetryForReactTool || - !hydratedToolIds.current.has(section.id) - ) { - void hydrateToolBody(section.id); - } + void hydrateToolBody(section.id); }); }, 250); @@ -1051,18 +883,6 @@ export const ToolboxRoot: React.FunctionComponent = () => { section.liveToolBodyElement } /> - ) : section.legacyToolBodyHtml ? ( -
) : ( Loading {section.englishLabel}... diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx index d3d6f2588cef..04da0c2f5f9f 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx @@ -3,7 +3,6 @@ import { renderRoot } from "../../../utils/reactRender"; import { kCanvasToolId } from "../toolIds"; import { EnableAllImageEditing } from "../../js/bloomImages"; import { getCanvasElementManager } from "./canvasElementPageBridge"; -import $ from "jquery"; import type { CanvasElementManager } from "../../js/canvasElementManager/CanvasElementManager"; import CanvasToolControls from "./CanvasToolControls"; @@ -33,17 +32,6 @@ export class CanvasTool extends ToolboxToolReactAdaptor { public featureName? = kCanvasToolId; - public isExperimental(): boolean { - return false; - } - - public beginRestoreSettings(_settings: string): JQueryPromise { - // Nothing to do, so return an already-resolved promise. - const result = $.Deferred(); - result.resolve(); - return result; - } - public newPageReady() { const canvasElementManager = getCanvasElementManager(); if (!canvasElementManager) { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx index d17d9944f08b..998e281fa73c 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx @@ -74,7 +74,6 @@ import { CanvasSnapProvider } from "../../js/canvasElementManager/CanvasSnapProv import { CanvasGuideProvider } from "../../js/canvasElementManager/CanvasGuideProvider"; import { kIdForDragActivityTabControl } from "./DragActivityTabControl"; import { RequiresSubscriptionOverlayWrapper } from "../../../react_components/requiresSubscription"; -import $ from "jquery"; // This is the main code that manages the Bloom Games, including Drag Activities. // See especially DragActivityControls, which is the main React component for the tool, @@ -1773,21 +1772,6 @@ export class GameTool extends ToolboxToolReactAdaptor { return kGameToolId; } - public isExperimental(): boolean { - return false; // Todo: probably soon true, but first we need to make a control to turn it on - } - - public toolRequiresEnterprise(): boolean { - return true; // Todo: implement this more fully, probably using RequiresBloomEnterprise - } - - public beginRestoreSettings(_settings: string): JQueryPromise { - // Nothing to do, so return an already-resolved promise. - const result = $.Deferred(); - result.resolve(); - return result; - } - private lastPageId = ""; public newPageReady() { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx index 1939e861c08e..afb43ba50248 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx @@ -364,14 +364,6 @@ export class ImageDescriptionAdapter extends ToolboxToolReactAdaptor { } } - public isExperimental(): boolean { - return false; - } - - public toolRequiresEnterprise(): boolean { - return false; - } - public id(): string { return ImageDescriptionAdapter.kToolID; } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx b/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx index 011d7772007c..60da95957827 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx @@ -362,12 +362,4 @@ export class ImpairmentVisualizerAdaptor extends ToolboxToolReactAdaptor { public detachFromPage() { ImpairmentVisualizerControls.removeImpairmentVisualizerMarkup(); } - - public isExperimental(): boolean { - return false; - } - - public toolRequiresEnterprise(): boolean { - return false; - } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx index 2589f48d2188..a16b259c2d26 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx @@ -56,18 +56,9 @@ export class MotionTool extends ToolboxToolReactAdaptor { this.setupImageObserver(); return root as HTMLDivElement; } - public beginRestoreSettings(settings: string): JQueryPromise { - //Nothing to do, so return an already-resolved promise. - const result = $.Deferred(); - result.resolve(); - return result; - } public isAlwaysEnabled(): boolean { return false; } - public isExperimental(): boolean { - return false; - } public newPageReady() { this.makeRectsVisible(); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx index 76a0e64d7d67..993f82a57397 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx @@ -21,7 +21,6 @@ import { chooseAndProcessVideo } from "../../js/ChooseAndProcessVideo"; import { selectVideoContainer } from "../../js/videoUtils"; import { getCanvasElementManager } from "../canvas/canvasElementPageBridge"; import { kCanvasElementSelector } from "../canvas/canvasElementConstants"; -import $ from "jquery"; import VolumeUpIcon from "@mui/icons-material/VolumeUp"; import VolumeOffIcon from "@mui/icons-material/VolumeOff"; @@ -987,21 +986,6 @@ export class SignLanguageTool extends ToolboxToolReactAdaptor { return root as HTMLDivElement; } - public isExperimental(): boolean { - return false; - } - - public toolRequiresEnterprise(): boolean { - return false; - } - - public beginRestoreSettings(settings: string): JQueryPromise { - // Nothing to do, so return an already-resolved promise. - const result = $.Deferred(); - result.resolve(); - return result; - } - // Specify 'true' to get only containers marked as selected public static getVideoContainers(selected?: boolean): HTMLElement[] { const page = ToolBox.getPage(); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx index 18d48657b367..86ac23a472a9 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx @@ -199,12 +199,4 @@ export default class TalkingBookTool extends ToolboxToolReactAdaptor { public id() { return "talkingBook"; } - - public hasRestoredSettings: boolean; - - // Some things were impossible to do i18n on via the jade/pug - // This gives us a hook to finish up the more difficult spots - public finishToolLocalization(paneDOM: HTMLElement) { - // So far unneeded in talkingBook - } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts index 72281f5f7f91..57899d35e51e 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -84,16 +84,10 @@ export interface ITool { newPageReady(); detachFromPage(); // called when a page is going away AND before hideTool id(): string; // without trailing "Tool"! - hasRestoredSettings: boolean; isAlwaysEnabled(): boolean; - isExperimental(): boolean; // If this is true, the tool may only be selected on pages that have data-tool-id matching this tool's id. requiresToolId(): boolean; - // Some things were impossible to do i18n on via the jade/pug - // This gives us a hook to finish up the more difficult spots - finishToolLocalization(pane: HTMLElement); - // Implement this if the tool uses React. // It should return the main content of the tool, which must be a single div. // (toolbox will construct the h3 element which goes along with it in the accordion @@ -470,53 +464,6 @@ export class ToolBox { }); $("body").find("*[data-i18n]").localize(); // run localization - get("currentUiLanguage", (result) => { - const langName = result.data; - - const nodeList = document.querySelectorAll( - ':not([data-i18n=""])', - ); - for (let i = 0; i < nodeList.length; ++i) { - const node = nodeList.item(i); - - if (!node.hasAttribute("data-i18n")) { - // Nodes which don't have data-18n will match the selector that it's not equal to "", - // but we definitely don't want to apply language text-specific markup to those non-leaf nodes. - continue; - } - - // TODO: This only works when the tool is loaded up for the first time. - // It doesn't work if you open a new tool after the talking book tool is initialized for the first time. - // TODO: How to re-translate when UI lang changed. - const i18nId = - node.getAttribute("data-i18n"); - if (!i18nId) { - node.setAttribute("lang", langName); - } else { - // Double-check that it's actually in this language and not just using an English fallback - theOneLocalizationManager - .asyncGetTextInLang( - i18nId, - "", - langName, - "", - ) - .done((result) => { - if (result) { - node.setAttribute( - "lang", - langName, - ); - } else { - node.removeAttribute( - "lang", - ); // Or maybe set to "en" instead? - } - }); - } - } - }); - // Now bind the window's resize function to the toolbox resizer $(window).bind("resize", () => { clearTimeout(resizeTimer); // resizeTimer variable is defined outside of ready function @@ -542,46 +489,6 @@ export class ToolBox { ); } - // Adds "lang" attributes into the DOM for toolbox elements which have internationalization. (AKA, have data-i18n) - // TODO: This only works with non-React toolbox components. For now, we only need it for talking book tool though. - public static insertLangAttributesIntoToolboxElements() { - get("currentUiLanguage", (result) => { - const langName = result.data; - - const nodeList = document.querySelectorAll(':not([data-i18n=""])'); - for (let i = 0; i < nodeList.length; ++i) { - const node = nodeList.item(i); - - if (!node.hasAttribute("data-i18n")) { - // Nodes which don't have data-18n will match the selector that it's not equal to "", - // but we definitely don't want to apply language text-specific markup to those non-leaf nodes. - continue; - } - - const i18nId = node.getAttribute("data-i18n"); - if (!i18nId) { - node.setAttribute("lang", langName); - } else { - // Double-check that it's actually in this language and not just using an English fallback - theOneLocalizationManager - .asyncGetTextInLang(i18nId, "", langName, "") - .done((result) => { - if (result) { - node.setAttribute("lang", langName); - } else { - node.removeAttribute("lang"); // Or maybe set to "en" instead? - } - }); - } - } - }); - } - - //currently just a wrapper around the global, to be enhanced someday when we get rid of all the globals - public getToolIfOffered(toolId: string): ITool { - return getITool(toolId); - } - public isToolActive(toolId: string): boolean { const tools = $("*[data-toolId]"); const filteredTools = tools.filter(function () { @@ -590,45 +497,6 @@ export class ToolBox { return filteredTools.length > 0; } - // Ensure the requested tool is available in the toolbox accordion without changing the - // currently-active tool. This supports scenarios like clicking on a canvas background while - // another tool is open: we want to make Canvas available, but not steal focus. - public ensureToolEnabled(toolId: string): void { - const toolIdWithTool = ToolBox.addToolToString(toolId); - if (this.isToolActive(toolIdWithTool)) { - return; - } - const toolboxElt = $("#toolbox"); - const activeToolId = getActiveToolIdFromCurrentToolboxUi(); - beginAddTool(toolIdWithTool, false, () => { - const adapter = getToolboxReactAdapter(); - if (adapter) { - if (activeToolId) { - adapter.setActiveToolByToolId(activeToolId); - } - return; - } - - toolboxElt.accordion("refresh"); - if (activeToolId) { - const activeHeader = toolboxElt - .find("> h3") - .filter(function () { - return $(this).attr("data-toolId") === activeToolId; - }) - .first(); - if (activeHeader.length > 0) { - const activeIndex = toolboxElt - .find("> h3") - .index(activeHeader); - if (activeIndex >= 0) { - toolboxElt.accordion("option", "active", activeIndex); - } - } - } - }); - } - // Enables a tool from an in-page action, ensuring the toolbox is visible. public enableToolFromPage(toolId: string): void { if (!this.toolboxIsShowing()) { @@ -726,18 +594,6 @@ function getToolboxReactAdapter(): IToolboxReactAdapter | undefined { return adapter; } -function getActiveToolIdFromCurrentToolboxUi(): string | undefined { - const adapter = getToolboxReactAdapter(); - if (adapter) { - return adapter.getActiveToolId(); - } - - const activeHeader = $("#toolbox") - .find("> h3.ui-accordion-header-active") - .get(0) as HTMLElement | undefined; - return activeHeader?.getAttribute("data-toolId") || undefined; -} - // This primarily calls the detachFromPage method of the current tool, if any. // It also tries to find the current toolbox instance (in the right iframe, wherever it is called), // and runs any cleanup tasks that have been registered for when closing the tool. @@ -1127,7 +983,6 @@ function activateTool(newTool: ITool) { return; } // Always re-restore settings so tool state tracks the current book. - newTool.hasRestoredSettings = true; newTool .beginRestoreSettings(savedSettings as unknown as string) .then(() => { @@ -1168,10 +1023,7 @@ async function activateToolInternalAsync( `activateToolInternalAsync called for uninitialized tool: ${newTool.id()}`, ); } - newTool.finishToolLocalization(toolElt); - - // Await it so that we can guarantee that newPageReady() and insertLangAttributesIntoToolboxElements() - // happen after showTool. + // Await it so that we can guarantee that newPageReady() happens after showTool. await newTool.showTool(); postString("logger/writeEvent", `Toolbox activated: ${newTool.id()}`); @@ -1180,9 +1032,6 @@ async function activateToolInternalAsync( // (This apparently solves the single flash mentioned in BL-10471.) await newTool.newPageReady(); scheduleDelayedNewPageReady(newTool); - - // Note: Begins some async work too, but currently no need to await its result. - ToolBox.insertLangAttributesIntoToolboxElements(); } /** @@ -1861,34 +1710,6 @@ async function addFeatureStatusMessageTitlesToSubscriptionBadges( await Promise.all(promises); } -/** - * Adds one tool to the toolbox - * @param {String} newContent - * @param {String} toolId - * @param {Boolean} openTool - */ -function loadToolboxToolText( - newContent: string, - toolId: string, - openTool: boolean, -) { - const parts = $($.parseHTML(newContent, document, true)); - - parts.filter("*[data-i18n]").localize(); - parts.find("*[data-i18n]").localize(); - - // expect parts to have 2 items, an h3 and a div - if (parts.length < 2) return; - - // get the toolbox tool label - const header = parts.filter("h3").first(); - if (header.length < 1) return; // we used to have a tool that was empty and didn't get added. - - // get the tool content div - const content = parts.filter("div").first(); - - loadToolboxTool(header, content, toolId, openTool); -} function loadToolboxTool( header: JQuery, content: JQuery, diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts index 9fb1578f1e49..bfc971633c6c 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts @@ -148,7 +148,6 @@ const toolboxBundle: ToolboxBundleApi = { TalkingBookTool, canUndo, undo, - applyToolboxStateToPageLegacy: applyToolboxStateToPage, setActiveDragActivityTab, getTheOneAudioRecorderForExportOnly, copyLeveledReaderStatsToClipboard, diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts index a3c5234bf56d..7b87561f1215 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts @@ -17,7 +17,6 @@ declare global { } interface ToolboxApi { - getToolIfOffered?: (toolId: string) => ToolboxToolApi | undefined; getCurrentTool?: () => CurrentToolApi | undefined; } @@ -36,7 +35,6 @@ declare global { TalkingBookTool: unknown; canUndo: unknown; undo: unknown; - applyToolboxStateToPageLegacy: unknown; setActiveDragActivityTab: unknown; getTheOneAudioRecorderForExportOnly: unknown; copyLeveledReaderStatsToClipboard: unknown; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx b/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx index 5f282eda3943..97e375442745 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx @@ -12,7 +12,6 @@ export default abstract class ToolboxToolReactAdaptor imageUpdated(_img: HTMLImageElement | undefined): void { // does nothing by default } - public hasRestoredSettings: boolean; public abstract makeRootElement(): HTMLDivElement; public abstract id(): string; @@ -32,9 +31,6 @@ export default abstract class ToolboxToolReactAdaptor public isAlwaysEnabled(): boolean { return false; } - public isExperimental(): boolean { - return false; - } public featureName?: string; public beginRestoreSettings(_settings: string): JQueryPromise { @@ -59,7 +55,6 @@ export default abstract class ToolboxToolReactAdaptor public newPageReady() {} public detachFromPage() {} public configureElements(_container: HTMLElement) {} - public finishToolLocalization(_pane: HTMLElement) {} /* eslint-enable @typescript-eslint/no-empty-function */ public static getPageFrame(): HTMLIFrameElement { From f3de730ea757deb8defad374a5c6fdb60ef3ac91 Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Fri, 24 Jul 2026 15:47:44 -0700 Subject: [PATCH 02/19] Fix BL-16608 Cleanup toolbox infrastructure (2/7): collapse the React adapter bridge https://issues.bloomlibrary.org/youtrack/issue/BL-16608 ToolboxRoot (the React toolbox UI) used to publish window.toolboxReactAdapter, and legacy code in toolbox.ts checked for it at every call site, falling back to driving the old jQuery accordion when absent. Since ToolboxRoot is always rendered before ToolBox.initialize(), those fallbacks were unreachable. - Removed every no-adapter fallback branch in toolbox.ts: setCurrentTool's entire jQuery-accordion block (header index probing, accordion option calls, the accordionactivate handler), loadToolboxTool's accordion refresh/activate branch, resizeToolbox's accordion refresh, and showToolboxChanged's switchTool fallback. Dropped the now-unused jquery.onSafe import. - Replaced the window-global handshake with a small module, toolboxReactAdapter.ts, holding the IToolboxReactAdapter interface and set/get accessors (a separate module avoids an import cycle, since ToolboxRoot.tsx imports from toolbox.ts). Timing semantics preserved: the accessor returns undefined until ToolboxRoot mounts. - Dropped IToolboxReactAdapter.isEnabled() (always true; it existed only to gate the fallbacks) and getActiveToolId() (no callers). - Removed the Window.toolboxReactAdapter declaration from toolboxGlobals.d.ts. - The Playwright harness now exposes a test-only accessor (window.getToolboxReactAdapterForTests) since page.evaluate cannot import modules; the uitest uses it. The hidden legacy #toolbox DOM and its accordion initialization still exist; they go away in the next commit. Verified: pnpm typecheck passes, pnpm lint 0 errors, full vitest suite 550 passed / 5 skipped, build/agent-vite.sh production bundle compiles. The ToolboxRoot Playwright uitests could not be run: they fail on a pre-existing component-tester issue (React 17 pinned, but the harness imports react-dom/client) that predates this branch. Co-Authored-By: Claude Fable 5 --- .../bookEdit/toolbox/ToolboxRoot.tsx | 17 +- .../bookEdit/toolbox/toolbox.ts | 184 ++++-------------- .../bookEdit/toolbox/toolboxGlobals.d.ts | 8 - .../bookEdit/toolbox/toolboxReactAdapter.ts | 41 ++++ .../ToolboxRootTestHarness.tsx | 27 +++ .../toolbox-root-react.uitest.ts | 8 +- 6 files changed, 115 insertions(+), 170 deletions(-) create mode 100644 src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts diff --git a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx index 408c9c98537b..98219eda417d 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx @@ -16,6 +16,7 @@ import { } from "../../utils/colorUtils"; import { getMasterToolList } from "./toolbox"; import { kToolboxHeaderZIndex } from "./toolboxZIndexes"; +import { setToolboxReactAdapter } from "./toolboxReactAdapter"; import { SubscriptionBadgeWithTooltipAndDialog } from "../../react_components/requiresSubscription"; // React host for the toolbox sidebar. @@ -585,24 +586,16 @@ export const ToolboxRoot: React.FunctionComponent = () => { } } - // We expose this adapter on the window so legacy code which may not even be useing module syntax - // can use it. We can change it to an export when all tools are modules, and get rid of it when - // all tools are React components. - window.toolboxReactAdapter = { - isEnabled: () => true, + // Register the adapter that the legacy toolbox code uses to drive and observe + // which accordion section is active. See toolboxReactAdapter.ts. + setToolboxReactAdapter({ setActiveToolByToolId: (toolId: string) => { setActiveSection(normalizeToolId(toolId)); }, - getActiveToolId: () => { - if (!expandedSectionId) { - return undefined; - } - return toToolboxToolId(expandedSectionId); - }, onActiveToolChanged: (callback: (toolId: string) => void) => { activeToolChangedCallbacks.current.push(callback); }, - }; + }); }, [expandedSectionId, sections, setActiveSection]); // The old jQuery toolbox logic still runs for now, and it calls .show() on #toolbox. diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts index 57899d35e51e..485f070e20db 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -3,7 +3,6 @@ import $ from "jquery"; import "../../modified_libraries/jquery-ui/jquery-ui-1.10.3.custom.min.js"; import "../../lib/jquery.i18n.custom"; -import "../../lib/jquery.onSafe"; import axios from "axios"; import { get, postString, wrapAxios } from "../../utils/bloomApi"; import theOneLocalizationManager from "../../lib/localizationManager/localizationManager"; @@ -21,6 +20,7 @@ import { callOnBlur, setExtraFunctionToHandleBlurTasks, } from "../../utils/menuCloseOnBlur"; +import { getToolboxReactAdapter } from "./toolboxReactAdapter"; export { isLongPressEvaluating }; export { callOnBlur as registerMenuCloseOnBlur }; @@ -107,19 +107,6 @@ export interface IReactTool { featureName?: string; } -// The toolbox is progressively migrating to React. Recently, in toolboxRoot.tsx, we made -// the root of the whole toolbox a React component. The code here has not been fully -// integrated into the new approach, along with several tools that are not yet React. -// This interface, which is exported by the React component, allows the legacy code -// to interact with the React component, e.g., to set the active tool, -// or to be notified when the active tool changes. -interface IToolboxReactAdapter { - isEnabled(): boolean; - setActiveToolByToolId(toolId: string): void; - getActiveToolId(): string | undefined; - onActiveToolChanged(callback: (toolId: string) => void): void; -} - // Class that represents the whole toolbox. Gradually we will move more functionality in here. export class ToolBox { public toolboxIsShowing() { @@ -473,8 +460,8 @@ export class ToolBox { // loaded them all, now we can deal with settings. restoreToolboxSettings(); $("#toolbox").show(); - // I don't know why, but the accordion refresh inside resizeToolbox is needed - // to (at least) make the accordion icons appear, and it has to happen on a later cycle. + // resizeToolbox fits the toolbox root to the window; do it on a + // later cycle, once the toolbox has been laid out. setTimeout(resizeToolbox, 0); } else { // optimize: maybe we can overlap these? @@ -570,30 +557,6 @@ const masterToolList: ITool[] = []; let currentTool: ITool | undefined = undefined; let toolboxReactActivationHooked = false; -// The AI decided to create this react adapter object and save in in a window variable. -// It gets set in a useEffect in the React component that is the root of the toolbox. -// This function retrieves it. Once the toolbox has started up, it should always -// successfully return a valid adapter object. AI has built fallback code that tries to -// do various things in other ways when it is not available. Most of that fallback code -// is probably already redundant, but it's hard to be sure which. I'm inclined to leave -// it until we get all the tools migrated to React; then we can do a lot of simplification -// and probably get rid the adapter and fallbacks entirely; instead, each component -// will belong to its own accordion section and will be able to manage its own state -// and lifecycle. -function getToolboxReactAdapter(): IToolboxReactAdapter | undefined { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const adapter = (window as any).toolboxReactAdapter as - | IToolboxReactAdapter - | undefined; - if (!adapter) { - return undefined; - } - if (!adapter.isEnabled()) { - return undefined; - } - return adapter; -} - // This primarily calls the detachFromPage method of the current tool, if any. // It also tries to find the current toolbox instance (in the right iframe, wherever it is called), // and runs any cleanup tasks that have been registered for when closing the tool. @@ -1044,96 +1007,45 @@ function setCurrentTool(toolID: string) { toolID = ToolBox.addToolToString(toolID); const adapter = getToolboxReactAdapter(); - if (adapter) { - if (!toolboxReactActivationHooked) { - adapter.onActiveToolChanged((newToolName: string) => { - switchTool(newToolName); - }); - toolboxReactActivationHooked = true; - } + if (!adapter) { + // ToolboxRoot has not mounted yet, so there is no React accordion to activate + // anything in. We don't expect this: see getToolboxReactAdapter(). + return; + } - if (!toolID) { + if (!toolboxReactActivationHooked) { + adapter.onActiveToolChanged((newToolName: string) => { + switchTool(newToolName); + }); + toolboxReactActivationHooked = true; + } + + // NOTE: tools without a "data-toolId" attribute (such as the More tool) cannot be the "currentTool." + if (!toolID) { + toolID = + ($("#toolbox").find("> h3").first().attr("data-toolId") as + | string + | undefined) ?? ""; + } + + if (toolID) { + const tool = masterToolList.find( + (possibleTool) => + ToolBox.addToolToString(possibleTool.id()) === toolID, + ); + if (tool && !isToolInitialized(tool)) { + // The tool we were asked for isn't in the toolbox (e.g., it was disabled + // since we saved the setting), so fall back to whatever is first. toolID = ($("#toolbox").find("> h3").first().attr("data-toolId") as | string | undefined) ?? ""; } - - if (toolID) { - const tool = masterToolList.find( - (possibleTool) => - ToolBox.addToolToString(possibleTool.id()) === toolID, - ); - if (tool && !isToolInitialized(tool)) { - toolID = - ($("#toolbox").find("> h3").first().attr("data-toolId") as - | string - | undefined) ?? ""; - } - } - - if (toolID) { - adapter.setActiveToolByToolId(toolID); - } - return; } - // NOTE: tools without a "data-toolId" attribute (such as the More tool) cannot be the "currentTool." - let idx = 0; - const toolbox = $("#toolbox"); - - const accordionHeaders = toolbox.find("> h3"); if (toolID) { - let foundTool = false; - // find the index of the tool whose "data-toolId" attribute equals the value of "currentTool" - accordionHeaders.each(function () { - if ($(this).attr("data-toolId") === toolID) { - foundTool = true; - // break from the each() loop - return false; - } - idx++; - return true; // continue the each() loop - }); - if (!foundTool) { - idx = 0; - toolID = ""; - } - } - if (!toolID) { - // Leave idx at 0, and update currentTool to the corresponding ID. - toolID = toolbox.find("> h3").first().attr("data-toolId"); + adapter.setActiveToolByToolId(toolID); } - if (idx >= accordionHeaders.length - 1) { - // don't pick the More... tool, pick whatever happens to be first. - idx = 0; - } - - // turn off animation - const ani = toolbox.accordion("option", "animate"); - toolbox.accordion("option", "animate", false); - - // the index must be passed as an int, a string will not work. - toolbox.accordion("option", "active", idx); - - // turn animation back on - toolbox.accordion("option", "animate", ani); - - // when a tool is activated, save its data-toolId so state can be restored when Bloom is restarted. - // We do this after we actually set the initial tool, because setting the intial tool may not CHANGE - // the active tool (if it's already the one we want, typically the first), so we can't rely on - // the activate event happening in the initial call. Instead, we make SURE to call it for the - // tool we are making active. - toolbox.onSafe("accordionactivate.toolbox", (event, ui) => { - let newToolName = ""; - if (ui.newHeader.attr("data-toolId")) { - newToolName = ui.newHeader.attr("data-toolId").toString(); - } - switchTool(newToolName); - }); - //alert("switching to " + currentTool + " which has index " + toolIndex); - //setTimeout(e => switchTool(currentTool), 700); - switchTool(toolID); } // Parameter 'toolId' is the complete tool id with the 'Tool' suffix @@ -1173,10 +1085,7 @@ function beginAddTool( if (isToolInitialized(tool)) { if (openTool && toolbox.toolboxIsShowing()) { const toolName = ToolBox.addToolToString(tool.id()); - const adapter = getToolboxReactAdapter(); - if (adapter) { - adapter.setActiveToolByToolId(toolName); - } + getToolboxReactAdapter()?.setActiveToolByToolId(toolName); } if (whenLoaded) { @@ -1635,9 +1544,6 @@ function resizeToolbox() { // Set toolbox container height to fit in new window size // Then toolbox Resize() will adjust it to fit the container root.height(windowHeight - 25); // 25 is the top: value set for div.toolboxRoot in toolbox.less - if (!getToolboxReactAdapter()) { - $("#toolbox").accordion("refresh"); - } } /** @@ -1748,20 +1654,9 @@ function loadToolboxTool( // if requested, open the tool that was just inserted if (openTool && toolbox.toolboxIsShowing()) { - const adapter = getToolboxReactAdapter(); - if (adapter) { - const toolId = header.attr("data-toolId"); - if (toolId) { - adapter.setActiveToolByToolId(toolId); - } - } else { - toolboxElt.accordion("refresh"); - const id = header.attr("id"); - const toolNumber = parseInt( - id.substring(id.lastIndexOf("-") + 1), - 10, - ); - toolboxElt.accordion("option", "active", toolNumber); // must pass as integer + const insertedToolId = header.attr("data-toolId"); + if (insertedToolId) { + getToolboxReactAdapter()?.setActiveToolByToolId(insertedToolId); } } @@ -1804,11 +1699,6 @@ function showToolboxChanged(wasShowing: boolean): void { // the talking book tool. newToolName = "talkingBookTool"; } - const adapter = getToolboxReactAdapter(); - if (adapter) { - adapter.setActiveToolByToolId(newToolName); - return; - } - switchTool(newToolName); + getToolboxReactAdapter()?.setActiveToolByToolId(newToolName); } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts index 7b87561f1215..6001d30cc718 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts @@ -1,13 +1,6 @@ export {}; declare global { - interface ToolboxReactAdapterApi { - isEnabled: () => boolean; - setActiveToolByToolId: (toolId: string) => void; - getActiveToolId: () => string | undefined; - onActiveToolChanged: (callback: (toolId: string) => void) => void; - } - interface ToolboxToolApi { makeRootElement?: () => HTMLDivElement; } @@ -42,7 +35,6 @@ declare global { } interface Window { - toolboxReactAdapter?: ToolboxReactAdapterApi; toolboxBundle?: ToolboxBundleApi; } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts new file mode 100644 index 000000000000..dd96c20a420f --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts @@ -0,0 +1,41 @@ +// The root of the toolbox is a React component (ToolboxRoot.tsx), but a good deal of the +// toolbox is still the legacy, non-React code in toolbox.ts. This module is the single +// narrow channel between the two: ToolboxRoot registers an implementation of +// IToolboxReactAdapter when it mounts, and the legacy code uses it to make a tool active +// and to be notified when the user makes a different tool active. +// +// It lives in its own module (rather than being exported from ToolboxRoot.tsx) because +// ToolboxRoot.tsx imports from toolbox.ts, so having toolbox.ts import from ToolboxRoot.tsx +// would create an import cycle. +// +// When all the tools are React components, each one will belong to its own accordion +// section and manage its own state and lifecycle, and this module can go away. +export interface IToolboxReactAdapter { + // Makes the tool with this id (with or without the "Tool" suffix) the active, + // expanded section of the React accordion. + setActiveToolByToolId(toolId: string): void; + // Registers a callback to be told whenever the active tool changes, including + // as a result of setActiveToolByToolId(). + onActiveToolChanged(callback: (toolId: string) => void): void; +} + +let theOneToolboxReactAdapter: IToolboxReactAdapter | undefined; + +/** + * Called by ToolboxRoot once it has mounted (and again whenever the state it closes + * over changes), making the adapter available to the legacy toolbox code. + */ +export function setToolboxReactAdapter(adapter: IToolboxReactAdapter): void { + theOneToolboxReactAdapter = adapter; +} + +/** + * The adapter published by ToolboxRoot. Returns undefined only until ToolboxRoot has + * mounted; in practice that is before anything asks for it, since toolboxBootstrap + * renders ToolboxRoot before initializing the legacy toolbox, and the legacy code only + * asks for the adapter in response to a user action or an API response. Callers must + * still allow for undefined, but need not do anything useful in that case. + */ +export function getToolboxReactAdapter(): IToolboxReactAdapter | undefined { + return theOneToolboxReactAdapter; +} diff --git a/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx b/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx index a9ce390547f8..b0a2dab950b1 100644 --- a/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx +++ b/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx @@ -13,6 +13,24 @@ import { CanvasTool } from "../../bookEdit/toolbox/canvas/canvasTool"; import { GameTool } from "../../bookEdit/toolbox/games/GameTool"; import { SettingsTool } from "../../bookEdit/toolbox/settings/settingsTool"; +import { + getToolboxReactAdapter, + IToolboxReactAdapter, +} from "../../bookEdit/toolbox/toolboxReactAdapter"; +import { useMountEffect } from "../../utils/useMountEffect"; + +declare global { + interface Window { + // Test-only hook. The legacy toolbox code gets the adapter by importing + // getToolboxReactAdapter(), but our Playwright tests run inside the page, where + // they can't import a module, so this harness hands them the accessor. It is the + // accessor rather than the adapter itself because ToolboxRoot doesn't register an + // adapter until it has mounted. + getToolboxReactAdapterForTests?: () => IToolboxReactAdapter | undefined; + } +} + + // ToolboxRoot only renders a section for a tool that is in the master tool list, and tools // put themselves there by being registered. In the running app that happens as a side effect // of loading toolboxBootstrap. We deliberately do NOT import that module here: besides @@ -44,5 +62,14 @@ function registerToolsOnce() { registerToolsOnce(); export const ToolboxRootTestHarness: React.FunctionComponent = () => { + // Publishing the test hook is a side effect that has nothing to do with rendering, + // and it only needs to happen once, so a mount effect is the right home for it. + useMountEffect(() => { + window.getToolboxReactAdapterForTests = getToolboxReactAdapter; + return () => { + window.getToolboxReactAdapterForTests = undefined; + }; + }); + return ; }; diff --git a/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/component-tests/toolbox-root-react.uitest.ts b/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/component-tests/toolbox-root-react.uitest.ts index 6c11856cc86d..8c1aad8f838b 100644 --- a/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/component-tests/toolbox-root-react.uitest.ts +++ b/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/component-tests/toolbox-root-react.uitest.ts @@ -100,9 +100,11 @@ test.describe("ToolboxRoot React mode", () => { detail: { toolId: "decodableReaderTool" }, }), ); - window.toolboxReactAdapter?.setActiveToolByToolId( - "decodableReaderTool", - ); + // The harness publishes this accessor for us; see ToolboxRootTestHarness.tsx. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (window as any) + .getToolboxReactAdapterForTests?.() + ?.setActiveToolByToolId("decodableReaderTool"); }); await expect(getToolHeader(page, "Decodable Reader Tool")).toBeVisible({ From 747bcd8f44467772f985172d7f05b4698ec7c87a Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Fri, 24 Jul 2026 16:47:28 -0700 Subject: [PATCH 03/19] Fix BL-16608 Cleanup toolbox infrastructure (3/7): remove the jQuery accordion and hidden legacy #toolbox https://issues.bloomlibrary.org/youtrack/issue/BL-16608 The React toolbox (ToolboxRoot) used to adopt its tool bodies out of a hidden legacy jQuery-UI accordion that toolbox.ts still built and kept in sync. Now ToolboxRoot builds each tool body directly from the tool's makeRootElement(), and the hidden accordion is gone entirely. - ToolboxRoot.tsx: sections own their body element; deleted the adoption machinery (ensureReactToolBodyElement, LiveToolBodyHost adoption, getExistingToolboxContentElement, getLiveToolBodyElement), the hidden
, the force-hide MutationObserver, the 250ms hydrate poll, the toolbox-tool-added/-removed CustomEvent listeners, and the CSS hacks neutralizing leaked ui-accordion classes (BL-16501/BL-16538). - IToolboxReactAdapter grew the minimal surface the legacy code needs: addTool/removeTool/hasTool/getFirstToolId, plus isToolboxUiReady(). - toolbox.ts: beginAddTool now just announces the tool to React (the hand-built

header, subscription-badge helpers, loadToolboxTool, and the accordion init/localize are deleted); isToolInitialized, isToolActive, adjustToolListForPage, activateToolFromId, setCurrentTool, and showToolboxChanged all run off adapter state; removed the jquery-ui and jquery.i18n.custom imports and resizeToolbox (the root is sized by CSS). - readerToolsModel.saveState/restoreState: replaced the "does the accordion widget exist" guards with isToolboxUiReady(), preserving reader stage and level persistence; removed both accordion("refresh") calls in readerTools.ts. - audioRecording.ts: the busy-cursor element list targeted the hidden #toolbox (so the cursor never showed); now targets .toolboxRoot. - Styles: deleted the ui-accordion/ui-widget rules from toolbox.less and the six per-tool less files, and the jquery-ui theme link from both toolbox pugs. (The .ui-dialog z-index rule stays for the readerSetup dialog.) - toolboxGlobals.d.ts: dropped types that existed only for the removed window.toolboxBundle reach-through. Verified: pnpm typecheck passes (raw tsc error count unchanged at the pre-existing 61, none in touched files), eslint 0 errors, full vitest suite 550 passed / 5 skipped, build/agent-vite.sh bundle compiles. Live smoke test in Bloom over CDP: toolbox renders and fills the window, enabling/disabling Decodable Reader via More... adds/removes the section alphabetically, tool switching works, the decodable stage UI renders, reader state persistence posts fire (state\tdecodableReader\tstage:3, state\tleveledReader\t1), and no page errors occurred. Co-Authored-By: Claude Fable 5 --- .../bookEdit/toolbox/ToolboxRoot.tsx | 637 +++++------------- .../bookEdit/toolbox/canvas/canvasTool.less | 6 - .../imageDescription/imageDescription.less | 5 - .../impairmentVisualizer.less | 7 - .../bookEdit/toolbox/motion/motion.less | 4 - .../bookEdit/toolbox/music/music.less | 4 - .../bookEdit/toolbox/readers/readerTools.ts | 10 +- .../toolbox/readers/readerToolsModel.ts | 23 +- .../toolbox/signLanguage/signLanguage.less | 4 - .../toolbox/talkingBook/audioRecording.ts | 2 +- .../bookEdit/toolbox/toolbox.less | 118 +--- .../bookEdit/toolbox/toolbox.pug | 1 - .../bookEdit/toolbox/toolbox.ts | 434 +++--------- .../bookEdit/toolbox/toolbox.vite-dev.pug | 1 - .../bookEdit/toolbox/toolboxGlobals.d.ts | 17 +- .../bookEdit/toolbox/toolboxReactAdapter.ts | 37 +- .../toolbox-root-react.uitest.ts | 41 +- 17 files changed, 308 insertions(+), 1043 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx index 98219eda417d..a4cd1ff90298 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx @@ -19,37 +19,26 @@ import { kToolboxHeaderZIndex } from "./toolboxZIndexes"; import { setToolboxReactAdapter } from "./toolboxReactAdapter"; import { SubscriptionBadgeWithTooltipAndDialog } from "../../react_components/requiresSubscription"; -// React host for the toolbox sidebar. +// React host for the toolbox sidebar. It owns which tools the toolbox is offering, which +// one is expanded, and the DOM node that each tool renders itself into. // -// This component bridges old and new toolbox systems while migration is in progress. It was generated by Copilot -// to allow the root of the toolbox to be rendered in React while still supporting several tools that have not yet -// been migrated to React. The goal is to eventually remove all legacy toolbox code and have a fully React-managed toolbox. -// The descriptions and plans below were also generated by Copilot and may or may not be helpful. -// - it renders accordion sections in React, -// - reuses already-instantiated legacy tool DOM when available, -// - and exposes a small adapter so legacy code can still control active tool state. -// -// Completed migration target: -// - React is the single owner of toolbox state and rendering, -// - each tool is rendered through a React component/factory (no legacy HTML injection), -// - no DOM adoption from legacy #toolbox roots, -// - no hydration polling or legacy add/remove bridge events, -// - and toolbox globals are reduced to a minimal compatibility surface (or removed). -// -// Practical completion checklist for this file: -// - remove existing legacy element adoption paths, -// - replace legacy event synchronization with direct React/store updates, -// - remove hidden legacy #toolbox root handling, -// - then simplify comments to describe a fully React-managed toolbox. +// Each tool still hands us a plain DOM element (from its ITool.makeRootElement()) rather +// than a React component, so a small host component (ToolBodyHost) puts that element into +// the React layout. When every tool is a React component, each section can render its +// tool directly and both that host and toolboxReactAdapter.ts can go away. type ToolboxSection = { + // The tool's id without the historical "Tool" suffix, e.g. "canvas". id: string; englishLabel: string; l10nKey: string; - liveToolBodyElement?: HTMLDivElement; + // The element the tool renders itself into. Created once, when the section is created. + toolBodyElement?: HTMLDivElement; }; -const alwaysOnToolIds: string[] = ["talkingBook"]; +// Tools the toolbox offers whether or not the enabledTools API mentions them. +// "settings" is the "More..." section, which is how the user enables the others. +const alwaysOnToolIds: string[] = ["talkingBook", "settings"]; const subscriptionToolIds = new Set(["canvas", "motion", "music"]); @@ -92,7 +81,7 @@ const normalizeToolId = (toolId: string): string => { }; // Convert normalized IDs back to the toolbox's traditional "*Tool" names when -// we need to notify legacy listeners. +// we need to notify the legacy toolbox code. const toToolboxToolId = (toolId: string): string => { if (!toolId) { return toolId; @@ -130,24 +119,35 @@ const getToolboxLabelInfo = ( }; }; -const sortToolIdsAlphabetically = (toolIds: string[]): string[] => { - return [...toolIds].sort((a, b) => - getToolboxLabelInfo(a).englishLabel.localeCompare( - getToolboxLabelInfo(b).englishLabel, - undefined, - { - sensitivity: "base", - }, - ), +// Ask the tool for the element it renders itself into. Returns undefined if we don't know +// about the tool at all, which can happen if settings were saved by a later version of Bloom. +const makeToolBodyElement = ( + normalizedToolId: string, +): HTMLDivElement | undefined => { + const tool = getMasterToolList().find( + (candidate) => candidate.id() === normalizedToolId, + ); + if (!tool) { + return undefined; + } + + const toolBodyElement = tool.makeRootElement(); + // Some tool stylesheets still select their body by this attribute. + toolBodyElement.setAttribute( + "data-toolid", + toToolboxToolId(normalizedToolId), ); + return toolBodyElement; }; const makeSectionFromToolId = (toolId: string): ToolboxSection => { - const labelInfo = getToolboxLabelInfo(toolId); + const normalizedToolId = normalizeToolId(toolId); + const labelInfo = getToolboxLabelInfo(normalizedToolId); return { - id: toolId, + id: normalizedToolId, englishLabel: labelInfo.englishLabel, l10nKey: labelInfo.l10nKey, + toolBodyElement: makeToolBodyElement(normalizedToolId), }; }; @@ -184,151 +184,20 @@ const parseEnabledToolIds = (value: string): string[] => { return Array.from(toolIds); }; -const buildSectionsFromEnabledToolIds = ( - enabledToolIds: string[], -): ToolboxSection[] => { - const withMoreTabLast = sortToolIdsAlphabetically( - enabledToolIds.filter((id) => id !== "settings"), - ); - withMoreTabLast.push("settings"); - - return sortSectionsAlphabeticallyWithSettingsLast( - withMoreTabLast.map((toolId) => makeSectionFromToolId(toolId)), - ); -}; - -// Locate an existing legacy tool body element, checking both old and new data -// attribute spellings and both normalized/"*Tool" ID variants. -const getExistingToolboxContentElement = ( - toolId: string, -): HTMLDivElement | undefined => { - const normalizedToolId = normalizeToolId(toolId); - const candidateToolIds = new Set([ - toolId, - normalizedToolId, - toToolboxToolId(toolId), - toToolboxToolId(normalizedToolId), - ]); - - for (const candidateToolId of candidateToolIds) { - const contentElement = document.querySelector( - `#toolbox > div[data-toolid='${candidateToolId}'], #toolbox > div[data-toolId='${candidateToolId}']`, - ) as HTMLDivElement | null; - if (contentElement) { - return contentElement; - } - } - - return undefined; -}; - -const getLiveToolBodyElement = (toolId: string): HTMLDivElement | undefined => { - // Use the element created by legacy beginAddTool() so we don't instantiate - // a second React tool root and desynchronize tool state. - return getExistingToolboxContentElement(toolId); -}; - -// This is part of the mess that Copilot made to allow the root of the toolbox to be -// rendered in React while still supporting several tools that have not yet been -// migrated to React. It was one of my first big refactors using CoPilot, and I should -// not have been so quick to just be glad it seemed to work. For some reason, its -// solution was to maintain an old jquery accordion in parallel with a new React -// implementation. Somehow, across these two versions of the toolbox that exist in -// parallel, a React tool can think it has a root element to render into, but it -// doesn't actually exist. Then we get stuck in a state where the tool's content -// is permanently replaced by a loading message. This function detects this -// situation and adjusts things so that the root element for the tool content -// gets created when needed. -// I don't think there's any good reason not to use the normal function declaration -// syntax except that Copilot seems to have done everything else in this file this way. -// I don't want to put a lot of effort into cleaning it up because I hope sometime soon -// we rework the remaining few non-React tools and can then get rid of the duplication -// and manage everything in normal React. I expect most of the code in this file to -// go away. -const ensureReactToolBodyElement = ( - toolId: string, -): HTMLDivElement | undefined => { - const existing = getExistingToolboxContentElement(toolId); - if (existing) { - return existing; - } - - const normalizedToolId = normalizeToolId(toolId); - - const tool = getMasterToolList().find((candidate) => { - return candidate.id() === normalizedToolId; - }); - if (!tool || !tool.makeRootElement) { - return undefined; - } - - const legacyToolboxRoot = document.getElementById("toolbox"); - if (!legacyToolboxRoot) { - return undefined; - } - - const fullToolId = toToolboxToolId(normalizedToolId); - const content = tool.makeRootElement(); - content.setAttribute("data-toolId", fullToolId); - - const existingHeader = Array.from(legacyToolboxRoot.children).find( - (child) => { - return ( - child.tagName.toLowerCase() === "h3" && - child.getAttribute("data-toolId") === fullToolId - ); - }, - ) as HTMLElement | undefined; - if (!existingHeader) { - const header = document.createElement("h3"); - header.setAttribute("data-toolId", fullToolId); - legacyToolboxRoot.appendChild(header); - } - - legacyToolboxRoot.appendChild(content); - return content; -}; - -const getLegacyCurrentToolId = (): string | undefined => { - const toolbox = window.toolboxBundle?.getTheOneToolbox?.(); - const currentTool = toolbox?.getCurrentTool?.(); - const currentToolId = currentTool?.id?.(); - if (!currentToolId) { - return undefined; - } - - return normalizeToolId(currentToolId); -}; - -// Hosts a legacy-owned DOM subtree inside React layout while preserving the -// original element instance (so tool state and event wiring stay intact). -const LiveToolBodyHost: React.FunctionComponent<{ element: HTMLDivElement }> = ( +// Puts a tool's own DOM element (the one it renders itself into) into the React layout, +// keeping the original element instance so the tool's state and event wiring stay intact. +const ToolBodyHost: React.FunctionComponent<{ element: HTMLDivElement }> = ( props, ) => { const hostRef = React.useRef(null); - const clearLegacyAccordionSizing = React.useCallback( - (element: HTMLDivElement) => { - element.style.height = "100%"; - element.style.width = "100%"; - element.style.minWidth = "0"; - element.style.flex = "1 1 auto"; - element.style.overflow = ""; - element.style.display = "block"; - }, - [], - ); - React.useEffect(() => { const host = hostRef.current; if (!host) { return; } - clearLegacyAccordionSizing(props.element); - if (!host.contains(props.element)) { - host.innerHTML = ""; host.appendChild(props.element); } @@ -337,7 +206,7 @@ const LiveToolBodyHost: React.FunctionComponent<{ element: HTMLDivElement }> = ( host.removeChild(props.element); } }; - }, [props.element, clearLegacyAccordionSizing]); + }, [props.element]); return (
= ( align-items: stretch; min-height: 0; min-width: 0; + + // Tools expect their root element to fill the space the toolbox gives + // them; several of them then use height:100% internally to push a Help + // link to the bottom. + > * { + width: 100%; + height: 100%; + min-width: 0; + flex: 1 1 auto; + display: block; + } `} >
); }; // This component is the root of the whole toolbox sidebar. It is rendered into a dedicated -// host element created by the edit-frame page pug. +// host element created by the toolbox page pug. export const ToolboxRoot: React.FunctionComponent = () => { const [sections, setSections] = React.useState([]); const [expandedSectionId, setExpandedSectionId] = React.useState(); const activeToolChangedCallbacks = React.useRef< ((toolId: string) => void)[] >([]); - const hydratedToolIds = React.useRef>(new Set()); - - // Expand the given section and tell the legacy toolbox code about it. - // The legacy code keeps its own idea of which tool is current and drives each tool's - // showTool()/hideTool() lifecycle from it, so every path that changes which section is - // expanded has to go through here. A path that quietly changed only the React state left - // the two out of sync, so the tool the user could see was never activated: that is how - // visiting a game page killed Talking Book highlighting (BL-16602). - // Passing undefined means "nothing is expanded"; we deliberately don't notify legacy in - // that case, since it has no representation for "no current tool" and re-expanding the - // same section will notify with the same tool anyway. - const setActiveSection = React.useCallback( - (sectionId: string | undefined) => { - setExpandedSectionId(sectionId); - if (!sectionId) { - return; - } - const toolId = toToolboxToolId(sectionId); - activeToolChangedCallbacks.current.forEach((callback) => { - callback(toolId); - }); + // The authoritative copy of the sections, so that the adapter methods the legacy + // toolbox code calls can read and update the list synchronously. (React state is + // updated from it, for rendering.) + const sectionsRef = React.useRef([]); + + const applySections = React.useCallback( + (nextSections: ToolboxSection[]) => { + sectionsRef.current = nextSections; + setSections(nextSections); }, [], ); - // Resolve body content for a section. - // Prefer a live legacy element to avoid duplicate tool roots; otherwise - // ask the tool to make its React root. - const hydrateToolBody = React.useCallback(async (toolId: string) => { - if (hydratedToolIds.current.has(toolId)) { - return; - } - - hydratedToolIds.current.add(toolId); - - const liveToolBodyElement = getLiveToolBodyElement(toolId); - if (liveToolBodyElement) { - // Adopt the current live node from #toolbox so we don't create a duplicate - // root for the same legacy tool. - setSections((previousSections) => - previousSections.map((section) => { - if (section.id !== toolId) { - return section; - } - return { - ...section, - liveToolBodyElement, - }; - }), - ); - return; - } - - const createdReactToolBodyElement = ensureReactToolBodyElement(toolId); - if (createdReactToolBodyElement) { - setSections((previousSections) => - previousSections.map((section) => { - if (section.id !== toolId) { - return section; - } - return { - ...section, - liveToolBodyElement: createdReactToolBodyElement, - }; - }), - ); - return; - } - - hydratedToolIds.current.delete(toolId); + const makeToolActive = React.useCallback((normalizedToolId: string) => { + setExpandedSectionId(normalizedToolId); + const toolboxToolId = toToolboxToolId(normalizedToolId); + activeToolChangedCallbacks.current.forEach((callback) => { + callback(toolboxToolId); + }); }, []); - // Load enabled toolbox tools and begin hydrating each section body. + // Load the tools the toolbox should offer. (The legacy toolbox code independently + // announces the same tools through addTool(); whichever gets there first wins, and + // the other is a no-op.) React.useEffect(() => { axios .get("/bloom/api/toolbox/enabledTools") - .then(async (response) => { + .then((response) => { const parsedIds = parseEnabledToolIds(response.data); const masterList = getMasterToolList(); - for (let i = parsedIds.length - 1; i >= 0; i--) { - const toolId = parsedIds[i]; - if (!masterList.some((tool) => tool.id() === toolId)) { - parsedIds.splice(i, 1); - } - } - const builtSections = - buildSectionsFromEnabledToolIds(parsedIds); - setSections(builtSections); + const knownIds = parsedIds.filter((toolId) => + masterList.some((tool) => tool.id() === toolId), + ); + const existingIds = new Set( + sectionsRef.current.map((section) => section.id), + ); + const newSections = knownIds + .filter((toolId) => !existingIds.has(toolId)) + .map((toolId) => makeSectionFromToolId(toolId)); + applySections( + sortSectionsAlphabeticallyWithSettingsLast([ + ...sectionsRef.current, + ...newSections, + ]), + ); }) .catch((error) => { throw error; }); - }, [hydrateToolBody]); + }, [applySections]); - // Start hydration only after section state exists to avoid races where a tool - // can be marked hydrated before there is any section to receive its body. + // Register the adapter that the legacy toolbox code uses to say which tools the + // toolbox offers, to make one of them active, and to observe which one is active. + // See toolboxReactAdapter.ts. React.useEffect(() => { - sections.forEach((section) => { - if ( - section.liveToolBodyElement || - hydratedToolIds.current.has(section.id) - ) { - return; - } - - void hydrateToolBody(section.id); - }); - }, [sections, hydrateToolBody]); - - // Keep unresolved sections hydrated over time because legacy tool roots are - // not always available immediately when sections are first created (or recreated), - // and there may be some possibility that they get removed by non-react code. - // I don't fully understand why this is necessary, but in the early stages of - // developing this wrapper, it happened quite often that a tool was opened and - // nothing was there. This polling is defensive attempt to prevent that. - // Once everything is fully in React, this should be unnecessary and can be removed. - React.useEffect(() => { - const intervalId = window.setInterval(() => { - sections.forEach((section) => { - const hasConnectedLiveToolBodyElement = - !!section.liveToolBodyElement && - section.liveToolBodyElement.isConnected; - - if (hasConnectedLiveToolBodyElement) { - return; - } - - if ( - section.id === expandedSectionId && - hydratedToolIds.current.has(section.id) - ) { - // Forget that we hydrated the tool the user is looking at, so that - // hydrateToolBody() will actually try again to get it a body element. - hydratedToolIds.current.delete(section.id); - } - - void hydrateToolBody(section.id); - }); - }, 250); - - return () => { - window.clearInterval(intervalId); - }; - }, [sections, hydrateToolBody, expandedSectionId]); - - // Synchronize toolbox add/remove events from legacy code into React section - // state so both systems reflect the same offered tools. - React.useEffect(() => { - const onToolAdded = (event: Event) => { - const customEvent = event as CustomEvent<{ toolId: string }>; - const addedToolId = normalizeToolId(customEvent.detail.toolId); - - setSections((previousSections) => { - if ( - previousSections.some( - (section) => section.id === addedToolId, - ) - ) { - return previousSections; - } - - return sortSectionsAlphabeticallyWithSettingsLast([ - ...previousSections, - makeSectionFromToolId(addedToolId), - ]); - }); - }; - - const onToolRemoved = (event: Event) => { - const customEvent = event as CustomEvent<{ toolId: string }>; - const removedToolId = normalizeToolId(customEvent.detail.toolId); - hydratedToolIds.current.delete(removedToolId); - - setSections((previousSections) => - previousSections.filter( - (section) => section.id !== removedToolId, - ), - ); - - if (expandedSectionId !== removedToolId) { - // The tool that went away wasn't the active one, so the active tool is unaffected. - return; - } - - // The active tool has just been taken away from us (e.g. leaving a game page removes - // the Game tool, which that page required). Something else has to become active, and - // it must go through setActiveSection so that the legacy code actually activates it. - // (Before the React toolbox, the jQuery accordion's refresh did the equivalent, firing - // its activate event when the active panel disappeared.) - const replacementSection = sections.find( - (section) => section.id !== removedToolId, - ); - setActiveSection(replacementSection?.id); - }; - - // These events get dispatched by legacy toolbox code when tools are added or removed - // from the legacy #toolbox container. - window.addEventListener("toolbox-tool-added", onToolAdded); - window.addEventListener("toolbox-tool-removed", onToolRemoved); - - return () => { - window.removeEventListener("toolbox-tool-added", onToolAdded); - window.removeEventListener("toolbox-tool-removed", onToolRemoved); - }; - // sections and expandedSectionId are dependencies because onToolRemoved has to know - // what is currently active and what is left to activate in its place. Re-subscribing - // when they change can't lose an event: the removal and re-adding of the listener - // happen together, synchronously, when React commits. - }, [hydrateToolBody, sections, expandedSectionId, setActiveSection]); - - // Expose activation adapter so legacy toolbox code can drive and observe React accordion state. - React.useEffect(() => { - if (!expandedSectionId) { - const legacyCurrentToolId = getLegacyCurrentToolId(); - if ( - legacyCurrentToolId && - sections.some((section) => section.id === legacyCurrentToolId) - ) { - setExpandedSectionId(legacyCurrentToolId); - } - } - - // Register the adapter that the legacy toolbox code uses to drive and observe - // which accordion section is active. See toolboxReactAdapter.ts. setToolboxReactAdapter({ setActiveToolByToolId: (toolId: string) => { - setActiveSection(normalizeToolId(toolId)); + makeToolActive(normalizeToolId(toolId)); }, onActiveToolChanged: (callback: (toolId: string) => void) => { activeToolChangedCallbacks.current.push(callback); }, + addTool: (toolId: string) => { + const normalizedToolId = normalizeToolId(toolId); + if ( + sectionsRef.current.some( + (section) => section.id === normalizedToolId, + ) + ) { + return; + } + applySections( + sortSectionsAlphabeticallyWithSettingsLast([ + ...sectionsRef.current, + makeSectionFromToolId(normalizedToolId), + ]), + ); + }, + removeTool: (toolId: string) => { + const normalizedToolId = normalizeToolId(toolId); + const remainingSections = sectionsRef.current.filter( + (section) => section.id !== normalizedToolId, + ); + if (remainingSections.length === sectionsRef.current.length) { + return; + } + applySections(remainingSections); + // Only change which section is expanded if we just removed the expanded + // one. The awkward functional update guards against a stale value of + // expandedSectionId. + setExpandedSectionId((previousExpandedSectionId) => + previousExpandedSectionId === normalizedToolId + ? remainingSections[0]?.id + : previousExpandedSectionId, + ); + }, + hasTool: (toolId: string) => { + const normalizedToolId = normalizeToolId(toolId); + return sectionsRef.current.some( + (section) => section.id === normalizedToolId, + ); + }, + getFirstToolId: () => { + const firstToolSection = sectionsRef.current.find( + (section) => section.id !== "settings", + ); + return firstToolSection + ? toToolboxToolId(firstToolSection.id) + : undefined; + }, }); - }, [expandedSectionId, sections, setActiveSection]); - - // The old jQuery toolbox logic still runs for now, and it calls .show() on #toolbox. - // Keep that legacy root hidden so only the React root is visible. - // (For now quite a lot of legacy code still uses the #toolbox root; it's even where - // we get some of the tools which we move into the React accordion. So we can't just - // remove it yet, though that's the long-term plan. For now just hide it.) - React.useEffect(() => { - const legacyToolboxElement = document.getElementById("toolbox"); - if (!legacyToolboxElement) { - return; - } - - const forceHideLegacyToolbox = () => { - legacyToolboxElement.style.setProperty( - "display", - "none", - "important", - ); - }; - - forceHideLegacyToolbox(); - - const observer = new MutationObserver(() => { - forceHideLegacyToolbox(); - }); - observer.observe(legacyToolboxElement, { - attributes: true, - attributeFilter: ["style", "class"], - }); - - return () => { - observer.disconnect(); - }; - }, []); + }, [applySections, makeToolActive]); return (
{ `} disableGutters expanded={expandedSectionId === section.id} - // MUI reports the state the accordion is heading TO, not the state - // it is in: it calls onChange(event, !expanded). So clicking a - // closed tool's header arrives here as true, and clicking the open - // one's arrives as false. (MUI's own name for the parameter is just - // "expanded", which reads like "this one is the active tool" and is - // the opposite of what it means -- hence the name used here.) - onChange={(_event, willBeExpanded) => { - if (!willBeExpanded) { - // Clicking the open tool's header can't close it: the - // toolbox always has an active tool, and the effect that - // syncs us with the legacy toolbox re-expands whatever - // legacy still thinks is current. Honoring the collapse - // therefore only produced a flash, in which the tools - // below jumped up and back down. (BL-16533) - return; + onChange={(_event, expanded) => { + if (expanded) { + makeToolActive(section.id); + } else { + setExpandedSectionId(undefined); } - setActiveSection(section.id); }} > { min-height: 100%; overflow: visible; + // The Decodable and Leveled reader tool bodies + // were laid out to suit the small left padding + // that the old jQuery-UI accordion content panels + // gave them, so keep that. div[data-toolid="leveledReaderTool"], - div[data-toolId="decodableReaderTool"] { - width: 100% !important; - min-width: 0; - align-self: stretch !important; - display: block !important; + div[data-toolid="decodableReaderTool"] { + padding-left: 3px; box-sizing: border-box; - margin-right: 0 !important; - padding-right: 0 !important; - } - - // The Decodable/Leveled reader tool bodies are - // adopted from the old jQuery-UI accordion and - // still wear its header classes (ui-accordion-header, - // ui-state-default) plus jQuery hover/focus handlers - // that add ui-state-hover/-focus. Here they are tool - // *bodies*, not headers, so that theming is wrong: left - // alone, hovering an opened reader tool paints its whole - // body the jQuery-UI "hover" lightcoral (#f08080). - // Neutralize the stray header theming. (BL-16501) - div[data-toolid].ui-accordion-header { - &, - &.ui-state-hover, - &.ui-state-focus, - &.ui-state-active { - background: transparent !important; - border: none !important; - font-weight: normal; - } - } - - // Those same adopted bodies also keep the - // header icon jQuery-UI's _createIcons() - // prepends to every header: a - // ui-icon-triangle-1-e sprite. As a body - // (not a header) it renders as a stray - // right-pointing arrowhead in the top-left - // corner. The rule above only neutralized the - // header background/border, not this child - // icon, so hide it too. (BL-16538) - div[data-toolid] - span.ui-accordion-header-icon { - display: none !important; } `} > - {section.liveToolBodyElement ? ( - ) : ( @@ -887,19 +564,13 @@ export const ToolboxRoot: React.FunctionComponent = () => { ))}
-

); }; export const renderToolboxRoot = (): void => { // Bootstraps the React toolbox into the dedicated host element created by - // the edit-frame page markup. + // the toolbox page markup. const hostElement = document.getElementById("toolbox-react-root"); if (!hostElement) { return; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.less b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.less index 0e39c662a84a..f526a4083dce 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.less @@ -3,12 +3,6 @@ @SideMargin: 9px; @ControlVerticalSpacing: 10px; -.ui-accordion h3[data-toolId="canvasTool"] { - span.ui-accordion-header-icon { - background-image: url("/bloom/bookEdit/toolbox/canvas/Canvas Icon.svg") !important; - } -} - #canvasToolControls { display: flex; flex-direction: column; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.less b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.less index e3fedb0d55b1..067c3aab6956 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.less @@ -1,10 +1,5 @@ @import "../../../bloomUI.less"; -.ui-accordion - h3[data-toolId="imageDescriptionTool"] - span.ui-accordion-header-icon { - background-image: url("/bloom/bookEdit/toolbox/imageDescription/ImageDescriptionToolIcon.svg") !important; -} .imageDescriptionTool { display: flex; flex-direction: column; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.less b/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.less index 18383f5e8a29..1b805f274899 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.less @@ -9,10 +9,3 @@ flex-direction: column; justify-content: space-between; } - -// from https://commons.wikimedia.org/wiki/File:Q10874_noun_3918_ccJordanDelcros_blind.svg? -.ui-accordion - h3[data-toolId="impairmentVisualizer"] - span.ui-accordion-header-icon { - background-image: url("/bloom/bookEdit/toolbox/impairmentVisualizer/blind-eye-white.svg") !important; -} diff --git a/src/BloomBrowserUI/bookEdit/toolbox/motion/motion.less b/src/BloomBrowserUI/bookEdit/toolbox/motion/motion.less index 765031805915..0e37cc6b5ee7 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/motion/motion.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/motion/motion.less @@ -63,7 +63,3 @@ } } } - -.ui-accordion h3[data-toolId="motionTool"] span.ui-accordion-header-icon { - background-image: url("/bloom/bookEdit/toolbox/motion/motion.svg") !important; -} diff --git a/src/BloomBrowserUI/bookEdit/toolbox/music/music.less b/src/BloomBrowserUI/bookEdit/toolbox/music/music.less index 5c427e21ca8c..ec118db46250 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/music/music.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/music/music.less @@ -92,7 +92,3 @@ margin-top: 9px; } } - -.ui-accordion h3[data-toolId="musicTool"] span.ui-accordion-header-icon { - background-image: url("/bloom/bookEdit/toolbox/music/music-notes-white.svg") !important; -} diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/readerTools.ts b/src/BloomBrowserUI/bookEdit/toolbox/readers/readerTools.ts index 6890e39f84ba..d362b1f62306 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/readers/readerTools.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/readerTools.ts @@ -186,19 +186,25 @@ function processDLRMessage(event: MessageEvent): void { } } +/** + * Loads the Synphony (reader) settings and updates the Decodable Reader tool's controls + * to match. Returns a promise that resolves when that is done. + */ export function beginInitializeDecodableReaderTool(): JQueryPromise { // load synphony settings and then finish init return beginLoadSynphonySettings().then(() => { getTheOneReaderToolsModel().updateControlContents(); - $("#toolbox").accordion("refresh"); }); } +/** + * Loads the Synphony (reader) settings and updates the Leveled Reader tool's controls + * to match. Returns a promise that resolves when that is done. + */ export function beginInitializeLeveledReaderTool(): JQueryPromise { // load synphony settings return beginLoadSynphonySettings().then(() => { getTheOneReaderToolsModel().updateControlContents(); - $("#toolbox").accordion("refresh"); }); } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolsModel.ts b/src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolsModel.ts index 7eba3a8c50c1..216016963d02 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolsModel.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolsModel.ts @@ -38,6 +38,7 @@ import { allPromiseSettled, setTimeoutPromise, } from "../../../utils/asyncUtils"; +import { isToolboxUiReady } from "../toolboxReactAdapter"; const SortType = { alphabetic: "alphabetic", @@ -1532,14 +1533,14 @@ export class ReaderToolsModel { return dataWords; } + /** + * Persists the decodable-reader stage/sort and the leveled-reader level in the book. + * Does nothing until the toolbox UI exists: before that (and in unit tests, where it + * never does) there is no user-chosen state worth saving, and saving would overwrite + * the book's real settings with defaults. + */ public saveState(): void { - // this is needed for unit testing - const toolbox = $("#toolbox"); - if (typeof toolbox.accordion !== "function") return; - - // this is also needed for unit testing - const active = toolbox.accordion("option", "active"); - if (isNaN(active)) return; + if (!isToolboxUiReady()) return; postString( "editView/saveToolboxSetting", @@ -1555,10 +1556,12 @@ export class ReaderToolsModel { ); } + /** + * Restores the stage/level the book was last using. Like saveState(), does nothing + * until the toolbox UI exists (in particular, in unit tests). + */ public restoreState(): void { - // this is needed for unit testing - const toolbox = $("#toolbox"); - if (typeof toolbox.accordion !== "function") return; + if (!isToolboxUiReady()) return; const state = new DRTState(); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguage.less b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguage.less index e203e9c829f1..c67b84b08df2 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguage.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguage.less @@ -156,10 +156,6 @@ } } -.ui-accordion h3[data-toolId="signLanguageTool"] span.ui-accordion-header-icon { - background-image: url("/bloom/bookEdit/toolbox/signLanguage/signLanguageTool.svg") !important; -} - #videoImport, #showInFolder, #videoDelete { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts index 16af85fca4b6..76c869cf7943 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts @@ -4650,7 +4650,7 @@ export default class AudioRecording implements IAudioRecorder { private getElementsToUpdateForCursor(): (Element | null)[] { const elementsToUpdate: (Element | null)[] = []; - elementsToUpdate.push(document.getElementById("toolbox")); + elementsToUpdate.push(document.querySelector(".toolboxRoot")); const pageBody = this.getPageDocBody(); if (pageBody) { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.less b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.less index fbfc2981eec9..5c73f7bacf56 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.less @@ -11,12 +11,6 @@ @helpLinkTopMargin: 45px; @bloomEnterpriseBadgeSize: 20px; -// The Games tool wants a white bar at the very top, so defeat the usual padding -.ui-accordion .ui-accordion-content[data-toolid="dragActivityTool"] { - padding-top: 0; - padding-left: 1px !important; -} - div.toolboxRoot { display: inline-block; background-color: @toolboxBackgroundColor; @@ -33,12 +27,6 @@ div.toolboxRoot { width: @toolboxWidth; z-index: @toolboxZIndex; - .ui-widget-content { - background: none; // clear out the backgroudn image jquery-ui adds - background-color: @toolboxBackgroundColor; - border: none; - } - // multiple tools add a Help link after the rest of the tool content .helpLinkWrapper { margin-top: @helpLinkTopMargin; // put some space before the Help link @@ -74,42 +62,18 @@ div.toolboxRoot { } } -.ui-accordion-header { - -moz-user-select: none; -} span.scroll-button { color: white; float: left; } -div.ui-accordion { - padding-left: 0; - padding-right: 0; -} -div.ui-accordion-content { - // Defeat some jquery-ui defaults - padding-left: 3px !important; - padding-right: 0 !important; -} -// The reader tools need the values above instead. -// I would like to reset them like below, but there is some really complicated -// stuff going on with the placement of the ReaderToolSwitch which I -// haven't figured out and don't want to spend more time on. -// Eventually this will all be replaced with React components anyway. -div.ui-accordion-content:not([data-toolid="leveledReaderTool"]):not( - [data-toolid="decodableReaderTool"] - ) { - // Defeat some jquery-ui defaults - padding: 0 !important; - margin: 0 !important; - top: 0 !important; -} // Reserve space for the vertical scrollbar on the reader tools, so that when // their content grows tall enough to need one, the scrollbar's appearance // doesn't shrink the content width and reflow/crowd everything. (The dark // scrollbar *styling* now comes from the shared `bloomDarkScrollbars` class on // the toolbox root — see bloomUI.less — rather than a rule here.) (BL-16585) -div.ui-accordion-content[data-toolid="leveledReaderTool"], -div.ui-accordion-content[data-toolid="decodableReaderTool"] { +// The scrolling container is the MUI AccordionDetails hosting the tool's body. +div.MuiAccordionDetails-root:has([data-toolid="leveledReaderTool"]), +div.MuiAccordionDetails-root:has([data-toolid="decodableReaderTool"]) { scrollbar-gutter: stable; } @@ -159,66 +123,6 @@ div.checkbox { -moz-user-select: none; } -.ui-accordion h3 { - padding-left: 28px; -} - -.ui-accordion h3[data-toolId] { - display: flex; - align-items: center; - gap: 12px; - padding-right: 12px; -} - -.ui-accordion-header .toolbox-accordion-header-text { - flex-grow: 1; -} - -// We are fighting here to get our icons displayed instead of the ones the -// accordion wants to display (typical expand/contract icons). The data-toolId -// helps make this rule more specific so it beats the accordion rule. -// The size makes our arbitrarily sized svg images fit. (Some panels have a -// slightly different size icon and override.) -// The background position defeats an accordion rule that is aimed at selecting -// an icon from a collection-of-icons image by position. -// The background-image:none prevents displaying one of the icons from the -// accordion collection if one of our tools does not provide an icon. -// It is usually overridden by a more specific rule with a particular data-toolId. -// The display:inline-block defeats an accordion rule that sometimes tries to -// hide the icon altogether (display:none). -.ui-accordion h3[data-toolId] span.ui-accordion-header-icon { - background-size: 16px 16px; - background-position: 0 0; - display: inline-block; - background-image: none; -} - -.ui-accordion h3[data-toolId="talkingBookTool"] span.ui-accordion-header-icon { - background-size: 12px 16px; - background-position: 2px 0; - display: inline-block; -} - -// There should be one of these rules for each tool. Newer ones are being placed -// in the tool's own less file. If your tool shouldn't have an icon you need a similar -// rule to force it not to have a background image, since otherwise the one at -// position 0,0 in the accordion image collection is used. -.ui-accordion - h3[data-toolId="decodableReaderTool"] - span.ui-accordion-header-icon { - background-image: url("/bloom/images/keys-white.png") !important; -} - -.ui-accordion - h3[data-toolId="leveledReaderTool"] - span.ui-accordion-header-icon { - background-image: url("/bloom/images/steps-white.png") !important; -} - -.ui-accordion h3[data-toolId="talkingBookTool"] span.ui-accordion-header-icon { - background-image: url("/bloom/images/microphone-white.svg") !important; -} - .subscription-badge { background: no-repeat center/80% url("../../../images/bloom-enterprise-badge.svg") !important; @@ -235,10 +139,6 @@ div.checkbox { z-index: 20000; } -.hideExperimental .experimental { - display: none; -} - // override the color in this lib, which isn't under less control .pure-drawer, .pure-toggle-label { @@ -248,18 +148,6 @@ div.checkbox { #pusherContainer { background-color: @bloom-darkestBackground !important; } -#toolbox h3 { - background: none; - background-color: @bloom-unselectedTabBackground; - border-color: @bloom-unselectedTabBackground; - &.ui-state-hover, - &.ui-state-active { - background-color: @bloom-blue !important; - border-color: @bloom-blue !important; - } - z-index: 1005; // higher than disablingOverlay in Talking Book tool -} - *.disabled { opacity: 0.4; pointer-events: none; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.pug b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.pug index 81d88681df43..a686398d6a64 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.pug +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.pug @@ -4,7 +4,6 @@ block headContent meta(charset='UTF-8') title Content of the Toolbox view (far right pane in book edit) //- Keep this list in sync with WorkspaceView.GetWorkspaceAdditionalHtml() and ViteDevBodyEndHtml imports. - link(rel='stylesheet', href='/bloom/themes/bloom-jqueryui-theme/jquery-ui-1.8.16.custom.css') link(rel='stylesheet', href='/bloom/bookEdit/html/font-awesome/css/font-awesome.min.css') link(rel='stylesheet', href='/bloom/bookEdit/toolbox/toolbox.css') link(rel='stylesheet', href='/bloom/bookEdit/toolbox/talkingBook/audioRecording.css') diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts index 485f070e20db..7b8f3a22d437 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -1,11 +1,6 @@ -/// - import $ from "jquery"; -import "../../modified_libraries/jquery-ui/jquery-ui-1.10.3.custom.min.js"; -import "../../lib/jquery.i18n.custom"; import axios from "axios"; import { get, postString, wrapAxios } from "../../utils/bloomApi"; -import theOneLocalizationManager from "../../lib/localizationManager/localizationManager"; import { hookupLinkHandler } from "../../utils/linkHandler"; import { ckeditableSelector, @@ -14,8 +9,6 @@ import { } from "../../utils/shared"; import { GameTool } from "./games/GameTool"; import { isLongPressEvaluating } from "../longPressShared"; -import { getFeatureStatusAsync } from "../../react_components/featureStatus"; -import { showRequiresSubscriptionDialogInAnyView } from "../../react_components/requiresSubscription"; import { callOnBlur, setExtraFunctionToHandleBlurTasks, @@ -88,13 +81,9 @@ export interface ITool { // If this is true, the tool may only be selected on pages that have data-tool-id matching this tool's id. requiresToolId(): boolean; - // Implement this if the tool uses React. // It should return the main content of the tool, which must be a single div. - // (toolbox will construct the h3 element which goes along with it in the accordion - // and set its data-toolId attr; this method is however responsible to - // localize the content of the div.) - // It may be unimplemented for older tools where beginAddTool() already knows - // where to find an HTML file for the tool content. + // ToolboxRoot renders the section header (label, icon, subscription badge) around it; + // this method is however responsible to localize the content of the div. makeRootElement(): HTMLDivElement; // notifies the tool that an image has been changed on the page. // If the change only affects one image, it may be passed; otherwise, all should be fixed. @@ -120,6 +109,11 @@ export class ToolBox { )).click(); } private builtToolbox: boolean = false; + /** + * Adds or removes the tools that are only offered on pages that ask for them + * (see ITool.requiresToolId()), according to this page's data-tool-id, and makes + * the required tool current. + */ public adjustToolListForPage(page: HTMLElement) { let requiredToolId = page.getAttribute("data-tool-id"); // Books made from the Leveled/Decodable Reader templates have pages that carry @@ -138,43 +132,29 @@ export class ToolBox { newToolId = requiredToolId || undefined; // This function is the main task of adjustToolListForPage. It may have to be postponed - // until we've finished otherwise setting up the toolbox; in particular, we can't refresh - // the accordion before we first set it up. + // until we've finished otherwise setting up the toolbox. // It's possible there will be a tiny bit of flicker if the book opens on a page that // has a required tool as we first initialize the toolbox without that tool and then // add it. But this is fairly rare and I have not found it noticeable. const doAdjustment = () => { - if (!this.builtToolbox) { + const adapter = getToolboxReactAdapter(); + if (!this.builtToolbox || !adapter) { setTimeout(doAdjustment, 100); return; } - const toolbox = document.getElementById("toolbox") as HTMLElement; let toolsAdjusted = false; - for (let i = 0; i < masterToolList.length; i++) { - if (masterToolList[i].requiresToolId()) { - // We may need to add or remove the specified tool - - // Adapt the tool object id to the value used as the ID of the element - // for that tool in the toolbox. - const toolId = ToolBox.addToolToString( - masterToolList[i].id(), - ); - // Get the header element that represents the tool in the DOM. - const toolHeader = toolbox.querySelector( - "[data-toolid='" + - ToolBox.addToolToString(toolId) + - "']", - ) as HTMLElement; - const haveTool = !!toolHeader; - const wantTool = requiredToolId === masterToolList[i].id(); - if (haveTool !== wantTool) { - // add or remove as needed. - showOrHideTool( - ToolBox.addToolToString(masterToolList[i].id()), - wantTool, - ); // required tools don't have check boxes. - toolsAdjusted = wantTool; - } + for (const tool of masterToolList) { + if (!tool.requiresToolId()) { + continue; + } + // We may need to add or remove this tool. + const toolId = ToolBox.addToolToString(tool.id()); + const haveTool = adapter.hasTool(toolId); + const wantTool = requiredToolId === tool.id(); + if (haveTool !== wantTool) { + // add or remove as needed. (Required tools don't have check boxes.) + showOrHideTool(toolId, wantTool); + toolsAdjusted = wantTool; } } // We haven't called showOrHideTool, so the active tool hasn't changed. @@ -443,26 +423,11 @@ export class ToolBox { } toolsToLoad.push("settings"); - $("#toolbox").hide(); const loadNextTool = () => { if (toolsToLoad.length === 0) { - $("#toolbox").accordion({ - heightStyle: "fill", - }); - $("body").find("*[data-i18n]").localize(); // run localization - - // Now bind the window's resize function to the toolbox resizer - $(window).bind("resize", () => { - clearTimeout(resizeTimer); // resizeTimer variable is defined outside of ready function - resizeTimer = setTimeout(resizeToolbox, 100); - }); this.builtToolbox = true; // loaded them all, now we can deal with settings. restoreToolboxSettings(); - $("#toolbox").show(); - // resizeToolbox fits the toolbox root to the window; do it on a - // later cycle, once the toolbox has been laid out. - setTimeout(resizeToolbox, 0); } else { // optimize: maybe we can overlap these? const nextToolId = toolsToLoad.pop(); @@ -476,12 +441,12 @@ export class ToolBox { ); } + /** + * Is the toolbox currently offering this tool a section? (Despite the name, this does + * not mean the tool is the *current* tool; it never did.) + */ public isToolActive(toolId: string): boolean { - const tools = $("*[data-toolId]"); - const filteredTools = tools.filter(function () { - return $(this).attr("data-toolId") === toolId; - }); - return filteredTools.length > 0; + return !!getToolboxReactAdapter()?.hasTool(toolId); } // Enables a tool from an in-page action, ensuring the toolbox is visible. @@ -492,15 +457,16 @@ export class ToolBox { setToolEnabledFromSettings(toolId, true); } + /** + * Makes the given tool (id without the "Tool" suffix) the current tool, enabling it + * first if necessary. Called in response to in-page actions, e.g. clicking a video + * placeholder to get the Sign Language tool. + */ public activateToolFromId(toolId: string) { if (!getITool(toolId)) { - // Normally we won't even give a way to see this tool if it's - // not available for experimental reasons, but sometimes (e.g. - // clicking on a video placeholder, it will help the user to - // say why nothing is happening. - const msg = - "This tool requires that you enable Settings : Advanced Program Settings : Show Experimental Features"; - alert(msg); + // Every tool we know about is registered unconditionally, so this means the + // caller asked for a tool that doesn't exist. + console.error(`activateToolFromId: there is no tool "${toolId}".`); return; } // Making it visible first allows the simulated click to actually activate the tool. @@ -512,23 +478,17 @@ export class ToolBox { this.toggleToolbox(); } - if (isToolEnabledInToolbox(toolId)) { - // Already enabled; just make it the active tool. + // The tool may be present without being in enabledToolIds if it is a + // required-for-this-page tool (see adjustToolListForPage). + if ( + isToolEnabledInToolbox(toolId) || + this.isToolActive(ToolBox.addToolToString(toolId)) + ) { setCurrentTool(toolId); } else { - // Not a required-for-this-page tool that's already present, and not yet enabled. - const toolbox = document.getElementById("toolbox") as HTMLElement; - const toolHeader = toolbox.querySelector( - "[data-toolid='" + ToolBox.addToolToString(toolId) + "']", - ) as HTMLElement; - if (toolHeader) { - // Present in the accordion (e.g. a required tool) but not in enabledToolIds. - setCurrentTool(toolId); - } else { - // Genuinely disabled: enable it, which persists the state and updates - // enabledToolIds, then activates it (showOrHideTool opens it by default). - setToolEnabledFromSettings(toolId, true); - } + // Genuinely disabled: enable it, which persists the state and updates + // enabledToolIds, then activates it (showOrHideTool opens it by default). + setToolEnabledFromSettings(toolId, true); } } @@ -664,18 +624,8 @@ function showOrHideTool( if (turnOn) { beginAddTool(tool, openTool); } else { - $("*[data-toolId]") - .filter(function () { - return $(this).attr("data-toolId") === tool; - }) - .remove(); - window.dispatchEvent( - new CustomEvent("toolbox-tool-removed", { - detail: { toolId: tool }, - }), - ); + getToolboxReactAdapter()?.removeTool(tool); } - resizeToolbox(); } export function restoreToolboxSettings() { @@ -941,51 +891,25 @@ function switchTool(newToolName: string): void { function activateTool(newTool: ITool) { if (newTool && toolbox.toolboxIsShowing()) { - const toolElt = getToolElement(newTool); - if (!toolElt) { + if (!isToolInitialized(newTool)) { return; } // Always re-restore settings so tool state tracks the current book. newTool .beginRestoreSettings(savedSettings as unknown as string) .then(() => { - activateToolInternalAsync(newTool, toolElt); + activateToolInternalAsync(newTool); }); } } -function getToolElement(tool: ITool): HTMLElement | null { - let toolElement: HTMLElement | null = null; - if (tool) { - const toolName = ToolBox.addToolToString(tool.id()); - $("#toolbox") - .find("> h3") - .each(function () { - if ($(this).attr("data-toolId") === toolName) { - // REVIEW: this may in fact be unneeded but I'm just trying to get eslint set up and conceivably it is intentional - // eslint-disable-next-line @typescript-eslint/no-this-alias - toolElement = this; - return false; // break from the each() loop - } - return true; // continue the each() loop - }); - } - return toolElement; -} - +// Does the toolbox have a section for this tool? Only then does it have somewhere to +// display itself and does it make sense to run its lifecycle methods. function isToolInitialized(tool: ITool): boolean { - return !!getToolElement(tool); + return toolbox.isToolActive(ToolBox.addToolToString(tool.id())); } -async function activateToolInternalAsync( - newTool: ITool, - toolElt: HTMLElement | null, -): Promise { - if (!toolElt) { - throw new Error( - `activateToolInternalAsync called for uninitialized tool: ${newTool.id()}`, - ); - } +async function activateToolInternalAsync(newTool: ITool): Promise { // Await it so that we can guarantee that newPageReady() happens after showTool. await newTool.showTool(); @@ -998,8 +922,9 @@ async function activateToolInternalAsync( } /** - * This function attempts to activate the tool whose "data-toolId" attribute is equal to the value - * of "currentTool" (the last tool displayed). + * Attempts to make the given tool the current one (normally the tool the book was last + * using). If the toolbox isn't offering that tool, falls back to the first tool it does + * offer. Passing an empty id also means "whatever tool is first". */ function setCurrentTool(toolID: string) { // I'm downright grumpy about how this code sometimes uses names with "Tool" appended, sometimes doesn't. @@ -1008,7 +933,7 @@ function setCurrentTool(toolID: string) { const adapter = getToolboxReactAdapter(); if (!adapter) { - // ToolboxRoot has not mounted yet, so there is no React accordion to activate + // ToolboxRoot has not mounted yet, so there is no toolbox UI to activate // anything in. We don't expect this: see getToolboxReactAdapter(). return; } @@ -1020,12 +945,10 @@ function setCurrentTool(toolID: string) { toolboxReactActivationHooked = true; } - // NOTE: tools without a "data-toolId" attribute (such as the More tool) cannot be the "currentTool." + // NOTE: the More (settings) section cannot be the "currentTool", so getFirstToolId() + // never returns it. if (!toolID) { - toolID = - ($("#toolbox").find("> h3").first().attr("data-toolId") as - | string - | undefined) ?? ""; + toolID = adapter.getFirstToolId() ?? ""; } if (toolID) { @@ -1036,10 +959,7 @@ function setCurrentTool(toolID: string) { if (tool && !isToolInitialized(tool)) { // The tool we were asked for isn't in the toolbox (e.g., it was disabled // since we saved the setting), so fall back to whatever is first. - toolID = - ($("#toolbox").find("> h3").first().attr("data-toolId") as - | string - | undefined) ?? ""; + toolID = adapter.getFirstToolId() ?? ""; } } @@ -1048,9 +968,9 @@ function setCurrentTool(toolID: string) { } } -// Parameter 'toolId' is the complete tool id with the 'Tool' suffix -// Can return undefined in the case of an experimental tool with -// Advanced Program Settings: Show Experimental Features unchecked. +// Parameter 'toolId' may be spelled with or without the 'Tool' suffix. +// Returns undefined if we know of no such tool, e.g. because the book's settings were +// saved by a version of Bloom that had a tool this one doesn't. function getITool(toolId: string): ITool { // I'm downright grumpy about how this code sometimes uses names with "Tool" appended, sometimes doesn't. // For now I'm just making functions work with either form. @@ -1062,18 +982,15 @@ function getITool(toolId: string): ITool { } /** - * Requests a tool from localhost and loads it into the toolbox. - * These tools are the tools enabled by the user, tools that are - * always enabled (like the talking book tool), and the settings - * "tool". + * Tells the toolbox UI to offer a section for this tool, and optionally to open it. + * These tools are the tools enabled by the user, tools that are always enabled + * (like the talking book tool), and the settings ("More...") tool. */ -// these last three parameters were never used: function requestTool(checkBoxId, toolId, loadNextCallback, tools, currentTool) { function beginAddTool( toolId: string, openTool: boolean, whenLoaded?: () => void, ): void { - // new-style tool implemented in React const tool = getITool(toolId); if (!tool) { console.error( @@ -1082,76 +999,19 @@ function beginAddTool( return; } - if (isToolInitialized(tool)) { - if (openTool && toolbox.toolboxIsShowing()) { - const toolName = ToolBox.addToolToString(tool.id()); - getToolboxReactAdapter()?.setActiveToolByToolId(toolName); - } - - if (whenLoaded) { - whenLoaded(); - } - return; - } - - const content = $(tool.makeRootElement()); - - // the settings for the toolbox is React, but - // its localization works a little differently - // than the other toolbox tools. So, special-case - // handling is needed for the settings - const isSettingsTool = tool.id() === "settings"; - const toolName = ToolBox.addToolToString(tool.id()); - // const parts = $("

" - // + "Music Tool

"); - - const toolIdUpper = - tool.id()[0].toUpperCase() + tool.id().substring(1, tool.id().length); - const i18Id = isSettingsTool - ? "EditTab.Toolbox.More" - : "EditTab.Toolbox." + - toolIdUpper + - (toolName.indexOf(checkLeaveOffTool) === -1 ? "Tool" : ""); - // Not sure this will always work, but we can do something more complicated...maybe a new method - // on ITool...if we need it. Note that this is just a way to come up with the English, - // we don't do it to localizations. But in English, the code value beats the xlf one. - const toolLabel = isSettingsTool - ? "More..." - : ToolBox.addToolToString( - toolIdUpper.replace(/([A-Z])/g, " $1").trim(), - true, - ); - - const reactTool = tool as unknown as IReactTool; - - // Currently, all subscription tools are React, so we haven't implemented a way to add the subscription badge to old-style tools - const possibleSubscriptionBadge = reactTool.featureName - ? `` - : ""; - const header = $( - `

${toolLabel}
${possibleSubscriptionBadge}

`, - ); - header.attr("data-toolId", toolName); - content.attr("data-toolId", toolName); - - // Check feature status asynchronously and apply subscription requirements if needed - if (reactTool.featureName) { - header.attr("data-feature", reactTool.featureName); - addFeatureStatusMessageTitlesToSubscriptionBadges(header); + const adapter = getToolboxReactAdapter(); + // Adding a tool that is already there does nothing, so it is safe to do this + // whether or not the toolbox is already offering it. + adapter?.addTool(toolName); - getFeatureStatusAsync(reactTool.featureName).then((featureStatus) => { - if (featureStatus && featureStatus.subscriptionTier !== "Basic") { - header.addClass("requiresSubscription"); - } - }); + if (openTool && toolbox.toolboxIsShowing()) { + adapter?.setActiveToolByToolId(toolName); } - loadToolboxTool(header, content, toolId, openTool); if (whenLoaded) { whenLoaded(); } - //} } let keydownEventCounter = 0; @@ -1537,136 +1397,6 @@ export function removeCommentsFromEditableHtml(editable: HTMLElement) { } } -let resizeTimer; -function resizeToolbox() { - const windowHeight = $(window).height(); - const root = $(".toolboxRoot"); - // Set toolbox container height to fit in new window size - // Then toolbox Resize() will adjust it to fit the container - root.height(windowHeight - 25); // 25 is the top: value set for div.toolboxRoot in toolbox.less -} - -/** - * Gets the localized title text for a feature based on its status - * @param featureName The name of the feature to get status for - * @returns A Promise that resolves to the localized title text - */ -async function getFeatureEnabledAndMessage( - featureName: string, -): Promise<{ enabled: boolean; message: string }> { - return new Promise<{ enabled: boolean; message: string }>((resolve) => { - get(`features/status?featureName=${featureName}`, (c) => { - const featureStatus = c.data; - const localizedTier = featureStatus?.localizedTier; - - let titleText: string; - if (featureStatus.enabled) { - titleText = theOneLocalizationManager.getText( - "Subscription.FeatureIsIncludedSentence", - "This feature is included in your {0} subscription.", - localizedTier, - ); - } else { - titleText = theOneLocalizationManager.getText( - "Subscription.RequiredTierForFeatureSentence", - 'This feature requires a Bloom subscription tier of at least "{0}".', - localizedTier, - ); - } - resolve({ enabled: featureStatus.enabled, message: titleText }); - }); - }); -} - -function showSubscriptionDialog(featureName: string): void { - showRequiresSubscriptionDialogInAnyView(featureName); -} - -/** - * Adds feature status message titles to subscription badges found in the specified jQuery element - * @param element The jQuery element containing subscription badges - */ -async function addFeatureStatusMessageTitlesToSubscriptionBadges( - element: JQuery, -): Promise { - const subscriptionBadges = element.find(".subscription-badge"); - const promises: Promise[] = []; - subscriptionBadges.each(function (_i, subscriptionBadge: HTMLElement) { - if (subscriptionBadge.hasAttribute("title")) return; - const featureName = - subscriptionBadge.parentElement?.getAttribute("data-feature"); - if (!featureName) return; - - const promise = (async () => { - const { enabled, message } = - await getFeatureEnabledAndMessage(featureName); - subscriptionBadge.setAttribute("title", message); - if (!enabled) { - subscriptionBadge.addEventListener("click", () => - showSubscriptionDialog(featureName), - ); - subscriptionBadge.style.cursor = "pointer"; - } - })(); - - promises.push(promise); - }); - - // Wait for all the promises to complete - await Promise.all(promises); -} - -function loadToolboxTool( - header: JQuery, - content: JQuery, - toolId, - openTool: boolean, -) { - const toolboxElt = $("#toolbox"); - const label = header.text(); - - // Where to insert the new tool? We want to keep them alphabetical except for More...which is always last, - // so insert before the first one with text alphabetically greater than this (if any). - if (toolboxElt.children().length === 0) { - // none yet...this will be the "more" tool which we insert first. - toolboxElt.append(header); - toolboxElt.append(content); - } else { - let insertBefore = toolboxElt - .children() // children() includes both the headers and the contents of the tools - .filter(".ui-accordion-header") // we only want to sort this into the headers... - .filter(function () { - // Note that we aren't (as of 4.4) setting the "locale" of the browser to match the - // UI language. In my tests, it's stuck at "en-US" (navigator.language). But if we ever do - // set this, then this will do a better job of ordering. Meanwhile, no worse. - return label.localeCompare($(this).text()) < 0; - }) - .first(); - if (insertBefore.length === 0) { - // Nothing is greater, but still insert before "More". Two children represent "More", so before the second last. - insertBefore = $( - toolboxElt.children()[toolboxElt.children.length - 2], - ); - } - header.insertBefore(insertBefore); - content.insertBefore(insertBefore); - } - - // if requested, open the tool that was just inserted - if (openTool && toolbox.toolboxIsShowing()) { - const insertedToolId = header.attr("data-toolId"); - if (insertedToolId) { - getToolboxReactAdapter()?.setActiveToolByToolId(insertedToolId); - } - } - - window.dispatchEvent( - new CustomEvent("toolbox-tool-added", { - detail: { toolId: toolId }, - }), - ); -} - function showToolboxChanged(wasShowing: boolean): void { postString( "editView/saveToolboxSetting", @@ -1685,20 +1415,12 @@ function showToolboxChanged(wasShowing: boolean): void { } } else { // starting up for the very first time in this book...no tool is current, - // so select and properly initialize the first one. - let newToolName = $("#toolbox") - .find("> h3") - .first() - .attr("data-toolId"); - if (!newToolName) { - // This should never happen; we're just being defensive. - // At one point (BL-5330) this code could run against the document in the wrong iframe - // and fail to find the #toolbox div; then we get a null and end up saving - // current tool as "undefined" with various bad results. Just in case it happens again - // somehow, we hard code that in this situation we default to - // the talking book tool. - newToolName = "talkingBookTool"; - } - getToolboxReactAdapter()?.setActiveToolByToolId(newToolName); + // so select and properly initialize the first one. If the toolbox somehow has + // no tool sections at all, fall back to the talking book tool, which is always + // enabled. (This should never happen; we're just being defensive.) + const adapter = getToolboxReactAdapter(); + adapter?.setActiveToolByToolId( + adapter.getFirstToolId() ?? "talkingBookTool", + ); } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.vite-dev.pug b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.vite-dev.pug index f40d1c670432..5590dfb335e0 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.vite-dev.pug +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.vite-dev.pug @@ -103,7 +103,6 @@ block endBodyScripts console.log('Underscore ready via Vite prebundle'); //- Note, the list of files is duplicated in a different format in toolbox.pug. - await import("http://localhost:5173/themes/bloom-jqueryui-theme/jquery-ui-1.8.16.custom.css"); await import("http://localhost:5173/bookEdit/html/font-awesome/css/font-awesome.min.css"); await import("http://localhost:5173/bookEdit/toolbox/toolbox.less"); await import("http://localhost:5173/bookEdit/toolbox/talkingBook/audioRecording.less"); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts index 6001d30cc718..ac4532280a94 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts @@ -1,20 +1,11 @@ export {}; declare global { - interface ToolboxToolApi { - makeRootElement?: () => HTMLDivElement; - } - - interface CurrentToolApi { - id: () => string; - } - - interface ToolboxApi { - getCurrentTool?: () => CurrentToolApi | undefined; - } - + // The set of functions the toolbox iframe publishes as window.toolboxBundle, for + // other frames (and C#) to call. Consumers get the real types by casting to + // IToolboxFrameExports (see workspaceFrames.ts), so these are just names. interface ToolboxBundleApi { - getTheOneToolbox: () => ToolboxApi | undefined; + getTheOneToolbox: unknown; scheduleMarkupUpdateAfterPaste: unknown; applyToolboxStateToPage: unknown; removeToolboxMarkup: unknown; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts index dd96c20a420f..feedf62f41a7 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts @@ -1,8 +1,9 @@ // The root of the toolbox is a React component (ToolboxRoot.tsx), but a good deal of the // toolbox is still the legacy, non-React code in toolbox.ts. This module is the single // narrow channel between the two: ToolboxRoot registers an implementation of -// IToolboxReactAdapter when it mounts, and the legacy code uses it to make a tool active -// and to be notified when the user makes a different tool active. +// IToolboxReactAdapter when it mounts, and the legacy code uses it to say which tools the +// toolbox is offering, to make one of them active, and to be notified when the user makes +// a different tool active. // // It lives in its own module (rather than being exported from ToolboxRoot.tsx) because // ToolboxRoot.tsx imports from toolbox.ts, so having toolbox.ts import from ToolboxRoot.tsx @@ -10,20 +11,35 @@ // // When all the tools are React components, each one will belong to its own accordion // section and manage its own state and lifecycle, and this module can go away. +// +// Every toolId parameter and result here may be spelled with or without the historical +// "Tool" suffix ("canvas" and "canvasTool" mean the same tool); the implementation +// normalizes them. export interface IToolboxReactAdapter { - // Makes the tool with this id (with or without the "Tool" suffix) the active, - // expanded section of the React accordion. + // Makes the tool with this id the active, expanded section of the React accordion. setActiveToolByToolId(toolId: string): void; // Registers a callback to be told whenever the active tool changes, including // as a result of setActiveToolByToolId(). onActiveToolChanged(callback: (toolId: string) => void): void; + // Adds a section for this tool, building its body from the tool's makeRootElement(). + // Does nothing if the toolbox is already offering the tool. + addTool(toolId: string): void; + // Removes this tool's section, if it has one. If it was the active section, the first + // remaining tool becomes active. + removeTool(toolId: string): void; + // Is the toolbox currently offering a section for this tool? + hasTool(toolId: string): boolean; + // The id (with the "Tool" suffix) of the first tool section, or undefined if there + // are no tool sections. The "More..." (settings) section doesn't count; it is not a + // tool that can be current. + getFirstToolId(): string | undefined; } let theOneToolboxReactAdapter: IToolboxReactAdapter | undefined; /** - * Called by ToolboxRoot once it has mounted (and again whenever the state it closes - * over changes), making the adapter available to the legacy toolbox code. + * Called by ToolboxRoot once it has mounted, making the adapter available to the legacy + * toolbox code. */ export function setToolboxReactAdapter(adapter: IToolboxReactAdapter): void { theOneToolboxReactAdapter = adapter; @@ -39,3 +55,12 @@ export function setToolboxReactAdapter(adapter: IToolboxReactAdapter): void { export function getToolboxReactAdapter(): IToolboxReactAdapter | undefined { return theOneToolboxReactAdapter; } + +/** + * Has the toolbox UI been created? Code that persists or restores toolbox state can use + * this to tell "we are running in the real toolbox" from "we are running in a unit test + * (or too early in startup) where there is no toolbox UI and nothing should be saved". + */ +export function isToolboxUiReady(): boolean { + return !!theOneToolboxReactAdapter; +} diff --git a/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/component-tests/toolbox-root-react.uitest.ts b/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/component-tests/toolbox-root-react.uitest.ts index 8c1aad8f838b..ad6983626555 100644 --- a/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/component-tests/toolbox-root-react.uitest.ts +++ b/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/component-tests/toolbox-root-react.uitest.ts @@ -41,22 +41,11 @@ test.describe("ToolboxRoot React mode", () => { await expect(talkingBook).toHaveAttribute("aria-expanded", "false"); }); - test("initial selection follows restored current tool", async ({ + test("legacy code can make a section active through the adapter", async ({ page, }) => { await routeToolboxApis(page); - await page.addInitScript(() => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (window as any).toolboxBundle = { - getTheOneToolbox: () => ({ - getCurrentTool: () => ({ - id: () => "decodableReader", - }), - }), - }; - }); - await page.route("**/bloom/api/toolbox/enabledTools", async (route) => { await route.fulfill({ status: 200, @@ -71,6 +60,14 @@ test.describe("ToolboxRoot React mode", () => { timeout: 15000, }); + await page.evaluate(() => { + // The harness publishes this accessor for us; see ToolboxRootTestHarness.tsx. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (window as any) + .getToolboxReactAdapterForTests?.() + ?.setActiveToolByToolId("decodableReaderTool"); + }); + await expect( getToolHeader(page, "Decodable Reader Tool"), ).toHaveAttribute("aria-expanded", "true"); @@ -95,16 +92,11 @@ test.describe("ToolboxRoot React mode", () => { ); await page.evaluate(() => { - window.dispatchEvent( - new CustomEvent("toolbox-tool-added", { - detail: { toolId: "decodableReaderTool" }, - }), - ); // The harness publishes this accessor for us; see ToolboxRootTestHarness.tsx. // eslint-disable-next-line @typescript-eslint/no-explicit-any - (window as any) - .getToolboxReactAdapterForTests?.() - ?.setActiveToolByToolId("decodableReaderTool"); + const adapter = (window as any).getToolboxReactAdapterForTests?.(); + adapter?.addTool("decodableReaderTool"); + adapter?.setActiveToolByToolId("decodableReaderTool"); }); await expect(getToolHeader(page, "Decodable Reader Tool")).toBeVisible({ @@ -165,11 +157,10 @@ test.describe("ToolboxRoot React mode", () => { }); await page.evaluate(() => { - window.dispatchEvent( - new CustomEvent("toolbox-tool-added", { - detail: { toolId: "decodableReaderTool" }, - }), - ); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (window as any) + .getToolboxReactAdapterForTests?.() + ?.addTool("decodableReaderTool"); }); await expect( From f07ffd497d42510047b1a58732e4cc535a0e91d5 Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Fri, 24 Jul 2026 21:38:46 -0700 Subject: [PATCH 04/19] Fix BL-16608 Cleanup toolbox infrastructure (4/7): single source of truth for tool ids and metadata https://issues.bloomlibrary.org/youtrack/issue/BL-16608 The same tool facts were maintained in three places: toolbox.ts, hardcoded tables in ToolboxRoot.tsx (alwaysOnToolIds, subscriptionToolIds, toolIconPathByToolId, label special-casing), and kToolDefs in SettingsToolControls.tsx. - toolIds.ts now owns the canonical id scheme and every boundary conversion. The in-memory id is ITool.id() without the historical "Tool" suffix; toCanonicalToolId/toPersistedToolName/toEnabledSettingName convert at the meta.json / save-API boundaries (including the "Visualizer" no-suffix rule and the "...Check" fossil), getToolLabelInfo is the one copy of the label/i18n-key convention, and compareToolsByLabel is the shared ordering. ToolBox.addToolToString/checkLeaveOffTool and ToolboxRoot's normalizeToolId/toToolboxToolId are gone. - Per-tool UI metadata now comes from the tool classes: new optional ITool.iconPath() implemented by the nine tools that have icons; always-on and subscription-badge status are asked of the tool (isAlwaysEnabled(), featureName). The More... checkbox list is derived by filtering the registered tools (not always-enabled, not page-required, not settings) and sorting by label - byte-identical to the old hardcoded list. - /bloom/api/toolbox/enabledTools is fetched once, by ToolBox.initialize(); ToolboxRoot no longer fetches it and is populated solely through adapter.addTool. - The adapter methods now require canonical ids. Tightening exposed a latent bug: talkingBookTool.isImageDescriptionToolActive() passed "imageDescriptionTool" and only worked because the adapter was tolerant; it now uses kImageDescriptionToolId (protects BL-8515 behavior). - switchTool's fuzzy startsWith match and getITool's chop-the-suffix match are replaced with exact matches after toCanonicalToolId (equivalent for all 11 ids; no id is a prefix of another). - ToolboxView.cs: updated the stale "how to add a tool" class comment (it referenced a Settings.pug and less icon rules that no longer exist and documented the wrong i18n key convention). Persisted formats are unchanged; every read/write of meta.json tool names, the current-tool setting, and per-tool state strings was traced (all 11 derived i18n keys verified to exist verbatim in the en XLF). Verified: pnpm typecheck passes, eslint 0 errors (0 new warnings), full vitest suite 550 passed / 5 skipped, build/agent-vite.sh bundle compiles. Co-Authored-By: Claude Fable 5 --- .../bookEdit/toolbox/ToolboxRoot.tsx | 245 ++++---------- .../bookEdit/toolbox/canvas/canvasTool.tsx | 5 + .../imageDescription/imageDescription.tsx | 5 + .../impairmentVisualizer.tsx | 5 + .../bookEdit/toolbox/motion/motionTool.tsx | 5 + .../toolbox/music/musicToolControls.tsx | 5 + .../decodableReader/decodableReaderTool.tsx | 4 + .../leveledReader/leveledReaderTool.tsx | 5 + .../toolbox/settings/SettingsToolControls.tsx | 104 +++--- .../toolbox/settings/settingsTool.tsx | 3 +- .../toolbox/signLanguage/signLanguageTool.tsx | 5 + .../toolbox/talkingBook/talkingBookTool.tsx | 10 +- .../bookEdit/toolbox/toolIds.ts | 112 ++++++ .../bookEdit/toolbox/toolbox.ts | 320 ++++++++---------- .../bookEdit/toolbox/toolboxReactAdapter.ts | 34 +- .../toolbox/toolboxToolReactAdaptor.tsx | 9 + src/BloomExe/Edit/ToolboxView.cs | 28 +- 17 files changed, 465 insertions(+), 439 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx index a4cd1ff90298..a6f2b200ce5d 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx @@ -1,6 +1,5 @@ import * as React from "react"; import { renderRoot } from "../../utils/reactRender"; -import axios from "axios"; import { css } from "@emotion/react"; import Accordion from "@mui/material/Accordion"; import AccordionDetails from "@mui/material/AccordionDetails"; @@ -18,42 +17,40 @@ import { getMasterToolList } from "./toolbox"; import { kToolboxHeaderZIndex } from "./toolboxZIndexes"; import { setToolboxReactAdapter } from "./toolboxReactAdapter"; import { SubscriptionBadgeWithTooltipAndDialog } from "../../react_components/requiresSubscription"; - -// React host for the toolbox sidebar. It owns which tools the toolbox is offering, which -// one is expanded, and the DOM node that each tool renders itself into. +import { + compareToolsByLabel, + getToolLabelInfo, + kSettingsToolId, + kTalkingBookToolId, + toPersistedToolName, +} from "./toolIds"; + +// React host for the toolbox sidebar. It holds the list of tools the toolbox is offering, +// which one is expanded, and the DOM node that each tool renders itself into. +// +// It does not decide which tools to offer: toolbox.ts asks the server which tools the book +// has enabled and tells us about each one through the adapter's addTool(), which is the +// only way a section is ever created. // // Each tool still hands us a plain DOM element (from its ITool.makeRootElement()) rather // than a React component, so a small host component (ToolBodyHost) puts that element into // the React layout. When every tool is a React component, each section can render its // tool directly and both that host and toolboxReactAdapter.ts can go away. +// Everything the toolbox needs in order to show one tool's section. It all comes from the +// tool itself (see ITool) or is derived from its id (see toolIds.ts). type ToolboxSection = { - // The tool's id without the historical "Tool" suffix, e.g. "canvas". + // The tool's canonical id, i.e. what its ITool.id() returns, e.g. "canvas". id: string; englishLabel: string; l10nKey: string; + // The icon to show in the section header; undefined for sections without one. + iconPath?: string; + // Set only for tools that require a subscription, in which case the section header + // gets a badge for this feature. + featureName?: string; // The element the tool renders itself into. Created once, when the section is created. - toolBodyElement?: HTMLDivElement; -}; - -// Tools the toolbox offers whether or not the enabledTools API mentions them. -// "settings" is the "More..." section, which is how the user enables the others. -const alwaysOnToolIds: string[] = ["talkingBook", "settings"]; - -const subscriptionToolIds = new Set(["canvas", "motion", "music"]); - -const toolIconPathByToolId: Record = { - talkingBook: "/bloom/images/microphone-white.svg", - decodableReader: "/bloom/images/keys-white.png", - leveledReader: "/bloom/images/steps-white.png", - signLanguage: "/bloom/bookEdit/toolbox/signLanguage/signLanguageTool.svg", - music: "/bloom/bookEdit/toolbox/music/music-notes-white.svg", - motion: "/bloom/bookEdit/toolbox/motion/motion.svg", - canvas: "/bloom/bookEdit/toolbox/canvas/Canvas%20Icon.svg", - imageDescription: - "/bloom/bookEdit/toolbox/imageDescription/ImageDescriptionToolIcon.svg", - impairmentVisualizer: - "/bloom/bookEdit/toolbox/impairmentVisualizer/blind-eye-white.svg", + toolBodyElement: HTMLDivElement; }; const toolboxHeaderIconStyles = css` @@ -66,88 +63,25 @@ const toolboxHeaderIconStyles = css` flex-shrink: 0; `; -// Normalize mixed naming conventions (e.g., "canvas" vs "canvasTool") so -// React and legacy code can refer to the same logical tool. -const normalizeToolId = (toolId: string): string => { - if (!toolId) { - return toolId; - } - - if (toolId.endsWith("Tool")) { - return toolId.substring(0, toolId.length - 4); - } - - return toolId; -}; - -// Convert normalized IDs back to the toolbox's traditional "*Tool" names when -// we need to notify the legacy toolbox code. -const toToolboxToolId = (toolId: string): string => { - if (!toolId) { - return toolId; - } - if (toolId.endsWith("Tool") || toolId.endsWith("Visualizer")) { - return toolId; - } - return `${toolId}Tool`; -}; - -const getToolboxLabelInfo = ( - toolId: string, -): { englishLabel: string; l10nKey: string } => { - const normalizedToolId = normalizeToolId(toolId); - if (normalizedToolId === "settings") { - return { - englishLabel: "More...", - l10nKey: "EditTab.Toolbox.More", - }; - } - - const toolIdUpper = - normalizedToolId[0].toUpperCase() + - normalizedToolId.substring(1, normalizedToolId.length); - const englishBaseLabel = toolIdUpper.replace(/([A-Z])/g, " $1").trim(); - const endsWithVisualizer = normalizedToolId.endsWith("Visualizer"); - - return { - englishLabel: endsWithVisualizer - ? englishBaseLabel - : `${englishBaseLabel} Tool`, - l10nKey: endsWithVisualizer - ? `EditTab.Toolbox.${toolIdUpper}` - : `EditTab.Toolbox.${toolIdUpper}Tool`, - }; -}; - -// Ask the tool for the element it renders itself into. Returns undefined if we don't know -// about the tool at all, which can happen if settings were saved by a later version of Bloom. -const makeToolBodyElement = ( - normalizedToolId: string, -): HTMLDivElement | undefined => { +// Gathers everything we need to show a section for this tool. The tool must be one the +// toolbox knows about: toolbox.ts only asks us for tools it found in the master list. +const makeSectionFromToolId = (toolId: string): ToolboxSection => { const tool = getMasterToolList().find( - (candidate) => candidate.id() === normalizedToolId, - ); - if (!tool) { - return undefined; - } - + (candidate) => candidate.id() === toolId, + )!; + const labelInfo = getToolLabelInfo(toolId); const toolBodyElement = tool.makeRootElement(); - // Some tool stylesheets still select their body by this attribute. - toolBodyElement.setAttribute( - "data-toolid", - toToolboxToolId(normalizedToolId), - ); - return toolBodyElement; -}; + // Some tool stylesheets still select their body by this attribute, using the + // historical "Tool"-suffixed name. + toolBodyElement.setAttribute("data-toolid", toPersistedToolName(toolId)); -const makeSectionFromToolId = (toolId: string): ToolboxSection => { - const normalizedToolId = normalizeToolId(toolId); - const labelInfo = getToolboxLabelInfo(normalizedToolId); return { - id: normalizedToolId, + id: toolId, englishLabel: labelInfo.englishLabel, l10nKey: labelInfo.l10nKey, - toolBodyElement: makeToolBodyElement(normalizedToolId), + iconPath: tool.iconPath(), + featureName: tool.featureName, + toolBodyElement: toolBodyElement, }; }; @@ -155,15 +89,11 @@ const sortSectionsAlphabeticallyWithSettingsLast = ( sections: ToolboxSection[], ): ToolboxSection[] => { const settingsSection = sections.find( - (section) => section.id === "settings", + (section) => section.id === kSettingsToolId, ); const nonSettingsSections = sections - .filter((section) => section.id !== "settings") - .sort((a, b) => - a.englishLabel.localeCompare(b.englishLabel, undefined, { - sensitivity: "base", - }), - ); + .filter((section) => section.id !== kSettingsToolId) + .sort((a, b) => compareToolsByLabel(a.id, b.id)); if (!settingsSection) { return nonSettingsSections; @@ -172,18 +102,6 @@ const sortSectionsAlphabeticallyWithSettingsLast = ( return [...nonSettingsSections, settingsSection]; }; -const parseEnabledToolIds = (value: string): string[] => { - const normalized = value - .split(",") - .map((toolId) => toolId.trim()) - .filter((toolId) => !!toolId) - .map((toolId) => normalizeToolId(toolId)); - - const toolIds = new Set(normalized); - alwaysOnToolIds.forEach((toolId) => toolIds.add(toolId)); - return Array.from(toolIds); -}; - // Puts a tool's own DOM element (the one it renders itself into) into the React layout, // keeping the original element instance so the tool's state and event wiring stay intact. const ToolBodyHost: React.FunctionComponent<{ element: HTMLDivElement }> = ( @@ -257,75 +175,40 @@ export const ToolboxRoot: React.FunctionComponent = () => { [], ); - const makeToolActive = React.useCallback((normalizedToolId: string) => { - setExpandedSectionId(normalizedToolId); - const toolboxToolId = toToolboxToolId(normalizedToolId); + const makeToolActive = React.useCallback((toolId: string) => { + setExpandedSectionId(toolId); activeToolChangedCallbacks.current.forEach((callback) => { - callback(toolboxToolId); + callback(toolId); }); }, []); - // Load the tools the toolbox should offer. (The legacy toolbox code independently - // announces the same tools through addTool(); whichever gets there first wins, and - // the other is a no-op.) - React.useEffect(() => { - axios - .get("/bloom/api/toolbox/enabledTools") - .then((response) => { - const parsedIds = parseEnabledToolIds(response.data); - const masterList = getMasterToolList(); - const knownIds = parsedIds.filter((toolId) => - masterList.some((tool) => tool.id() === toolId), - ); - const existingIds = new Set( - sectionsRef.current.map((section) => section.id), - ); - const newSections = knownIds - .filter((toolId) => !existingIds.has(toolId)) - .map((toolId) => makeSectionFromToolId(toolId)); - applySections( - sortSectionsAlphabeticallyWithSettingsLast([ - ...sectionsRef.current, - ...newSections, - ]), - ); - }) - .catch((error) => { - throw error; - }); - }, [applySections]); - // Register the adapter that the legacy toolbox code uses to say which tools the // toolbox offers, to make one of them active, and to observe which one is active. // See toolboxReactAdapter.ts. React.useEffect(() => { setToolboxReactAdapter({ setActiveToolByToolId: (toolId: string) => { - makeToolActive(normalizeToolId(toolId)); + makeToolActive(toolId); }, onActiveToolChanged: (callback: (toolId: string) => void) => { activeToolChangedCallbacks.current.push(callback); }, addTool: (toolId: string) => { - const normalizedToolId = normalizeToolId(toolId); if ( - sectionsRef.current.some( - (section) => section.id === normalizedToolId, - ) + sectionsRef.current.some((section) => section.id === toolId) ) { return; } applySections( sortSectionsAlphabeticallyWithSettingsLast([ ...sectionsRef.current, - makeSectionFromToolId(normalizedToolId), + makeSectionFromToolId(toolId), ]), ); }, removeTool: (toolId: string) => { - const normalizedToolId = normalizeToolId(toolId); const remainingSections = sectionsRef.current.filter( - (section) => section.id !== normalizedToolId, + (section) => section.id !== toolId, ); if (remainingSections.length === sectionsRef.current.length) { return; @@ -335,24 +218,20 @@ export const ToolboxRoot: React.FunctionComponent = () => { // one. The awkward functional update guards against a stale value of // expandedSectionId. setExpandedSectionId((previousExpandedSectionId) => - previousExpandedSectionId === normalizedToolId + previousExpandedSectionId === toolId ? remainingSections[0]?.id : previousExpandedSectionId, ); }, hasTool: (toolId: string) => { - const normalizedToolId = normalizeToolId(toolId); return sectionsRef.current.some( - (section) => section.id === normalizedToolId, + (section) => section.id === toolId, ); }, getFirstToolId: () => { - const firstToolSection = sectionsRef.current.find( - (section) => section.id !== "settings", - ); - return firstToolSection - ? toToolboxToolId(firstToolSection.id) - : undefined; + return sectionsRef.current.find( + (section) => section.id !== kSettingsToolId, + )?.id; }, }); }, [applySections, makeToolActive]); @@ -485,8 +364,10 @@ export const ToolboxRoot: React.FunctionComponent = () => { `} > { } data-toolid={section.id} style={{ - backgroundImage: `url(${toolIconPathByToolId[section.id] || ""})`, + backgroundImage: `url(${section.iconPath ?? ""})`, }} > { {section.englishLabel} - {subscriptionToolIds.has(section.id) && ( + {section.featureName && ( )} @@ -549,15 +430,9 @@ export const ToolboxRoot: React.FunctionComponent = () => { } `} > - {section.toolBodyElement ? ( - - ) : ( - - Loading {section.englishLabel}... - - )} +
diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx index 04da0c2f5f9f..7f8d3e16d9dc 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx @@ -30,6 +30,11 @@ export class CanvasTool extends ToolboxToolReactAdaptor { return kCanvasToolId; } + /** The icon for this tool's section header in the toolbox. */ + public iconPath(): string { + return "/bloom/bookEdit/toolbox/canvas/Canvas%20Icon.svg"; + } + public featureName? = kCanvasToolId; public newPageReady() { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx index afb43ba50248..6c25410ba615 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx @@ -368,6 +368,11 @@ export class ImageDescriptionAdapter extends ToolboxToolReactAdaptor { return ImageDescriptionAdapter.kToolID; } + /** The icon for this tool's section header in the toolbox. */ + public iconPath(): string { + return "/bloom/bookEdit/toolbox/imageDescription/ImageDescriptionToolIcon.svg"; + } + // If we declare the function in this normal way and pass it to addEventListener, // we get the wrong 'this' and can't get at this.reactControls. // private descriptionGotFocus(e: Event) { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx b/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx index 60da95957827..4e87e5a08a27 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx @@ -349,6 +349,11 @@ export class ImpairmentVisualizerAdaptor extends ToolboxToolReactAdaptor { return "impairmentVisualizer"; } + /** The icon for this tool's section header in the toolbox. */ + public iconPath(): string { + return "/bloom/bookEdit/toolbox/impairmentVisualizer/blind-eye-white.svg"; + } + public showTool() { if (!this.controlsElement) return; this.controlsElement.updateSimulations(undefined); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx index a16b259c2d26..a3789550f4e3 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx @@ -310,6 +310,11 @@ export class MotionTool extends ToolboxToolReactAdaptor { return kMotionToolId; } + /** The icon for this tool's section header in the toolbox. */ + public iconPath(): string { + return "/bloom/bookEdit/toolbox/motion/motion.svg"; + } + public featureName? = kMotionToolId; private getBloomCanvasToAnimate(): HTMLElement | null { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/music/musicToolControls.tsx b/src/BloomBrowserUI/bookEdit/toolbox/music/musicToolControls.tsx index 360d0244197b..50f250bbc5f3 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/music/musicToolControls.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/music/musicToolControls.tsx @@ -461,6 +461,11 @@ export class MusicToolAdaptor extends ToolboxToolReactAdaptor { return kMusicToolId; } + /** The icon for this tool's section header in the toolbox. */ + public iconPath(): string { + return "/bloom/bookEdit/toolbox/music/music-notes-white.svg"; + } + public featureName? = kMusicToolId; public showTool() { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx index b6332124bd4f..20ea458ecf49 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx @@ -26,6 +26,10 @@ export class DecodableReaderTool extends ToolboxToolReactAdaptor { public id(): string { return "decodableReader"; } + /** The icon for this tool's section header in the toolbox. */ + public iconPath(): string { + return "/bloom/images/keys-white.png"; + } public newPageReady(): void { const model = getTheOneReaderToolsModel(); model.setMarkupType(isReaderToolEnabledOnCurrentPage(false) ? 1 : 0); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx index 17c434c65f0f..5be05ccac585 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx @@ -27,6 +27,11 @@ export class LeveledReaderTool extends ToolboxToolReactAdaptor { return "leveledReader"; } + /** The icon for this tool's section header in the toolbox. */ + public iconPath(): string { + return "/bloom/images/steps-white.png"; + } + // this function restores the level that was last saved, // as well as the data for that stage, so that the tool // doesn't restart at level 1 all the time diff --git a/src/BloomBrowserUI/bookEdit/toolbox/settings/SettingsToolControls.tsx b/src/BloomBrowserUI/bookEdit/toolbox/settings/SettingsToolControls.tsx index 6bab0cda4600..603e8cc8bf14 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/settings/SettingsToolControls.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/settings/SettingsToolControls.tsx @@ -1,7 +1,9 @@ import { FunctionComponent, useState } from "react"; import { BloomCheckbox } from "../../../react_components/BloomCheckBox"; import { + getMasterToolList, isToolEnabledInToolbox, + ITool, setToolEnabledFromSettings, setToolboxSettingsChangeHandler, } from "../toolbox"; @@ -10,14 +12,38 @@ import { SubscriptionBadgeWithTooltipAndDialog } from "../../../react_components import { ThemeProvider } from "@mui/material/styles"; import { toolboxTheme } from "../../../bloomMaterialUITheme"; import { useMountEffect } from "../../../utils/useMountEffect"; +import { + compareToolsByLabel, + getToolLabelInfo, + kSettingsToolId, +} from "../toolIds"; + +/** + * The tools the "More..." section offers a checkbox for, in the order it shows them: + * every registered tool except the ones the user has no say over, that is, the tools that + * are always enabled (Talking Book), the tools that are only offered on pages that ask for + * them (Games; see ITool.requiresToolId()), and this "More..." section itself. + * The order is the same one the toolbox uses for its sections: alphabetical by label. + */ +const getToolsOfferedAsCheckboxes = (): ITool[] => + getMasterToolList() + .filter( + (tool) => + !tool.isAlwaysEnabled() && + !tool.requiresToolId() && + tool.id() !== kSettingsToolId, + ) + .sort((a, b) => compareToolsByLabel(a.id(), b.id())); +// One tool's checkbox. Everything except whether it is currently ticked comes from the +// tool itself (see ITool) or is derived from its id (see toolIds.ts). const ToolboxCheckbox: FunctionComponent<{ - tool: string; - l10nKeySuffix: string; - toolLabel: string; + toolId: string; + // Set only for tools that require a subscription, in which case we show a badge. + featureName?: string; shouldCheck: boolean; - requiresSubscription?: boolean; }> = (props) => { + const labelInfo = getToolLabelInfo(props.toolId); return (
{ // Pass true so that, when enabling, the tool opens after a // brief delay letting the user see this checkbox tick before // the "More..." section collapses to reveal the tool. (BL-16501) - setToolEnabledFromSettings(props.tool, checked!, true); + setToolEnabledFromSettings(props.toolId, checked!, true); }} /> - {props.requiresSubscription && ( + {props.featureName && (
)} @@ -66,55 +92,14 @@ const ToolboxCheckbox: FunctionComponent<{ }; export const SettingsToolControls: FunctionComponent = () => { - const kToolDefs = [ - { - tool: "canvas", - l10nKeySuffix: "CanvasTool", - toolLabel: "Canvas Tool", - requiresSubscription: true, - }, - { - tool: "decodableReader", - l10nKeySuffix: "DecodableReaderTool", - toolLabel: "Decodable Reader Tool", - }, - { - tool: "imageDescription", - l10nKeySuffix: "ImageDescriptionTool", - toolLabel: "Image Description Tool", - }, - { - tool: "impairmentVisualizer", - l10nKeySuffix: "ImpairmentVisualizer", - toolLabel: "Impairment Visualizer", - }, - { - tool: "leveledReader", - l10nKeySuffix: "LeveledReaderTool", - toolLabel: "Leveled Reader Tool", - }, - { - tool: "motion", - l10nKeySuffix: "MotionTool", - toolLabel: "Motion Tool", - requiresSubscription: true, - }, - { - tool: "music", - l10nKeySuffix: "MusicTool", - toolLabel: "Music Tool", - requiresSubscription: true, - }, - { - tool: "signLanguage", - l10nKeySuffix: "SignLanguageTool", - toolLabel: "Sign Language Tool", - }, - ]; + const toolsOffered = getToolsOfferedAsCheckboxes(); const [checkedState, setCheckedState] = useState>( () => Object.fromEntries( - kToolDefs.map((t) => [t.tool, isToolEnabledInToolbox(t.tool)]), + toolsOffered.map((tool) => [ + tool.id(), + isToolEnabledInToolbox(tool.id()), + ]), ), ); @@ -137,11 +122,12 @@ export const SettingsToolControls: FunctionComponent = () => { margin-top: 6px; `} > - {kToolDefs.map((t) => ( + {toolsOffered.map((tool) => ( ))}
diff --git a/src/BloomBrowserUI/bookEdit/toolbox/settings/settingsTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/settings/settingsTool.tsx index 65cc39e53f30..5b829cbe52da 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/settings/settingsTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/settings/settingsTool.tsx @@ -1,6 +1,7 @@ import { renderRoot } from "../../../utils/reactRender"; import ToolboxToolReactAdaptor from "../toolboxToolReactAdaptor"; import { SettingsToolControls } from "./SettingsToolControls"; +import { kSettingsToolId } from "../toolIds"; // This class renders the SettingsToolControls React component // for the toolbox. The settings are the menu of tools that @@ -22,6 +23,6 @@ export class SettingsTool extends ToolboxToolReactAdaptor { // returns the id for this "tool" so that it // can be properly bootstrapped public id(): string { - return "settings"; + return kSettingsToolId; } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx index 993f82a57397..2757cdf38a18 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx @@ -1045,6 +1045,11 @@ export class SignLanguageTool extends ToolboxToolReactAdaptor { return "signLanguage"; } + /** The icon for this tool's section header in the toolbox. */ + public iconPath(): string { + return "/bloom/bookEdit/toolbox/signLanguage/signLanguageTool.svg"; + } + // This function is saved in a variable so we can remove the same listener we added. private containerClickListener: EventListener = (event: MouseEvent) => { // The reason for the listener: to select the current element diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx index 86ac23a472a9..66d9c8ef7fd2 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx @@ -7,6 +7,7 @@ import { getAudioRecorder, getOrCreateAudioRecorder } from "./audioRecording"; import * as AudioRecorder from "./audioRecording"; import ToolboxToolReactAdaptor from "../toolboxToolReactAdaptor"; import { TalkingBookToolControls } from "./TalkingBookToolControls"; +import { kImageDescriptionToolId, kTalkingBookToolId } from "../toolIds"; // This class renders the TalkingBookToolControls React component // in the toolbox, and passes into it an instance of the audioRecorder. @@ -164,7 +165,7 @@ export default class TalkingBookTool extends ToolboxToolReactAdaptor { } private isImageDescriptionToolActive(): boolean { - return getTheOneToolbox().isToolActive("imageDescriptionTool"); + return getTheOneToolbox().isToolActive(kImageDescriptionToolId); } private showImageDescriptionsIfAny() { @@ -197,6 +198,11 @@ export default class TalkingBookTool extends ToolboxToolReactAdaptor { } public id() { - return "talkingBook"; + return kTalkingBookToolId; + } + + /** The icon for this tool's section header in the toolbox. */ + public iconPath(): string { + return "/bloom/images/microphone-white.svg"; } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolIds.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolIds.ts index 11c305ee7fa8..388d11e04914 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolIds.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolIds.ts @@ -5,7 +5,119 @@ // that wouldn't cause a lot of other things to get dragged into your bundle. // (Referencing a static variable in the relevant tool would cause a lot of things to get dragged into certain bundles) // These are also used as feature names when the tool requires a subscription. +// +// This file is also the single place that knows how a tool's canonical id relates to the +// other spellings of it that appear at our boundaries: the "Tool"-suffixed names in +// persisted data and stylesheets, and the English labels and localization keys of the +// toolbox section headers and "More..." checkboxes. export const kCanvasToolId = "canvas"; export const kGameToolId = "game"; +export const kImageDescriptionToolId = "imageDescription"; export const kMotionToolId = "motion"; export const kMusicToolId = "music"; +// The "More..." section, which is where the user turns the other tools on and off. +// It is a tool like the others, except that it can never be the "current" tool of a book. +export const kSettingsToolId = "settings"; +export const kTalkingBookToolId = "talkingBook"; + +// Historically, tool names in some contexts carry a "Tool" suffix: the "current" tool and +// the tool "name"s in a book's meta.json, and the data-toolid attribute that some tool +// stylesheets select on. The canonical, in-memory id of a tool is always the unsuffixed +// one that its id() method returns, e.g. "talkingBook". The two functions below are the +// only code that knows about the suffix, and they are called only at those boundaries. +const kPersistedNameSuffix = "Tool"; +// One tool id already reads as the name of a tool ("impairmentVisualizer"), so it has +// never taken the suffix. +const kIdEndingThatTakesNoSuffix = "Visualizer"; + +/** + * The canonical id (what the tool's id() returns, with no "Tool" suffix) of a tool, given + * either its canonical id or the "Tool"-suffixed name used in persisted data. Use when + * reading a tool name that came from outside the toolbox code. + */ +export function toCanonicalToolId(toolIdOrPersistedName: string): string { + if (toolIdOrPersistedName.endsWith(kPersistedNameSuffix)) { + return toolIdOrPersistedName.substring( + 0, + toolIdOrPersistedName.length - kPersistedNameSuffix.length, + ); + } + return toolIdOrPersistedName; +} + +/** + * The name to use for a tool where the historical "Tool" suffix is expected: the book's + * meta.json (the "current" tool and the enabled-tool names) and the data-toolid attribute + * of a tool's body element. Given a canonical tool id, appends the suffix, except to ids + * that never took it. + */ +export function toPersistedToolName(toolId: string): string { + if ( + !toolId || + toolId.endsWith(kPersistedNameSuffix) || + toolId.endsWith(kIdEndingThatTakesNoSuffix) + ) { + return toolId; + } + return toolId + kPersistedNameSuffix; +} + +// The other historical spelling: the editView/saveToolboxSetting API identifies the +// enabled/disabled setting of a tool by the id of the checkbox that used to control it, +// which was the tool's id followed by "Check". The C# side strips the suffix off again +// (see ToolboxView.SaveToolboxSettings), so what lands in the book's meta.json is the +// canonical tool id. +const kEnabledSettingNameSuffix = "Check"; + +/** + * The name that editView/saveToolboxSetting expects for the setting that says whether a + * tool is enabled in this book. Given a canonical tool id. + */ +export function toEnabledSettingName(toolId: string): string { + return toolId + kEnabledSettingNameSuffix; +} + +/** + * The English label and localization key of a tool, used both for its toolbox section + * header and for its checkbox in the "More..." section. Both are derived from the + * canonical tool id: "decodableReader" gives "Decodable Reader Tool" and + * "EditTab.Toolbox.DecodableReaderTool". + */ +export function getToolLabelInfo(toolId: string): { + englishLabel: string; + l10nKey: string; +} { + if (toolId === kSettingsToolId) { + return { + englishLabel: "More...", + l10nKey: "EditTab.Toolbox.More", + }; + } + + const capitalizedId = toolId[0].toUpperCase() + toolId.substring(1); + const spacedLabel = capitalizedId.replace(/([A-Z])/g, " $1").trim(); + // An id ending in "Visualizer" already reads as the name of a tool, so neither the + // label nor the key gets "Tool" added (the same rule as toPersistedToolName). + if (capitalizedId.endsWith(kIdEndingThatTakesNoSuffix)) { + return { + englishLabel: spacedLabel, + l10nKey: `EditTab.Toolbox.${capitalizedId}`, + }; + } + return { + englishLabel: `${spacedLabel} Tool`, + l10nKey: `EditTab.Toolbox.${capitalizedId}Tool`, + }; +} + +/** + * Orders two tools the way the toolbox presents them: alphabetically by English label. + * (The toolbox itself puts the "More..." section last, whatever this says about it.) + */ +export function compareToolsByLabel(toolIdA: string, toolIdB: string): number { + return getToolLabelInfo(toolIdA).englishLabel.localeCompare( + getToolLabelInfo(toolIdB).englishLabel, + undefined, + { sensitivity: "base" }, + ); +} diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts index 7b8f3a22d437..244a64ff3ccb 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -13,12 +13,20 @@ import { callOnBlur, setExtraFunctionToHandleBlurTasks, } from "../../utils/menuCloseOnBlur"; -import { getToolboxReactAdapter } from "./toolboxReactAdapter"; +import { + getToolboxReactAdapter, + whenToolboxReactAdapterReady, +} from "./toolboxReactAdapter"; +import { + kSettingsToolId, + kTalkingBookToolId, + toCanonicalToolId, + toEnabledSettingName, + toPersistedToolName, +} from "./toolIds"; export { isLongPressEvaluating }; export { callOnBlur as registerMenuCloseOnBlur }; -const checkLeaveOffTool: string = "Visualizer"; - type ToolboxSettings = Record & { current?: string; visibility?: string; @@ -28,15 +36,14 @@ let savedSettings: ToolboxSettings = {}; let keypressTimer: ReturnType | null = null; -// This variable stores all the ids of the enabled tools, so +// This variable stores the canonical ids of all the enabled tools, so // that the React toolbox settings can initially check the // checkboxes that correspond to the enabled tools let enabledToolIds = new Set(); -// checks if the tool is currently enabled by using its -// name and the enabledToolIds set -export function isToolEnabledInToolbox(toolName: string): boolean { - return enabledToolIds.has(toolName); +// Is the tool with this canonical id currently enabled? +export function isToolEnabledInToolbox(toolId: string): boolean { + return enabledToolIds.has(toolId); } // a function to update the state of the checkboxes in the toolbox settings, @@ -53,11 +60,19 @@ export function setToolboxSettingsChangeHandler( changeToolboxSettingsState = handler; } +export interface IReactTool { + // For tools that require a subscription. This will trigger an indicator communicating that this + // featureName requires a subscription. + featureName?: string; +} + // Each tool implements this interface and adds an instance of its implementation to the // list maintained here. The methods support the different things individual tools -// can be asked to do by the rest of the system. +// can be asked to do by the rest of the system. Everything the toolbox needs to know +// about a tool, including the metadata it shows in the tool's section header, comes from +// here (or is derived from id(); see toolIds.ts). // See ToolboxView.cs class comment for a summary of how to add a new tool. -export interface ITool { +export interface ITool extends IReactTool { beginRestoreSettings(settings: string): JQueryPromise; configureElements(container: HTMLElement); showTool(); // called when a new tool is chosen, but not necessarily when a new page is displayed. @@ -76,7 +91,7 @@ export interface ITool { // allow for this possibility and not repeat any work that was already done. newPageReady(); detachFromPage(); // called when a page is going away AND before hideTool - id(): string; // without trailing "Tool"! + id(): string; // the canonical id, without trailing "Tool"! isAlwaysEnabled(): boolean; // If this is true, the tool may only be selected on pages that have data-tool-id matching this tool's id. requiresToolId(): boolean; @@ -88,12 +103,10 @@ export interface ITool { // notifies the tool that an image has been changed on the page. // If the change only affects one image, it may be passed; otherwise, all should be fixed. imageUpdated(img: HTMLImageElement | undefined): void; -} - -export interface IReactTool { - // For tools that require a subscription. This will trigger an indicator communicating that this - // featureName requires a subscription. - featureName?: string; + // The URL of the icon to show in this tool's toolbox section header, e.g. + // "/bloom/images/microphone-white.svg". Undefined for the few sections that don't + // have an icon. + iconPath(): string | undefined; } // Class that represents the whole toolbox. Gradually we will move more functionality in here. @@ -148,12 +161,11 @@ export class ToolBox { continue; } // We may need to add or remove this tool. - const toolId = ToolBox.addToolToString(tool.id()); - const haveTool = adapter.hasTool(toolId); + const haveTool = adapter.hasTool(tool.id()); const wantTool = requiredToolId === tool.id(); if (haveTool !== wantTool) { // add or remove as needed. (Required tools don't have check boxes.) - showOrHideTool(toolId, wantTool); + showOrHideTool(tool.id(), wantTool); toolsAdjusted = wantTool; } } @@ -312,27 +324,6 @@ export class ToolBox { getTheOneToolbox().doWhenClosingTool.push(task); } - // Append "Tool" to the tool name if it's not already there. - // Put a space between the name and "Tool" if addSpace is true. - public static addToolToString( - toolName: string, - addSpace: boolean = false, - ): string { - if (toolName) { - if ( - toolName.indexOf(checkLeaveOffTool) === -1 && - toolName.indexOf("Tool") === -1 - ) { - if (addSpace) { - return toolName + " Tool"; - } else { - return toolName + "Tool"; - } - } - } - return toolName; - } - // In the process of moving this to shared.ts, but a lot of // code still expects to find it here. public static getPageFrame(): HTMLIFrameElement { @@ -364,9 +355,13 @@ export class ToolBox { masterToolList.push(tool); } + /** + * The tools the book has enabled, as a comma-separated list of tool names. This is the + * one place we ask; the answer drives which sections the toolbox offers. + */ private getEnabledTools() { - // Using axios directly because api calls for returning the promise. - return axios.get("/bloom/api/toolbox/enabledTools"); + // Using axios directly because we want the promise. + return axios.get("/bloom/api/toolbox/enabledTools"); } // Called from document.ready, initializes the whole toolbox. @@ -383,73 +378,70 @@ export class ToolBox { }); hookupLinkHandler(); - // Using axios directly because bloomApi doesn't support merging promises with .all wrapAxios( - axios.all([this.getEnabledTools()]).then( - axios.spread((enabledTools) => { - // remove any experimental tools the user doesn't want - // TODO: give each experimental tool it's own setting once we have any experimental tools again. - // Presumably use the tool id as the keyword in the list of experimental features. - const toolsToLoad = enabledTools.data - .split(",") - .map((toolId: string) => toolId.trim()) - .filter((toolId: string) => toolId.length > 0) - .map((toolId: string) => - toolId.endsWith("Tool") - ? toolId.substring(0, toolId.length - 4) - : toolId, - ); - // remove any tools we don't know about. This might happen where settings were saved in a later version of Bloom. - for (let i = toolsToLoad.length - 1; i >= 0; i--) { - if ( - !masterToolList.some( - (mod) => mod.id() === toolsToLoad[i], - ) - ) { - toolsToLoad.splice(i, 1); - } + this.getEnabledTools().then((enabledTools) => { + // TODO: give each experimental tool its own setting once we have any + // experimental tools again. Presumably use the tool id as the keyword in + // the list of experimental features. + // The names in this list come from the book's meta.json, so they may have + // the historical "Tool" suffix; from here on we work in canonical ids. + const toolsToLoad = enabledTools.data + .split(",") + .map((toolName: string) => toolName.trim()) + .filter((toolName: string) => toolName.length > 0) + .map((toolName: string) => toCanonicalToolId(toolName)); + // remove any tools we don't know about. This might happen where settings were saved in a later version of Bloom. + for (let i = toolsToLoad.length - 1; i >= 0; i--) { + if ( + !masterToolList.some( + (mod) => mod.id() === toolsToLoad[i], + ) + ) { + toolsToLoad.splice(i, 1); } + } - enabledToolIds = new Set(toolsToLoad); + enabledToolIds = new Set(toolsToLoad); - for (let j = 0; j < masterToolList.length; j++) { - // add any tools we always show - if ( - masterToolList[j].isAlwaysEnabled() && - !toolsToLoad.includes(masterToolList[j].id()) - ) { - toolsToLoad.push(masterToolList[j].id()); - } + for (let j = 0; j < masterToolList.length; j++) { + // add any tools we always show + if ( + masterToolList[j].isAlwaysEnabled() && + !toolsToLoad.includes(masterToolList[j].id()) + ) { + toolsToLoad.push(masterToolList[j].id()); } + } - toolsToLoad.push("settings"); - const loadNextTool = () => { - if (toolsToLoad.length === 0) { - this.builtToolbox = true; - // loaded them all, now we can deal with settings. - restoreToolboxSettings(); - } else { - // optimize: maybe we can overlap these? - const nextToolId = toolsToLoad.pop(); - const toolId = ToolBox.addToolToString(nextToolId); - beginAddTool(toolId, false, () => loadNextTool()); - } - }; - loadNextTool(); - }), - ), + // The "More..." section, which is how the user enables the other tools, + // is always offered. + toolsToLoad.push(kSettingsToolId); + const loadNextTool = () => { + if (toolsToLoad.length === 0) { + this.builtToolbox = true; + // loaded them all, now we can deal with settings. + restoreToolboxSettings(); + } else { + // optimize: maybe we can overlap these? + const nextToolId = toolsToLoad.pop()!; + beginAddTool(nextToolId, false, () => loadNextTool()); + } + }; + // Adding the tools requires the toolbox UI, which mounts asynchronously. + whenToolboxReactAdapterReady(() => loadNextTool()); + }), ); } /** - * Is the toolbox currently offering this tool a section? (Despite the name, this does - * not mean the tool is the *current* tool; it never did.) + * Is the toolbox currently offering this tool (canonical id) a section? (Despite the + * name, this does not mean the tool is the *current* tool; it never did.) */ public isToolActive(toolId: string): boolean { return !!getToolboxReactAdapter()?.hasTool(toolId); } - // Enables a tool from an in-page action, ensuring the toolbox is visible. + // Enables a tool (canonical id) from an in-page action, ensuring the toolbox is visible. public enableToolFromPage(toolId: string): void { if (!this.toolboxIsShowing()) { this.toggleToolbox(); @@ -458,9 +450,9 @@ export class ToolBox { } /** - * Makes the given tool (id without the "Tool" suffix) the current tool, enabling it - * first if necessary. Called in response to in-page actions, e.g. clicking a video - * placeholder to get the Sign Language tool. + * Makes the given tool (canonical id) the current tool, enabling it first if + * necessary. Called in response to in-page actions, e.g. clicking a video placeholder + * to get the Sign Language tool. */ public activateToolFromId(toolId: string) { if (!getITool(toolId)) { @@ -480,10 +472,7 @@ export class ToolBox { // The tool may be present without being in enabledToolIds if it is a // required-for-this-page tool (see adjustToolListForPage). - if ( - isToolEnabledInToolbox(toolId) || - this.isToolActive(ToolBox.addToolToString(toolId)) - ) { + if (isToolEnabledInToolbox(toolId) || this.isToolActive(toolId)) { setCurrentTool(toolId); } else { // Genuinely disabled: enable it, which persists the state and updates @@ -543,7 +532,7 @@ export function getActiveToolId(): string | undefined { // it just long enough for the user to see the checkbox they ticked. (BL-16501) const kShowToolAfterEnableDelayMs = 300; -// Pending deferred "open this tool" timers, keyed by tool name, so a later toggle +// Pending deferred "open this tool" timers, keyed by canonical tool id, so a later toggle // of the same tool can cancel an open that hasn't fired yet. // We deliberately don't clear this map on toolbox teardown/navigation: each timer // is ~300ms and removes its own entry when it fires, so at most a couple of very @@ -554,7 +543,7 @@ const pendingShowToolTimeouts = new Map< >(); // modifies the enabledToolIds set, the saved active -// state of the tool in question, and the presence of +// state of the tool in question (canonical id), and the presence of // the tool in the toolbox, whenever the tool is checked // or unchecked in the toolbox settings. // deferShowToRevealCheckbox is set only by the "More..." settings checkboxes: @@ -563,38 +552,33 @@ const pendingShowToolTimeouts = new Map< // ticked. Other callers (e.g. activating a tool from an in-page action) leave it // false so the tool opens immediately. (BL-16501) export function setToolEnabledFromSettings( - toolName: string, + toolId: string, turnOn: boolean, deferShowToRevealCheckbox: boolean = false, ): void { if (turnOn) { - enabledToolIds.add(toolName); + enabledToolIds.add(toolId); } else { - enabledToolIds.delete(toolName); + enabledToolIds.delete(toolId); } - const toolId = - toolName.indexOf(checkLeaveOffTool) === -1 - ? toolName + "Tool" - : toolName; - postString( "editView/saveToolboxSetting", - "active\t" + toolName + "Check\t" + (turnOn ? "1" : "0"), + "active\t" + toEnabledSettingName(toolId) + "\t" + (turnOn ? "1" : "0"), ); if (changeToolboxSettingsState !== undefined) { - changeToolboxSettingsState(toolName, turnOn); + changeToolboxSettingsState(toolId, turnOn); } // A pending deferred open (below) reflects an earlier state; this call // supersedes it, so cancel it. Without this, ticking a tool on and then off // again within the delay would let the stale timer re-add the disabled tool // (the disable runs synchronously and would otherwise be overtaken). - const pendingTimeout = pendingShowToolTimeouts.get(toolName); + const pendingTimeout = pendingShowToolTimeouts.get(toolId); if (pendingTimeout !== undefined) { clearTimeout(pendingTimeout); - pendingShowToolTimeouts.delete(toolName); + pendingShowToolTimeouts.delete(toolId); } if (turnOn && deferShowToRevealCheckbox) { @@ -604,27 +588,27 @@ export function setToolEnabledFromSettings( // checkbox they just ticked. Briefly delay so the checkmark is visible // before the section collapses to reveal the newly-enabled tool. (BL-16501) const timeout = setTimeout(() => { - pendingShowToolTimeouts.delete(toolName); + pendingShowToolTimeouts.delete(toolId); // Guard against the tool having been turned off again during the delay. - if (enabledToolIds.has(toolName)) { + if (enabledToolIds.has(toolId)) { showOrHideTool(toolId, true); } }, kShowToolAfterEnableDelayMs); - pendingShowToolTimeouts.set(toolName, timeout); + pendingShowToolTimeouts.set(toolId, timeout); } else { showOrHideTool(toolId, turnOn); } } function showOrHideTool( - tool: string, + toolId: string, turnOn: boolean, openTool: boolean = true, ) { if (turnOn) { - beginAddTool(tool, openTool); + beginAddTool(toolId, openTool); } else { - getToolboxReactAdapter()?.removeTool(tool); + getToolboxReactAdapter()?.removeTool(toolId); } } @@ -649,13 +633,11 @@ export function applyToolboxStateToUpdatedPage() { savedSettings = result.data; // savedSettings["current"] is always set to the last active tool for the book, // except for new books where it is null. In that case, the default value - // should be talkingBookTool. (BL-16026) - const currentFromBook = ToolBox.addToolToString( - (savedSettings && savedSettings["current"]) || "talkingBookTool", + // should be the talking book tool. (BL-16026) + const currentFromBook = toCanonicalToolId( + (savedSettings && savedSettings["current"]) || kTalkingBookToolId, ); - const currentInToolbox = currentTool - ? ToolBox.addToolToString(currentTool.id()) - : ""; + const currentInToolbox = currentTool ? currentTool.id() : ""; const shouldBeVisible = !!( savedSettings && savedSettings["visibility"] ); @@ -831,9 +813,9 @@ function restoreToolboxSettingsWhenPageReady(settings: ToolboxSettings) { // OK, CKEditor is done (or page doesn't use it), we can finally do the real initialization. const opts = settings; // currentTool is always set except for new books. For new books, it is undefined and we want - // to treat that the same as if it were set to "talkingBookTool" so that the tool will display - // the first time the user opens the toolbox. (BL-16026) - const currentTool = opts["current"] || "talkingBookTool"; + // to treat that the same as if it were set to the talking book tool so that the tool will + // display the first time the user opens the toolbox. (BL-16026) + const currentTool = opts["current"] || kTalkingBookToolId; const shouldBeVisible = !!opts["visibility"]; if (toolbox.toolboxIsShowing() !== shouldBeVisible) { @@ -854,20 +836,22 @@ export function removeToolboxMarkup() { detachCurrentTool(); } -function switchTool(newToolName: string): void { - // Have Bloom remember which tool is active. (Might be none) - postString("editView/saveToolboxSetting", "current\t" + newToolName); +/** + * Called when the toolbox UI reports that a different section is now the active one. + * newToolId is a canonical tool id (the toolbox UI only ever reports tools it is offering, + * and it was told about them by their canonical ids). + */ +function switchTool(newToolId: string): void { + // Have Bloom remember which tool is active. (Might be none.) The book's meta.json + // has always stored this with the historical "Tool" suffix. + postString( + "editView/saveToolboxSetting", + "current\t" + toPersistedToolName(newToolId), + ); let newTool: ITool | null = null; - if (newToolName) { - for (let i = 0; i < masterToolList.length; i++) { - // the newToolName comes from meta.json and we've changed our minds a few times about - // whether it should end in "Tool" so what's in the meta.json might have it or not. - // For robustness we will recognize any tool name that starts with the (no -Tool) - // name we're looking for. - if (newToolName.startsWith(masterToolList[i].id())) { - newTool = masterToolList[i]; - } - } + if (newToolId) { + newTool = + masterToolList.find((tool) => tool.id() === newToolId) ?? null; } const canActivateNewTool = !!newTool && isToolInitialized(newTool); const shouldSwitchAwayFromCurrent = @@ -906,7 +890,7 @@ function activateTool(newTool: ITool) { // Does the toolbox have a section for this tool? Only then does it have somewhere to // display itself and does it make sense to run its lifecycle methods. function isToolInitialized(tool: ITool): boolean { - return toolbox.isToolActive(ToolBox.addToolToString(tool.id())); + return toolbox.isToolActive(tool.id()); } async function activateToolInternalAsync(newTool: ITool): Promise { @@ -925,11 +909,11 @@ async function activateToolInternalAsync(newTool: ITool): Promise { * Attempts to make the given tool the current one (normally the tool the book was last * using). If the toolbox isn't offering that tool, falls back to the first tool it does * offer. Passing an empty id also means "whatever tool is first". + * The id may arrive in either spelling, because one caller passes the book's saved + * "current" tool name straight from meta.json. */ -function setCurrentTool(toolID: string) { - // I'm downright grumpy about how this code sometimes uses names with "Tool" appended, sometimes doesn't. - // For now I'm just making functions work with either form. - toolID = ToolBox.addToolToString(toolID); +function setCurrentTool(toolId: string) { + toolId = toCanonicalToolId(toolId); const adapter = getToolboxReactAdapter(); if (!adapter) { @@ -939,46 +923,41 @@ function setCurrentTool(toolID: string) { } if (!toolboxReactActivationHooked) { - adapter.onActiveToolChanged((newToolName: string) => { - switchTool(newToolName); + adapter.onActiveToolChanged((newToolId: string) => { + switchTool(newToolId); }); toolboxReactActivationHooked = true; } // NOTE: the More (settings) section cannot be the "currentTool", so getFirstToolId() // never returns it. - if (!toolID) { - toolID = adapter.getFirstToolId() ?? ""; + if (!toolId) { + toolId = adapter.getFirstToolId() ?? ""; } - if (toolID) { + if (toolId) { const tool = masterToolList.find( - (possibleTool) => - ToolBox.addToolToString(possibleTool.id()) === toolID, + (possibleTool) => possibleTool.id() === toolId, ); if (tool && !isToolInitialized(tool)) { // The tool we were asked for isn't in the toolbox (e.g., it was disabled // since we saved the setting), so fall back to whatever is first. - toolID = adapter.getFirstToolId() ?? ""; + toolId = adapter.getFirstToolId() ?? ""; } } - if (toolID) { - adapter.setActiveToolByToolId(toolID); + if (toolId) { + adapter.setActiveToolByToolId(toolId); } } -// Parameter 'toolId' may be spelled with or without the 'Tool' suffix. +// Parameter 'toolId' may be spelled with or without the 'Tool' suffix, since it may have +// come from persisted data. // Returns undefined if we know of no such tool, e.g. because the book's settings were // saved by a version of Bloom that had a tool this one doesn't. function getITool(toolId: string): ITool { - // I'm downright grumpy about how this code sometimes uses names with "Tool" appended, sometimes doesn't. - // For now I'm just making functions work with either form. - const reactToolId = - toolId.indexOf("Tool") > -1 - ? toolId.substring(0, toolId.length - 4) - : toolId; // strip off "Tool" - return masterToolList.find((tool) => tool.id() === reactToolId)!; + const canonicalToolId = toCanonicalToolId(toolId); + return masterToolList.find((tool) => tool.id() === canonicalToolId)!; } /** @@ -999,14 +978,13 @@ function beginAddTool( return; } - const toolName = ToolBox.addToolToString(tool.id()); const adapter = getToolboxReactAdapter(); // Adding a tool that is already there does nothing, so it is safe to do this // whether or not the toolbox is already offering it. - adapter?.addTool(toolName); + adapter?.addTool(tool.id()); if (openTool && toolbox.toolboxIsShowing()) { - adapter?.setActiveToolByToolId(toolName); + adapter?.setActiveToolByToolId(tool.id()); } if (whenLoaded) { @@ -1420,7 +1398,7 @@ function showToolboxChanged(wasShowing: boolean): void { // enabled. (This should never happen; we're just being defensive.) const adapter = getToolboxReactAdapter(); adapter?.setActiveToolByToolId( - adapter.getFirstToolId() ?? "talkingBookTool", + adapter.getFirstToolId() ?? kTalkingBookToolId, ); } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts index feedf62f41a7..c2fdbed8776b 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts @@ -12,9 +12,9 @@ // When all the tools are React components, each one will belong to its own accordion // section and manage its own state and lifecycle, and this module can go away. // -// Every toolId parameter and result here may be spelled with or without the historical -// "Tool" suffix ("canvas" and "canvasTool" mean the same tool); the implementation -// normalizes them. +// Every toolId parameter and result here is a canonical tool id, i.e. what the tool's +// ITool.id() returns, with no "Tool" suffix (e.g. "canvas", not "canvasTool"). See +// toolIds.ts for where the suffixed spellings are converted at our boundaries. export interface IToolboxReactAdapter { // Makes the tool with this id the active, expanded section of the React accordion. setActiveToolByToolId(toolId: string): void; @@ -29,20 +29,42 @@ export interface IToolboxReactAdapter { removeTool(toolId: string): void; // Is the toolbox currently offering a section for this tool? hasTool(toolId: string): boolean; - // The id (with the "Tool" suffix) of the first tool section, or undefined if there - // are no tool sections. The "More..." (settings) section doesn't count; it is not a - // tool that can be current. + // The id of the first tool section, or undefined if there are no tool sections. + // The "More..." (settings) section doesn't count; it is not a tool that can be current. getFirstToolId(): string | undefined; } let theOneToolboxReactAdapter: IToolboxReactAdapter | undefined; +// Actions given to whenToolboxReactAdapterReady() before ToolboxRoot had mounted. Each is +// run (and forgotten) as soon as it has. +const actionsWaitingForAdapter: ((adapter: IToolboxReactAdapter) => void)[] = + []; + /** * Called by ToolboxRoot once it has mounted, making the adapter available to the legacy * toolbox code. */ export function setToolboxReactAdapter(adapter: IToolboxReactAdapter): void { theOneToolboxReactAdapter = adapter; + actionsWaitingForAdapter.splice(0).forEach((action) => action(adapter)); +} + +/** + * Runs the action as soon as ToolboxRoot has published its adapter (immediately, if it + * already has). Startup renders ToolboxRoot before initializing the rest of the toolbox, + * but React mounts asynchronously, so code that must not silently do nothing (in + * particular, populating the toolbox with the book's tools) waits here rather than + * assuming the adapter already exists. + */ +export function whenToolboxReactAdapterReady( + action: (adapter: IToolboxReactAdapter) => void, +): void { + if (theOneToolboxReactAdapter) { + action(theOneToolboxReactAdapter); + return; + } + actionsWaitingForAdapter.push(action); } /** diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx b/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx index 97e375442745..6b87d2e9f247 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx @@ -19,6 +19,15 @@ export default abstract class ToolboxToolReactAdaptor return false; } + /** + * The URL of the icon for this tool's toolbox section header. Tools that don't show one + * (the "More..." section, and tools that only appear on pages that ask for them) don't + * override this. + */ + public iconPath(): string | undefined { + return undefined; + } + protected adaptReactElement( element: ReactElement, ): HTMLDivElement { diff --git a/src/BloomExe/Edit/ToolboxView.cs b/src/BloomExe/Edit/ToolboxView.cs index 6ad4e5f388d1..7996a4a78526 100644 --- a/src/BloomExe/Edit/ToolboxView.cs +++ b/src/BloomExe/Edit/ToolboxView.cs @@ -17,8 +17,8 @@ namespace Bloom.Edit /// Thus, unlike other View classes in Bloom, ToolboxView does not inherit from a Control class, /// nor are there ever any instances; all methods are currently static. /// Currently necessary steps to add a new tool: - /// - Add the tool's folder to ToolboxView.GetToolboxServerDirectories(). /// - Create a folder under BloomBrowserUI/bookEdit/toolbox. Its name should match the toolId (see below). + /// - Usually you will add a line for that folder to GetToolboxServerDirectories() in this file. /// - Create a file in that folder with extension .tsx to contain the React code of the panel /// - it (or another file) should have a class which implements ITool /// - minimally this must implement id() to return the tool ID @@ -28,20 +28,18 @@ namespace Bloom.Edit /// ToolBox.registerTool(new MyWonderfulTool()); /// - should implement makeRootElement() to create one div, the react root. /// - the returned root should already have been passed to ReactDOM.render(). - /// - Make a new xlf entry with ID EditTab.Toolbox.{UCToolId}.Heading, - /// where UCToolId is the capitalized version of your tool Id, e.g., "Music". - /// We currently assume the default English value of this will be UCToolId Tool, e.g., "Music Tool" - /// (This supports localization of the tool's accordion tab label.) - /// - In some toolbox less file (typically a new one for your tool, but could be toolbox.less) - /// you need to create a rule like - /// .ui-accordion h3[data-toolId="motionTool"] span.ui-accordion-header-icon { - /// background-image:url('/bloom/images/motion.svg') !important; - /// } - /// which specifies the icon for your tool. (And create the icon in the BloomBrowserUI/images folder). - /// - Usually you will add a line to GetToolboxServerDirectories() in this file - /// - Add two lines like this to src\BloomBrowserUI\bookEdit\toolbox\settings\Settings.pug - /// .checkbox.clear#musicCheck(data-tool='musicTool', onclick='workspaceBundle.showOrHideTool_click(this);') - /// .checkbox-label(data-i18n='EditTab.Toolbox.Music.Heading') Music Tool + /// - should implement iconPath() to return the URL of the icon for the tool's section + /// header, e.g. "/bloom/bookEdit/toolbox/motion/motion.svg" (create the icon in the + /// tool's own folder, or in BloomBrowserUI/images). + /// - Make a new xlf entry with ID EditTab.Toolbox.{UCToolId}Tool, where UCToolId is the + /// capitalized version of your tool Id, e.g., "Music", giving the key + /// "EditTab.Toolbox.MusicTool". We currently assume the default English value of this + /// will be UCToolId Tool, e.g., "Music Tool". (This localizes both the tool's section + /// header and its checkbox in the "More..." section. See toolIds.ts, which derives + /// the label and key from the tool id, for the exact convention.) + /// That is all: the toolbox derives the section header (label, icon, subscription badge) and + /// the tool's checkbox in the "More..." section from the ITool implementation, so there is no + /// list of tools to add to anywhere else. /// public class ToolboxView { From 6f349c2fff75f5ba34464e12e6a2dc055b1a8066 Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Fri, 24 Jul 2026 22:02:41 -0700 Subject: [PATCH 05/19] Fix BL-16608 Cleanup toolbox infrastructure (5/7): simplify the tool contract https://issues.bloomlibrary.org/youtrack/issue/BL-16608 - Merged IReactTool into ITool: featureName is now an optional readonly member of ITool, and the "tool as unknown as IReactTool" casts are gone. - beginRestoreSettings is now honestly typed: beginRestoreSettings(settings: IToolboxSettings): Promise. The old signature claimed a string but callers passed the settings object through "as unknown as string" and implementations cast it back. IToolboxSettings models what GET toolbox/settings actually returns (current, visibility, and one "State" string per tool). The base implementation returns Promise.resolve() instead of a manufactured resolved $.Deferred, and the three real overrides (talkingBook, decodableReader, leveledReader) are now async methods without the double casts. jQuery import dropped from toolboxToolReactAdaptor.tsx and leveledReaderTool.tsx. - Page-access helpers deduplicated into utils/shared.ts (finishing the move the old comments described as in progress): isXmatterPage joins getPageIFrame/getPageIframeBody/getBloomPageElement, and the duplicate statics on ToolBox and ToolboxToolReactAdaptor are deleted, with all callers importing from shared.ts directly. One reconciled difference: the adaptor's isXmatter decoded the class attribute with decodeURIComponent; the merged version reads it raw (class is never URL-encoded, and decoding could throw on a stray %). Known minor timing nuance: tools that don't override beginRestoreSettings now resolve one microtask later, so showTool() runs after currentTool is assigned - strictly more consistent, and the one consumer that cared (audioRecording.doesCurrentToolPlayAudio) tolerates both orders. Verified: pnpm typecheck passes (raw tsgo error set identical to baseline), eslint 0 errors (one warning fewer than baseline, none new), full vitest suite 550 passed / 5 skipped, build/agent-vite.sh bundle compiles. Co-Authored-By: Claude Fable 5 --- .../toolbox/canvas/CanvasToolControls.tsx | 6 +- .../bookEdit/toolbox/games/GameTool.tsx | 18 +-- .../bookEdit/toolbox/games/ThemeChooser.tsx | 4 +- .../imageDescription/imageDescription.tsx | 10 +- .../impairmentVisualizer.tsx | 7 +- .../bookEdit/toolbox/motion/motionTool.tsx | 4 +- .../toolbox/music/musicToolControls.tsx | 5 +- .../toolbox/readers/ReaderToolSwitch.tsx | 5 +- .../decodableReader/decodableReaderTool.tsx | 120 +++++++++--------- .../leveledReader/leveledReaderTool.tsx | 70 +++++----- .../toolbox/readers/readerToolPageState.ts | 4 +- .../toolbox/signLanguage/signLanguageTool.tsx | 6 +- .../TalkingBook-React-Conversion-Plan.md | 2 +- .../toolbox/talkingBook/audioRecording.ts | 6 +- .../toolbox/talkingBook/talkingBookTool.tsx | 15 ++- .../bookEdit/toolbox/toolbox.ts | 114 +++++++---------- .../toolbox/toolboxToolReactAdaptor.tsx | 72 +++-------- src/BloomBrowserUI/utils/shared.ts | 28 ++++ 18 files changed, 236 insertions(+), 260 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/CanvasToolControls.tsx b/src/BloomBrowserUI/bookEdit/toolbox/canvas/CanvasToolControls.tsx index dce99af3b854..409826034c56 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/CanvasToolControls.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/CanvasToolControls.tsx @@ -3,7 +3,7 @@ import tinycolor from "tinycolor2"; import * as React from "react"; import { useState, useEffect, useCallback, useRef } from "react"; -import ToolboxToolReactAdaptor from "../toolboxToolReactAdaptor"; +import { isXmatterPage } from "../../../utils/shared"; import "./canvasTool.less"; import { getWorkspaceBundleExports } from "../../js/workspaceFrames"; import { @@ -117,7 +117,7 @@ const CanvasToolControls: React.FunctionComponent = () => { // and partly we want to show a special message if someone tries to use it there. const [pageTypeForbidsCanvasTools, setPageTypeForbidsCanvasTools] = useState( - ToolboxToolReactAdaptor.isXmatter({ + isXmatterPage({ returnFalseForCustomPage: true, }), ); @@ -238,7 +238,7 @@ const CanvasToolControls: React.FunctionComponent = () => { const refreshFromCurrentPage = useCallback(() => { bubbleSpecInitialization(); setPageTypeForbidsCanvasTools( - ToolboxToolReactAdaptor.isXmatter({ + isXmatterPage({ returnFalseForCustomPage: true, }), ); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx index 998e281fa73c..32debf7adb2c 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx @@ -22,7 +22,7 @@ import { CanvasElementVideoItem, setGeneratedDraggableId, } from "../canvas/CanvasElementItem"; -import { ToolBox } from "../toolbox"; +import { getBloomPageElement, getPageIframeBody } from "../../../utils/shared"; import { adjustDraggablesForLanguage, classSetter, @@ -672,7 +672,7 @@ export const playTabIndex = 3; // the page itself, because we don't want it saved. It's better than putting it on the body, // because that doesn't work in Bloom Player due to the way we polyfill scoped styles. const updateTabClass = (tabIndex: number) => { - const pageBody = ToolBox.getPage(); + const pageBody = getPageIframeBody(); const page = pageBody?.getElementsByClassName( "bloom-page", )[0] as HTMLElement; @@ -701,7 +701,7 @@ const updateTabClass = (tabIndex: number) => { }; const getPage = () => { - const pageBody = ToolBox.getPage(); + const pageBody = getPageIframeBody(); return pageBody?.getElementsByClassName("bloom-page")[0] as HTMLElement; }; @@ -972,7 +972,7 @@ const DragActivityControls: React.FunctionComponent<{ // Get various state values from the current page, initially and whenever it changes. useEffect(() => { const getStateFromPage = () => { - const pageBody = ToolBox.getPage(); + const pageBody = getPageIframeBody(); const page = pageBody?.getElementsByClassName( "bloom-page", )[0] as HTMLElement; @@ -1715,7 +1715,7 @@ export class GameTool extends ToolboxToolReactAdaptor { this.renderRoot(); } public static areGameTabsActive(): boolean { - const page = GameTool.getBloomPage(); + const page = getBloomPageElement(); return ( !!page && !!page.ownerDocument.getElementById(kIdForDragActivityTabControl) @@ -1778,7 +1778,7 @@ export class GameTool extends ToolboxToolReactAdaptor { // This really only needs to be done once, but I haven't found a good place to do that, // and it's not expensive. setDefaultSoundUrls(defaultCorrectSoundUrl, defaultWrongSoundUrl); - const page = GameTool.getBloomPage(); + const page = getBloomPageElement(); randomlyAssignTargetsIfNeeded(page); const pageFrameExports = getEditablePageBundleExports(); @@ -1819,7 +1819,7 @@ export class GameTool extends ToolboxToolReactAdaptor { } public detachFromPage() { - const page = GameTool.getBloomPage(); + const page = getBloomPageElement(); if (page) { undoPrepareActivity(page); } @@ -1926,7 +1926,7 @@ function scheduleRemoveBloomSelectedFromTargets(page: HTMLElement): void { // getToolboxBundleExports()?.setActiveDragActivityTab(), which works in any bundle. // Even in this file, a calling function could be running in the page bundle. export function setActiveDragActivityTab(tab: number) { - const page = GameTool.getBloomPage(); + const page = getBloomPageElement(); const pageFrameExports = getEditablePageBundleExports(); if (!page || !pageFrameExports) { // just loading page?? @@ -2040,7 +2040,7 @@ export function setActiveDragActivityTab(tab: number) { // Replace the origami control with the Game tab control if the page is a game. export function setupDragActivityTabControl() { - const page = GameTool.getBloomPage(); + const page = getBloomPageElement(); if (!page) { return; } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/games/ThemeChooser.tsx b/src/BloomBrowserUI/bookEdit/toolbox/games/ThemeChooser.tsx index 9529b1e3bd29..43954b93b4ff 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/games/ThemeChooser.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/games/ThemeChooser.tsx @@ -1,7 +1,7 @@ import { css, ThemeProvider } from "@emotion/react"; import * as React from "react"; import { useEffect, useState } from "react"; -import { ToolBox } from "../toolbox"; +import { getPageIframeBody } from "../../../utils/shared"; import { getVariationsOnClass } from "../../../utils/getVariationsOnClass"; import { kOptionPanelBackgroundColor, @@ -13,7 +13,7 @@ import { InfoIconUrl } from "../../../react_components/icons/InfoIconUrl"; import BloomSelect from "../../../react_components/bloomSelect"; const getPage = () => { - const pageBody = ToolBox.getPage(); + const pageBody = getPageIframeBody(); return pageBody?.getElementsByClassName("bloom-page")[0] as HTMLElement; }; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx index 6c25410ba615..81583c05a6e1 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx @@ -4,7 +4,7 @@ import $ from "jquery"; import * as React from "react"; import { renderForInstance } from "../../../utils/reactRender"; import { post } from "../../../utils/bloomApi"; -import { ToolBox } from "../toolbox"; +import { getPageIframeBody, isXmatterPage } from "../../../utils/shared"; import { getEditablePageBundleExports } from "../../js/workspaceFrames"; import "./imageDescription.less"; import ToolboxToolReactAdaptor from "../toolboxToolReactAdaptor"; @@ -185,12 +185,12 @@ export class ImageDescriptionToolControls extends React.Component< this.setState({ enabled: true, descriptionNotNeeded: noDescriptionNeeded === "true", - isXmatterPage: ToolBox.isXmatterPage(), + isXmatterPage: isXmatterPage(), }); } public setStateForNewPage(): void { - const page = ToolboxToolReactAdaptor.getPage(); + const page = getPageIframeBody(); if (!page) { this.setDisabledState(); return; @@ -358,7 +358,7 @@ export class ImageDescriptionAdapter extends ToolboxToolReactAdaptor { } public detachFromPage() { - const page = ToolBox.getPage(); + const page = getPageIframeBody(); if (page) { hideImageDescriptions(page); } @@ -394,7 +394,7 @@ export class ImageDescriptionAdapter extends ToolboxToolReactAdaptor { const imageDescControls = this.reactControls; if (imageDescControls) { imageDescControls.setStateForNewPage(); - const page = ToolBox.getPage(); + const page = getPageIframeBody(); if (!page) { return; } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx b/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx index 4e87e5a08a27..716016c18e54 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx @@ -2,6 +2,7 @@ import { css } from "@emotion/react"; import * as React from "react"; import ToolboxToolReactAdaptor from "../toolboxToolReactAdaptor"; +import { getPageIframeBody } from "../../../utils/shared"; import { Div, Span } from "../../../react_components/l10nComponents"; import { get, postDataWithConfig } from "../../../utils/bloomApi"; import "./impairmentVisualizer.less"; @@ -142,7 +143,7 @@ export class ImpairmentVisualizerControls extends React.Component< // ones, which are done a pixel at a time, so if they are being updated frequently (like // during a drag), this optimization really helps make things less jerky. public updateSimulations(img: HTMLImageElement | undefined) { - const page = ToolboxToolReactAdaptor.getPage(); + const page = getPageIframeBody(); if (!page || !page.ownerDocument) return; const body = page.ownerDocument.body; if (this.simulatingCataracts) { @@ -190,7 +191,7 @@ export class ImpairmentVisualizerControls extends React.Component< } public static removeImpairmentVisualizerMarkup() { - const page = ToolboxToolReactAdaptor.getPage(); + const page = getPageIframeBody(); if (!page || !page.ownerDocument) return; ImpairmentVisualizerControls.removeColorBlindnessMarkup(page); const body = page.ownerDocument.body; @@ -238,7 +239,7 @@ export class ImpairmentVisualizerControls extends React.Component< // updates to correctly handle when it shows and hides. return; } - const page = ToolboxToolReactAdaptor.getPage(); + const page = getPageIframeBody(); if (!page || !page.ownerDocument) return; const canvas = page.ownerDocument.createElement("canvas"); const imageContainer = img.parentElement; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx index a3789550f4e3..548df17a6304 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx @@ -24,7 +24,7 @@ import { getFeatureStatusAsync } from "../../../react_components/featureStatus"; import { TransformBasedAnimator } from "bloom-player"; import { getCanvasElementManager } from "../canvas/canvasElementPageBridge"; import { kBloomCanvasClass } from "../canvas/canvasElementConstants"; -import { animateStyleName } from "../../../utils/shared"; +import { animateStyleName, isXmatterPage } from "../../../utils/shared"; import { ThemeProvider } from "@mui/material/styles"; import { toolboxTheme } from "../../../bloomMaterialUITheme"; @@ -964,7 +964,7 @@ export class MotionTool extends ToolboxToolReactAdaptor { let motionChecked = true; let motionPossible = !doNotHaveAPicture; - if (!bloomCanvasToAnimate || ToolboxToolReactAdaptor.isXmatter()) { + if (!bloomCanvasToAnimate || isXmatterPage()) { // if there's no place to put an image, we can't be enabled. // And we don't support Motion in xmatter (BL-5427), // in part because we use background-image there and haven't fully supported diff --git a/src/BloomBrowserUI/bookEdit/toolbox/music/musicToolControls.tsx b/src/BloomBrowserUI/bookEdit/toolbox/music/musicToolControls.tsx index 50f250bbc5f3..eccfc108a97b 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/music/musicToolControls.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/music/musicToolControls.tsx @@ -2,6 +2,7 @@ import * as React from "react"; import { css, ThemeProvider } from "@emotion/react"; import { toolboxTheme } from "../../../bloomMaterialUITheme"; import ToolboxToolReactAdaptor from "../toolboxToolReactAdaptor"; +import { getBloomPageElement, getPageIFrame } from "../../../utils/shared"; import { Div, Label, Span } from "../../../react_components/l10nComponents"; import { RadioGroup } from "../../../react_components/RadioGroup"; import { get } from "../../../utils/bloomApi"; @@ -289,7 +290,7 @@ export class MusicToolControls extends React.Component { setPlayState(false); return; } - const bookSrc = ToolboxToolReactAdaptor.getPageFrame().src; + const bookSrc = getPageIFrame().src; const index = bookSrc.lastIndexOf("/"); const bookFolderUrl = bookSrc.substring(0, index + 1); const musicUrl = encodeURI(bookFolderUrl + "audio/" + audioFileName); @@ -331,7 +332,7 @@ export class MusicToolControls extends React.Component { this.pausePlaying(); // pauses player and sets playing state to false break; case "continueMusic": { - const bloomPage = ToolboxToolReactAdaptor.getBloomPage(); + const bloomPage = getBloomPageElement(); if (bloomPage) { bloomPage.removeAttribute(MusicToolControls.musicAttrName); } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/ReaderToolSwitch.tsx b/src/BloomBrowserUI/bookEdit/toolbox/readers/ReaderToolSwitch.tsx index 6ed639ff8b81..b6dda82f05bc 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/readers/ReaderToolSwitch.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/ReaderToolSwitch.tsx @@ -2,7 +2,8 @@ import { css } from "@emotion/react"; import * as React from "react"; import { ThemeProvider } from "@emotion/react"; import { toolboxTheme } from "../../../bloomMaterialUITheme"; -import { ToolBox, applyToolboxStateToUpdatedPage } from "../toolbox"; +import { applyToolboxStateToUpdatedPage } from "../toolbox"; +import { getPageIframeBody } from "../../../utils/shared"; import { BloomSwitch } from "../../../react_components/BloomSwitch"; import { postBoolean } from "../../../utils/bloomApi"; import { isReaderToolEnabledOnCurrentPage } from "./readerToolPageState"; @@ -47,7 +48,7 @@ export const ReaderToolSwitch: React.FunctionComponent<{ // Set the class on the page we are currently working with in edit mode. // This just ensures our display is correct while editing. Persisting the value is done below. - ToolBox.getPage()?.classList.toggle( + getPageIframeBody()?.classList.toggle( `${prefix}-reader`, checked, ); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx index 20ea458ecf49..adc10ec32d69 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx @@ -5,7 +5,7 @@ import { getTheOneReaderToolsModel, MarkupType } from "../readerToolsModel"; import { get } from "../../../../utils/bloomApi"; import { isReaderToolEnabledOnCurrentPage } from "../readerToolPageState"; import { renderRoot } from "../../../../utils/reactRender"; -import { isLongPressEvaluating } from "../../toolbox"; +import { isLongPressEvaluating, IToolboxSettings } from "../../toolbox"; import StyleEditor from "../../../StyleEditor/StyleEditor"; import $ from "jquery"; @@ -51,72 +51,68 @@ export class DecodableReaderTool extends ToolboxToolReactAdaptor { getTheOneReaderToolsModel().setCkEditorLoaded(); // we don't call showTool until it is. // Toggle render is handled in newPageReady(), where page reader classes are settled. } - public beginRestoreSettings(settings: string): JQueryPromise { - return beginInitializeDecodableReaderTool().then(() => { - const restoreDone = $.Deferred(); - const model = getTheOneReaderToolsModel(); - const decodableReaderState = ( - settings as unknown as Record - )["decodableReaderState"]; - // This wrapper function ensures that the promise gets resolved, - // even in the very unlikely case that setStageNumber fails. - const runStageRestore = ( - work: () => void | Promise, - ): void => { - try { - Promise.resolve(work()).then( - () => restoreDone.resolve(), - () => restoreDone.resolve(), - ); - } catch { - restoreDone.resolve(); - } - }; + /** Restores the stage (and sort order) this book was last using; see ITool. */ + public async beginRestoreSettings( + settings: IToolboxSettings, + ): Promise { + await beginInitializeDecodableReaderTool(); + const model = getTheOneReaderToolsModel(); + const decodableReaderState = settings["decodableReaderState"]; + // This wrapper function ensures that we still finish restoring, + // even in the very unlikely case that setStageNumber fails. + const runStageRestore = async ( + work: () => void | Promise, + ): Promise => { + try { + await work(); + } catch { + // Nothing useful we can do about it; don't hold up showing the tool. + } + }; - if (decodableReaderState) { - const decState = decodableReaderState; - if (decState.startsWith("stage:")) { - const parts = decState.split(";"); - const stage = parseInt(parts[0].substring("stage:".length)); - const sort = parts[1].substring("sort:".length); - // The true's passed here prevent re-saving the state we just read. - // One non-obvious implication is that simply opening a stage-4 book - // will not switch the default stage for new books to 4. That only - // happens when you CHANGE the stage in the toolbox. - if (model.sort !== sort) { - model.setSort(sort, true); - } - if (model.stageNumber === stage) { - restoreDone.resolve(); - return restoreDone.promise(); - } - runStageRestore(() => model.setStageNumber(stage, true)); - } else { - // old state - const stage = parseInt(decState, 10); - if (model.stageNumber === stage) { - restoreDone.resolve(); - return restoreDone.promise(); - } - runStageRestore(() => model.setStageNumber(stage, true)); + if (decodableReaderState) { + if (decodableReaderState.startsWith("stage:")) { + const parts = decodableReaderState.split(";"); + const stage = parseInt(parts[0].substring("stage:".length)); + const sort = parts[1].substring("sort:".length); + // The true's passed here prevent re-saving the state we just read. + // One non-obvious implication is that simply opening a stage-4 book + // will not switch the default stage for new books to 4. That only + // happens when you CHANGE the stage in the toolbox. + if (model.sort !== sort) { + model.setSort(sort, true); } + if (model.stageNumber === stage) { + return; + } + await runStageRestore(() => model.setStageNumber(stage, true)); } else { - get( - "readers/io/defaultStage", - (result) => { - // Presumably a brand new book. We'd better save the settings we come up with in it. - const stage = parseInt(result.data, 10); - if (model.stageNumber === stage) { - restoreDone.resolve(); - return; - } - runStageRestore(() => model.setStageNumber(stage)); - }, - () => restoreDone.resolve(), - ); + // old state + const stage = parseInt(decodableReaderState, 10); + if (model.stageNumber === stage) { + return; + } + await runStageRestore(() => model.setStageNumber(stage, true)); } + return; + } - return restoreDone.promise(); + await new Promise((resolve) => { + get( + "readers/io/defaultStage", + (result) => { + // Presumably a brand new book. We'd better save the settings we come up with in it. + const stage = parseInt(result.data, 10); + if (model.stageNumber === stage) { + resolve(); + return; + } + runStageRestore(() => model.setStageNumber(stage)).then( + () => resolve(), + ); + }, + () => resolve(), + ); }); } public setupReaderKeyAndFocusHandlers(container: HTMLElement): void { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx index 5be05ccac585..2779a138f40c 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx @@ -5,7 +5,7 @@ import { isReaderToolEnabledOnCurrentPage } from "../readerToolPageState"; import { beginInitializeLeveledReaderTool } from "../readerTools"; import { getTheOneReaderToolsModel } from "../readerToolsModel"; import { LeveledReaderToolControls } from "./LeveledReaderToolControls"; -import $ from "jquery"; +import { IToolboxSettings } from "../../toolbox"; // This class renders the LeveledReaderToolControls React component // in the toolbox, and implements all the functionality/logic needed @@ -35,41 +35,39 @@ export class LeveledReaderTool extends ToolboxToolReactAdaptor { // this function restores the level that was last saved, // as well as the data for that stage, so that the tool // doesn't restart at level 1 all the time - public beginRestoreSettings(opts: string): JQueryPromise { - return beginInitializeLeveledReaderTool().then(() => { - const restoreDone = $.Deferred(); - // opts can be undefined/null when the tool is activated for a book that has no - // saved leveled-reader settings. Guard before indexing so we fall through to the - // default-level path instead of throwing an unhandled promise rejection that - // aborts tool activation (Sentry BLOOM-DESKTOP-FFH). - const leveledReaderState = ( - opts as unknown as Record | undefined - )?.["leveledReaderState"]; - if (leveledReaderState) { - // The true passed here prevents re-saving the state we just read. - // One non-obvious implication is that simply opening a level-4 book - // will not switch the default level for new books to 4. That only - // happens when you CHANGE the level in the toolbox. - getTheOneReaderToolsModel().setLevelNumber( - parseInt(leveledReaderState, 10), - true, - ); - restoreDone.resolve(); - } else { - get( - "readers/io/defaultLevel", - (result) => { - // Presumably a brand new book. We'd better save the settings we come up with in it. - getTheOneReaderToolsModel().setLevelNumber( - parseInt(result.data, 10), - ); - restoreDone.resolve(); - }, - () => restoreDone.resolve(), - ); - } - - return restoreDone.promise(); + public async beginRestoreSettings( + settings: IToolboxSettings, + ): Promise { + await beginInitializeLeveledReaderTool(); + // Despite the type, settings can be undefined/null at runtime when the tool is + // activated for a book that has no saved leveled-reader settings. Guard before + // indexing so we fall through to the default-level path instead of throwing an + // unhandled promise rejection that aborts tool activation + // (Sentry BLOOM-DESKTOP-FFH). + const leveledReaderState = settings?.["leveledReaderState"]; + if (leveledReaderState) { + // The true passed here prevents re-saving the state we just read. + // One non-obvious implication is that simply opening a level-4 book + // will not switch the default level for new books to 4. That only + // happens when you CHANGE the level in the toolbox. + getTheOneReaderToolsModel().setLevelNumber( + parseInt(leveledReaderState, 10), + true, + ); + return; + } + await new Promise((resolve) => { + get( + "readers/io/defaultLevel", + (result) => { + // Presumably a brand new book. We'd better save the settings we come up with in it. + getTheOneReaderToolsModel().setLevelNumber( + parseInt(result.data, 10), + ); + resolve(); + }, + () => resolve(), + ); }); } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolPageState.ts b/src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolPageState.ts index 723eee902b25..b699ea4d8bc1 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolPageState.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolPageState.ts @@ -1,10 +1,10 @@ -import { ToolBox } from "../toolbox"; +import { getPageIframeBody } from "../../../utils/shared"; export function isReaderToolEnabledOnCurrentPage( isForLeveled: boolean, ): boolean { const prefix = isForLeveled ? "leveled" : "decodable"; - return !!ToolBox.getPage()?.classList.contains(`${prefix}-reader`); + return !!getPageIframeBody()?.classList.contains(`${prefix}-reader`); } export function isReaderToolTurnedOff(isForLeveled: boolean): boolean { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx index 2757cdf38a18..ad203cf9433a 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx @@ -1,7 +1,7 @@ import * as React from "react"; import { renderForInstance } from "../../../utils/reactRender"; import { Label } from "../../../react_components/l10nComponents"; -import { ToolBox } from "../toolbox"; +import { getPageIframeBody } from "../../../utils/shared"; import ToolboxToolReactAdaptor from "../toolboxToolReactAdaptor"; import "./signLanguage.less"; import { @@ -988,7 +988,7 @@ export class SignLanguageTool extends ToolboxToolReactAdaptor { // Specify 'true' to get only containers marked as selected public static getVideoContainers(selected?: boolean): HTMLElement[] { - const page = ToolBox.getPage(); + const page = getPageIframeBody(); if (!page) { return []; } @@ -1098,7 +1098,7 @@ export class SignLanguageTool extends ToolboxToolReactAdaptor { } private syncSelectionFromCurrentPage() { - const pageBody = ToolBox.getPage(); + const pageBody = getPageIframeBody(); if (!pageBody) { // Tool activation can race with page readiness. window.setTimeout(() => this.syncSelectionFromCurrentPage(), 100); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/TalkingBook-React-Conversion-Plan.md b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/TalkingBook-React-Conversion-Plan.md index 86395fd1f9a6..e4dba39f94f4 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/TalkingBook-React-Conversion-Plan.md +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/TalkingBook-React-Conversion-Plan.md @@ -177,7 +177,7 @@ Two lifecycle facts that shape everything: - Registration is already done: `ToolBox.registerTool(new TalkingBookTool())` in `toolboxBootstrap.ts`; `talkingBook` is in `alwaysOnToolIds` and has an icon entry in `ToolboxRoot.tsx`. -- `IReactTool.featureName` exists for subscription-badged tools; Talking Book doesn't +- `ITool.featureName` exists for subscription-badged tools; Talking Book doesn't need it. - Styling: MUI (`@mui/material`) + Emotion `css` prop, wrapped in `` (from `bloomMaterialUITheme`), using Bloom's diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts index 76c869cf7943..eda936e0e8c7 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts @@ -41,7 +41,7 @@ import * as toastr from "toastr"; import WebSocketManager, { IBloomWebSocketEvent, } from "../../../utils/WebSocketManager"; -import { getActiveToolId, ToolBox } from "../toolbox"; +import { getActiveToolId } from "../toolbox"; import * as React from "react"; import { renderRoot } from "../../../utils/reactRender"; import { @@ -70,7 +70,7 @@ import { FeatureStatus, getFeatureStatusAsync, } from "../../../react_components/featureStatus"; -import { animateStyleName } from "../../../utils/shared"; +import { animateStyleName, getPageIframeBody } from "../../../utils/shared"; import jQuery from "jquery"; import { AudioHighlightManager, @@ -2196,7 +2196,7 @@ export default class AudioRecording implements IAudioRecorder { // together in one place. public async setShowingImageDescriptions(isOn: boolean) { this.showingImageDescriptions = isOn; - const page = ToolBox.getPage(); + const page = getPageIframeBody(); if (this.showingImageDescriptions) { if (page) { // we should always have a page, but testing makes lint happy diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx index 66d9c8ef7fd2..045189d80b5e 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx @@ -1,8 +1,8 @@ import { hideImageDescriptions } from "../imageDescription/imageDescriptionUtils"; import { kBloomCanvasClass } from "../canvas/canvasElementConstants"; import { beginLoadSynphonySettings } from "../readers/readerTools"; -import { getTheOneToolbox } from "../toolbox"; -import { ToolBox } from "../toolbox"; +import { getTheOneToolbox, IToolboxSettings } from "../toolbox"; +import { getPageIframeBody } from "../../../utils/shared"; import { getAudioRecorder, getOrCreateAudioRecorder } from "./audioRecording"; import * as AudioRecorder from "./audioRecording"; import ToolboxToolReactAdaptor from "../toolboxToolReactAdaptor"; @@ -27,10 +27,13 @@ export default class TalkingBookTool extends ToolboxToolReactAdaptor { />, ); } - public beginRestoreSettings(settings: string): JQueryPromise { + /** This tool saves no state of its own; see ITool.beginRestoreSettings(). */ + public async beginRestoreSettings( + _settings: IToolboxSettings, + ): Promise { // Nothing to do except that we need the sentence ending punctuation settings // from the leveled reader tool. (We share sentence parsing via libSynphony.) - return beginLoadSynphonySettings(); + await beginLoadSynphonySettings(); } public isAlwaysEnabled(): boolean { @@ -144,7 +147,7 @@ export default class TalkingBookTool extends ToolboxToolReactAdaptor { if (audioRecorder) { audioRecorder.removeRecordingSetup(); } - const page = ToolBox.getPage(); + const page = getPageIframeBody(); if (page) { hideImageDescriptions(page); TalkingBookTool.enshroudPhraseDelimiters(page); @@ -171,7 +174,7 @@ export default class TalkingBookTool extends ToolboxToolReactAdaptor { private showImageDescriptionsIfAny() { // If we have any image descriptions we need to show them so we can record them. // (BL-8515) Unless the image description tool is not currently active. - const page = ToolBox.getPage(); + const page = getPageIframeBody(); if (!page) { return; } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts index 244a64ff3ccb..df83c44b6495 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -27,12 +27,24 @@ import { export { isLongPressEvaluating }; export { callOnBlur as registerMenuCloseOnBlur }; -type ToolboxSettings = Record & { +/** + * The toolbox settings for the current book, as the server sends them (GET + * /bloom/api/toolbox/settings; see ToolboxView.HandleSettings()). Apart from "current" and + * "visibility", it has one "State" property for each tool that has saved state in + * this book; the value is whatever opaque string that tool chose to save, and only that tool + * knows how to interpret it (e.g. settings["decodableReaderState"]). + */ +export interface IToolboxSettings { + // The tool the book was last using, in the historical persisted spelling (e.g. + // "talkingBookTool"). Missing or empty for a new book. current?: string; + // "visible" if the toolbox was open in this book, otherwise an empty string. visibility?: string; -}; + // The per-tool state properties described above, keyed "State". + [stateKey: string]: string | undefined; +} -let savedSettings: ToolboxSettings = {}; +let savedSettings: IToolboxSettings = {}; let keypressTimer: ReturnType | null = null; @@ -60,20 +72,21 @@ export function setToolboxSettingsChangeHandler( changeToolboxSettingsState = handler; } -export interface IReactTool { - // For tools that require a subscription. This will trigger an indicator communicating that this - // featureName requires a subscription. - featureName?: string; -} - // Each tool implements this interface and adds an instance of its implementation to the // list maintained here. The methods support the different things individual tools // can be asked to do by the rest of the system. Everything the toolbox needs to know // about a tool, including the metadata it shows in the tool's section header, comes from // here (or is derived from id(); see toolIds.ts). // See ToolboxView.cs class comment for a summary of how to add a new tool. -export interface ITool extends IReactTool { - beginRestoreSettings(settings: string): JQueryPromise; +export interface ITool { + // For tools that require a subscription. This will trigger an indicator communicating that this + // featureName requires a subscription. + readonly featureName?: string; + // Gives the tool a chance to restore whatever it saved in the book's toolbox settings + // (its own "State" property, if any) before it is shown. Called each time the + // tool becomes the current tool, so it also serves to make the tool's state track the + // current book. The returned promise must resolve when the tool is ready to be shown. + beginRestoreSettings(settings: IToolboxSettings): Promise; configureElements(container: HTMLElement); showTool(); // called when a new tool is chosen, but not necessarily when a new page is displayed. hideTool(); // called when changing tools or hiding the toolbox. @@ -324,33 +337,6 @@ export class ToolBox { getTheOneToolbox().doWhenClosingTool.push(task); } - // In the process of moving this to shared.ts, but a lot of - // code still expects to find it here. - public static getPageFrame(): HTMLIFrameElement { - return getPageIFrame(); - } - - // In the process of moving this to shared.ts as getPageIframeBody, but a lot of - // code still expects to find it here. - // The body of the editable page, a root for searching for document content. - public static getPage(): HTMLElement | null { - return getPageIframeBody(); - } - - public static isXmatterPage(): boolean { - const page = ToolBox.getPage(); - if (!page) return false; - const bloomPage = page.querySelector(".bloom-page"); - if (!bloomPage) return false; - const classes = bloomPage.getAttribute("class"); - if (!classes) return false; - return ( - // Enhance: when our typescript "groks" string.include(), it would simplify things. - classes.indexOf("bloom-frontMatter") > -1 || - classes.indexOf("bloom-backMatter") > -1 - ); - } - public static registerTool(tool: ITool) { masterToolList.push(tool); } @@ -615,7 +601,7 @@ function showOrHideTool( export function restoreToolboxSettings() { get("toolbox/settings", (result) => { savedSettings = result.data; - const pageFrame = ToolBox.getPageFrame(); + const pageFrame = getPageIFrame(); const contentWin = pageFrame.contentWindow; if (contentWin && contentWin.document.readyState === "loading") { // We can't finish restoring settings until the main document is loaded, so arrange to call the next stage when it is. @@ -656,27 +642,23 @@ export function applyToolboxStateToUpdatedPage() { doWhenPageReady(() => { const activeTool = currentTool; if (activeTool && isToolInitialized(activeTool)) { - activeTool - .beginRestoreSettings( - savedSettings as unknown as string, - ) - .then(() => { - if (currentTool !== activeTool) { - return; - } + activeTool.beginRestoreSettings(savedSettings).then(() => { + if (currentTool !== activeTool) { + return; + } - // Re-run tool UI setup on page/book switches. Some tools - // (for example reader toggle controls) are initialized in showTool(). - Promise.resolve(activeTool.showTool()).then(() => { - if ( - currentTool === activeTool && - isToolInitialized(activeTool) - ) { - activeTool.newPageReady(); - scheduleDelayedNewPageReady(activeTool); - } - }); + // Re-run tool UI setup on page/book switches. Some tools + // (for example reader toggle controls) are initialized in showTool(). + Promise.resolve(activeTool.showTool()).then(() => { + if ( + currentTool === activeTool && + isToolInitialized(activeTool) + ) { + activeTool.newPageReady(); + scheduleDelayedNewPageReady(activeTool); + } }); + }); // We used to call updateMarkup() here // Now we don't because it would mess up the Talking Book Tool // if you really need it, add call to updateMarkup to currentTool's implementation of newPageReady. @@ -701,8 +683,8 @@ function scheduleDelayedNewPageReady(tool: ITool): void { } function doWhenPageReady(action: () => void) { - const page = ToolBox.getPage(); - if (!page || !ToolBox.getPageFrame()) { + const page = getPageIframeBody(); + if (!page || !getPageIFrame()) { // Somehow, despite firing this function when the document is supposedly ready, // it may not really be ready when this is first called. If it doesn't even have a body yet, // we need to try again later. @@ -740,7 +722,7 @@ function doWhenCkEditorReadyCore( }, page: HTMLElement, ): void { - const contentWindow = ToolBox.getPageFrame().contentWindow as + const contentWindow = getPageIFrame().contentWindow as | (Window & { CKEDITOR?: typeof CKEDITOR }) | null; if (contentWindow?.CKEDITOR) { @@ -808,7 +790,7 @@ function doWhenCkEditorReadyCore( } } -function restoreToolboxSettingsWhenPageReady(settings: ToolboxSettings) { +function restoreToolboxSettingsWhenPageReady(settings: IToolboxSettings) { doWhenPageReady(() => { // OK, CKEditor is done (or page doesn't use it), we can finally do the real initialization. const opts = settings; @@ -879,11 +861,9 @@ function activateTool(newTool: ITool) { return; } // Always re-restore settings so tool state tracks the current book. - newTool - .beginRestoreSettings(savedSettings as unknown as string) - .then(() => { - activateToolInternalAsync(newTool); - }); + newTool.beginRestoreSettings(savedSettings).then(() => { + activateToolInternalAsync(newTool); + }); } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx b/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx index 6b87d2e9f247..d183c0ff7b14 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx @@ -1,14 +1,12 @@ import { renderRoot } from "../../utils/reactRender"; -import $ from "jquery"; -import { ITool, IReactTool } from "./toolbox"; +import { ITool, IToolboxSettings } from "./toolbox"; import { ReactElement } from "react"; import { isPageBloomGame } from "./games/GameInfo"; +import { getBloomPageElement } from "../../utils/shared"; // Provides a base class with some common code for react-based tools that live // in Bloom's Edit Page Toolbox. -export default abstract class ToolboxToolReactAdaptor - implements ITool, IReactTool -{ +export default abstract class ToolboxToolReactAdaptor implements ITool { imageUpdated(_img: HTMLImageElement | undefined): void { // does nothing by default } @@ -40,13 +38,16 @@ export default abstract class ToolboxToolReactAdaptor public isAlwaysEnabled(): boolean { return false; } + // See ITool.featureName. Tools that require a subscription set this. public featureName?: string; - public beginRestoreSettings(_settings: string): JQueryPromise { + /** + * Restores this tool's state from the book's saved toolbox settings (see ITool). + * Tools that save no state don't override this. + */ + public beginRestoreSettings(_settings: IToolboxSettings): Promise { // Nothing to do, so return an already-resolved promise. - const result = $.Deferred(); - result.resolve(); - return result; + return Promise.resolve(); } // We need these to implement the interface, but don't need them to do anything. /* eslint-disable @typescript-eslint/no-empty-function */ @@ -66,65 +67,32 @@ export default abstract class ToolboxToolReactAdaptor public configureElements(_container: HTMLElement) {} /* eslint-enable @typescript-eslint/no-empty-function */ - public static getPageFrame(): HTMLIFrameElement { - return parent.window.document.getElementById( - "page", - ) as HTMLIFrameElement; - } - - // The body of the editable page, a root for searching for document content. - public static getPage(): HTMLElement | null { - const page = this.getPageFrame(); - if (!page || !page.contentWindow) return null; - return page.contentWindow.document.body; - } - - public static getBloomPage(): HTMLElement | null { - const page = this.getPage(); - if (!page) return null; - return page.querySelector(".bloom-page") as HTMLElement; - } + // Note: the general helpers for getting at the page being edited (the page iframe, its + // body, the .bloom-page element, and whether the page is xmatter) live in + // utils/shared.ts. The few below remain here because they are about particular things + // tools do with the page: attributes we deliberately store URL-encoded, and games. + /** The value of an attribute of the .bloom-page element that we store URL-encoded. */ public static getBloomPageAttrDecoded(name: string): string | undefined { - const page = this.getBloomPage(); + const page = getBloomPageElement(); if (!page) return undefined; const v = page.getAttribute(name); return v ? decodeURIComponent(v) : undefined; } + /** Stores a value, URL-encoded, in an attribute of the .bloom-page element. */ public static encodeAndSetPageAttr( name: string, unencodedValue: string, ): void { - const page = this.getBloomPage(); + const page = getBloomPageElement(); if (!page) return; page.setAttribute(name, encodeURIComponent(unencodedValue)); } - // Generally returns true if the page is xmatter. Some callers (enabling canvas tool) want to treat - // a custom page as not being xmatter, so we support an override for that. Could be just a boolean, - // but using an object with a named field makes it clearer what the argument is for where it is used. - public static isXmatter( - args: { returnFalseForCustomPage: boolean } = { - returnFalseForCustomPage: false, - }, - ): boolean { - const pageClass = this.getBloomPageAttrDecoded("class"); - if (!pageClass) return false; // paranoia - if ( - args?.returnFalseForCustomPage && - pageClass.indexOf("bloom-customLayout") >= 0 - ) { - return false; - } - return ( - pageClass.indexOf("bloom-frontMatter") >= 0 || - pageClass.indexOf("bloom-backMatter") >= 0 - ); - } - + /** Is the page currently being edited one of our games? */ public static isCurrentPageABloomGame(): boolean { - const page = this.getBloomPage(); + const page = getBloomPageElement(); if (!page) { return false; // huh?? } diff --git a/src/BloomBrowserUI/utils/shared.ts b/src/BloomBrowserUI/utils/shared.ts index 1126a75d09b0..9998c642d64a 100644 --- a/src/BloomBrowserUI/utils/shared.ts +++ b/src/BloomBrowserUI/utils/shared.ts @@ -35,6 +35,34 @@ export function getBloomPageElement(): HTMLElement | null { ) as HTMLElement | null; } +/** + * Is the page currently being edited part of the front or back matter? + * Some callers (e.g. deciding whether the canvas tool may be used) want to treat a custom + * page as not being xmatter, so we support an override for that. Could be just a boolean, + * but using an object with a named field makes it clearer what the argument is for where it + * is used. + * Returns false if there is no page yet, or (improbably) it has no classes. + * (The class attribute, unlike some others we put on the page, is never URL-encoded.) + */ +export function isXmatterPage( + args: { returnFalseForCustomPage: boolean } = { + returnFalseForCustomPage: false, + }, +): boolean { + const pageClasses = getBloomPageElement()?.getAttribute("class"); + if (!pageClasses) return false; + if ( + args.returnFalseForCustomPage && + pageClasses.includes("bloom-customLayout") + ) { + return false; + } + return ( + pageClasses.includes("bloom-frontMatter") || + pageClasses.includes("bloom-backMatter") + ); +} + // We saw one failure where the page iframe and its body already existed, but the editable // .bloom-page element had not been inserted yet when other React code tried to read it. // That is only a single observed case, so we do not know how often it happens, but the ordering From 486b9d54296e02ee43ce30cfd2112826e11d7ab0 Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Sat, 25 Jul 2026 22:23:23 -0700 Subject: [PATCH 06/19] Fix BL-16608 Cleanup toolbox infrastructure (6/7): update docs and tests https://issues.bloomlibrary.org/youtrack/issue/BL-16608 - Rewrote bookEdit/toolbox/ReadMe.txt for the current architecture (ToolboxRoot / toolbox.ts / toolboxReactAdapter / toolIds / ToolboxToolReactAdaptor) with an accurate how-to-add-a-tool recipe; the old text described the jQuery accordion and "modifying toolbox.jade". - Deleted TalkingBook-React-Conversion-Plan.md (532 lines): the conversion it plans is done, and its "retire the old non-React toolbox framework" step is this branch. Durable facts already live in AGENTS.md and ReadMe. - ToolboxView.cs: corrected the remaining stale steps in the add-a-tool class comment, and pruned GetToolboxServerDirectories to the directories something still fetches by bare name. The list feeds BloomFileLocator's bare-filename search path (not URL serving; image requests never consult it), so signLanguage, imageDescription, and canvas - whose less is bundled and whose only URL fetches are images - are removed; kept the toolbox root, talkingBook, motion, music (their css files are URL-linked from toolbox.pug / WorkspaceView.cs / editablePage.ts) and readers/readerSetup (real iframe fetch of ReaderSetup.html). - Rewrote the ToolboxRoot Playwright harness/uitest to be faithful to the new design: the harness stands in for toolbox.ts by registering real tools (ImpairmentVisualizer, SettingsTool) plus two lightweight stand-ins carrying motion/canvas ids, icons, and featureNames, then populates through the real adapter (addTool + setActiveToolByToolId) with canonical ids. All 7 test intents kept; the stale .toolbox-react-header-icon and .subscription-badge assertions (classes that never existed on the React UI) are gone. These uitests remain unrunnable due to the pre-existing component-tester React 17 / react-dom/client mismatch - verified by inspection and typecheck only. - Canvas e2e helpers now target the MUI accordion header (.MuiAccordionSummary-root with the canonical data-toolid span) instead of the deleted legacy h3 headers; the subscription-badge probe targets the real badge img. - Comment sweep: toolboxReactAdapter.ts and ToolboxRoot.tsx headers no longer claim tools are "not yet React". Verified: pnpm typecheck passes (both the main and component-tester projects show only their pre-existing unrelated errors), eslint 0 errors, full vitest suite 550 passed / 5 skipped, and build/agent-dotnet.sh build src/BloomExe/BloomExe.csproj succeeds with 0 errors. Canvas e2e and uitests not run (need a live Bloom / blocked by the pre-existing React-version issue). Co-Authored-By: Claude Fable 5 --- .../canvas-e2e-tests/helpers/canvasFrames.ts | 4 +- ...e5-lifecycle-subscription-disabled.spec.ts | 7 +- .../bookEdit/toolbox/ReadMe.txt | 52 +- .../bookEdit/toolbox/ToolboxRoot.tsx | 19 +- .../TalkingBook-React-Conversion-Plan.md | 532 ------------------ .../bookEdit/toolbox/toolboxReactAdapter.ts | 26 +- .../ToolboxRootTestHarness.tsx | 141 +++-- .../toolbox-root-react.uitest.ts | 359 +++++------- src/BloomExe/Edit/ToolboxView.cs | 38 +- 9 files changed, 331 insertions(+), 847 deletions(-) delete mode 100644 src/BloomBrowserUI/bookEdit/toolbox/talkingBook/TalkingBook-React-Conversion-Plan.md diff --git a/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/helpers/canvasFrames.ts b/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/helpers/canvasFrames.ts index ddc2f42d9e7a..c9aa1b53e211 100644 --- a/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/helpers/canvasFrames.ts +++ b/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/helpers/canvasFrames.ts @@ -71,9 +71,11 @@ export const openCanvasToolTab = async (toolboxFrame: Frame): Promise => { return; } + // The toolbox is a MUI accordion (see ToolboxRoot.tsx): each section's clickable header + // is an AccordionSummary, and the icon inside it carries the tool's canonical id. const canvasToolHeader = toolboxFrame .locator( - 'h3[data-toolid="canvasTool"], h3[data-toolid="canvas"], h3[data-toolid*="canvas"], h3:has-text("Canvas")', + '.MuiAccordionSummary-root:has([data-toolid="canvas"]), .MuiAccordionSummary-root:has-text("Canvas Tool")', ) .first(); diff --git a/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/specs/14-phase5-lifecycle-subscription-disabled.spec.ts b/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/specs/14-phase5-lifecycle-subscription-disabled.spec.ts index 6dfbf0949107..6d28e7d6c4f7 100644 --- a/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/specs/14-phase5-lifecycle-subscription-disabled.spec.ts +++ b/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/specs/14-phase5-lifecycle-subscription-disabled.spec.ts @@ -268,8 +268,13 @@ test("S1: Set Destination menu row shows subscription badge when canvas subscrip paletteItem: "navigation-image-button", }); + // The Canvas tool's section header (a MUI AccordionSummary, identified by the canonical + // tool id on its icon) shows the subscription badge via + // SubscriptionBadgeWithTooltipAndDialog, which renders it as an . const canvasToolBadgeCount = await canvasTestContext.toolboxFrame - .locator('h3[data-toolid="canvasTool"] .subscription-badge') + .locator( + '.MuiAccordionSummary-root:has([data-toolid="canvas"]) img[src*="bloom-enterprise-badge.svg"]', + ) .count(); if (canvasToolBadgeCount === 0) { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/ReadMe.txt b/src/BloomBrowserUI/bookEdit/toolbox/ReadMe.txt index f9079273de56..db679f5b1d25 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/ReadMe.txt +++ b/src/BloomBrowserUI/bookEdit/toolbox/ReadMe.txt @@ -1,11 +1,45 @@ -The intended code organization of the toolbox is as follows: -- Files in the root folder (this one, toolbox) should contain only generic code for managing the toolbox as a whole -- Anything that is part of the implementation of a particular tool should be in one of the child folders, of which there is one for each tool in the accordion. +The toolbox is the sidebar of the Edit tab. Every tool in it is a React component. -A partial exception to this is a chunk of code which is either shared by the Decodable Reader and Leveled Reader tool, -or at least has not yet been teased apart. Much of this code is in files or folders with names starting with Reader or containing Synphony. -For now, all such code is in the decodableReader folder (since a slightly larger share of it really belongs there). +Code organization +- Files in this root folder are the generic machinery for managing the toolbox as a whole: + - ToolboxRoot.tsx the React root: one MUI Accordion section per tool, whose body is + the element the tool's ITool.makeRootElement() returns. + - toolbox.ts the ITool interface and the non-React orchestration: it asks the + server which tools this book has enabled, drives each tool's + lifecycle (showTool/newPageReady/updateMarkup/...), and owns the + keystroke-to-markup machinery. + - toolboxReactAdapter.ts the narrow channel by which toolbox.ts tells ToolboxRoot which + tools to offer and which one is active. (Separate module only to + avoid an import cycle.) + - toolIds.ts the canonical tool ids, and the single place that knows how a + canonical id maps to the other spellings at our boundaries (the + historical "Tool"/"Check" suffixes in persisted data, and the + English label and l10n key of a tool). + - toolboxToolReactAdaptor.tsx the base class real tools extend; it supplies no-op + lifecycle defaults so a tool implements only what it cares about. + - toolboxBootstrap.ts the toolbox bundle's entry point: renders ToolboxRoot, registers + one instance of each tool, and starts toolbox.ts. +- Anything that is part of the implementation of a particular tool belongs in that tool's + own child folder, one per tool. -It is a goal of our design that code outside the folder of an individual tool should not know about the tool. -Ideally it should be possible to add a new tool to the accordion without modifying any file outside the new folder -that is added, except for modifying toolbox.jade to get the files included. \ No newline at end of file +A partial exception is a chunk of code shared by the Decodable Reader and Leveled Reader +tools, or at least not yet teased apart. Its files and folders have names starting with +Reader or containing Synphony, and for now all of it is in the readers folder. + +It is a goal of our design that code outside the folder of an individual tool should not +know about the tool. + +To add a new tool +1. Create a folder here whose name is the tool's canonical id (no "Tool" suffix). +2. In it, write a class that extends ToolboxToolReactAdaptor, implementing at least id() + and makeRootElement(), plus iconPath() if the section header should show an icon, and + whichever lifecycle methods the tool needs (see the ITool comments in toolbox.ts). +3. Register one instance of it in toolboxBootstrap.ts: ToolBox.registerTool(new MyTool()). +4. Add an XLF entry for the label, whose key follows the convention in toolIds.ts + getToolLabelInfo() (e.g. id "music" gives key "EditTab.Toolbox.MusicTool" and English + "Music Tool"); see .github/skills/xlf-strings/SKILL.md. + +That is all. The section header (label, icon, subscription badge), the tool's checkbox in +the "More..." section, and the alphabetical ordering are all derived from the ITool +implementation and its id, so there is no list of tools to update anywhere else. +See also the ToolboxView class comment in src/BloomExe/Edit/ToolboxView.cs. diff --git a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx index a6f2b200ce5d..fd41260f0423 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx @@ -32,10 +32,11 @@ import { // has enabled and tells us about each one through the adapter's addTool(), which is the // only way a section is ever created. // -// Each tool still hands us a plain DOM element (from its ITool.makeRootElement()) rather -// than a React component, so a small host component (ToolBodyHost) puts that element into -// the React layout. When every tool is a React component, each section can render its -// tool directly and both that host and toolboxReactAdapter.ts can go away. +// Every tool is a React component, but a tool hands us the already-rendered root DOM +// element of its component (from its ITool.makeRootElement()) rather than an element type +// we could render ourselves. So a small host component (ToolBodyHost) puts that element +// into the React layout, which also means a tool keeps its state as sections open and +// close. // Everything the toolbox needs in order to show one tool's section. It all comes from the // tool itself (see ITool) or is derived from its id (see toolIds.ts). @@ -162,9 +163,9 @@ export const ToolboxRoot: React.FunctionComponent = () => { const activeToolChangedCallbacks = React.useRef< ((toolId: string) => void)[] >([]); - // The authoritative copy of the sections, so that the adapter methods the legacy - // toolbox code calls can read and update the list synchronously. (React state is - // updated from it, for rendering.) + // The authoritative copy of the sections, so that the adapter methods toolbox.ts + // calls can read and update the list synchronously. (React state is updated from it, + // for rendering.) const sectionsRef = React.useRef([]); const applySections = React.useCallback( @@ -182,8 +183,8 @@ export const ToolboxRoot: React.FunctionComponent = () => { }); }, []); - // Register the adapter that the legacy toolbox code uses to say which tools the - // toolbox offers, to make one of them active, and to observe which one is active. + // Register the adapter that toolbox.ts uses to say which tools the toolbox offers, + // to make one of them active, and to observe which one is active. // See toolboxReactAdapter.ts. React.useEffect(() => { setToolboxReactAdapter({ diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/TalkingBook-React-Conversion-Plan.md b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/TalkingBook-React-Conversion-Plan.md deleted file mode 100644 index e4dba39f94f4..000000000000 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/TalkingBook-React-Conversion-Plan.md +++ /dev/null @@ -1,532 +0,0 @@ -# Plan: Convert the Talking Book tool to React - -**Status:** proposed / not yet started. -**Scope of this document:** Phase A (the React conversion, one PR) and Phase B (moving the -engine into the page iframe, a separate follow-up PR). Step 6 (retiring the old non-React -toolbox framework) is a third PR, described briefly at the end so it isn't lost. - -Talking Book is the **last** non-React toolbox tool. Converting it lets us later delete -the entire legacy non-React tool framework (Step 6). - -## 0. Sequencing rationale — why the engine move is a separate PR - -An earlier draft of this plan did the React conversion and the engine move in one PR. -They are now split, for two reasons: - -1. **The engine move is not needed for the goal.** Deleting the legacy framework only - requires the tool *UI* to be React. The engine can keep living in the toolbox iframe, - reaching into the page exactly as it does today. -2. **The page iframe reloads on every page switch.** `switchContentPage` in - `workspaceRoot.ts` (~line 134) sets `iframe.src = newSource`, destroying the page - iframe's whole JS realm. Today `theOneAudioRecorder` lives in the toolbox iframe and - **survives for the whole editing session**; moved page-side, it would be torn down and - reconstructed on *every page switch*. That is a real architectural change (see §6), not - the mechanical "flip the wiring" the earlier draft implied — it needs its own design - and its own PR. - -The split is also what makes Phase B safe: after Phase A the engine no longer touches any -toolbox DOM (it talks to the UI only through a state object and a couple of registered -element refs), so Phase B becomes a pure relocation-and-lifetime problem. - ---- - -## 1. Current architecture (verified against source, 2026-07) - -### 1.1 `talkingBook.ts` -`TalkingBookTool implements ITool` directly (not via `ToolboxToolReactAdaptor`). Its -`makeRootElement()` **throws** — the tool's UI is the pug file -`talkingBookToolboxTool.pug`, routed in through two legacy hard-coded maps: - -- `subpath` in `toolbox.ts` (~line 1273): `talkingBookTool: "talkingBook/talkingBookToolboxTool.html"` -- `legacyToolSubPathByToolId` in `ToolboxRoot.tsx` (~line 71): `talkingBook: "talkingBook/talkingBookToolboxTool.html"` - -It is a thin lifecycle shim over the `AudioRecording` singleton, but note the exact -forwards (the conversion must preserve all of them): - -- `showTool()` → module-level `initializeTalkingBookToolAsync()` (lazily constructs the - singleton) then `getAudioRecorder().setupForRecordingAsync()`. -- `newPageReady()` → `showImageDescriptionsIfAny()` (its own page-DOM logic, BL-8515) - then `getAudioRecorder().handleNewPageReady(TalkingBookTool.deshroudPhraseDelimiters)`. -- `hideTool()` → `handleToolHiding()`. -- `detachFromPage()` → `removeRecordingSetup()`, then `hideImageDescriptions(page)` and - `TalkingBookTool.enshroudPhraseDelimiters(page)`. -- `updateMarkupAsync()` → `getAudioRecorder().getUpdateMarkupAction()`; - `isUpdateMarkupAsync()` returns `true`. -- `beginRestoreSettings()` → `beginLoadSynphonySettings()` (shares sentence-ending - punctuation settings with the Leveled Reader tool — easy to lose in conversion, since - the adaptor's default `beginRestoreSettings` is a no-op). -- `isAlwaysEnabled()` returns `true` (adaptor default is `false` — must override). -- The static `enshroudPhraseDelimiters`/`deshroudPhraseDelimiters` helpers operate on the - *page* DOM and stay with the tool. - -The lifecycle comment at the top of the file (lines ~38–48) enumerates when -showTool/newPageReady/updateMarkup fire — it is the manual-test matrix (§8). - -### 1.2 `audioRecording.ts` (~4,900 lines) — engine **and** tool UI, running in the toolbox iframe -The important, counter-intuitive fact: **the `AudioRecording` object runs in the -_toolbox_ iframe, not the page iframe.** Evidence: - -- The constructor reads toolbox controls via its own `document`: `#audio-split` - (→ `this.audioSplitButton`) and `#audio-meter` (→ `this.levelCanvas`), and calls - `updateDisplay()` which touches `#audio-split-wrapper` and - `#advanced-talking-book-controls-react-container`. (It does *not* touch `#player`; - that is wired in `initializeTalkingBookToolAsync`, and `#disablingOverlay` is grabbed - in `setupForRecordingAsync`.) -- `initializeTalkingBookToolAsync` (the class method, ~line 229) wires jQuery handlers to - the toolbox buttons: `#audio-record` mousedown/mouseup, click handlers on - `#audio-play` (with a **ctrl-click eSpeak-preview easter egg** — preserve it), - `#audio-split` (opens the Adjust Timings dialog via `getWorkspaceBundleExports()`), - `#audio-next`, `#audio-prev`, `#audio-clear`, `#audio-listen`, `#audio-input-dev`; - plus `#player` events (`onended`, `onerror`, `ondurationchange`), `toastr.options` - (position `toast-toolbox-bottom`), a `WholeTextBoxAudio` feature-status fetch, and - `pullDefaultRecordingModeAsync()`. -- It reaches _into_ the page iframe for content via `getPageFrame()` - (`parent.window.document.getElementById("page")`, ~line 2366) and `getPageDocBody()` - — ~26 direct call sites plus ~9 via `getPageDocBodyJQuery()`. -- It also installs listeners **in the page document**: a capture-phase `mousedown` on the - page body (`moveRecordingHighlightToClick`) and a `MutationObserver` watching - visibility-affecting class changes (`watchElementsThatMightChangeAffectingVisibility`). - These already run cross-iframe today and are re-installed in `handleNewPageReady`. - -So the object owns two very different kinds of responsibility: - -1. **Tool UI** (toolbox side): the 7 main buttons, their counters, the level meter - canvas, device selection (`#audio-input-dev` icon + `#audio-devlist` jQuery menu), - the `