Skip to content

Fix change field handling of narration (BL-16368) #7939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
JohnThomson wants to merge 2 commits into
base: master
Choose a base branch
from
+220 −85
Draft

Fix change field handling of narration (BL-16368) #7939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e307c1e
Fix change field handling of narration (BL-16368)
JohnThomson Jun 1, 2026
9aff65d
Post-review improvements
JohnThomson Jun 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
244 changes: 161 additions & 83 deletions src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1017,22 +1017,23 @@ function adjustAutoSizeForVisibleEditableInTranslationGroup(tg: HTMLElement) {
OverflowChecker.AdjustSizeOrMarkOverflow(visibleEditable);
}

function setEditableContentFromKnownDataBookValueIfAny(
async function setEditableContentFromKnownDataBookValueIfAny(
editable: HTMLElement,
dataBook: string | null,
tg: HTMLElement,
) {
if (!dataBook) {
return;
return true;
}
wrapWithRequestPageContentDelay(
return wrapWithRequestPageContentDelay(
() =>
new Promise<void>((resolve, reject) => {
new Promise<boolean>((resolve, reject) => {
get(
`editView/getDataBookValue?lang=${editable.getAttribute("lang")}&dataBook=${dataBook}`,
(result) => {
try {
const content = result.data;
const response = result.data;
const content = response.content;
// content comes from a source that looked empty, we don't want to overwrite something the user may
// already have typed here.
// But it may well have something in it, because we usually have an empty paragraph to start with.
Expand All @@ -1043,13 +1044,19 @@ function setEditableContentFromKnownDataBookValueIfAny(
// the user can always correct it back to what he just typed.
const temp = document.createElement("div");
temp.innerHTML = content || "";
if (temp.textContent.trim() !== "") {
const keptExistingContent =
temp.textContent.trim() === "";
if (!keptExistingContent) {
editable.innerHTML = content;
applyEditableAudioIdentityFromDataBookValue(
editable,
response,
);
}
adjustAutoSizeForVisibleEditableInTranslationGroup(
tg,
);
resolve();
resolve(keptExistingContent);
} catch (error) {
reject(error);
}
Expand All @@ -1063,6 +1070,53 @@ function setEditableContentFromKnownDataBookValueIfAny(
);
}

function applyEditableAudioIdentityFromDataBookValue(
editable: HTMLElement,
dataBookValue: {
id?: string;
dataAudioRecordingMode?: string;
dataDuration?: string;
dataAudioRecordingEndTimes?: string;
recordingMd5?: string;
hasAudioSentenceClass?: boolean;
hasBloomPostAudioSplitClass?: boolean;
},
) {
const syncAttribute = (attributeName: string, value?: string) => {
if (value) {
editable.setAttribute(attributeName, value);
} else {
editable.removeAttribute(attributeName);
}
};

if (dataBookValue.id) {
editable.setAttribute("id", dataBookValue.id);
}

syncAttribute(
"data-audiorecordingmode",
dataBookValue.dataAudioRecordingMode,
);
syncAttribute("data-duration", dataBookValue.dataDuration);
syncAttribute(
"data-audiorecordingendtimes",
dataBookValue.dataAudioRecordingEndTimes,
);
syncAttribute("recordingmd5", dataBookValue.recordingMd5);

if (dataBookValue.hasAudioSentenceClass) {
editable.classList.add("audio-sentence");
} else {
editable.classList.remove("audio-sentence");
}
if (dataBookValue.hasBloomPostAudioSplitClass) {
editable.classList.add("bloom-postAudioSplit");
} else {
editable.classList.remove("bloom-postAudioSplit");
}
}

function applyAppearanceClassForEditable(editable: HTMLElement) {
editable.classList.remove(
"bloom-contentFirst",
Expand Down Expand Up @@ -1153,7 +1207,7 @@ function makeLanguageMenuItem(
}
}

setEditableContentFromKnownDataBookValueIfAny(
void setEditableContentFromKnownDataBookValueIfAny(
editableInLang,
dataBookValue,
tg,
Expand Down Expand Up @@ -1369,25 +1423,30 @@ function makeFieldTypeMenuItem(
l10nId: null,
english: noneLabel,
onClick: () => {
clearFieldTypeClasses();
for (const editable of Array.from(
tg.getElementsByClassName("bloom-editable"),
) as HTMLElement[]) {
editable.removeAttribute("data-book");
// There's a bit of guess-work involved in what would be most helpful here.
// clearFieldTypeClasses removes any field-type-specific style class,
// and we generally expect a bloom-editable to have some style class.
// Should it be Normal-style or Bubble-style? Bubble-style is the default
// for canvas elements, so I decided to go with that.
const hasStyleClass = Array.from(editable.classList).some(
(className) => className.endsWith("-style"),
);
if (!hasStyleClass) {
editable.classList.add("Bubble-style");
void wrapWithRequestPageContentDelay(async () => {
clearFieldTypeClasses();
for (const editable of Array.from(
tg.getElementsByClassName("bloom-editable"),
) as HTMLElement[]) {
theOneCanvasElementManager?.makeEditableAudioIndependent(
editable,
);
editable.removeAttribute("data-book");
// There's a bit of guess-work involved in what would be most helpful here.
// clearFieldTypeClasses removes any field-type-specific style class,
// and we generally expect a bloom-editable to have some style class.
// Should it be Normal-style or Bubble-style? Bubble-style is the default
// for canvas elements, so I decided to go with that.
const hasStyleClass = Array.from(
editable.classList,
).some((className) => className.endsWith("-style"));
if (!hasStyleClass) {
editable.classList.add("Bubble-style");
}
}
}
adjustAutoSizeForVisibleEditableInTranslationGroup(tg);
setMenuOpen(false);
adjustAutoSizeForVisibleEditableInTranslationGroup(tg);
setMenuOpen(false);
}, "setCanvasFieldTypeToNone");
},
icon: !activeType && <CheckIcon css={getMenuIconCss()} />,
},
Expand All @@ -1397,71 +1456,90 @@ function makeFieldTypeMenuItem(
l10nId: null,
english: fieldType.label,
onClick: () => {
clearFieldTypeClasses();
const editables = Array.from(
tg.getElementsByClassName("bloom-editable"),
) as HTMLElement[];
if (fieldType.readOnly) {
const readOnlyDiv = document.createElement("div");
readOnlyDiv.setAttribute(
"data-derived",
fieldType.dataDerived,
);
if (fieldType.hint) {
readOnlyDiv.setAttribute("data-hint", fieldType.hint);
}
if (fieldType.functionOnHintClick) {
void wrapWithRequestPageContentDelay(async () => {
clearFieldTypeClasses();
const editables = Array.from(
tg.getElementsByClassName("bloom-editable"),
) as HTMLElement[];
if (fieldType.readOnly) {
const readOnlyDiv = document.createElement("div");
readOnlyDiv.setAttribute(
"data-functiononhintclick",
fieldType.functionOnHintClick,
"data-derived",
fieldType.dataDerived,
);
}
readOnlyDiv.classList.add(...fieldType.classes);
tg.parentElement!.insertBefore(readOnlyDiv, tg);
tg.remove();
// Tempting to do this here, but we're going to reload the page,
// and it's only after the reload that the div we just created will have
// its content. Another call to this function will happen after the reload,
// and that can do a better job of fixing it. Doing it now would just
// reduce the height to its minimum, forcing the new content to be shown on
// one line.
//ensureFieldFitsOnCustomPage(readOnlyDiv);
// Reload the page to get the derived content loaded.
post("common/saveChangesAndRethinkPageEvent", () => {});
} else {
removeConflictingStyleClasses(fieldType, editables);
tg.classList.add(...fieldType.classes);
for (const editable of editables) {
editable.classList.add(...fieldType.editableClasses);
editable.setAttribute("data-book", fieldType.dataBook);
if (
fieldsControlledByAppearanceSystem.includes(
fieldType.dataBook,
)
) {
applyAppearanceClassForEditable(editable);
} else {
editable.classList.remove(
"bloom-contentFirst",
"bloom-contentSecond",
"bloom-contentThird",
if (fieldType.hint) {
readOnlyDiv.setAttribute(
"data-hint",
fieldType.hint,
);
}
if (
editable.classList.contains(
"bloom-visibility-code-on",
)
) {
setEditableContentFromKnownDataBookValueIfAny(
editable,
if (fieldType.functionOnHintClick) {
readOnlyDiv.setAttribute(
"data-functiononhintclick",
fieldType.functionOnHintClick,
);
}
readOnlyDiv.classList.add(...fieldType.classes);
tg.parentElement!.insertBefore(readOnlyDiv, tg);
tg.remove();
// Tempting to do this here, but we're going to reload the page,
// and it's only after the reload that the div we just created will have
// its content. Another call to this function will happen after the reload,
// and that can do a better job of fixing it. Doing it now would just
// reduce the height to its minimum, forcing the new content to be shown on
// one line.
//ensureFieldFitsOnCustomPage(readOnlyDiv);
// Reload the page to get the derived content loaded.
post("common/saveChangesAndRethinkPageEvent", () => {});
} else {
const fieldTypeChanged =
activeType !== fieldType.dataBook;
removeConflictingStyleClasses(fieldType, editables);
tg.classList.add(...fieldType.classes);
for (const editable of editables) {
editable.classList.add(
...fieldType.editableClasses,
);
editable.setAttribute(
"data-book",
fieldType.dataBook,
tg,
);
if (
fieldsControlledByAppearanceSystem.includes(
fieldType.dataBook,
)
) {
applyAppearanceClassForEditable(editable);
} else {
editable.classList.remove(
"bloom-contentFirst",
"bloom-contentSecond",
"bloom-contentThird",
);
}
let keptExistingContent = false;
if (
editable.classList.contains(
"bloom-visibility-code-on",
)
) {
keptExistingContent =
await setEditableContentFromKnownDataBookValueIfAny(
editable,
fieldType.dataBook,
tg,
);
}
if (fieldTypeChanged && keptExistingContent) {
theOneCanvasElementManager?.makeEditableAudioIndependent(
editable,
);
}
}
adjustAutoSizeForVisibleEditableInTranslationGroup(tg);
}
adjustAutoSizeForVisibleEditableInTranslationGroup(tg);
}
setMenuOpen(false);
setMenuOpen(false);
}, "setCanvasFieldType");
},
icon: activeType === fieldType.dataBook && (
<CheckIcon css={getMenuIconCss()} />
Expand Down
35 changes: 34 additions & 1 deletion src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6183,7 +6183,10 @@ export class CanvasElementManager {
return patriarchDuplicateElement;
}

private copyAnySoundFileAndAttributesForEditable(
/// <summary>
/// Copies any audio-related ids and files from one editable canvas field to another.
/// </summary>
public copyAnySoundFileAndAttributesForEditable(
sourceElement: HTMLElement,
copiedElement: HTMLElement,
): void {
Expand Down Expand Up @@ -6225,6 +6228,36 @@ export class CanvasElementManager {
}
}

/// <summary>
/// Makes audio-sentence ids in the editable independent by assigning new ids and copying
/// any corresponding audio files when they exist.
/// </summary>
public makeEditableAudioIndependent(editable: HTMLElement): void {
if (!editable) {
return;
}

const audioElements: Element[] = [];
if (
editable.classList.contains("audio-sentence") &&
editable.getAttribute("id")
Comment on lines +6242 to +6243

@cubic-dev-ai cubic-dev-ai Bot Jun 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The new audio-independence logic skips TextBox recordings, so some kept recordings may retain shared ids instead of being duplicated with a new id.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts, line 6242:

<comment>The new audio-independence logic skips `TextBox` recordings, so some kept recordings may retain shared ids instead of being duplicated with a new id.</comment>

<file context>
@@ -6225,6 +6228,36 @@ export class CanvasElementManager {
+
+        const audioElements: Element[] = [];
+        if (
+            editable.classList.contains("audio-sentence") &&
+            editable.getAttribute("id")
+        ) {
</file context>
Suggested change
editable.classList.contains("audio-sentence") &&
editable.getAttribute("id")
(editable.classList.contains("audio-sentence") ||
editable.getAttribute("data-audiorecordingmode") ===
"TextBox") &&
editable.getAttribute("id")

@JohnThomson JohnThomson Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a hallucination. The new logic calls makeEditableAudioIndependent(), which includes the passed element in the list it passes to copySoundFileAndAttributes(). The latter does copy ids and various other attributes.

@cubic-dev-ai cubic-dev-ai Bot Jun 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I've saved this as a new learning to improve future reviews.

) {
audioElements.push(editable);
}

audioElements.push(
...Array.from(editable.querySelectorAll(".audio-sentence[id]")),
);

audioElements.forEach((audioElement) => {
const oldId = audioElement.getAttribute("id");
if (!oldId) {
return;
}
this.copySoundFileAndAttributes(audioElement, oldId, audioElement);
});
}

private copySoundFileAndAttributes(
sourceElement: Element,
sourceId: string,
Expand Down
26 changes: 25 additions & 1 deletion src/BloomExe/web/controllers/EditingViewApi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,31 @@ private void HandleGetDataBookValue(ApiRequest request)
var dataBook = request.RequiredParam("dataBook");
var multiText = View.Model.CurrentBook.BookData.GetMultiTextVariableOrEmpty(dataBook);
var value = multiText.GetExactAlternative(lang) ?? "";
request.ReplyWithText(value);
var matchingDataDivElement =
View.Model.CurrentBook.RawDom.SelectSingleNode(
$"//div[@id='bloomDataDiv']/div[@data-book='{dataBook}' and @lang='{lang}']"

@cubic-dev-ai cubic-dev-ai Bot Jun 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Do not interpolate request parameters directly into XPath; malformed or crafted values can break or alter the query.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomExe/web/controllers/EditingViewApi.cs, line 162:

<comment>Do not interpolate request parameters directly into XPath; malformed or crafted values can break or alter the query.</comment>

<file context>
@@ -157,7 +157,32 @@ private void HandleGetDataBookValue(ApiRequest request)
-            request.ReplyWithText(value);
+            var matchingDataDivElement =
+                View.Model.CurrentBook.RawDom.SelectSingleNode(
+                    $"//div[@id='bloomDataDiv']/div[@data-book='{dataBook}' and @lang='{lang}']"
+                ) as SafeXmlElement;
+
</file context>

@JohnThomson JohnThomson Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's saying we should not put {dataBook} and {lang} into the xpath because someone might find a way to use fake values to inject something dangerous. But we do this all over, and I'm not sure what reasonable alternative there might be here. I don't think it's worth fixing.

@cubic-dev-ai cubic-dev-ai Bot Jun 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context—understood this isn’t a change you want to make here.

@greptile-apps greptile-apps Bot Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 XPath injection risk from unsanitized params

dataBook and lang are inserted directly into the XPath string using string interpolation. A value containing a single quote (e.g. a lang attribute set to en' in a crafted book file) would malform the expression, causing a parse error or — in adversarial input — an injected predicate. Standard language tags never carry single quotes, but a corrupt or hand-crafted book could set unusual lang values.

Consider escaping or validating both parameters before interpolation, or use a parameterised XPath API if the underlying library supports one.

@andrew-polk andrew-polk Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed

) as SafeXmlElement;

request.ReplyWithJson(
new
{
content = value,
id = matchingDataDivElement?.GetAttribute("id"),
dataAudioRecordingMode = matchingDataDivElement?.GetAttribute(
"data-audiorecordingmode"
),
dataDuration = matchingDataDivElement?.GetAttribute("data-duration"),
dataAudioRecordingEndTimes = matchingDataDivElement?.GetAttribute(
"data-audiorecordingendtimes"
),
recordingMd5 = matchingDataDivElement?.GetAttribute("recordingmd5"),
hasAudioSentenceClass = matchingDataDivElement?.HasClass("audio-sentence")
?? false,
hasBloomPostAudioSplitClass = matchingDataDivElement?.HasClass(
"bloom-postAudioSplit"
) ?? false,
}
);
}

/// <summary>
Expand Down