Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/components/Editable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import getSetting from '../selectors/getSetting'
import getThoughtById from '../selectors/getThoughtById'
import hasMulticursorSelector from '../selectors/hasMulticursor'
import rootedParentOf from '../selectors/rootedParentOf'
import thoughtToPath from '../selectors/thoughtToPath'
import batchEditingStore from '../stores/batchEditing'
import editingValueStore from '../stores/editingValue'
import editingValueUntrimmedStore from '../stores/editingValueUntrimmed'
Expand All @@ -53,6 +54,7 @@ import haptics from '../util/haptics'
import head from '../util/head'
import isDivider from '../util/isDivider'
import isDocumentEditable from '../util/isDocumentEditable'
import isThoughtArchived from '../util/isThoughtArchived'
import strip from '../util/strip'
import stripEmptyFormattingTags from '../util/stripEmptyFormattingTags'
import trimHtml from '../util/trimHtml'
Expand Down Expand Up @@ -212,6 +214,13 @@ const Editable = ({
dispatch((dispatch, getState) => {
const state = getState()

// Do not set the cursor on an archived thought. After a focused thought is archived by
// dragging it to the DropGutter, a spurious tap/focus event can fire on its (now hidden)
// Editable, which would otherwise override the cursor that archiveThought placed on the
// previous sibling. The Editable's path may no longer include the =archive ancestor, so the
// thought's real path is resolved to determine whether it is archived. (#4077)
if (isThoughtArchived(state, thoughtToPath(state, head(path)))) return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right to me. If hidden thoughts are shown, then we very well may want to set the cursor on the archived thought.

A better solution would be to prevent the spurious tap/focus event to begin with.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced the archived-path guard. The override actually came from a spurious post-drag focus on the now-hidden Editable, so onFocus now only sets the cursor when isVisible. This prevents the spurious focus from re-cursoring without ever blocking archived thoughts: when hidden thoughts are shown, isVisible is true and the cursor sets normally.


// do not set cursor if it is unchanged and we are not entering when keyboard is open
if ((!isKeyboardOpen || state.isKeyboardOpen) && equalPath(state.cursor, path)) return

Expand Down
30 changes: 30 additions & 0 deletions src/e2e/puppeteer/__tests__/DropGutter.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { KnownDevices } from 'puppeteer'
import { ElementHandle } from 'puppeteer'
import { JSHandle } from 'puppeteer'
import type { WindowEm } from '../../../initialize'
import click from '../helpers/click'
import clickThought from '../helpers/clickThought'
import emulate from '../helpers/emulate'
Expand Down Expand Up @@ -133,4 +134,33 @@ describe('DropGutter: mobile only', () => {
expect(await isThoughtInDOM('b')).toBe(true)
expect(await isThoughtInDOM('c')).toBe(true)
})

it('should move the cursor to the previous sibling when a thought is archived via DropGutter (#4077)', async () => {
await paste(`
- One
- Two
- Three
`)

await clickThought('Two')

await dragToDropGutter(await waitForEditable('Two'))

await waitForAlertContent('Removed 1 thought')

// Wait for the browser's asynchronous focus restoration to settle. This is what could override
// the cursor that archiveThought correctly placed on the previous sibling (#4077).
await new Promise(resolve => setTimeout(resolve, 1000))

// The cursor should be placed on the previous sibling ("One"), not remain on the archived thought.
const cursorValue = await page.evaluate(() => {
const em = window.em as WindowEm
const cursor = em.testHelpers.getState().cursor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getState() is definitely not allowed in puppeteer tests. This is an integration test and should not have direct access to Redux state. It needs to interact with the browser like the user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed getState() from all three DropGutter tests; they now read the cursor value via getEditingText() ([data-editing=true] [data-editable]).

if (!cursor) return null
const id = cursor[cursor.length - 1]
return em.getThoughtById(id)?.value ?? null
})

expect(cursorValue).toBe('One')
})
})