Skip to content

Stop cleanUpNbsps orphaning CKEditor's zero-width filling char in titles (BL-16808) - #8332

Merged
StephenMcConnel merged 1 commit into
Version6.4from
BL-16808
Sep 9, 2026
Merged

Stop cleanUpNbsps orphaning CKEditor's zero-width filling char in titles (BL-16808)#8332
StephenMcConnel merged 1 commit into
Version6.4from
BL-16808

Conversation

@andrew-polk

@andrew-polk andrew-polk commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Problem. Follow-up to #8305. A tester found that on 6.4 a title typed as "One Two" with Enter (or Shift+Enter) pressed at the start of "Two" still reaches the Publish tab and bloomlibrary.org as "OneTwo", even though #8305 now puts a line break between the two lines. Show Hidden Characters reveals a zero-width space at the start of the second line. 6.5 does not have the problem.

Cause. CKEditor plants a zero-width "filling char" (U+200B) at the caret whenever it re-selects a caret that sits next to an inline element, such as the line-break span Shift+Enter inserts, and removes it later by holding a reference to that text node. Bloom's half-second tidy-up after every keystroke took a CKEditor bookmark and restored it, which is such a re-select, and it also assigned the box's innerHTML whether or not anything had changed. Each rewrite detached the node CKEditor was tracking, so the character stayed as ordinary text, a fresh one was planted on the next pause, and the result was saved. Chromium renders a newline adjacent to U+200B as nothing at all, so the line break became invisible. Reproduced in a running 6.4 build over CDP, and master fixed both halves in BL-16558 and BL-16717, which is why 6.5 is clean.

What the PR does. Ports the two master changes: cleanUpNbsps assigns innerHTML only when it actually converted a non-breaking space, and the tidy-up takes a bookmark only when a tool is active or the box holds a comment or an nbsp, the only cases in which anything rewrites it. With both, the tester's Enter and Shift+Enter sequences are clean at every step in a running Bloom, Show Hidden Characters included, and the saved title is "One \nTwo". Five new tests check node identity and the gate. Not done: cleaning books that already carry a stray zero-width space, and stripping one that CKEditor itself plants after, say, a Backspace next to a line break and that Show Hidden Characters then orphans; master strips those at save (BL-16490), which was judged too risky for 6.4 because Khmer, Thai and Myanmar text uses U+200B legitimately.

Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16808

Devin review


This change is Reviewable

Comment thread src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts
Comment thread src/BloomBrowserUI/bookEdit/toolbox/toolboxSpec.ts
@andrew-polk

Copy link
Copy Markdown
Contributor Author

[Claude Fable 5.1] Consulted Devin on 2026-09-08 20:27 UTC up to commit 3037a63ac8cbd7f7cfb4710a69395e110c53c22a. It raised one non-severe bug and one Investigate flag, both about the same accepted limit (the box is still rebuilt on the rare pause where a non-breaking space is converted); each has its own resolved thread above with the reasoning. No informational items. CI passed.

@andrew-polk

Copy link
Copy Markdown
Contributor Author

[Claude Fable 5.1] Consulted Devin on 2026-09-08 21:11 UTC up to commit 32b5e5e168d186fff43dc0da72277472ee520a6c (and on the two commits before it this run). Each re-review repeated the same non-severe bug and Investigate flag about the accepted limit already answered in the resolved threads above (the box is still rebuilt on the rare pause where a non-breaking space is converted); nothing new. No informational items. CI passed.

@andrew-polk

Copy link
Copy Markdown
Contributor Author

[Claude Fable 5.1] Consulted Devin on 2026-09-08 21:48 UTC up to commit cd867a2bfd3edd62bf5a5a3f9f07748bf91b624c (comment-wording change). Same two repeated findings as before, already answered in the resolved threads above; nothing new.

…lling char in titles (BL-16808)

Follow-up to #8305. A title typed "One Two" with Enter or Shift+Enter at the
start of "Two" still published as "OneTwo" on 6.4: Show Hidden Characters
showed a U+200B at the start of the second line, and Chromium renders a
newline adjacent to U+200B as nothing.

CKEditor plants that "filling char" whenever it re-selects a caret next to an
inline element (the bloom-linebreak span, for one) and removes it later by
node reference. Bloom's half-second tidy-up after every keystroke took and
restored a CKEditor bookmark, which is such a re-select, and assigned the
box's innerHTML whether or not anything had changed. Each rewrite detached the
tracked node, so the character stayed as text, a fresh one was planted on the
next pause, and the result was saved. Reproduced in a running 6.4 build over
CDP; master fixed both halves in BL-16558 and BL-16717.

Port those two changes:
- cleanUpNbsps assigns innerHTML only when it actually converted an nbsp.
- The tidy-up takes a bookmark only when a tool is active or the box holds a
  comment or an nbsp (editableMightBeRewritten), the only cases in which
  anything rewrites it.

With both, Enter and Shift+Enter are clean at every step in a running Bloom,
Show Hidden Characters included, and the saved title is "One \nTwo". Five new
tests check node identity, the bookmark refill on both paths, and the gate.

Not done: cleaning books that already carry a stray U+200B, and master's
save-time strip (BL-16490), judged too risky for 6.4 because Khmer, Thai and
Myanmar text uses U+200B legitimately.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@andrew-polk
andrew-polk marked this pull request as ready for review September 8, 2026 21:48

@StephenMcConnel StephenMcConnel left a comment

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.

@StephenMcConnel reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions.

@StephenMcConnel StephenMcConnel left a comment

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.

@StephenMcConnel resolved 2 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on andrew-polk).

@StephenMcConnel
StephenMcConnel merged commit 5c726cf into Version6.4 Sep 9, 2026
2 checks passed
@StephenMcConnel
StephenMcConnel deleted the BL-16808 branch September 9, 2026 14:36
andrew-polk added a commit that referenced this pull request Sep 9, 2026
Brings the BL-16808 follow-up (#8332). Version6.5 already had the same two
code changes from master (BL-16558's conditional rewrite in cleanUpNbsps and
BL-16717's bookmark gate), so the conflicts in toolbox.ts and toolboxSpec.ts
were comments and one test line. Resolved to Version6.5's side, keeping from
6.4 only: a sentence in each comment recording the BL-16808 consequence (an
orphaned ckeditor filling char saved into the book), the   escape in the
test's sanity check instead of a literal invisible character, and the
`if (bookmarks)` guard in the async markup branch, which also clears a
pre-existing tsc error there (TS2345, `any[] | undefined`).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.

2 participants