Skip to content

Conversation

siltomato
Copy link
Collaborator

@siltomato siltomato commented Aug 12, 2025

This PR fixes an issue where selecting across text with a lynx insight was breaking the selection. The problematic case was selecting backwards with keyboard selection (shift + arrow keys).

Additionally, in order to fix cursor lag, the editor will switch to the system cursor when the user presses and holds any key that would cause cursor movement. The custom local Quill cursor will be switched back when keys released.


This change is Reviewable

@siltomato siltomato added the will require testing PR should not be merged until testers confirm testing is complete label Aug 12, 2025
Copy link

codecov bot commented Aug 12, 2025

Codecov Report

❌ Patch coverage is 89.74359% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.23%. Comparing base (a37e588) to head (42484de).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...re/ClientApp/src/app/shared/text/text.component.ts 89.74% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3362      +/-   ##
==========================================
+ Coverage   82.21%   82.23%   +0.01%     
==========================================
  Files         608      608              
  Lines       36199    36232      +33     
  Branches     5911     5943      +32     
==========================================
+ Hits        29761    29795      +34     
+ Misses       5579     5564      -15     
- Partials      859      873      +14     

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

@siltomato siltomato marked this pull request as ready for review August 12, 2025 15:46
@Nateowami Nateowami changed the title SF 3373 Fix editor text selection over lynx insight SF 3373 Fix editor text selection over Lynx insight Aug 12, 2025
@pmachapman pmachapman self-requested a review August 12, 2025 23:12
@pmachapman pmachapman self-assigned this Aug 12, 2025
@pmachapman pmachapman changed the title SF 3373 Fix editor text selection over Lynx insight SF-3373 Fix editor text selection over Lynx insight Aug 12, 2025
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @siltomato)


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

      .subscribe(event => {
        this.isShiftDown = event.shiftKey;
      });

If you use shift to select multiple characters, keep shift down, then click the taskbar (or anything other than the web browser), release shift, then click inside another verse, the selected verse doesn't get updated correctly.

One way to fix that would be to updated isShiftDown on mousedown, i.e.:

    fromEvent<MouseEvent>(document, 'mousedown')
      .pipe(quietTakeUntilDestroyed(this.destroyRef))
      .subscribe(event => {
        this.isShiftDown = event.shiftKey;
      });

Code quote:

    fromEvent<KeyboardEvent>(document, 'keydown')
      .pipe(quietTakeUntilDestroyed(this.destroyRef))
      .subscribe(event => {
        this.isShiftDown = event.shiftKey;
      });

@siltomato siltomato force-pushed the fix/sf-3373-text-selection-over-lynx-insight branch from 37029c7 to ffdcc0b Compare August 13, 2025 16:59
Copy link
Collaborator Author

@siltomato siltomato left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pmachapman)


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

Previously, pmachapman (Peter Chapman) wrote…

If you use shift to select multiple characters, keep shift down, then click the taskbar (or anything other than the web browser), release shift, then click inside another verse, the selected verse doesn't get updated correctly.

One way to fix that would be to updated isShiftDown on mousedown, i.e.:

    fromEvent<MouseEvent>(document, 'mousedown')
      .pipe(quietTakeUntilDestroyed(this.destroyRef))
      .subscribe(event => {
        this.isShiftDown = event.shiftKey;
      });

Ah, nice catch. I opted for window 'blur', as it seems to handle this case in addition to the case where the window focus changes without a mouse event (like Alt + Tab).

Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

@pmachapman pmachapman added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Aug 13, 2025
@siltomato siltomato force-pushed the fix/sf-3373-text-selection-over-lynx-insight branch from ffdcc0b to 42484de Compare August 29, 2025 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants