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

selection.set does not always open keyboard #2753

Open
raineorshine opened this issue Jan 4, 2025 · 2 comments
Open

selection.set does not always open keyboard #2753

raineorshine opened this issue Jan 4, 2025 · 2 comments
Labels
bug Something isn't working preassigned I have someone in mind

Comments

@raineorshine
Copy link
Contributor

raineorshine commented Jan 4, 2025

The goal of this issue is to:

  • Determine what is wrong with selection.set and why focus() is needed (if possible).
  • Determine if it is safe to move the focus() call into selection.set.
  • Remove focus() from useEditMode to restore the invariant that all selection behavior occurs via the selection module.

I'm not quite sure why this is necessary, but it was having the following problem on iOS Safari:

  1. Tap a thought - cursor moves, keyboard stays closed

  2. Tap it again - keyboard opens

  3. Close keyboard

  4. Tap it again - keyboard does not open

Originally posted by @ethan-james in #2752 (comment)

Related: #2770 #2772

@raineorshine raineorshine added the bug Something isn't working label Jan 4, 2025
@raineorshine raineorshine added this to the 🏹 Browser Selection milestone Jan 4, 2025
@raineorshine
Copy link
Contributor Author

raineorshine commented Jan 15, 2025

As pointed out in #2772, selection.isText() returns false when the focusNode is set to the editable div. This stems from an annoying native browser behavior where the same selection at the end of an input may be represented in two different ways:

  • focusNode is a TEXT_NODE with focusOffset equal to the number of characters in the text.
  • focusNode is an ELEMENT_NODE with focusOffset equal to 1 (the number of nodes in the parent)

This seems to be throwing off https://github.com/cybersemics/em/blob/650c3d2e38e109ec4e6b3f0c9a2b4e73ece43789/src/device/asyncFocus.ts.

@raineorshine
Copy link
Contributor Author

raineorshine commented Jan 15, 2025

Changing the asyncFocus short circuit condition is possibly part of the solution:

diff --git a/src/device/asyncFocus.ts b/src/device/asyncFocus.ts
index 77873951a5..c49e1acbba 100644
--- a/src/device/asyncFocus.ts
+++ b/src/device/asyncFocus.ts
@@ -32,8 +32,8 @@ export const AsyncFocus: () => () => void = () => {
 
   document.body.prepend(hiddenInput)
   return () => {
-    // do not set the selection if it is already on a text node
-    if (!selection.isText()) {
+    // do not set the selection if it is already on a contenteditable
+    if (!selection.isContentEditable()) {
       hiddenInput.disabled = false
       hiddenInput.focus()
       // the hidden input should not be a valid focus target unless this function was invoked
diff --git a/src/device/selection.ts b/src/device/selection.ts
index af42d834b6..047afe1beb 100644
--- a/src/device/selection.ts
+++ b/src/device/selection.ts
@@ -66,6 +66,12 @@ const isEditable = (node?: Node | null) => {
   )
 }
 
+/** Returns true if the selection is on a contenteditable, either on the TEXT_NODE or ELEMENT_NODE. */
+export const isContentEditable = (node?: Node | null) => {
+  const focusNode = window.getSelection()?.focusNode as HTMLElement | undefined
+  return focusNode?.isContentEditable
+}
+
 /** Returns true if the selection is on a thought. */
 // We should see if it is possible to just use state.editing and selection.isActive()
 export const isThought = (): boolean => {

It's possible that selection.set() has a bug, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working preassigned I have someone in mind
Projects
None yet
Development

No branches or pull requests

1 participant