Skip to content

BL-16822 Inline images: teach the code that walks a text block's children - #8322

Draft
hatton wants to merge 1 commit into
BL-16822-inline-4-interactionsfrom
BL-16822-inline-5-neighbours
Draft

BL-16822 Inline images: teach the code that walks a text block's children#8322
hatton wants to merge 1 commit into
BL-16822-inline-4-interactionsfrom
BL-16822-inline-5-neighbours

Conversation

@hatton

@hatton hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member

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.

Card: https://issues.bloomlibrary.org/youtrack/issue/BL-16822


🤖 Generated with Claude Code

https://claude.ai/code/session_014DCBGajN5YyAYPBenEf1yy

Devin review


This change is Reviewable

Comment thread src/BloomBrowserUI/bookEdit/bloomField/BloomField.ts
@hatton

hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

[Claude Opus 5 (1M context) from Hatton's machine during devin-review]

Consulted Devin on 2026-09-07 12:46 MDT up to commit 2212d546e4.

It raised 1 thing. Each has its own review thread with the reasoning and the outcome; in short:

  • Held delete bypasses image protection — real, and a hole in the very guard this PR rewrites: holding Delete got a picture past it. Fixed, with a test for each half.

The fixes were amended into this branch after the review, so the commit above is not the current head; a fresh review is running against the new one. The only GitHub check on this PR is pr-automation (the review trigger), which passed; the front-end unit suite (915 tests), the C# spreadsheet tests (415) and the end-to-end suite (85) were all run locally and are green.

@hatton
hatton force-pushed the BL-16822-inline-4-interactions branch from 1411da9 to daf1f21 Compare September 7, 2026 21:09
@hatton
hatton force-pushed the BL-16822-inline-5-neighbours branch from 443ae96 to 80a62c7 Compare September 7, 2026 21:09
Comment thread src/BloomExe/Book/TranslationGroupManager.cs
Comment thread src/BloomBrowserUI/bookEdit/bloomField/BloomField.ts
@hatton

hatton commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

[Claude Opus 5 (1M context) from Hatton's machine during devin-review]

Consulted Devin again on 2026-09-07 14:00 MDT, on commit 443ae9675e (the head after the first round of fixes).

It raised 2 new things. Each has its own review thread with the reasoning and the outcome; in short:

  • Duplicate repair repeats inline pictures — real: merging two same-language divs that each held a copy of one picture left the reader seeing it twice. Fixed, with a test.
  • Overlapping keys share one snapshot — checked case by case and left: the snapshot can only be older than the current keystroke, which is the safe direction.

It also re-reported the findings the reviewed commit already fixes; those are the first round's threads, which carry their outcomes.

These fixes were amended into this branch after the review, so the commit above is not the current head. The only GitHub check on this PR is pr-automation (the review trigger). Run locally and green after these fixes: the front-end unit suite (920 tests), the end-to-end suite (85 passed, 1 skipped), and the C# spreadsheet and translation-group tests (417 and 50).

…dren

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
@hatton
hatton force-pushed the BL-16822-inline-4-interactions branch from daf1f21 to 7de94f8 Compare September 7, 2026 21:55
@hatton
hatton force-pushed the BL-16822-inline-5-neighbours branch from 80a62c7 to cc8c4bf Compare September 7, 2026 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant