-
Notifications
You must be signed in to change notification settings - Fork 141
More robust autoscroll and autoscroll prevention #4435
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
Open
fbmcipher
wants to merge
31
commits into
main
Choose a base branch
from
issue-3765
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
15cfdb3
feat(autoscroll): add focusWithoutAutoscroll v2 behind an A/B debug t…
fbmcipher ff391c4
fix(autoscroll): unify v2 focus entry point and fix empty-thought tap
fbmcipher 63f0768
refactor(autoscroll): adopt focusWithoutAutoscroll, remove v1 and deb…
fbmcipher 206885e
fix(useEditMode): restore native browser selection on non-iOS platforms
fbmcipher f86f52a
fix(autoscroll): restore per-keystroke scroll rhythm at the bottom edge
fbmcipher dd52ea2
docs(focusWithoutAutoscroll): explain why iOS rejects async selection…
fbmcipher 5eeb594
docs(focusWithoutAutoscroll): clarify selection-driven autoscroll sup…
fbmcipher 2f7a91b
docs(autoscrollEdges): clarify trigger-edge model and platform notes
fbmcipher 8368c2c
fix(autoscroll): remove top trigger buffer so a top scroll reveals on…
fbmcipher 008ce64
style(focusWithoutAutoscroll): remove trailing whitespace flagged by …
fbmcipher d70856f
fix(autoscroll): compute smooth-scroll threshold from virtualKeyboard…
fbmcipher 7268c77
docs(asyncFocus): align priming comment with focusWithoutAutoscroll t…
fbmcipher 2a94efb
docs(autoscroll): simplify comments and unify trigger-buffer/landing-…
fbmcipher 2916ab1
docs: improve comments
fbmcipher 4e28334
refactor(autoscroll): retire viewport virtualKeyboardHeight; iOSSafar…
fbmcipher 16e7798
docs(focusWithoutAutoscroll): clarify that scrollCursorIntoView solel…
fbmcipher ad3925b
feat(autoscroll): add symmetric top trigger buffer
fbmcipher bff79a4
Merge remote-tracking branch 'origin/main' into issue-3765
fbmcipher 5ff5a01
fix empty note falling back to iOS native autoscroll
fbmcipher aeb9dd5
Use ScreenOrientation API for iOS keyboard prediction cache
Copilot 72ca5db
Fix TypeScript errors in drag-and-drop multiselect test
Copilot 6b1d070
Merge branch 'main' into issue-3765
fbmcipher a3d655c
fix(Note): preserve desktop double-click word selection
fbmcipher 3ebf1cd
Merge remote-tracking branch 'origin/main' into issue-3765
fbmcipher a5eade5
Merge remote-tracking branch 'origin/main' into issue-3765
fbmcipher 563d7a7
fix(autoscroll): skip top-edge correction when nothing is above to re…
fbmcipher 8fda58d
refactor(autoscroll): generalize reachability guard; clarify edge/buf…
fbmcipher 31ca58d
test(autoscroll): regenerate colored-text snapshot to the phantom-fre…
fbmcipher ff406e5
fix(Editable): preserve cursor offset when refocusing the same thought
fbmcipher 66d9b37
Merge remote-tracking branch 'origin/main' into issue-3765
fbmcipher aa0dc01
Merge remote-tracking branch 'origin/main' into issue-3765
fbmcipher File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,13 @@ | ||
| import React, { useCallback, useEffect, useRef } from 'react' | ||
| import React, { useEffect, useRef } from 'react' | ||
| import { useSelector } from 'react-redux' | ||
| import { useStore } from 'react-redux' | ||
| import { useDispatch } from 'react-redux' | ||
| import Path from '../../@types/Path' | ||
| import { setCursorActionCreator as setCursor } from '../../actions/setCursor' | ||
| import { isMac, isSafari, isTouch } from '../../browser' | ||
| import { LongPressState } from '../../constants' | ||
| import asyncFocus from '../../device/asyncFocus' | ||
| import focusWithoutAutoscroll from '../../device/focusWithoutAutoscroll' | ||
| import getCaretOffset from '../../device/getCaretOffset' | ||
| import preventAutoscroll, { preventAutoscrollEnd } from '../../device/preventAutoscroll' | ||
| import * as selection from '../../device/selection' | ||
| import usePrevious from '../../hooks/usePrevious' | ||
| import hasMulticursor from '../../selectors/hasMulticursor' | ||
|
|
@@ -40,7 +39,6 @@ const useEditMode = ({ | |
| const disabledRef = useRef(false) | ||
| const editableNonce = useSelector(state => state.editableNonce) | ||
| const showSidebar = useSelector(state => state.showSidebar) | ||
| const fontSize = useSelector(state => state.fontSize) | ||
| const isCursor = useSelector(state => equalPath(path, state.cursor)) | ||
| const hadSidebar = usePrevious(showSidebar) | ||
| const store = useStore() | ||
|
|
@@ -55,18 +53,7 @@ const useEditMode = ({ | |
| () => { | ||
| // Get the cursorOffset directly from the store rather than subscribing to it reactively with useSelector. | ||
| // Otherwise, it will try to set the selection while typing. | ||
| const { cursorOffset, lastUndoableActionType } = store.getState() | ||
|
|
||
| /** Set the selection to the current Editable at the cursor offset. */ | ||
| const setSelectionToCursorOffset = () => { | ||
| // do not set the selection on hidden thoughts, otherwise it will cause a faulty focus event when switching windows | ||
| // https://github.com/cybersemics/em/issues/1596 | ||
| if (style?.visibility === 'hidden') { | ||
| selection.clear() | ||
| } else { | ||
| selection.set(contentRef.current, { offset: cursorOffset ?? 0 }) | ||
| } | ||
| } | ||
| const { cursorOffset } = store.getState() | ||
|
|
||
| // allow transient editable to have focus on render | ||
| const shouldSetSelection = | ||
|
|
@@ -80,28 +67,19 @@ const useEditMode = ({ | |
| !dragHold && | ||
| !disabledRef.current) | ||
|
|
||
| if (shouldSetSelection) { | ||
| preventAutoscroll(contentRef.current) | ||
|
|
||
| /* | ||
| When a new thought is created, the Shift key should be on when Auto-Capitalization is enabled. | ||
| On Mobile Safari, Auto-Capitalization is broken if the selection is set synchronously (#999). | ||
| Only breaks on Enter or Backspace, not gesture. | ||
|
|
||
| setTimeout fixes it, however it introduces an infinite loop when a nested empty thought is created. | ||
| Not calling asyncFocus when the selection is already on a thought prevents the infinite loop. | ||
| Also, setTimeout is frequently pushed into the next frame and the keyboard will intermittently close on iOS Safari. | ||
| Replacing setTimeout with requestAnimationFrame guarantees (hopefully?) that it will be processed before the next repaint, | ||
| keeping the keyboard open while rapidly deleting thoughts. (#3129) | ||
| if (!shouldSetSelection) return | ||
|
|
||
| If the last action is swapParent, set the selection synchronously to keep the focus stable after the swap. | ||
| */ | ||
| if (isTouch && isSafari() && lastUndoableActionType !== 'swapParent' && !selection.isThought()) { | ||
| asyncFocus() | ||
| } | ||
|
|
||
| setSelectionToCursorOffset() | ||
| // do not set the selection on hidden thoughts, otherwise it will cause a faulty focus event when switching windows | ||
| // https://github.com/cybersemics/em/issues/1596 | ||
| if (style?.visibility === 'hidden') { | ||
| selection.clear() | ||
| return | ||
| } | ||
|
|
||
| // Whenever the cursor changes programmatically, focusWithoutAutoscroll focuses, places the | ||
| // caret, and suppresses the native focus + selection autoscroll that would otherwise jolt | ||
| // `position: fixed` elements on iOS (#3765). | ||
| focusWithoutAutoscroll(contentRef.current, { offset: cursorOffset ?? 0 }) | ||
| }, | ||
| // React Hook useEffect has missing dependencies: 'contentRef', 'editMode', and 'style?.visibility'. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
|
|
@@ -122,28 +100,11 @@ const useEditMode = ({ | |
| ], | ||
| ) | ||
|
|
||
| // Provide an escape hatch to allow the next default selection rather than setting it. | ||
| // This allows the user to set the selection in the middle of a non-cursor thought when keyboard is open. | ||
| // Otherwise the caret is moved to the beginning of the thought. | ||
| const allowDefaultSelection = useCallback(() => { | ||
| disabledRef.current = true | ||
| // enable on next tick, which is long enough to skip the next setSelectionToCursorOffset | ||
| setTimeout(() => { | ||
| disabledRef.current = false | ||
| }) | ||
| }, []) | ||
|
Comment on lines
-125
to
-134
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What changed that this is no longer required? |
||
|
|
||
| // Handles the caret positioning logic for the editable element. | ||
| useEffect(() => { | ||
| const editable = contentRef.current | ||
| if (!editable) return | ||
|
|
||
| /** Sets the DOM selection and updates the Redux cursor state. */ | ||
| const setCaretOffset = (offset: number) => { | ||
| selection.set(editable, { offset }) | ||
| dispatch(setCursor({ path, offset })) | ||
| } | ||
|
|
||
| /** Marks the beginning of a touch so that onMouseDown can determine whether a long press is occurring. */ | ||
| const onTouchStart = () => (pressingRef.current = true) | ||
|
|
||
|
|
@@ -153,8 +114,7 @@ const useEditMode = ({ | |
| /** | ||
| * Handles the mousedown event for the editable element. | ||
| * Prevents focus on non-cursor thoughts or during multiselect clicks. | ||
| * When editing or cursor is present (and multicursor is not active), computes the caret position manually so that it can be set on mouseup. | ||
| * Prevents default behavior and manages autoscroll for certain edge cases where browser selection would be incorrect. | ||
| * When editing or cursor is present (and multicursor is not active), computes the caret position manually and routes through focusWithoutAutoscroll so the iOS native autoscroll never fires. | ||
| */ | ||
| const onMouseDown = (e: MouseEvent) => { | ||
| // If CMD/CTRL is pressed, don't focus the editable. | ||
|
|
@@ -164,38 +124,45 @@ const useEditMode = ({ | |
| return | ||
| } | ||
|
|
||
| // If the press is ongoing (touchend has not been dispatched) then a long press is ongoing and setCaretOffset will interfere | ||
| // with default iOS Safari drag-and-drop text selection. | ||
| // If the press is ongoing (touchend has not been dispatched) then a long press is ongoing and manual caret | ||
| // positioning will interfere with default iOS Safari drag-and-drop text selection. | ||
| if (pressingRef.current) return | ||
|
|
||
| // If editing or the cursor is on the thought, allow the default browser selection or perform manual caret positioning so the offset is correct. | ||
| // If editing or the cursor is on the thought, perform manual caret positioning so the offset is correct. | ||
| // See: #981 | ||
| if (editingOrOnCursor && !isMulticursor) { | ||
| const { inVoidArea, offset } = getCaretOffset(editable, { | ||
| clientX: e.clientX, | ||
| clientY: e.clientY, | ||
| }) | ||
|
|
||
| if (offset !== null) { | ||
| // Prevent the browser from autoscrolling to this editable element. | ||
| // For some reason doesn't work on touchend. | ||
| preventAutoscroll(editable, { | ||
| // about the height of a single-line thought | ||
| bottomMargin: fontSize * 2, | ||
| }) | ||
|
|
||
| // Setting the caret offset will activate the declarative shouldSetSelection effect, which will call preventAutoscroll and selection.set | ||
| // all over again. Since the selection is managed imperatively in this handler, this duplicate behavior is undesirable. | ||
| allowDefaultSelection() | ||
| setCaretOffset(offset) | ||
|
|
||
| // It's important to avoid preventDefault when the tap is somewhere that can be handled by native browser selection behavior. | ||
| // If the tap is prevented, it will interfere with functionality like double tap or the context menu. If the selection is | ||
| // truly in a void area, then preventDefault will stop the caret from being placed on the wrong thought. | ||
| if (inVoidArea) { | ||
| e.preventDefault() | ||
| } | ||
| // Block the native mousedown so it can't focus the element and place the caret itself. | ||
| // There are two reasons we want to do this: | ||
| // 1. iOS Safari: always block; let focusWithoutAutoscroll re-takes focus cleanly. | ||
| // 2. Block when the tap lands in a void area (outside any text node) to stop the caret from being | ||
| // placed on the wrong thought. | ||
| if ((isTouch && isSafari()) || inVoidArea) { | ||
| e.preventDefault() | ||
| } | ||
|
|
||
| // getCaretOffset returns null for empty thoughts (no text nodes). We still need to take | ||
| // focus on those — otherwise tapping an empty thought that is already the cursor leaves | ||
| // it unfocused and the keyboard never opens. | ||
| const targetOffset = offset ?? 0 | ||
|
|
||
| // Dispatch setCursor first so Editable.onFocus's setCursorOnThought sees | ||
| // state.cursor === path and early-returns instead of setting cursorOffset to 0. | ||
| dispatch( | ||
| setCursor({ | ||
| path, | ||
| offset: targetOffset, | ||
| isKeyboardOpen: true, | ||
| cursorHistoryClear: true, | ||
| preserveMulticursor: true, | ||
| }), | ||
| ) | ||
|
|
||
| focusWithoutAutoscroll(editable, { offset: targetOffset }) | ||
| } else { | ||
| // There are areas on the outside edge of the thought that will fail to trigger onTouchEnd. | ||
| // In those cases, it is best to prevent onFocus or onClick, otherwise keyboard is open will be incorrectly activated. | ||
|
|
@@ -205,28 +172,20 @@ const useEditMode = ({ | |
| } | ||
| } | ||
|
|
||
| /** Prevents the thought from autoscrolling to the bottom of the screen when the keyboard is open. | ||
| * Autoscroll must be prevented until focus handling is complete, so preventAutoscrollEnd is deferred | ||
| * using queueMicrotask without introducing any additional delay. | ||
| */ | ||
| const onFocus = () => queueMicrotask(() => preventAutoscrollEnd(editable)) | ||
|
|
||
| editable.addEventListener('mousedown', onMouseDown) | ||
| if (isTouch && isSafari()) { | ||
| editable.addEventListener('touchstart', onTouchStart) | ||
| editable.addEventListener('touchend', onTouchEnd) | ||
| editable.addEventListener('focus', onFocus) | ||
| } | ||
|
|
||
| return () => { | ||
| editable.removeEventListener('mousedown', onMouseDown) | ||
| if (isTouch && isSafari()) { | ||
| editable.removeEventListener('touchstart', onTouchStart) | ||
| editable.removeEventListener('touchend', onTouchEnd) | ||
| editable.removeEventListener('focus', onFocus) | ||
| } | ||
| } | ||
| }, [contentRef, editingOrOnCursor, isCursor, isMulticursor, fontSize, allowDefaultSelection, path, dispatch]) | ||
| }, [contentRef, editingOrOnCursor, isMulticursor, path, dispatch]) | ||
|
|
||
| // Resume focus if sidebar was just closed and isEditing is true. | ||
| // Disable focus restoration on mobile until the hamburger menu & sidebar backdrop can be made to | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 confirm that Auto-Capitalization and swapParent behavior are preserved?