SF-3332 Prevent user from deleting footnotes

[RaymondLuong3]

Users were previously allowed to make a selection that included a cross reference or footnote. We do not want to allow users to select these notes which they would then be able to overwrite or delete. This PR introduces logic to identify any embed in a selection and update the selection be come before the embed.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.04%. Comparing base (7a1ae7f) to head (3b8aa08).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...re/ClientApp/src/app/shared/text/text.component.ts	94.44%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3915   +/-   ##
=======================================
  Coverage   81.04%   81.04%           
=======================================
  Files         659      659           
  Lines       42742    42757   +15     
  Branches     7015     6994   -21     
=======================================
+ Hits        34639    34653   +14     
- Misses       6944     6960   +16     
+ Partials     1159     1144   -15     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[marksvc]

This is reviewable in Devin Review.

[marksvc]

@marksvc reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on RaymondLuong3).

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts line 1955 at r1 (raw file):

        textualNoteIndex += op.insert.length;
      } else if (op.insert != null && typeof op.insert === 'object') {
        if (op.insert['note'] != null) break;

I would expect that other embeds can likewise be deleted, not just footnotes. Do we have reason to believe that it's only notes that are affected by the bug?

[RaymondLuong3]

@RaymondLuong3 made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts line 1955 at r1 (raw file):

Previously, marksvc wrote…

I would expect that other embeds can likewise be deleted, not just footnotes. Do we have reason to believe that it's only notes that are affected by the bug?

Good thinking. I could not think of a reason to allow selecting any embed, even blanks. I have updated this PR so that any embed selection is truncated. This will mainly be footnotes/cross references, but could be other embeds also.

[marksvc]

@marksvc reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on RaymondLuong3).

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts line 1934 at r2 (raw file):
Devin was concerned about the above, saying

the default parameter allowEmbedSelection = true means all other callers (isValidSelectionForCurrentSegment at line 1094, used by beforeinput/keydown/backspace/delete guards, and ) retain the old behavior where embed-containing selections are considered valid. The embed exclusion therefore relies solely on adjustSelection reshaping the selection (scheduled as a microtask via void Promise.resolve().then(() => this.adjustSelection()) at line 1664). If a user drags a selection over an embed and triggers a delete before that microtask runs, the validity checks would not block it. Worth confirming existing embed-restoration logic (getEmbedsAffectedByDelta) still protects against actual deletion in that race window.

I asked Claude about it, and Claude was concerned as well. I asked Claude to fix the concern, and it made commit:

Prevent backspace/delete/beforeinput over a selection spanning a footnote

isValidSelectionForCurrentSegment (used to gate keydown backspace/delete/cut
and beforeinput) called conformToValidSelectionForCurrentSegment with the
default allowEmbedSelection=true, so a selection merely containing an embed
was still treated as valid. The embed exclusion added previously only ran
via adjustSelection, which fires from a post-edit microtask and can't stop
a delete that already happened. Pass allowEmbedSelection=false so the
guards used by backspace, delete, cut, and beforeinput reject/trim
embed-spanning selections before the native edit is applied.

# src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts
@@ -1091,7 +1091,7 @@ export class TextComponent implements AfterViewInit, OnDestroy {
 
   /** Is a given selection range valid for editing the current segment? */
   isValidSelectionForCurrentSegment(sel: Range): boolean {
-    const newSel: Range | null = this.conformToValidSelectionForCurrentSegment(sel);
+    const newSel: Range | null = this.conformToValidSelectionForCurrentSegment(sel, false);
     return !(newSel == null || sel.index !== newSel.index || sel.length !== newSel.length);
   }

It added a unit test that had the same setup as your introduced test, but ended with these:

    const selectionSpanningFootnote: QuillRange = { index: segmentRange.index, length: segmentRange.length };

    expect(env.component.isValidSelectionForCurrentSegment(selectionSpanningFootnote))
      .withContext('a selection that spans a footnote should not be considered valid')
      .toBeFalse();
    expect(env.quillHandleBackspace(selectionSpanningFootnote))
      .withContext('backspace should not be allowed over a selection spanning a footnote')
      .toBeFalse();
    expect(env.quillHandleDelete(selectionSpanningFootnote))
      .withContext('delete should not be allowed over a selection spanning a footnote')
      .toBeFalse();

It does seem concerning that since adjustSelection is run as void Promise.resolve().then(() => this.adjustSelection()); that it might not happen fast enough before edits are made.

I don't want to just assume that the AI's concern is correct here. And I haven't carefully studied the situation. Can you comment on this?

[RaymondLuong3]

@RaymondLuong3 made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts line 1934 at r2 (raw file):

Previously, marksvc wrote…

Devin was concerned about the above, saying

the default parameter allowEmbedSelection = true means all other callers (isValidSelectionForCurrentSegment at line 1094, used by beforeinput/keydown/backspace/delete guards, and ) retain the old behavior where embed-containing selections are considered valid. The embed exclusion therefore relies solely on adjustSelection reshaping the selection (scheduled as a microtask via void Promise.resolve().then(() => this.adjustSelection()) at line 1664). If a user drags a selection over an embed and triggers a delete before that microtask runs, the validity checks would not block it. Worth confirming existing embed-restoration logic (getEmbedsAffectedByDelta) still protects against actual deletion in that race window.

I asked Claude about it, and Claude was concerned as well. I asked Claude to fix the concern, and it made commit:

Prevent backspace/delete/beforeinput over a selection spanning a footnote

isValidSelectionForCurrentSegment (used to gate keydown backspace/delete/cut
and beforeinput) called conformToValidSelectionForCurrentSegment with the
default allowEmbedSelection=true, so a selection merely containing an embed
was still treated as valid. The embed exclusion added previously only ran
via adjustSelection, which fires from a post-edit microtask and can't stop
a delete that already happened. Pass allowEmbedSelection=false so the
guards used by backspace, delete, cut, and beforeinput reject/trim
embed-spanning selections before the native edit is applied.

# src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts
@@ -1091,7 +1091,7 @@ export class TextComponent implements AfterViewInit, OnDestroy {
 
   /** Is a given selection range valid for editing the current segment? */
   isValidSelectionForCurrentSegment(sel: Range): boolean {
-    const newSel: Range | null = this.conformToValidSelectionForCurrentSegment(sel);
+    const newSel: Range | null = this.conformToValidSelectionForCurrentSegment(sel, false);
     return !(newSel == null || sel.index !== newSel.index || sel.length !== newSel.length);
   }

It added a unit test that had the same setup as your introduced test, but ended with these:

    const selectionSpanningFootnote: QuillRange = { index: segmentRange.index, length: segmentRange.length };

    expect(env.component.isValidSelectionForCurrentSegment(selectionSpanningFootnote))
      .withContext('a selection that spans a footnote should not be considered valid')
      .toBeFalse();
    expect(env.quillHandleBackspace(selectionSpanningFootnote))
      .withContext('backspace should not be allowed over a selection spanning a footnote')
      .toBeFalse();
    expect(env.quillHandleDelete(selectionSpanningFootnote))
      .withContext('delete should not be allowed over a selection spanning a footnote')
      .toBeFalse();

It does seem concerning that since adjustSelection is run as void Promise.resolve().then(() => this.adjustSelection()); that it might not happen fast enough before edits are made.

I don't want to just assume that the AI's concern is correct here. And I haven't carefully studied the situation. Can you comment on this?

That is interesting. I guess there is a very small chance that the microtask is not fast enough to prevent editing the selection. I doubt that would happen with a user, but the suggestion does not look difficult to implement. I have adapted the suggestion.

[marksvc]

Okay. Though I found that if I do the following, I am able to cut the footnote.

• hold shift.
• arrow to past a footnote
• still holding shift.
• right-click the selected text, and choose Cut.

As seen in the attached video.

Similarly, I can cut the footnote by doing the following.

• Click on one side of footnote.
• Hold shift.
• Click on other side of footnote.
• Still holding shift.
• Right-click the selected text, and choose Cut.

Note that if I try to do this to verse numbers, I am unsuccessful.

So, I approve of the improvement, though there is still the potential to create the problem by the above means. (And probably should be included in the same JIRA issue.)

Video showing problem.

@marksvc reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on RaymondLuong3).