Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
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
42 changes: 42 additions & 0 deletions src/actions/__tests__/archiveThought.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import State from '../../@types/State'
import archiveThought from '../../actions/archiveThought'
import cursorUp from '../../actions/cursorUp'
import newSubthought from '../../actions/newSubthought'
import newThought from '../../actions/newThought'
import toggleContextView from '../../actions/toggleContextView'
import { HOME_TOKEN } from '../../constants'
import contextToPath from '../../selectors/contextToPath'
import exportContext from '../../selectors/exportContext'
import getContexts from '../../selectors/getContexts'
import expectPathToEqual from '../../test-helpers/expectPathToEqual'
Expand Down Expand Up @@ -168,6 +170,46 @@ describe('normal view', () => {
expect(stateNew.cursor).toBe(null)
})

it('cursor should stay on the last known position when archiving a different thought that has a previous sibling (#4077)', () => {
const steps = [
importText({
text: `
- One
- Two
- Three
- Four
- Five
`,
}),
setCursor(['Five']),
(state: State) => archiveThought({ path: contextToPath(state, ['Two'])! })(state),

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.

archiveThought is curried so you don't need to pass state. Look at the way archiveThought is used in the other tests.

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.

Done — both tests now use the curried form, resolving the path from state first and calling archiveThought({ path })(state) without passing state to archiveThought.

]

const stateNew = reducerFlow(steps)(initialState())

expectPathToEqual(stateNew, stateNew.cursor, ['Five'])
})

it('cursor should stay on the last known position when archiving a different thought that has no previous sibling (#4077)', () => {
const steps = [
importText({
text: `
- One
- Two
- Three
- Four
- Five
`,
}),
setCursor(['Five']),
(state: State) => archiveThought({ path: contextToPath(state, ['One'])! })(state),
]

const stateNew = reducerFlow(steps)(initialState())

expectPathToEqual(stateNew, stateNew.cursor, ['Five'])
})

it('empty thought should be archived if it has descendants', () => {
const steps = [newThought('a'), newThought(''), newSubthought('b'), setCursor(['']), archiveThought({})]

Expand Down
19 changes: 14 additions & 5 deletions src/actions/archiveThought.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { registerActionMetadata } from '../util/actionMetadata.registry'
import appendToPath from '../util/appendToPath'
import equalThoughtValue from '../util/equalThoughtValue'
import head from '../util/head'
import isDescendantPath from '../util/isDescendantPath'
import isDivider from '../util/isDivider'
import isThoughtArchived from '../util/isThoughtArchived'
import parentOf from '../util/parentOf'
Expand Down Expand Up @@ -108,6 +109,12 @@ const archiveThought = (state: State, options: { path?: Path }): State => {
: // Case IV: delete very last thought; remove cursor
[null, undefined]

// Only relocate the cursor when it is on (or within) the thought being archived. When a
// different thought is archived (e.g. dragged to the DropGutter while the cursor is elsewhere),
// preserve the cursor's last known position rather than moving it to a sibling of the archived
// thought. (#4077)
const cursorOnArchived = isDescendantPath(state.cursor, path)

return reducerFlow([
...(isDeletable
? [
Expand Down Expand Up @@ -147,11 +154,13 @@ const archiveThought = (state: State, options: { path?: Path }): State => {
},
]),

setCursor({
path: cursorNew,
isKeyboardOpen: state.isKeyboardOpen,
offset,
}),
cursorOnArchived
? setCursor({
path: cursorNew,
isKeyboardOpen: state.isKeyboardOpen,
offset,
})
: null,
])(state)
}

Expand Down
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
92 changes: 92 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,95 @@ 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')
})

it('should keep the cursor on the last known position when a different thought is archived via DropGutter (#4077)', async () => {
await paste(`
- One
- Two
- Three
- Four
- Five
`)

// Put the cursor on "Five", then archive a different thought ("Two") by dragging it to the DropGutter.
await clickThought('Five')

await dragToDropGutter(await waitForEditable('Two'))

await waitForAlertContent('Removed 1 thought')

// Wait for any asynchronous focus restoration to settle.
await new Promise(resolve => setTimeout(resolve, 1000))

// The cursor should remain on "Five" (its last known position), not move to a sibling of the archived thought.
const cursorValue = await page.evaluate(() => {
const em = window.em as WindowEm
const cursor = em.testHelpers.getState().cursor
if (!cursor) return null
const id = cursor[cursor.length - 1]
return em.getThoughtById(id)?.value ?? null
})

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

it('should keep the cursor on the last known position when the first thought is archived via DropGutter (#4077)', async () => {
await paste(`
- One
- Two
- Three
- Four
- Five
`)

// Put the cursor on "Five", then archive the first thought ("One", which has no previous sibling) via the DropGutter.
await clickThought('Five')

await dragToDropGutter(await waitForEditable('One'))

await waitForAlertContent('Removed 1 thought')

// Wait for any asynchronous focus restoration to settle.
await new Promise(resolve => setTimeout(resolve, 1000))

// The cursor should remain on "Five" (its last known position), not move to the next sibling of the archived thought.
const cursorValue = await page.evaluate(() => {
const em = window.em as WindowEm
const cursor = em.testHelpers.getState().cursor
if (!cursor) return null
const id = cursor[cursor.length - 1]
return em.getThoughtById(id)?.value ?? null
})

expect(cursorValue).toBe('Five')
})
})
Loading