Note: Prevent programmatic selection on tap

[ethan-james]

Fixes #2839

Third time's the charm. I added a noteOffset property to state. If it is null, Note will not attempt to programmatically set the selection. If it is a number, it will be used to set the selection. Currently, the only number that is used is value.length which sets the caret at the end, replacing the previous selection.set(noteRef.current!, { end: true }).

[ethan-james]

I guess a self-contained solution would look something like the following:

    const exists = useSelector(state => !!findDescendant(state, thoughtId, '=note'))
    const hasExisted = usePrevious(exists)
    const isNew = exists && !hasExisted

    useEffect(() => {
      if (hasFocus && isNew) {
        selection.set(noteRef.current!, { end: true })
        // deleting a note, then closing the keyboard, then creating a new note could result in lack of focus,
        // perhaps related to iOS Safari's internal management of selection ranges and focus
        if (isTouch && isSafari()) noteRef.current?.focus()
      }
    }, [hasFocus, isNew])

[raineorshine]

Hi, thanks for getting started on this.

I believe we programmatically set the selection on a non-empty note during the note and swapNote commands.

[ethan-james]

I believe we programmatically set the selection on a non-empty note during the note and swapNote commands.

I see this in swapNote.ts:

setCursor({ path: parentNoteChildId ? appendToPath(parentOf(cursor), parentNoteChildId) : parentOf(cursor) })

which does not set noteFocus, and I can confirm that it sets the caret on the thought in main.

[raineorshine]

Yes, I see that now.

The note command sets the selection on the existing note though.

[ethan-james]

The note command sets the selection on the existing note though.

Ah, existing note, not only when a new note is created. Two possible solutions occur to me:

1. Re-create allowDefaultSelection from useEditMode to bypass the state-driven hasFocus effect handler on tap
2. add a flag to setCursor to indicate that the action is generated by a tap rather than programmatic

Option 2 means introducing something into the state that maybe doesn't belong there, but I've been looking into #2837 and I'm pretty sure that the mechanics of allowDefaultSelection are interfering with the double tap. So I'd be interested in hearing your thoughts on which approach would be preferable.

[raineorshine]

Option 1 is pretty ugly, so I would say Option 2 is the one to pursue.

Thanks!

[ethan-james]

Option 1 is pretty ugly, so I would say Option 2 is the one to pursue.

Thanks!

I updated the PR description to outline my new approach.

[raineorshine]

Thanks for the update!

While this is an interesting approach, my overall sense is that the middleware + action extension + ministore is a bit bulky for what we need here. If I'm understanding correctly, we need a back door to block the side effect in the Note component. I would suggest either committing to using the Redux state, or to using a back door/side channel, but not mixing the two.

• Redux state - We could add state.noteOffset: number | null (analogous to state.cursorOffset) and handle it entirely in Redux state.
  • Note the similar semantics: A value of null means that the caret is not forcefully set on re-render, allowing the device to set it, e.g. on click.
  • With the middleware approach, "there's no danger of ephemeral data lingering in the state": sounds beneficial, but I'm not sure there is actual danger is in this case. If noteOffset is only used in the useEffect, then it is effectively inert after the initial render.
• Back door - Alternatively, if the Redux state is insufficient, we can commit to a completely independent back door: either a Ref if it's contained within a component or a ministore if it is used across modules. Mixing this with middleware or extending the Action type is probably adding unnecessary complexity and coupling between different state management systems.

It might be a bit over-engineered, but my hope is that I can immediately use this same ministore for #2837 to cut down on some of the edge cases in Editable selections. I made a cursory attempt to strongly-type the metadata, but more work is needed there. I didn't want to get ahead of myself.

I'm hesitant to build a general system in anticipation of future specifics unless those future specifics are very known/well-defined. I usually prefer to solve the specifics, then generalize afterwards in a refactor if we notice repetition/redundancy.

[raineorshine]

Let's add a descriptive JSDOC comment here. It should explain the unique semantics of null.

(FYI This accidentally picked up the comment for pushQueue.)

[raineorshine]

Let's combine this with the outer if statement.

[raineorshine]

Thank you!