Skip to content
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.

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

Add documentation for highlight and fix bug #3282

Merged
merged 4 commits into from
Jan 5, 2024

Conversation

aaachen
Copy link
Contributor

@aaachen aaachen commented Dec 28, 2023

This change fixes the merge highlight issue: #2996

Problem

Consecutive highlights will automatically get merged into one.

The root cause is

  1. The highlight end offset is non-inclusive (highlight is only supported over text. The offset is counted by characters for all elements' text content)
  2. The overlapping highlight calculation treats the end offset as inclusive (see useSelection.ts)

Changes

  1. Corrected the logic
  2. Added documentations
  3. Correct a new visual bug resulting from changes of 1). See below section for description

Visual Bug

Example sequence:

selection-edge-case
selection cursor ends at empty area, selecting no text at the focus node

selection-edge-case-2
create highlight (note: the word share is not selected)

selection-edge-case-3
create another highlight, note the highlight overlap visual bug

selection-edge-case-4
removing one of highlight sometimes result in the patching to go wrong and jumble the source text#3165

The fix for this visual bug is to clip the selection to the preceding/succeeding DOM element by updating the selection range.

Testing

Manually tested

  1. Forward select

selection-fix-edge-forward-select-1
selection-fix-edge-forward-select-2
selection-fix-edge-forward-select-3

  1. Reverse select

selection-fix-edge-backward-select-1
selection-fix-edge-backward-select-2

Copy link

vercel bot commented Dec 28, 2023

@aaachen is attempting to deploy a commit to the omnivore Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Dec 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
omnivore-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2024 5:17am
omnivore-prod ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2024 5:17am

@jacksonh
Copy link
Contributor

FYI, this one looks good to me. Just fighting a couple fires so I don't want to merge it until early next week.

@jacksonh
Copy link
Contributor

jacksonh commented Jan 5, 2024

OK, going to get this one into our test environment finally. Sorry on the delay. Putting out some fires this week

@jacksonh jacksonh merged commit 68500a9 into omnivore-app:main Jan 5, 2024
5 of 6 checks passed
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