Make letter boxes grow wide enough for letters (BL-14541)

JohnThomson · JohnThomson · commit cddcf82576a6 · 2025-04-14T16:33:04.000-05:00
BL-14541 grew to be much more complex than this.
The current state of this card is handling width fairly well, but failing to get a reasonable height for မြန်မာ unless line height is 1.5, in which case ordinary roman letters overflow mysteriously. Something is wrong with measuring text. There are also many related tasks in the card which are not yet addressed here.
diff --git a/src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts b/src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts
@@ -1728,6 +1728,7 @@ export class CanvasElementManager {
             editable
         );
         this.alignControlFrameWithActiveElement();
+        this.adjustTarget(this.activeElement);
     }
 
     // Determine which of the side handles, if any, should have the class "bloom-currently-cropped"
diff --git a/src/BloomBrowserUI/bookEdit/toolbox/games/GamePromptDialog.tsx b/src/BloomBrowserUI/bookEdit/toolbox/games/GamePromptDialog.tsx
@@ -44,6 +44,10 @@ import {
 import { splitIntoGraphemes } from "../../../utils/textUtils";
 import { kCanvasElementClass } from "../overlay/canvasElementUtils";
 import { kBloomCanvasSelector } from "../../js/bloomImages";
+import OverflowChecker from "../../OverflowChecker/OverflowChecker";
+import { doesContainingPageHaveSameSizeMode } from "./gameUtilities";
+import { max } from "underscore";
+import { _0 } from "../../../react_components/Progress/ProgressBar.stories";
 
 export const GamePromptDialog: React.FunctionComponent<IGamePromptDialogProps> = props => {
     const promptL10nId = props.prompt?.getAttribute("data-caption-l10nid");
@@ -357,6 +361,7 @@ const initializeDialog = (prompt: HTMLElement, tg: HTMLElement | null) => {
         if (!promptEditable) {
             throw new Error("No promptEditable in dragActivity");
         }
+
         promptEditable.innerHTML = editable.innerHTML; // copy back to the permanent element so it gets saved.
         const promptText = editable.textContent ?? "";
         // Split the prompt text into letter groups consisting of a base letter and any combining marks.
@@ -368,77 +373,134 @@ const initializeDialog = (prompt: HTMLElement, tg: HTMLElement | null) => {
         // any letters. (Can become display:none if we have no letters.)
         setDraggableText(draggables[0], letters[0]);
         draggables[0].classList.remove("bloom-unused-in-lang");
-        const separation = draggables[0].offsetWidth + 15; // enhance: may want to increase this
-        // How many can we fit in one row inside the parent?
-        const maxBubbles = Math.floor(
-            (draggables[0].parentElement?.offsetWidth ?? 0 - draggableX) /
-                separation
-        );
+        const separation = 15; // enhance: may want to increase this
+
         // truncate to the number of draggables we can display
         // This is important because (e.g., with autorepeat or paste) we can get a massive number of draggables
         // very quickly, and performance degrades badly, making it hard to recover. Also, until the page relaods,
         // ones beyond this would be off-page and difficult to deal with. And when it does reload they will all
         // be on top of each other and only just visible.
-        letters.splice(maxBubbles);
         const newBubbles: HTMLElement[] = [];
-        if (draggables.length < letters.length) {
+        const makeNewDraggable = () => {
             // We have more letters than draggables. We'll add more draggables.
             const lastDraggable = draggables[draggables.length - 1];
-            for (let i = draggables.length; i < letters.length; i++) {
-                const newDraggable = lastDraggable.cloneNode(
-                    true
-                ) as HTMLElement;
-                setGeneratedDraggableId(newDraggable);
-                // Ensure the new draggable starts out empty.  See BL-14348.
-                // (This covers all languages present, visible or not.)
-                const paras = newDraggable.querySelectorAll(
-                    "div.Letter-style>p"
-                );
-                paras.forEach(p => {
-                    p.textContent = "";
-                    GameTool.setBlankClass(p.parentElement as HTMLElement);
-                });
-                lastDraggable.parentElement?.appendChild(newDraggable);
-                makeTargetForDraggable(newDraggable);
-                // It's available to push letter groups into
-                draggables.push(newDraggable);
-                // It needs refreshBubbleEditing to be called on it later.
-                newBubbles.push(newDraggable);
-                // It should be removed if we Cancel. This list has a longer lifetime.
-                createdBubbles.push(newDraggable);
-            }
-        }
+            const newDraggable = lastDraggable.cloneNode(true) as HTMLElement;
+            setGeneratedDraggableId(newDraggable);
+            // Ensure the new draggable starts out empty.  See BL-14348.
+            // (This covers all languages present, visible or not.)
+            const paras = newDraggable.querySelectorAll("div.Letter-style>p");
+            paras.forEach(p => {
+                p.textContent = "";
+                GameTool.setBlankClass(p.parentElement as HTMLElement);
+            });
+            lastDraggable.parentElement?.appendChild(newDraggable);
+            makeTargetForDraggable(
+                newDraggable,
+                page.getElementsByClassName("bloom-target-row")[0] as
+                    | HTMLElement
+                    | undefined
+            );
+            // It's available to push letter groups into
+            draggables.push(newDraggable);
+            // It needs refreshBubbleEditing to be called on it later.
+            newBubbles.push(newDraggable);
+            // It should be removed if we Cancel. This list has a longer lifetime.
+            createdBubbles.push(newDraggable);
+        };
+        // We need this loop to run over all the draggables and all the letters.
+        // When there are more letters than draggables, we will add a new one if there is room.
+        // When there are more draggables, or no more room, the remaining ones will be set
+        // empty for this language.
+        const iterations = Math.max(letters.length, draggables.length);
+        const minHeight = 50; // Enhance: get from page somehow
+        const minWidth = 50; // Enhance: get from page somehow
+        let newHeight = minHeight;
         // We deliberately don't remove draggables we don't need for this word. They might be in use
         // in some other language. This loop, as well as making the ones we want have the right content,
         // makes the ones we don't want invisible and empty.
-        for (let i = 0; i < draggables.length; i++) {
-            setDraggableText(draggables[i], letters[i]);
-            // up to the number of letters we have, they should be visible; others, not.
-            draggables[i].classList.toggle(
-                "bloom-unused-in-lang",
-                i >= letters.length
-            );
-            getTarget(draggables[i])?.classList.toggle(
-                "bloom-unused-in-lang",
-                i >= letters.length
-            );
-            copyContentToTarget(draggables[i]);
+        for (let i = 0; i < iterations; i++) {
+            if (i >= draggables.length) {
+                makeNewDraggable();
+            }
+            const draggable = draggables[i];
+            const target = getTarget(draggable) as HTMLElement;
+            let stopMakingLetters = false;
+            if (i < letters.length) {
+                draggable.classList.remove("bloom-unused-in-lang");
+                target.classList.remove("bloom-unused-in-lang");
+                setDraggableText(draggable, letters[i]);
+                copyContentToTarget(draggable);
+                target.style.width = `${minWidth}px`;
+                target.style.height = `${minHeight}px`;
+                // We'll measure the overflow of the target, because it has the extra border,
+                // so it can actually overflow when the draggable doesn't.
+                const editable = target?.getElementsByClassName(
+                    "bloom-visibility-code-on"
+                )[0] as HTMLElement;
+                // as currently implemented these indicate the overflow of the editable. But it may be bigger than the
+                // target, and certainly does not allow for a border on the target.
+                const [
+                    overflowX,
+                    overflowY
+                ] = OverflowChecker.getSelfOverflowAmounts(editable);
+                const row = target.parentElement as HTMLElement;
+                const bloomCanvas = row.parentElement as HTMLElement;
+                // The width the editable is currently, plus any overflow, plus the target border and padding in that direction.
+                const newWidth =
+                    overflowX +
+                    editable.offsetWidth +
+                    (target.offsetWidth - target.clientWidth);
+                if (
+                    row.offsetLeft + target.offsetLeft + newWidth >
+                    bloomCanvas.clientWidth
+                ) {
+                    // This one won't fit. We'll stop without adding it.
+                    stopMakingLetters = true;
+                } else {
+                    draggable.style.width = `${newWidth}px`;
+                    target.style.width = `${newWidth}px`;
+                    newHeight = Math.max(
+                        newHeight,
+                        // again, the actual height of the editable, plus our overflow estimate, plus any padding and border of the target.
+                        overflowY +
+                            editable.offsetHeight +
+                            (target.offsetHeight - target.clientHeight)
+                    );
+                }
+            } else {
+                stopMakingLetters = true;
+            }
+            if (stopMakingLetters) {
+                // either added them all or ran out of space
+                // We have more draggables than letters. Set the extra ones empty and invisible.
+                // (Don't delete them, because they might be in use in some other language.)
+                setDraggableText(draggable, "");
+                copyContentToTarget(draggable);
+                draggable.classList.add("bloom-unused-in-lang");
+                target.classList.add("bloom-unused-in-lang");
+                draggable.style.width = `${minWidth}px`;
+                draggable.style.height = `${minHeight}px`;
+                target.style.width = `${minWidth}px`;
+                target.style.height = `${minHeight}px`;
+            }
+        }
+        for (const d of draggables) {
+            d.style.height = `${newHeight}px`;
+            const target = getTarget(d) as HTMLElement;
+            target.style.height = `${newHeight}px`;
         }
-        const shuffledDraggables = draggables.slice();
-        shuffledDraggables.splice(letters.length); // don't want any invisible ones taking up space
+        const shuffledDraggables = draggables.slice(
+            0,
+            letters.length
+        ) as HTMLElement[]; // don't want any invisible ones taking up space
         shuffle(shuffledDraggables, isTheTextInDraggablesTheSame);
+        let left = draggableX;
         for (let i = 0; i < shuffledDraggables.length; i++) {
-            shuffledDraggables[i].style.left = `${draggableX +
-                i * separation}px`;
+            shuffledDraggables[i].style.left = `${left}px`;
+            left += shuffledDraggables[i].offsetWidth + separation;
             shuffledDraggables[i].style.top = `${draggableY}px`;
-            // Note that we use draggables, not shuffledDraggables, here. We want the targets
-            // in the correct order, not the random order.
-            const t = getTarget(draggables[i]);
-            if (t) {
-                t.style.left = `${targetX + i * separation}px`;
-                t.style.top = `${targetY}px`;
-            }
         }
+        // Target shouldn't need adjusting, but we'll show the arrow for this one.
         adjustTarget(draggables[0], getTarget(draggables[0]), true);
         // Must do this AFTER we finish setting the content. Among many other things it does,
         // it will attach a ckeditor to the new editable. That does very complex things
@@ -453,6 +515,7 @@ const initializeDialog = (prompt: HTMLElement, tg: HTMLElement | null) => {
                 false // don't make it active.
             );
         });
+
         // This seems to at least somewhat reduce the likelihood of losing focus
         // after a keystroke, especially with Keyman multi-character inserts (BL-14098).
         // I think it is harmless, since I can't think of any case where the text in the
diff --git a/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx
@@ -277,31 +277,37 @@ const makeArrowShape = (
     // These values make a line from the center of the draggable to the center of the target.
     const startX = draggable.offsetLeft + draggable.offsetWidth / 2;
     const startY = draggable.offsetTop + draggable.offsetHeight / 2;
-    const endXCenter = target.offsetLeft + target.offsetWidth / 2;
-    const endYCenter = target.offsetTop + target.offsetHeight / 2;
+    let targetLeft = target.offsetLeft;
+    let targetTop = target.offsetTop;
+    if (target.parentElement !== draggable.parentElement) {
+        targetLeft += target.parentElement!.offsetLeft;
+        targetTop += target.parentElement!.offsetTop;
+    }
+    const endXCenter = targetLeft + target.offsetWidth / 2;
+    const endYCenter = targetTop + target.offsetHeight / 2;
 
     // Figure out where the tip of the arrow should be. At least one of these will be changed,
     // so that we end up at a corner or side midpoint of the target.
     // (This assumes the two don't overlap. If they do, we don't make an arrow at all.)
     let endX = endXCenter;
     let endY = endYCenter;
-    if (target.offsetLeft > startX) {
+    if (targetLeft > startX) {
         // The target is entirely to the right of the center of the draggable.
         // We will go for one of the points on the left of the target.
-        endX = target.offsetLeft;
-    } else if (target.offsetLeft + target.offsetWidth < startX) {
+        endX = targetLeft;
+    } else if (targetLeft + target.offsetWidth < startX) {
         // The target is entirely to the left of the center of the draggable.
         // We will go for one of the points on the right of the target.
-        endX = target.offsetLeft + target.offsetWidth;
+        endX = targetLeft + target.offsetWidth;
     }
-    if (target.offsetTop > startY) {
+    if (targetTop > startY) {
         // The target is entirely below the center of the draggable.
         // We will go for one of the points on the top of the target.
-        endY = target.offsetTop;
-    } else if (target.offsetTop + target.offsetHeight < startY) {
+        endY = targetTop;
+    } else if (targetTop + target.offsetHeight < startY) {
         // The target is entirely above the center of the draggable.
         // We will go for one of points on the bottom of the target.
-        endY = target.offsetTop + target.offsetHeight;
+        endY = targetTop + target.offsetHeight;
     }
 
     if (!arrow) {
@@ -1964,7 +1970,8 @@ function pxToNumber(dimension: string): number {
 }
 
 export const makeTargetForDraggable = (
-    canvasElement: HTMLElement
+    canvasElement: HTMLElement,
+    parentForTarget?: HTMLElement
 ): HTMLElement => {
     const id = canvasElement.getAttribute("data-draggable-id");
     if (!id) {
@@ -1973,28 +1980,35 @@ export const makeTargetForDraggable = (
     // don't simplify to 'document.createElement'; may be in a different iframe
     const target = canvasElement.ownerDocument.createElement("div");
     target.setAttribute("data-target-of", id);
-    const left = pxToNumber(canvasElement.style.left);
-    const top = pxToNumber(canvasElement.style.top);
     const width = pxToNumber(canvasElement.style.width);
     const height = pxToNumber(canvasElement.style.height);
-    let newLeft = left + 20;
-    let newTop = top + height + 30;
-    if (newTop + height > canvasElement.parentElement!.clientHeight) {
-        newTop = Math.max(0, top - height - 30);
-    }
-    if (newLeft + width > canvasElement.parentElement!.clientWidth) {
-        newLeft = Math.max(0, left - width - 30);
+    if (!parentForTarget) {
+        // if we're putting it in some other parent, then positioning it relative to the
+        // thing for which it's a target doesn't make sense. It definitely isn't wanted for
+        // the current application, making a row of them in the letter game.
+        const left = pxToNumber(canvasElement.style.left);
+        const top = pxToNumber(canvasElement.style.top);
+
+        let newLeft = left + 20;
+        let newTop = top + height + 30;
+        if (newTop + height > canvasElement.parentElement!.clientHeight) {
+            newTop = Math.max(0, top - height - 30);
+        }
+        if (newLeft + width > canvasElement.parentElement!.clientWidth) {
+            newLeft = Math.max(0, left - width - 30);
+        }
+
+        // Review: can we do any more to make sure it's visible and not overlapping canvas element?
+        // Should we try to avoid overlapping other canvas elements and/or targets?
+        target.style.left = `${newLeft}px`;
+        target.style.top = `${newTop}px`;
     }
-    // Review: can we do any more to make sure it's visible and not overlapping canvas element?
-    // Should we try to avoid overlapping other canvas elements and/or targets?
-    target.style.left = `${newLeft}px`;
-    target.style.top = `${newTop}px`;
     target.style.width = `${width}px`;
     target.style.height = `${height}px`;
     // This allows it to get focus, which allows it to get the shadow effect we want when
     // clicked. But is that really right? We can't actually type there.
     target.setAttribute("tabindex", "0");
-    canvasElement.parentElement!.appendChild(target);
+    (parentForTarget ?? canvasElement.parentElement)!.appendChild(target);
     enableDraggingTargets(target);
     adjustTarget(canvasElement, target);
     return target;
diff --git a/src/BloomBrowserUI/bookEdit/toolbox/games/gameUtilities.tsx b/src/BloomBrowserUI/bookEdit/toolbox/games/gameUtilities.tsx
@@ -14,7 +14,12 @@ function doesPageHaveSameSizeMode(
     if (!page) {
         return false;
     }
-    return page.getAttribute("data-same-size") !== "false";
+    // Don't support same-size mode for this game. The new source page sets this false, but earlier
+    // ones just didn't have it, since same-size is usually the default.
+    return (
+        page.getAttribute("data-same-size") !== "false" &&
+        page.getAttribute("data-activity") !== "drag-letter-to-target"
+    );
 }
 
 // Returns true if we need to take into account that this element must be kept the same size
diff --git a/src/BloomBrowserUI/utils/measureText.ts b/src/BloomBrowserUI/utils/measureText.ts
@@ -77,9 +77,10 @@ export class MeasureText {
             .bottom;
         const baselineOfTextWithDefaultLineSpace = block.getBoundingClientRect()
             .bottom;
-        const fontDescent =
+        const fontDescent = Math.ceil(
             bottomOfTextWithDefaultLineSpace -
-            baselineOfTextWithDefaultLineSpace;
+                baselineOfTextWithDefaultLineSpace
+        );
 
         if (lineHeight !== null) div.style.lineHeight = lineHeight;
         const bottomOfTextWithActualLineSpace = div.getBoundingClientRect()
@@ -103,7 +104,9 @@ export class MeasureText {
         // line, but we decided it was not worth the effort to be that perfectionistic.
 
         // We only need enough space to draw what the browser thinks is the descent.
-        const canvasHeight = fontDescent;
+        // Plus a little extra because sometimes the font descent is not actually enough
+        // for the lowest possible descenders.
+        const canvasHeight = fontDescent + 4;
         const testingCanvas = this.createCanvas(width, canvasHeight);
         // The number of pixels of descent is one more than the index of
         // the bottom line on which we found part of a descender.
@@ -200,8 +203,11 @@ export class MeasureText {
         fontSize: number
     ): void {
         const context = canvas.getContext("2d");
-        if (context != null) {
+        if (context !== null) {
             context.textAlign = "start"; // was "top", which is not a valid choice
+            // means that the baseline of the text will align with the y coordinate passed
+            // as the last argument to fillText, which is zero, so basically we draw just
+            // the descenders at the top of the canvas.
             context.textBaseline = "alphabetic";
             context.font = `${fontSize}px '${fontFamily}'`;
             context.fillStyle = "white";
diff --git a/src/content/templates/template books/Games/Games.html b/src/content/templates/template books/Games/Games.html
diff --git a/src/content/templates/template books/Games/Games.less b/src/content/templates/template books/Games/Games.less

Original file line number	Diff line number	Diff line change
`@@ -1728,6 +1728,7 @@ export class CanvasElementManager {`
`1728`	`1728`	`editable`
`1729`	`1729`	`);`
`1730`	`1730`	`this.alignControlFrameWithActiveElement();`
	`1731`	`+ this.adjustTarget(this.activeElement);`
`1731`	`1732`	`}`
`1732`	`1733`
`1733`	`1734`	`// Determine which of the side handles, if any, should have the class "bloom-currently-cropped"`
Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,12 @@ function doesPageHaveSameSizeMode(`
`14`	`14`	`if (!page) {`
`15`	`15`	`return false;`
`16`	`16`	`}`
`17`		`- return page.getAttribute("data-same-size") !== "false";`
	`17`	`+ // Don't support same-size mode for this game. The new source page sets this false, but earlier`
	`18`	`+ // ones just didn't have it, since same-size is usually the default.`
	`19`	`+ return (`
	`20`	`+ page.getAttribute("data-same-size") !== "false" &&`
	`21`	`+ page.getAttribute("data-activity") !== "drag-letter-to-target"`
	`22`	`+ );`
`18`	`23`	`}`
`19`	`24`
`20`	`25`	`// Returns true if we need to take into account that this element must be kept the same size`