-
Notifications
You must be signed in to change notification settings - Fork 129
Fix for ios specific behavior of caret jumping to beginning/end on tap top/bottom edge #3410
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
base: main
Are you sure you want to change the base?
Fix for ios specific behavior of caret jumping to beginning/end on tap top/bottom edge #3410
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the caret no longer moves to the beginning or end of the editable, so it looks like the char bounding box calculations are correct.
- Now that we've determined that
clip-pathis ineffective, you can go ahead and remove it from the codebase. - Can you confirm that this works with nested HTML, like bold, italic, underline?
- I think there is some confusion about the expected behavior. The caret should move to the nearest word. Preventing the default behavior and stopping there creates an undesirable dead zone where tapping between thoughts does nothing at all. If you're going to prevent the default browser behavior, you'll have to programmatically set the caret to the expected offset.
src/components/Editable.tsx
Outdated
| const shouldPreventDefaultCaretBehaviour = useCallback( | ||
| (clientX: number, clientY: number, preventDefault: () => void): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is called shouldPreventDefaultCaretBehaviour which suggests a simple boolean predicate, yet it also takes a callback function. This hybrid approach is unusual. Try to separate the pure bits from the side effects and avoid mixing them.
src/e2e/puppeteer/test-puppeteer.sh
Outdated
|
|
||
| cleanup() { | ||
| stop_dev_server | ||
| stop_dev_server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidental whitespace change
src/components/Editable.tsx
Outdated
| /** Check if click is vertically aligned with a character (for edge cases). */ | ||
| const isVerticallyAligned = (rect: DOMRect | null): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call this isVerticallyInside or isVerticallyContained, since I think of alignment as having top or bottom edge exactly matching.
What does "edge cases" refer to?
src/components/Editable.tsx
Outdated
| return false | ||
| } | ||
|
|
||
| // Get the browser range for the click position |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call this "tap" instead of "click" throughout this function, since we're dealing with mobile, not desktop.
src/components/Editable.tsx
Outdated
| const offset = range.startOffset | ||
| const nodeTextLength = node.textContent?.length || 0 | ||
|
|
||
| /** Gets the bounding client rect for a character at the given offset. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also note that the bounding box includes all line-height, since that's very relevant for us.
Update:
Here is a quick workflow on how the mechanism works: a. Firstly we detect if the click/tap happened on a valid character or on a void area. The click is marked valid if either b. We get all the text nodes that can recieve a caret using document.createTreeWalker. c. Taking tap/click's Y coordinate, for each text node we calculate the vertical distance to the tap coordinate and find the nearest node where caret should be placed. d. In case of multiline nodes, we need to find which line we tapped near. So to do that we break the text node into individual lines. Each line has its own bounding rectangle, so again by comparing the vertical distance the closet line is identified. e. Once we know which text node and which line, we need to find the exact horizontal position. A binary search algorithm is used for efficiency where the exact charcter position is calculated. Then we get a cumulative offset from start of editable to the given text node position. f. Finally, we dispatch FYI, a lot of image snapshots had to be updated because we have removed the lineHeightOverlap calculation. So the y positioning of the editables differ. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! A couple changes I'd like to request before I do a full review.
- The snapshots should not be updated. If
lineHeightOverlapis removed, you will need to compensate for the change another way, such as adjusting the line height oryposition. (This is generalizable... the snapshots are always correct unless there is a design change.) - This needs much better encapsulation than it currently has. What should each function/component be allowed access to? How are responsibilities distributed? The helper functions you created are logical and help break up the code, but you need to take it a step further and think about the architecture. How can you hide implementation details? You're grouping functionality but not abstracting it.
Otherwise this looks like a robust algorithm that will yield better caret placement behavior than native Safari which will be a great improvement. It will also fix a known issue where caret placement is broken at cliffs. I look forward to doing a more thorough review once the above points are addressed.
…-default-caret-jumping-issue
… from click coordinates
78f06e6 to
64ab2be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what comment to put here as this was previously related with clip-path. However to preserve the y positions as it were, I have not removed the lineHeightOverlap calculation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should remove lineHeightOverlap completely and compensate by adjusting the actual line height, but we can do that in a separate refactor.
|
@BayuAri Ready for testing on mobile Safari! Let me know if you have any questions. |
raineorshine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the update!
The functionality is spread out over caretPositioning, textNodeUtils, and voidAreaDetection, so it's not quite as contained as it could be yet. Please co-locate functionality and hide implementation details when possible.
Please take some time to think about the architectural decisions here, and the difference between abstracting and grouping.
src/util/caretPositioning.ts
Outdated
| * @param hi - Upper bound of the search range (character index). | ||
| * @returns The character offset where the caret should be placed. | ||
| */ | ||
| const binarySearchOffset = (node: Text, clientX: number, lo: number, hi: number): number => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of a function is part of its public API, and should describe what it does, but not how it does it. "Binary search" is an implementation detail, and not something that should be exposed to the caller.
|
@karunkop @raineorshine Issue A: Getting IndexSizeError when trying to tap on the "This is an empty thought" or "Add a thought" placeholderStep to reproduce
##Current behavior Video: ScreenRecording_12-17-2025.21-44-16_1.MP4Note: This also happens on "This is an empty thought" placeholder text ##Expected behavior Issue B: It is hard to move between words that has 2-3 charactersThe tapping is not always responding accordingly Step to reproduce
Current behaviorCaret is not always moved to the tapped area. Caret.is.not.always.moved.to.the.tapped.area.MP4Expected behaviorCaret should move accordingly to user's tapping |
BayuAri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added on #3410 (comment)
…-default-caret-jumping-issue
@BayuAri , issue A has been fixed! You can test that. As for issue B, this is a default ios browser behavior. The same behavior also exists in |
raineorshine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks so much for the update! Comments below.
src/components/Editable.tsx
Outdated
| const handleVoidAreaTap = useCallback( | ||
| (clientX: number, clientY: number, preventDefault: () => void): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why preventDefault is being structured as a callback here. It's redundant with the return type, yet written as if it is independent. It should be handled by the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it is indeed redundant. I have refactored the function such that the caller itself handles the preventDefault.
src/components/Editable.tsx
Outdated
| // Update Redux cursor state | ||
| dispatch( | ||
| setCursor({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that dispatching setCursor is necessary? This should happen automatically after the selection is set due to the onFocus handler.
If this is not necessary, then dispatch and path are no longer dependencies and this callback can be factored out and combined with detectVoidArea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this seems necessary. When focus changes, onFocus calls setCursorOnThought which dispatches setCursor with offset: null. This lets the browser determine the position. Here since we are manually calculating the nearest node and offset, we need to dispatch setCursor with correct offset.
src/components/Editable.tsx
Outdated
| // Set caret manually | ||
| selection.restore({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you are using selection.restore instead of selection.set? selection.restore is only intended to be used with the result of selection.save and SavedSelection is effectively an internal type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don' think selection is necessary at all, as I just realised that the behavior is handled by dispatching setCursor in the first place.
src/util/caretPositioning.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions are only used by detectVoidArea, but it is factored out into its own file for some reason. They are both utility functions with dependencies on the DOM. They're the same level of abstraction, so there's no real benefit to separating them.
Related: #3410 (comment)
src/components/Editable.tsx
Outdated
|
|
||
| allowDefaultSelection() | ||
| // If not a void area tap, allow browser's default selection | ||
| if (!handleVoidAreaTap(e.clientX, e.clientY, () => e.preventDefault())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since handleVoidAreaTap is called in both onMouseDown and handleTouchStart, does it end up getting called twice on each tap? Is there a more efficient way to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does end up getting called twice on each tap. As of now I am not able to think of an efficient way to handle this though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should remove lineHeightOverlap completely and compensate by adjusting the actual line height, but we can do that in a separate refactor.
src/util/caretPositioning.ts
Outdated
| return findOffsetAtX(node, clientX, targetLine.start, targetLine.end) | ||
| } | ||
|
|
||
| export default calculateHorizontalOffset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/components/Editable.tsx
Outdated
| // Add event listener with { passive: false } to allow preventDefault | ||
| editable.addEventListener('touchstart', handleTouchStart, { passive: false }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify a little more how this manual event handling differs from using React onTouchStart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manual event handling is necessary here because react doesn't specifiy {passive: false} when attaching the listener. https://stackoverflow.com/questions/42101723/unable-to-preventdefault-inside-passive-event-listener, as discussed here, a passive listener cannot call preventDefault(). Upon doing so it logs a warning and has no effect. And hence in ios/safari touch devices, default behavior is not prevented and the caret still jumps to the end of the text when clicked in the void area.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
BayuAri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karunkop @raineorshine
Confirmed the new fix is working as expected.
Caret moves onto the beginning /end of the specific word instead of the whole thought content.
… utlity function for caret position offset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing a bit of dead zone where the caret offset is not updated.
I added an overlay so you can more easily see where my touchstart is.
window.addEventListener('touchstart', e => {
const touch = e.touches[0]
const dot = document.getElementById('touch-dot') ?? document.createElement('div')
dot.id = 'touch-dot'
dot.style.position = 'absolute'
dot.style.left = `${touch.clientX}px`
dot.style.top = `${touch.clientY}px`
dot.style.width = '1px'
dot.style.height = '1px'
dot.style.backgroundColor = 'rgb(0,255,0)'
dot.style.zIndex = '9999'
dot.style.pointerEvents = 'none'
document.body.appendChild(dot)
const ring = document.getElementById('touch-ring') ?? document.createElement('div')
ring.id = 'touch-ring'
ring.style.position = 'absolute'
ring.style.left = `${touch.clientX}px`
ring.style.top = `${touch.clientY}px`
ring.style.width = '15px'
ring.style.height = '15px'
ring.style.border = 'solid 1px rgb(0,255,0)'
ring.style.zIndex = '9999'
ring.style.pointerEvents = 'none'
ring.style.transform = 'translate(-50%, -50%)'
ring.style.borderRadius = '999px'
document.body.appendChild(ring)
})trim.2C14FB79-9B56-4306-93D7-275D59CEB004.MOV
This issue has to be tested pretty thoroughly since the dead zone is hard to hit. Perhaps we can come up with a way to automate this, but for now it's manual. Let me know when you are both confident it is working as expected and I will be happy to do a final review. Thanks!
|
@raineorshine Is this acceptable for now?
Video: IMG_0280.MOV |
|
Good catch, that’s a bug. |



Close #3374
Cause:
On iOS devices/ safari browser, when we click in the "void" area around text (e.g., padding, empty space after character), the browser's default behavior places the caret to the end of text. See the image below

Initially, this specific issue was believed to be solved in 1550e4f with clip-path. The commit was dated back to 2022, hence I am not sure whether this actually ever fixed the issue in safari/ios devices as it was not mentioned expliclity. The strong reasoning that this is a ios specific behavior comes from test in other note taking apps like notion where the behavior is found to be the same. i.e caret jumping to the end when clicked outside text vicinity. So even though with clip-path we avoid the dead-zone/gap, the issue still persists because of this reason.
Possible solution:
As a fix, I have implemented a solution that detects the space the user clicked and mark it as valid or invalid region depending on where the click occured. The function takes the click coordinates and calculates the caret position from it which is determined by
Rangeobject.getCharRecthelper gets a character's visual bounding box. This creates aRangespanning exactly one character (from targetOffset to targetOffset + 1), then gets its screen rectangle. This tells us the exact pixel coordinates of any character.Example: For text "Hello", getCharRect(0) returns the rect for "H", getCharRect(1) returns rect for "e", etc.
Now
isInsideCharRectchecks if click is inside a character's box. If the click lies anywhere outside character's box, it is marked as invalid and we prevent the default behavior of the device/browser viae.preventDefault(). If it lies within the boundary of character, default selection is allowed and no behavior is prevented.