Skip to content

Commit 80a62c7

Browse files
hattonclaude
hatton
and
claude
committed
BL-16822 Inline images: teach the code that walks a text block's children
Six places walk the children of a bloom-editable, or judge a block empty by its InnerText. A picture in the text is a non-editable island in there, and a block holding only a picture and the empty paragraph that has to follow it has no text at all, so each of them had something to get wrong. The production diffs are a few lines each; most of this commit is the tests that pin them. - TranslationGroupManager.FixDuplicateLanguageDivs discarded the div holding the picture and kept the genuinely empty one, because InnerText alone says the picture's div is the empty one. - BloomField's preventRemoval guard took its expected count once at page setup, so a picture added later was unprotected, and a picture the person deliberately deleted left the count permanently short and fired a browser undo on every keystroke afterwards. The count is now taken on each keydown and compared on its keyup, so what it guards is the keystroke. - The Talking Book tool recursed into the wrapper, reached the img, treated it as a leaf and wrote audio markup into it. It now stops at any contenteditable="false" island. - Source bubbles must not show the picture: a bubble is for reading another language's text, and the picture is the same in every language. The existing hasNoText pass already drops it; the test pins that, because the design leans on it. - The level-7 bloom-canvas migration must pass the wrapper by, which is why the wrapper has its own class rather than bloom-imageContainer. A real image container on the same page is still renamed, which proves the migration ran. - PublishModel.RemoveUnwantedLanguageData removes a div per unpublished language, and the image file survives only while something still refers to it. The prototype's copy is what keeps it alive, since "z" is always kept. Pinned, not changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DCBGajN5YyAYPBenEf1yy
1 parent daf1f21 commit 80a62c7

10 files changed

Lines changed: 1165 additions & 20 deletions

File tree

‎src/BloomBrowserUI/bookEdit/bloomField/BloomField.ts‎

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -867,20 +867,41 @@ export default class BloomField {
867867
// inadvertently remove the embedded images. So we introduced the "bloom-preventRemoval" class, and this
868868
// tries to safeguard elements bearing that class.
869869
private static PreventRemovalOfSomeElements(field: HTMLElement) {
870-
const numberThatShouldBeThere = $(field).find(
871-
".bloom-preventRemoval",
872-
).length;
873-
if (numberThatShouldBeThere > 0) {
874-
$(field).keyup((e) => {
875-
if (
876-
$(field).find(".bloom-preventRemoval").length <
877-
numberThatShouldBeThere
878-
) {
879-
document.execCommand("undo");
880-
e.preventDefault();
881-
}
882-
});
883-
}
870+
// The count is taken on each keydown and compared on the matching keyup, so what this
871+
// guards is the keystroke itself. Taking it once here instead would get two cases wrong,
872+
// and inline images reach both: an image added AFTER page setup was never counted and so
873+
// was unprotected, and an image the person deliberately deleted (from its menu) left the
874+
// count permanently short, so every keystroke they typed afterwards fired a browser undo.
875+
let countBeforeTheKeystroke = 0;
876+
// Auto-repeat sends a whole run of keydowns before the single keyup that ends them, so
877+
// only the first one of a run saw the field as it was before anything was deleted. Held
878+
// Delete used to get an image past this guard for exactly that reason: keydown number
879+
// two re-read the count AFTER the deletion, so the keyup had nothing to compare against
880+
// and the image stayed deleted.
881+
let aKeyIsDown = false;
882+
const countPreventRemoval = () =>
883+
$(field).find(".bloom-preventRemoval").length;
884+
$(field).keydown(() => {
885+
if (aKeyIsDown) return;
886+
aKeyIsDown = true;
887+
countBeforeTheKeystroke = countPreventRemoval();
888+
});
889+
$(field).keyup((e) => {
890+
aKeyIsDown = false;
891+
if (countPreventRemoval() < countBeforeTheKeystroke) {
892+
document.execCommand("undo");
893+
e.preventDefault();
894+
}
895+
countBeforeTheKeystroke = countPreventRemoval();
896+
});
897+
// A key held down while the focus leaves the field never delivers its keyup here, which
898+
// would leave the flag set and the count stale -- the state this guard used to be in
899+
// permanently. Losing the focus ends the run.
900+
// (A native listener, not jQuery's focusout: jQuery 3 synthesizes focusin/focusout from
901+
// focus/blur, which a dispatched focusout event does not go through.)
902+
field.addEventListener("focusout", () => {
903+
aKeyIsDown = false;
904+
});
884905

885906
//OK, now what if the above fails in some scenario? This adds a last-resort way of getting
886907
//bloom-editable back to the state it was in when the page was first created, by having

‎src/BloomBrowserUI/bookEdit/bloomField/bloomFieldSpec.ts‎

Lines changed: 183 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
///<reference path="BloomField.ts" />
22
///<reference path="../../typings/bundledFromTSC.d.ts"/>
3-
import { describe, it, expect, beforeEach, afterAll } from "vitest";
3+
import { describe, it, expect, beforeEach, afterAll, vi } from "vitest";
44
import { getTestRoot, removeTestRoot } from "../../utils/testHelper";
55
import BloomField from "./BloomField";
66
import $ from "jquery";
@@ -264,6 +264,188 @@ describe("BloomField", () => {
264264
expect($("div p").length).toBeGreaterThan(0);
265265
});
266266

267+
// Inline (Word-style) images use the same two protections that the old embedded-image
268+
// templates did, so these tests pin down that BloomField still recognizes them by class
269+
// when the class is on a .bloom-inlineImage wrapper. See inlineImages.ts.
270+
describe("inline image protections", () => {
271+
const inlineImageHtml =
272+
'<div class="bloom-inlineImage bloom-inlineImageRight bloom-keepFirstInField bloom-preventRemoval" contenteditable="false"><img src="placeHolder.png" alt=""></div>';
273+
274+
it("EnsureParagraphsPresent puts the <p> after a bloom-keepFirstInField inline image", () => {
275+
const editable = document.getElementById("simple")!;
276+
editable.innerHTML = inlineImageHtml;
277+
// Sanity check: no paragraph yet, and the image is the only child.
278+
expect(editable.querySelectorAll("p").length).toBe(0);
279+
expect(editable.children.length).toBe(1);
280+
281+
WireUp();
282+
283+
expect(editable.querySelectorAll("p").length).toBe(1);
284+
// The image must stay first (that is what the class means) with the paragraph
285+
// after it, since the text has to come after the float to wrap around it.
286+
expect(
287+
editable.firstElementChild!.classList.contains(
288+
"bloom-inlineImage",
289+
),
290+
).toBe(true);
291+
expect(editable.lastElementChild!.tagName).toBe("P");
292+
});
293+
294+
it("counts a bloom-preventRemoval inline image, so ctrl+a DEL is undone", () => {
295+
const editable = document.getElementById("simple")!;
296+
editable.innerHTML = inlineImageHtml + "<p>Some text</p>";
297+
WireUp();
298+
// jsdom has no execCommand, and we only want to know that BloomField asked for
299+
// the undo.
300+
const execCommand = vi.fn();
301+
(document as any).execCommand = execCommand;
302+
303+
// Simulate the damage ctrl+a DEL does: the keydown, then the removal it caused,
304+
// then the keyup. The guard compares the count across the keystroke, so the
305+
// keydown is part of the gesture, not scaffolding.
306+
editable.dispatchEvent(
307+
new KeyboardEvent("keydown", { bubbles: true }),
308+
);
309+
editable.querySelector(".bloom-inlineImage")!.remove();
310+
editable.dispatchEvent(
311+
new KeyboardEvent("keyup", { bubbles: true }),
312+
);
313+
314+
expect(execCommand).toHaveBeenCalledWith("undo");
315+
});
316+
317+
// What made this a test: the count used to be taken once, at page setup, so a picture
318+
// the person deleted from its own menu left it short for good and every keystroke they
319+
// typed afterwards fired a browser undo -- taking back their typing, character by
320+
// character. Comparing across the keystroke instead means a deletion nothing typed is
321+
// simply the new state of the box.
322+
it("does not undo the typing that follows a deliberate deletion of the image", () => {
323+
const editable = document.getElementById("simple")!;
324+
editable.innerHTML = inlineImageHtml + "<p>Some text</p>";
325+
WireUp();
326+
const execCommand = vi.fn();
327+
(document as any).execCommand = execCommand;
328+
329+
// The menu's Delete: no keystroke involved.
330+
editable.querySelector(".bloom-inlineImage")!.remove();
331+
332+
// And now the person types.
333+
for (let i = 0; i < 3; i++) {
334+
editable.dispatchEvent(
335+
new KeyboardEvent("keydown", { bubbles: true }),
336+
);
337+
editable.dispatchEvent(
338+
new KeyboardEvent("keyup", { bubbles: true }),
339+
);
340+
}
341+
342+
expect(execCommand).not.toHaveBeenCalled();
343+
});
344+
345+
// Holding Delete rather than pressing it: the browser's auto-repeat sends a run of
346+
// keydowns and one keyup at the end. Re-reading the count on every keydown meant the
347+
// repeat that followed the deletion recorded the ALREADY-LOWER count, so the keyup had
348+
// nothing to compare against and the picture stayed deleted.
349+
it("protects the image when delete is held down rather than pressed", () => {
350+
const editable = document.getElementById("simple")!;
351+
editable.innerHTML = inlineImageHtml + "<p>Some text</p>";
352+
WireUp();
353+
const execCommand = vi.fn();
354+
(document as any).execCommand = execCommand;
355+
356+
// First press: the deletion happens.
357+
editable.dispatchEvent(
358+
new KeyboardEvent("keydown", { bubbles: true, repeat: false }),
359+
);
360+
editable.querySelector(".bloom-inlineImage")!.remove();
361+
// ...and the key is still down, so auto-repeat keeps sending keydowns.
362+
for (let i = 0; i < 3; i++) {
363+
editable.dispatchEvent(
364+
new KeyboardEvent("keydown", {
365+
bubbles: true,
366+
repeat: true,
367+
}),
368+
);
369+
}
370+
// The one keyup, when they finally let go.
371+
editable.dispatchEvent(
372+
new KeyboardEvent("keyup", { bubbles: true }),
373+
);
374+
375+
expect(execCommand).toHaveBeenCalledWith("undo");
376+
});
377+
378+
// The flag that makes the above work has to be cleared when the field loses the focus,
379+
// or a key held down as the focus moves away would leave it set and the count stale --
380+
// and a stale count is what made every keystroke fire a browser undo.
381+
it("recovers if the focus leaves while a key is held down", () => {
382+
const editable = document.getElementById("simple")!;
383+
editable.innerHTML = inlineImageHtml + "<p>Some text</p>";
384+
WireUp();
385+
const execCommand = vi.fn();
386+
(document as any).execCommand = execCommand;
387+
388+
// A key goes down, and the focus leaves before its keyup arrives.
389+
editable.dispatchEvent(
390+
new KeyboardEvent("keydown", { bubbles: true }),
391+
);
392+
editable.dispatchEvent(
393+
new FocusEvent("focusout", { bubbles: true }),
394+
);
395+
// The picture goes, from the menu this time: no keystroke to blame.
396+
editable.querySelector(".bloom-inlineImage")!.remove();
397+
398+
// Now they type. The count must have been re-read, so this is just the new state.
399+
editable.dispatchEvent(
400+
new KeyboardEvent("keydown", { bubbles: true }),
401+
);
402+
editable.dispatchEvent(
403+
new KeyboardEvent("keyup", { bubbles: true }),
404+
);
405+
406+
expect(execCommand).not.toHaveBeenCalled();
407+
});
408+
409+
// The other half: an image inserted after page setup was never counted, so ctrl+a DEL
410+
// could take it out with nothing to put it back.
411+
it("protects an inline image inserted after the field was wired up", () => {
412+
const editable = document.getElementById("simple")!;
413+
editable.innerHTML = "<p>Some text</p>";
414+
WireUp();
415+
const execCommand = vi.fn();
416+
(document as any).execCommand = execCommand;
417+
// Sanity check: nothing to protect when the field was wired up.
418+
expect(
419+
editable.querySelectorAll(".bloom-preventRemoval").length,
420+
).toBe(0);
421+
422+
editable.insertAdjacentHTML("afterbegin", inlineImageHtml);
423+
editable.dispatchEvent(
424+
new KeyboardEvent("keydown", { bubbles: true }),
425+
);
426+
editable.querySelector(".bloom-inlineImage")!.remove();
427+
editable.dispatchEvent(
428+
new KeyboardEvent("keyup", { bubbles: true }),
429+
);
430+
431+
expect(execCommand).toHaveBeenCalledWith("undo");
432+
});
433+
434+
it("does not undo on a keyup that left the inline image alone", () => {
435+
const editable = document.getElementById("simple")!;
436+
editable.innerHTML = inlineImageHtml + "<p>Some text</p>";
437+
WireUp();
438+
const execCommand = vi.fn();
439+
(document as any).execCommand = execCommand;
440+
441+
editable.dispatchEvent(
442+
new KeyboardEvent("keyup", { bubbles: true }),
443+
);
444+
445+
expect(execCommand).not.toHaveBeenCalled();
446+
});
447+
});
448+
267449
// Content converted from other formats can arrive with a real heading as the first thing
268450
// in the box. Prepending an empty paragraph above it would show the reader a blank first
269451
// line, and saving the page would make that permanent.

‎src/BloomBrowserUI/bookEdit/bloomField/test.pug‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,45 @@ html
7575
display: block;
7676
clear: both;
7777
}
78+
79+
/* Inline (Word-style) images. A real book gets these from
80+
content/bookLayout/inlineImages.less; this hand-test page has no basePage.css,
81+
so the load-bearing rules are repeated here. */
82+
.bloom-editable:has(> .bloom-inlineImage) {
83+
/* contain the float, so the image can't hang out of the field */
84+
display: flow-root;
85+
}
86+
.bloom-inlineImage img {
87+
display: block;
88+
width: 100%;
89+
aspect-ratio: var(--inline-image-aspect-ratio, auto);
90+
}
91+
.bloom-inlineImageLeft,
92+
.bloom-inlineImageMiddle {
93+
/* the offset is transparent padding, and the wrap shape excludes it, so text
94+
flows through it at full width */
95+
padding-top: var(--inline-image-offset, 0px);
96+
shape-outside: inset(var(--inline-image-offset, 0px) 0 0 0);
97+
clear: both;
98+
}
99+
.bloom-inlineImageLeft {
100+
float: left;
101+
width: var(--inline-image-width, 40%);
102+
margin: 0 1em 0.5em 0;
103+
}
104+
.bloom-inlineImageMiddle {
105+
float: left;
106+
width: 100%;
107+
}
108+
.bloom-inlineImageMiddle img {
109+
width: var(--inline-image-width, 40%);
110+
margin: 0 auto;
111+
}
112+
#inlineImageLeft,
113+
#inlineImageMiddle {
114+
height: 220px;
115+
width: 400px;
116+
}
78117
script(type='text/javascript').
79118
$(document).ready(function () {
80119
$(".bloom-editable").each(function () {
@@ -103,6 +142,19 @@ html
103142
.caption(contenteditable='true', lang='teo')
104143
| Caption
105144
//- p.bloom-cloneToOtherLanguages.bloom-preventRemoval
145+
h4 Fields with an inline (Word-style) image
146+
ul
147+
li The text should wrap around the image, and the first lines should run at full width above it (that is the vertical offset).
148+
li You should not be able to delete the image, and it should not move up or down as you type.
149+
li In the second field the image is a full-width band: text above and below it, none beside it.
150+
+field#inlineImageLeft
151+
.bloom-inlineImage.bloom-inlineImageLeft.bloom-keepFirstInField.bloom-preventRemoval(contenteditable='false', style='--inline-image-width: 40%; --inline-image-offset: 40px; --inline-image-aspect-ratio: 4 / 3')
152+
img(src='../../images/experiment.png', alt='')
153+
p Docked left with a 40px offset. These first words should start at the very left edge of the field, above the beaker, and only the later lines should be pushed over to the right of it.
154+
+field#inlineImageMiddle
155+
.bloom-inlineImage.bloom-inlineImageMiddle.bloom-keepFirstInField.bloom-preventRemoval(contenteditable='false', style='--inline-image-width: 50%; --inline-image-offset: 30px; --inline-image-aspect-ratio: 4 / 3')
156+
img(src='../../images/experiment.png', alt='')
157+
p A middle band. This first line belongs above the picture, and everything after the band starts again below it at full width.
106158
+field#brAtStart
107159
br
108160
| brAtStart There was a br at the start here

‎src/BloomBrowserUI/bookEdit/sourceBubbles/SourceBubblesSpec.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,42 @@ describe("SourceBubbles", () => {
196196
]);
197197
});
198198

199+
// Inline (Word-style) images live inside each bloom-editable, so they are inside the
200+
// clone that becomes the source bubble too. They must not show up there: a source bubble
201+
// is for reading another language's text, and the picture is the same in every language
202+
// anyway. Nothing in this file does that on purpose -- the existing hasNoText pass drops
203+
// the text-less wrapper div, taking the img with it -- so this test pins that down,
204+
// because the whole v1 design leans on it. See INLINE-IMAGES-PLAN.md.
205+
it("MakeSourceTextDivForGroup drops inline images from the bubble", () => {
206+
const inlineImage =
207+
"<div class='bloom-inlineImage bloom-inlineImageRight bloom-keepFirstInField bloom-preventRemoval' contenteditable='false'><img src='flower.jpg'/></div>";
208+
const testHtml = $(
209+
[
210+
"<div id='testTarget' class='bloom-translationGroup'>",
211+
` <div class='bloom-editable' lang='es'>${inlineImage}<p>Spanish text</p></div>`,
212+
` <div class='bloom-editable bloom-content1 bloom-visibility-code-on' lang='en'>${inlineImage}<p>English text</p></div>`,
213+
` <div class='bloom-editable' lang='tpi'>${inlineImage}<p>Tok Pisin text</p></div>`,
214+
"</div>",
215+
].join("\n"),
216+
);
217+
$("body").append(testHtml);
218+
// Sanity check: the images really are in the group we are about to clone.
219+
expect($("#testTarget img").length).toBe(3);
220+
221+
const result = BloomSourceBubbles.MakeSourceTextDivForGroup(
222+
$("body").find("#testTarget")[0],
223+
);
224+
225+
// The bubble still has the source languages...
226+
expect(result.find("div.source-text").length).toBe(2);
227+
expect(result.find("div.source-text[lang=es]").text().trim()).toBe(
228+
"Spanish text",
229+
);
230+
// ...but no trace of the image.
231+
expect(result.find("img").length).toBe(0);
232+
expect(result.find(".bloom-inlineImage").length).toBe(0);
233+
});
234+
199235
it("Run CreateDropdownIfNecessary with pre-defined settings", () => {
200236
const testHtml = $(
201237
[

‎src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3811,7 +3811,12 @@ export default class AudioRecording implements IAudioRecorder {
38113811
name != "u" && // ckeditor underline
38123812
name != "sup" && // ckeditor superscript
38133813
name != "a" && // Allow users to manually insert hyperlinks 4.5, and support 4.6 hyperlinks
3814-
!$(child).hasClass("bloom-ui") // don't process transient UI elements (e.g. the format button)
3814+
!$(child).hasClass("bloom-ui") && // don't process transient UI elements (e.g. the format button)
3815+
// Don't process non-editable islands embedded in the text, such as an
3816+
// inline image (bloom-inlineImage). They hold no recordable text, and
3817+
// recursing into one ends at the img element, which would then be
3818+
// treated as a leaf and have audio markup written into it.
3819+
child.getAttribute("contenteditable") !== "false"
38153820
) {
38163821
processedChild = true;
38173822
updateFuncs.push(

0 commit comments

Comments
 (0)