diff --git a/src/actions/formatWithTag.ts b/src/actions/formatWithTag.ts index b5e0004303e..0adb1dcc989 100644 --- a/src/actions/formatWithTag.ts +++ b/src/actions/formatWithTag.ts @@ -4,14 +4,26 @@ import Thunk from '../@types/Thunk' import * as selection from '../device/selection' import pathToThought from '../selectors/pathToThought' import thoughtToPath from '../selectors/thoughtToPath' +import commandStateStore from '../stores/commandStateStore' import suppressFocusStore from '../stores/suppressFocus' import getCommandState from '../util/getCommandState' import strip from '../util/strip' import { editThoughtActionCreator as editThought } from './editThought' +/** Maps a FormattingCommand to the HTML tag used to wrap the formatted value. The FormattingCommand value matches the tag name only for `code`, so the others must be mapped explicitly (e.g. bold → b). */ +const formattingCommandTag: Record = { + [FormattingCommand.bold]: 'b', + [FormattingCommand.italic]: 'i', + [FormattingCommand.underline]: 'u', + [FormattingCommand.strikethrough]: 'strike', + [FormattingCommand.code]: 'code', + [FormattingCommand.foreColor]: 'span', + [FormattingCommand.backColor]: 'span', +} + /** Format the browser selection or cursor thought with the provided tag. */ export const formatWithTagActionCreator = - (tag: FormattingCommand): Thunk => + (command: FormattingCommand): Thunk => (dispatch, getState) => { const state = getState() if (!state.cursor) return @@ -20,15 +32,21 @@ export const formatWithTagActionCreator = const simplePath = thoughtToPath(state, thought.id) suppressFocusStore.update(true) + const tag = formattingCommandTag[command] const tagRegExp = new RegExp(`<${tag}[^>]*>|<\/${tag}>`, 'g') const thoughtSelected = (selection.text()?.length === 0 && strip(thought.value).length !== 0) || selection.text()?.length === strip(thought.value).length + // The command state that the toolbar button reads to render its active highlight. It is derived directly from the + // formatted result (not the DOM selection) because formatWithTag edits the thought value via editThought, which + // re-renders asynchronously; reading window.getSelection() here would return the stale, pre-format DOM. + let commandStateSource: string + if (thoughtSelected) { // format the entire thought - const isAllFormatted = getCommandState(thought.value)[tag] + const isAllFormatted = getCommandState(thought.value)[command] const withoutTag = thought.value.replace(tagRegExp, '') const newValue = isAllFormatted ? withoutTag // remove tag @@ -42,11 +60,12 @@ export const formatWithTagActionCreator = force: true, }), ) + commandStateSource = newValue } else { // format the selection const selectedText = selection.html() if (!selectedText) return - const isAllFormatted = getCommandState(selectedText)[tag] + const isAllFormatted = getCommandState(selectedText)[command] const withoutTag = selectedText.replace(tagRegExp, '') const newPart = isAllFormatted ? withoutTag // remove tag @@ -61,6 +80,14 @@ export const formatWithTagActionCreator = force: true, }), ) + // Reflect the state of the formatted selection (newPart), not the whole thought. + commandStateSource = newPart } + + // Refresh the command state store so the toolbar button reflects the new formatting. Unlike formatSelection (which + // uses document.execCommand and relies on the resulting selectionchange event), formatWithTag edits the thought + // value directly, so no selectionchange fires and the button highlight would otherwise go stale. + commandStateStore.update(getCommandState(commandStateSource)) + suppressFocusStore.update(false) } diff --git a/src/commands/__tests__/bold.ts b/src/commands/__tests__/bold.ts new file mode 100644 index 00000000000..e730559c5d7 --- /dev/null +++ b/src/commands/__tests__/bold.ts @@ -0,0 +1,100 @@ +import { importTextActionCreator as importText } from '../../actions/importText' +import { executeCommand, executeCommandWithMulticursor } from '../../commands' +import { HOME_TOKEN } from '../../constants' +import exportContext from '../../selectors/exportContext' +import store from '../../stores/app' +import commandStateStore from '../../stores/commandStateStore' +import { addMulticursorAtFirstMatchActionCreator as addMulticursor } from '../../test-helpers/addMulticursorAtFirstMatch' +import initStore from '../../test-helpers/initStore' +import { setCursorFirstMatchActionCreator as setCursor } from '../../test-helpers/setCursorFirstMatch' +import boldCommand from '../bold' + +beforeEach(initStore) + +describe('bold', () => { + // Regression test for #3995: applying bold to the cursor thought must update the command state store so the toolbar + // button highlights as active. formatWithTag edits the value directly (no document.execCommand selectionchange), so it + // must refresh the command state explicitly. + it('updates the command state so the toolbar button reflects the active formatting', () => { + store.dispatch([ + importText({ + text: ` + - a + - b + `, + }), + setCursor(['a']), + ]) + + executeCommand(boldCommand, { store }) + + expect(commandStateStore.getState().bold).toBe(true) + + // toggling again removes the formatting and deactivates the button state + executeCommand(boldCommand, { store }) + + expect(commandStateStore.getState().bold).toBe(false) + }) + + describe('multicursor', () => { + // Regression test for #3995: applying a text formatting command to a multiselection (e.g. from the Command Center + // on mobile) must format every selected thought. Formatting previously relied on document.execCommand, which is a + // no-op when no editable is focused (real iOS WebKit, and JSDOM in tests), so no formatting was applied. + it('applies bold formatting to all selected thoughts', () => { + store.dispatch([ + importText({ + text: ` + - a + - b + - c + `, + }), + setCursor(['a']), + addMulticursor(['a']), + addMulticursor(['b']), + ]) + + executeCommandWithMulticursor(boldCommand, { store }) + + const exported = exportContext(store.getState(), [HOME_TOKEN], 'text/plain') + expect(exported).toBe(`- __ROOT__ + - **a** + - **b** + - c`) + + // the toolbar button must reflect the active formatting after a multicursor command (#3995) + expect(commandStateStore.getState().bold).toBe(true) + }) + + // Regression test for #3995: when the cursor is on a thought that is NOT part of the multiselection (e.g. the + // selected thoughts are scrolled out of view and an unrelated thought holds the cursor), executeCommandWithMulticursor + // restores the original cursor after formatting. The cursor-change command state update must not run during multicursor + // execution, otherwise it reads the unselected cursor thought's value and deactivates the toolbar button. + it('keeps the command state active when the cursor is on an unselected thought', () => { + store.dispatch([ + importText({ + text: ` + - a + - b + - c + `, + }), + // cursor on c, which is not part of the multiselection {a, b} + setCursor(['c']), + addMulticursor(['a']), + addMulticursor(['b']), + ]) + + executeCommandWithMulticursor(boldCommand, { store }) + + const exported = exportContext(store.getState(), [HOME_TOKEN], 'text/plain') + expect(exported).toBe(`- __ROOT__ + - **a** + - **b** + - c`) + + // the toolbar button must remain active even though the restored cursor (c) is not bold (#3995) + expect(commandStateStore.getState().bold).toBe(true) + }) + }) +}) diff --git a/src/commands/bold.ts b/src/commands/bold.ts index df584bc1f8b..02bb6fe580f 100644 --- a/src/commands/bold.ts +++ b/src/commands/bold.ts @@ -1,7 +1,9 @@ import Command from '../@types/Command' -import { formatSelectionActionCreator as formatSelection } from '../actions/formatSelection' +import FormattingCommand from '../@types/FormattingCommand' +import { formatWithTagActionCreator as formatWithTag } from '../actions/formatWithTag' import Icon from '../components/icons/BoldTextIcon' import hasMulticursor from '../selectors/hasMulticursor' +import { scrollMulticursorIntoViewOnComplete } from '../stores/scrollMulticursorIntoView' import isDocumentEditable from '../util/isDocumentEditable' /** Toggles formatting of the current browser selection as bold. If there is no selection, formats the entire thought. */ @@ -10,14 +12,14 @@ const bold: Command = { label: 'Bold', description: 'Bolds the current thought or selected text.', descriptionInverse: 'Removes bold from the current thought or selected text.', - multicursor: true, + multicursor: { onComplete: scrollMulticursorIntoViewOnComplete }, svg: Icon, keyboard: { key: 'b', meta: true }, canExecute: state => { return isDocumentEditable() && (!!state.cursor || hasMulticursor(state)) }, exec: dispatch => { - dispatch(formatSelection('bold')) + dispatch(formatWithTag(FormattingCommand.bold)) }, // The isActive logic for formatting commands is handled differently than other commands because it references the CommandStateStore. This can be found in ToolbarButton (isButtonActive) } diff --git a/src/commands/code.ts b/src/commands/code.ts index 80125bcf5c9..2e9a9952627 100644 --- a/src/commands/code.ts +++ b/src/commands/code.ts @@ -3,6 +3,7 @@ import FormattingCommand from '../@types/FormattingCommand' import { formatWithTagActionCreator as formatWithTag } from '../actions/formatWithTag' import Icon from '../components/icons/CodeIcon' import hasMulticursor from '../selectors/hasMulticursor' +import { scrollMulticursorIntoViewOnComplete } from '../stores/scrollMulticursorIntoView' import isDocumentEditable from '../util/isDocumentEditable' /** Toggles formatting of the current browser selection as code. If there is no selection, formats the entire thought. */ @@ -11,7 +12,7 @@ const codeCommand: Command = { label: 'Code', description: 'Formats the current thought or selected text as code.', descriptionInverse: 'Removes code formatting from the current thought or selected text.', - multicursor: true, + multicursor: { onComplete: scrollMulticursorIntoViewOnComplete }, svg: Icon, keyboard: { key: 'k', meta: true }, canExecute: state => { diff --git a/src/commands/italic.ts b/src/commands/italic.ts index 0753d6be5d8..7e2af05252d 100644 --- a/src/commands/italic.ts +++ b/src/commands/italic.ts @@ -1,7 +1,9 @@ import Command from '../@types/Command' -import { formatSelectionActionCreator as formatSelection } from '../actions/formatSelection' +import FormattingCommand from '../@types/FormattingCommand' +import { formatWithTagActionCreator as formatWithTag } from '../actions/formatWithTag' import Icon from '../components/icons/ItalicTextIcon' import hasMulticursor from '../selectors/hasMulticursor' +import { scrollMulticursorIntoViewOnComplete } from '../stores/scrollMulticursorIntoView' import isDocumentEditable from '../util/isDocumentEditable' /** Toggles formatting of the current browser selection as italic. If there is no selection, formats the entire thought. */ @@ -12,12 +14,12 @@ const italic: Command = { descriptionInverse: 'Removes italics from the current thought or selected text. So much for being special.', svg: Icon, keyboard: { key: 'i', meta: true }, - multicursor: true, + multicursor: { onComplete: scrollMulticursorIntoViewOnComplete }, canExecute: state => { return isDocumentEditable() && (!!state.cursor || hasMulticursor(state)) }, exec: dispatch => { - dispatch(formatSelection('italic')) + dispatch(formatWithTag(FormattingCommand.italic)) }, // The isActive logic for formatting commands is handled differently than other commands because it references the CommandStateStore. This can be found in ToolbarButton (isButtonActive) } diff --git a/src/commands/strikethrough.ts b/src/commands/strikethrough.ts index 9c6470e69a9..6efc4caa876 100644 --- a/src/commands/strikethrough.ts +++ b/src/commands/strikethrough.ts @@ -1,7 +1,9 @@ import Command from '../@types/Command' -import { formatSelectionActionCreator as formatSelection } from '../actions/formatSelection' +import FormattingCommand from '../@types/FormattingCommand' +import { formatWithTagActionCreator as formatWithTag } from '../actions/formatWithTag' import Icon from '../components/icons/StrikethroughIcon' import hasMulticursor from '../selectors/hasMulticursor' +import { scrollMulticursorIntoViewOnComplete } from '../stores/scrollMulticursorIntoView' import isDocumentEditable from '../util/isDocumentEditable' /** Toggles formatting of the current browser selection as strikethrough. If there is no selection, formats the entire thought. */ @@ -12,13 +14,13 @@ const strikethrough: Command = { descriptionInverse: 'Removes strikethrough from the current thought or selected text.', svg: Icon, keyboard: { key: 's', meta: true }, - multicursor: true, + multicursor: { onComplete: scrollMulticursorIntoViewOnComplete }, canExecute: state => { return isDocumentEditable() && (!!state.cursor || hasMulticursor(state)) }, exec: (dispatch, getState, e) => { e.preventDefault() - dispatch(formatSelection('strikethrough')) + dispatch(formatWithTag(FormattingCommand.strikethrough)) }, // The isActive logic for formatting commands is handled differently than other commands because it references the CommandStateStore. This can be found in ToolbarButton (isButtonActive) } diff --git a/src/commands/underline.ts b/src/commands/underline.ts index 3aad0aa8a15..ffe849c9f93 100644 --- a/src/commands/underline.ts +++ b/src/commands/underline.ts @@ -1,7 +1,9 @@ import Command from '../@types/Command' -import { formatSelectionActionCreator as formatSelection } from '../actions/formatSelection' +import FormattingCommand from '../@types/FormattingCommand' +import { formatWithTagActionCreator as formatWithTag } from '../actions/formatWithTag' import Icon from '../components/icons/UnderlineIcon' import hasMulticursor from '../selectors/hasMulticursor' +import { scrollMulticursorIntoViewOnComplete } from '../stores/scrollMulticursorIntoView' import isDocumentEditable from '../util/isDocumentEditable' /** Toggles formatting of the current browser selection as underline. If there is no selection, formats the entire thought. */ @@ -14,12 +16,13 @@ const underline: Command = { keyboard: { key: 'u', meta: true }, multicursor: { preventSetCursor: true, + onComplete: scrollMulticursorIntoViewOnComplete, }, canExecute: state => { return isDocumentEditable() && (!!state.cursor || hasMulticursor(state)) }, exec: dispatch => { - dispatch(formatSelection('underline')) + dispatch(formatWithTag(FormattingCommand.underline)) }, // The isActive logic for formatting commands is handled differently than other commands because it references the CommandStateStore. This can be found in ToolbarButton (isButtonActive) } diff --git a/src/components/LayoutTree.tsx b/src/components/LayoutTree.tsx index fdfde9e8f42..4de8be4cd1a 100644 --- a/src/components/LayoutTree.tsx +++ b/src/components/LayoutTree.tsx @@ -7,14 +7,19 @@ import Index from '../@types/IndexType' import ThoughtId from '../@types/ThoughtId' import { isTouch } from '../browser' import { CONTENT_BOX_PADDING_LEFT, LongPressState } from '../constants' +import scrollCursorIntoView from '../device/scrollCursorIntoView' import testFlags from '../e2e/testFlags' import usePositionedThoughts from '../hooks/usePositionedThoughts' import useSizeTracking from '../hooks/useSizeTracking' import fauxCaretTreeProvider from '../recipes/fauxCaretTreeProvider' import { hasChildren } from '../selectors/getChildren' +import isAllSelected from '../selectors/isAllSelected' +import isMulticursorPath from '../selectors/isMulticursorPath' import linearizeTree from '../selectors/linearizeTree' import nextSibling from '../selectors/nextSibling' +import store from '../stores/app' import reactMinistore from '../stores/react-ministore' +import scrollMulticursorIntoViewStore from '../stores/scrollMulticursorIntoView' import scrollTopStore from '../stores/scrollTop' import viewportStore from '../stores/viewport' import head from '../util/head' @@ -247,6 +252,23 @@ const LayoutTree = () => { const cursorThoughtPositionedIndex = treeThoughtsPositioned.findIndex(thought => thought.isCursor) const cursorThoughtPositioned = treeThoughtsPositioned[cursorThoughtPositionedIndex] + // After a multicursor formatting command completes, scroll the topmost selected thought into view if it is offscreen. + // The selected thoughts may be scrolled out of view (e.g. when the command center is onscreen), so snap to the topmost + // selected thought to keep the formatted thoughts visible. scrollCursorIntoView is a no-op when the thought is already + // in view, so this does not snap when the selection is already in focus. See #3995. + scrollMulticursorIntoViewStore.useEffect(() => { + const stateNow = store.getState() + // When all thoughts are selected (Select All), every thought is already "in focus", so snapping to the topmost + // selected thought serves no purpose and instead causes a disruptive autoscroll (up, and on mobile up-and-down as + // the Command Center slides in). Skip the scroll entirely in that case. See #3995 Issue H. + if (isAllSelected(stateNow)) return + // treeThoughtsPositioned is in document order, so the first selected thought is the topmost one. + const topmost = treeThoughtsPositioned.find(thought => isMulticursorPath(stateNow, thought.path)) + if (!topmost) return + // Defer to the next tick so any layout changes from the formatting command have been committed. + setTimeout(() => scrollCursorIntoView(topmost.y, topmost.height)) + }) + // The indentDepth multipicand (0.9) causes the horizontal counter-indentation to fall short of the actual indentation, causing a progressive shifting right as the user navigates deeper. This provides an additional cue for the user's depth, which is helpful when autofocus obscures the actual depth, but it must stay small otherwise the thought width becomes too small. // The indentCursorAncestorTables multipicand (0.5) is smaller, since animating over by the entire width of column 1 is too abrupt. // (The same multiplicand is applied to the vertical translation that crops hidden thoughts above the cursor.) diff --git a/src/device/__tests__/scrollCursorIntoView.ts b/src/device/__tests__/scrollCursorIntoView.ts new file mode 100644 index 00000000000..81a1f9cc0c9 --- /dev/null +++ b/src/device/__tests__/scrollCursorIntoView.ts @@ -0,0 +1,80 @@ +import viewportStore from '../../stores/viewport' +import scrollCursorIntoView from '../scrollCursorIntoView' + +/** Viewport height used for the tests (approximately iPhone 15 Pro). */ +const VIEWPORT_HEIGHT = 852 + +/** Appends an element with the given attributes to the document body and stubs its getBoundingClientRect. */ +const appendElement = (attributes: Record, rect: { top: number; height: number }): HTMLElement => { + const el = document.createElement('div') + Object.entries(attributes).forEach(([key, value]) => el.setAttribute(key, value)) + el.getBoundingClientRect = () => + ({ + top: rect.top, + bottom: rect.top + rect.height, + height: rect.height, + left: 0, + right: 0, + width: 0, + x: 0, + y: rect.top, + toJSON: () => ({}), + }) as DOMRect + document.body.appendChild(el) + return el +} + +let scrollToSpy: ReturnType + +beforeEach(() => { + document.body.innerHTML = '' + + // mock the viewport + Object.defineProperty(window, 'innerHeight', { value: VIEWPORT_HEIGHT, configurable: true }) + Object.defineProperty(window, 'visualViewport', { value: { height: VIEWPORT_HEIGHT }, configurable: true }) + Object.defineProperty(window, 'scrollY', { value: 0, configurable: true, writable: true }) + viewportStore.update({ layoutTreeTop: 0 }) + + scrollToSpy = vi.fn() + window.scrollTo = scrollToSpy as typeof window.scrollTo + + // toolbar at the top, navbar at the bottom + appendElement({ id: 'toolbar' }, { top: 0, height: 50 }) + appendElement({ 'aria-label': 'nav' }, { top: VIEWPORT_HEIGHT - 50, height: 50 }) +}) + +it('does not scroll a thought that is already within the visible viewport', () => { + // a thought in the middle of the screen, with no Command Center open + scrollCursorIntoView(400, 40) + expect(scrollToSpy).not.toHaveBeenCalled() +}) + +it('scrolls a selected thought above the Command Center blur band so its text is readable', () => { + // The Command Center sheet covers the bottom 70% of the screen during a multiselection. + const commandCenterTop = VIEWPORT_HEIGHT * 0.3 + appendElement({ 'data-testid': 'command-menu-panel' }, { top: commandCenterTop, height: VIEWPORT_HEIGHT * 0.7 }) + + // The blur element fades in over a 110px band above the solid panel top; a thought left within that band + // would be blurred and unreadable. The thought should be scrolled fully above the blur band. See #3995 Issue G. + const BLUR_FALLOFF = 110 + scrollCursorIntoView(400, 40) + + expect(scrollToSpy).toHaveBeenCalledTimes(1) + const top = scrollToSpy.mock.calls[0][0].top as number + // after scrolling, the thought's bottom should be above the top of the blur band (panel top - blur falloff) + const thoughtBottomAfterScroll = 400 - top + 40 + expect(thoughtBottomAfterScroll).toBeLessThanOrEqual(commandCenterTop - BLUR_FALLOFF) +}) + +it('does not re-scroll a thought already visible above the Command Center panel (no jump)', () => { + // The Command Center sheet covers the bottom 70% of the screen during a multiselection. + const commandCenterTop = VIEWPORT_HEIGHT * 0.3 + appendElement({ 'data-testid': 'command-menu-panel' }, { top: commandCenterTop, height: VIEWPORT_HEIGHT * 0.7 }) + + // A thought whose bottom sits just above the solid panel top is visible and must not be scrolled, otherwise + // snapping the topmost selected thought into view jumps the page. The blur falloff is only applied to the + // scroll *target*, not to the "needs scrolling" detection, so this thought stays put. See #3995 Issue F. + scrollCursorIntoView(commandCenterTop - 60, 40) + + expect(scrollToSpy).not.toHaveBeenCalled() +}) diff --git a/src/device/scrollCursorIntoView.ts b/src/device/scrollCursorIntoView.ts index fe1dd9c3a87..564c0a5be57 100644 --- a/src/device/scrollCursorIntoView.ts +++ b/src/device/scrollCursorIntoView.ts @@ -2,6 +2,17 @@ import { isSafari, isTouch } from '../browser' import { PREVENT_AUTOSCROLL_TIMEOUT, isPreventAutoscrollInProgress } from '../device/preventAutoscroll' import viewportStore from '../stores/viewport' +/** + * The height (in px) of the Command Center's progressive blur falloff that extends above the solid panel. + * The blur element is taller than the panel by this amount (`blurHeight = sheetHeight + 110`) and fades in + * over this band via a mask gradient, so a thought scrolled to just above the panel top would still be + * blurred. It is added to the *scroll target* obstruction (not the "needs scrolling" detection) so that a + * thought scrolled out from behind the panel lands clear of the blur band, while thoughts already visible + * above the panel are not needlessly re-scrolled. + * Keep in sync with the `110` used in CommandCenter.tsx (blurHeight and the blur mask gradient). + */ +const COMMAND_CENTER_BLUR_FALLOFF = 110 + /** Scrolls the minimum amount necessary to move the viewport so that it includes the element. */ const scrollIntoViewIfNeeded = (y: number, height: number) => { // preventAutoscroll works by briefly increasing the element's height, which breaks isElementInViewport. @@ -32,8 +43,25 @@ const scrollIntoViewIfNeeded = (y: number, height: number) => { const toolbarRect = document.getElementById('toolbar')?.getBoundingClientRect() const toolbarBottom = toolbarRect ? toolbarRect.bottom : 0 const navbarRect = document.querySelector('[aria-label="nav"]')?.getBoundingClientRect() + + // The Command Center sheet covers the bottom portion of the screen during a multiselection. + // Treat the area it covers as a bottom obstruction (alongside the navbar) so that selected thoughts + // are scrolled above it rather than left hidden behind it. Without this, a selected thought in the + // covered area is considered "in view" and never scrolled out from behind the Command Center. See #3995 Issue G. + const commandCenterRect = document.querySelector('[data-testid="command-menu-panel"]')?.getBoundingClientRect() + const commandCenterHeight = commandCenterRect ? Math.max(0, visualViewportHeight - commandCenterRect.top) : 0 + // The obstruction used to decide *whether* a thought needs scrolling is the actual covered area (panel + navbar). + // Using only the panel here (not the blur falloff) keeps thoughts that are already visible above the panel from + // being needlessly re-scrolled, which would cause a jump (e.g. when snapping the topmost selected thought into + // view above the viewport). See #3995 Issue F. + const bottomObstruction = Math.max(navbarRect?.height ?? 0, commandCenterHeight) + // The obstruction used to decide *where* to scroll extends the Command Center by its blur falloff, so a thought + // scrolled out from behind the panel lands above the blur band (its text is clear) rather than within it. See #3995 Issue G. + const commandCenterTargetHeight = commandCenterHeight > 0 ? commandCenterHeight + COMMAND_CENTER_BLUR_FALLOFF : 0 + const bottomObstructionTarget = Math.max(navbarRect?.height ?? 0, commandCenterTargetHeight) + const isAboveViewport = yViewport < toolbarBottom - const isBelowViewport = yViewport + height > visualViewportHeight - (navbarRect?.height ?? 0) + const isBelowViewport = yViewport + height > visualViewportHeight - bottomObstruction if (!isAboveViewport && !isBelowViewport) return @@ -41,10 +69,10 @@ const scrollIntoViewIfNeeded = (y: number, height: number) => { // Therefore, we need to calculate the scroll position ourselves // leave a margin between the element and the viewport edge equal to half the element's height - // add offset to account for the navbar height and prevent scrolled to elements from being hidden below + // add offset to account for the bottom obstruction (navbar or Command Center) and prevent scrolled to elements from being hidden below const scrollYNew = isAboveViewport ? yDocument - (toolbarRect?.height ?? 0) - height / 2 - : yDocument - visualViewportHeight + height * 1.5 + (navbarRect?.height ?? 0) + : yDocument - visualViewportHeight + height * 1.5 + bottomObstructionTarget // scroll to 1 instead of 0 // otherwise Mobile Safari scrolls to the top after MultiGesture diff --git a/src/e2e/puppeteer/__tests__/multiselect.ts b/src/e2e/puppeteer/__tests__/multiselect.ts index 5860b9b2dc6..e52376ec3db 100644 --- a/src/e2e/puppeteer/__tests__/multiselect.ts +++ b/src/e2e/puppeteer/__tests__/multiselect.ts @@ -1,5 +1,7 @@ import { KnownDevices } from 'puppeteer' +import click from '../helpers/click' import emulate from '../helpers/emulate' +import exportThoughts from '../helpers/exportThoughts' import longPressThought from '../helpers/longPressThought' import multiselectThoughts from '../helpers/multiselectThoughts' import paste from '../helpers/paste' @@ -57,4 +59,44 @@ describe('mobile only', () => { expect(highlightedBullets.length).toBe(2) }) + + it('keeps the Command Center open and applies formatting when a text formatting command is run on a multiselection', async () => { + await paste(` + - a + - b + - c + `) + + const a = await waitForEditable('a') + const b = await waitForEditable('b') + + await longPressThought(a, { edge: 'right' }) + await longPressThought(b, { edge: 'right' }) + + // Wait for the Command Center to show the multiselection before tapping a formatting command. + await page.waitForFunction( + () => { + const panel = document.querySelector('[data-testid=command-center-panel]') + return panel?.textContent?.includes('2 thoughts selected') ?? false + }, + { timeout: 6000 }, + ) + + // Tap Bold on the toolbar to format the multiselection. + await click('[data-testid="toolbar-icon"][aria-label="Bold"]') + + // Wait past the alert auto-dismiss delay (Alert.tsx clearDelay defaults to 5000ms). Previously a multicursor + // command on mobile fell through to the desktop alert branch and dispatched the "n thoughts selected" alert, + // which auto-dismissed and cleared the multicursors, closing the Command Center a few seconds later (#3995 Issue B). + await new Promise(resolve => setTimeout(resolve, 6000)) + + // The Command Center should still be open with the multiselection intact. + const panelText = await page.$eval('[data-testid=command-center-panel]', el => el.textContent) + expect(panelText).toContain('2 thoughts selected') + + // The formatting should have been applied to both selected thoughts. + const output = await exportThoughts() + expect(output).toContain('**a**') + expect(output).toContain('**b**') + }) }) diff --git a/src/e2e/puppeteer/__tests__/scroll.ts b/src/e2e/puppeteer/__tests__/scroll.ts index bbceae4c090..afc4a848e75 100644 --- a/src/e2e/puppeteer/__tests__/scroll.ts +++ b/src/e2e/puppeteer/__tests__/scroll.ts @@ -1,5 +1,6 @@ import { WindowEm } from '../../../initialize' import clickThought from '../helpers/clickThought' +import command from '../helpers/command' import getEditingText from '../helpers/getEditingText' import paste from '../helpers/paste' import refresh from '../helpers/refresh' @@ -206,3 +207,58 @@ describe('autocrop', () => { expect(yDiff).toBeLessThan(1) }) }) + +describe('scrollMulticursorIntoView', () => { + it('does not autoscroll when applying formatting to a Select All multiselection', async () => { + // A short viewport guarantees the thoughts overflow so there is room to (incorrectly) scroll. See #3995 Issue H. + await page.setViewport({ width: 800, height: 500 }) + + const importText = ` + - 1 + - 2 + - 3 + - 4 + - 5 + - 6 + - 7 + - 8 + - 9 + - 10 + - 11 + - 12 + - 13 + - 14 + - 15 + - 16 + - 17 + - 18 + - 19 + - 20 + ` + await paste(importText) + await clickThought('20') + await waitForEditable('20') + + // Scroll to the bottom so the topmost selected thought is far above the viewport, maximizing any (incorrect) scroll. + await page.evaluate(() => { + const _em = window.em as WindowEm + _em.testFlags.throttledScrollCursorIntoView?.cancel() + window.scrollTo({ top: document.body.scrollHeight, behavior: 'instant' }) + }) + await new Promise(resolve => setTimeout(resolve, 100)) + + const scrollYBefore = await page.evaluate(() => Math.round(window.scrollY)) + // Sanity check: the page must actually be scrolled, otherwise there would be nothing to assert. + expect(scrollYBefore).toBeGreaterThan(0) + + // Select All, then apply a formatting command. Neither should scroll the page. See #3995 Issue H. + await command('selectAll') + await command('bold') + + // Wait for any (incorrect) scroll animation to settle. + await new Promise(resolve => setTimeout(resolve, 1000)) + + const scrollYAfter = await page.evaluate(() => Math.round(window.scrollY)) + expect(scrollYAfter).toBe(scrollYBefore) + }) +}) diff --git a/src/hooks/useScrollCursorIntoView.ts b/src/hooks/useScrollCursorIntoView.ts index 2783481988c..d9b2b2eb6e4 100644 --- a/src/hooks/useScrollCursorIntoView.ts +++ b/src/hooks/useScrollCursorIntoView.ts @@ -2,6 +2,7 @@ import { throttle } from 'lodash' import { useEffect, useRef } from 'react' import scrollCursorIntoView from '../device/scrollCursorIntoView' import testFlags from '../e2e/testFlags' +import store from '../stores/app' import editingValueStore from '../stores/editingValue' const throttledScrollCursorIntoView = throttle((y: number, height: number) => scrollCursorIntoView(y, height), 400) @@ -9,6 +10,14 @@ const throttledScrollCursorIntoView = throttle((y: number, height: number) => sc // Expose the throttled function so that tests can cancel its pending trailing call before asserting on the scroll position. testFlags.throttledScrollCursorIntoView = throttledScrollCursorIntoView +/** Returns true if a multiselection is active. While thoughts are multiselected, the single cursor is a transient + * artifact of executeCommandWithMulticursor iterating over (and then restoring) the cursor across each selected + * thought. Following it with a cursor autoscroll causes a disruptive up-and-down jump, and on mobile Safari the + * restored cursor can be scrolled behind the virtual keyboard. The single intended scroll is performed by the + * multicursor formatting command's onComplete handler (see LayoutTree), so cursor-follow autoscroll must stand down. + * See #3995 Issue F/H. */ +const isMulticursorActive = () => Object.keys(store.getState().multicursors).length > 0 + /** Call scrollCursorIntoView when the y position of its container changes, or when the editing value changes. */ const useScrollCursorIntoView = (y: number, height: number) => { const sizeRef = useRef({ y, height }) @@ -27,10 +36,20 @@ const useScrollCursorIntoView = (y: number, height: number) => { * Since sizeRef is an object, it is possible to mutate its properties within the existing closure after * React's render cycle runs and processes the effect above. That's why setTimeout is necessary here (#3083). */ - setTimeout(() => throttledScrollCursorIntoView(sizeRef.current.y, sizeRef.current.height)) + setTimeout(() => { + // Do not autoscroll to the transient cursor while a multicursor format rewrites the selected thoughts' values. + // The topmost selected thought is snapped into view once via the command's onComplete handler. See #3995 Issue F/H. + if (isMulticursorActive()) return + throttledScrollCursorIntoView(sizeRef.current.y, sizeRef.current.height) + }) }) - useEffect(() => scrollCursorIntoView(y, height), [height, y]) + useEffect(() => { + // Do not autoscroll to the transient cursor while a multicursor format reflows the selected thoughts' positions. + // The topmost selected thought is snapped into view once via the command's onComplete handler. See #3995 Issue F/H. + if (isMulticursorActive()) return + scrollCursorIntoView(y, height) + }, [height, y]) } export default useScrollCursorIntoView diff --git a/src/redux-middleware/__tests__/multicursorAlertMiddleware.ts b/src/redux-middleware/__tests__/multicursorAlertMiddleware.ts index 64a7ccc5a24..c5b134d30dd 100644 --- a/src/redux-middleware/__tests__/multicursorAlertMiddleware.ts +++ b/src/redux-middleware/__tests__/multicursorAlertMiddleware.ts @@ -2,7 +2,9 @@ import { importTextActionCreator as importText } from '../../actions/importText' import { toggleDropdownActionCreator as toggleDropdown } from '../../actions/toggleDropdown' import { undoActionCreator as undo } from '../../actions/undo' import { executeCommandWithMulticursor } from '../../commands' +import boldCommand from '../../commands/bold' import deleteCommand from '../../commands/delete' +import { AlertType } from '../../constants' import { initialize } from '../../initialize' import store from '../../stores/app' import { addMulticursorAtFirstMatchActionCreator as addMulticursor } from '../../test-helpers/addMulticursorAtFirstMatch' @@ -35,6 +37,68 @@ it('shows the Command Center on mobile when a multicursor is active', async () = expect(store.getState().showCommandCenter).toBe(true) }) +it('keeps the Command Center open while a multicursor formatting command executes', async () => { + await initialize() + + store.dispatch([ + importText({ + text: ` + - A + - B + - C`, + }), + setCursor(['C']), + // open the Command Center by activating a multicursor on the cursor thought (swipe up) + addMulticursor(['C']), + ]) + expect(store.getState().showCommandCenter).toBe(true) + + // Track whether the Command Center is ever closed mid-command. + // executeCommandWithMulticursor transiently clears and restores the multicursors, which previously flickered + // showCommandCenter true → false → true and re-animated the sheet on iOS. + let closedDuringExecution = false + const unsubscribe = store.subscribe(() => { + if (!store.getState().showCommandCenter) closedDuringExecution = true + }) + + // apply a formatting command (Bold) across the multicursor + executeCommandWithMulticursor(boldCommand, { store }) + + unsubscribe() + + expect(closedDuringExecution).toBe(false) + expect(store.getState().showCommandCenter).toBe(true) +}) + +it('does not show the multicursor alert on mobile while a multicursor command executes', async () => { + await initialize() + + store.dispatch([ + importText({ + text: ` + - A + - B + - C`, + }), + setCursor(['C']), + // open the Command Center by activating a multicursor on the cursor thought (swipe up) + addMulticursor(['C']), + ]) + expect(store.getState().showCommandCenter).toBe(true) + + // apply a formatting command (Bold) across the multicursor + executeCommandWithMulticursor(boldCommand, { store }) + + // Flush the throttled alert dispatch in the middleware. + vi.advanceTimersByTime(1000) + + // On mobile the Command Center—not the alert—reflects the multicursor selection. The "n thoughts selected" alert + // must never be shown on touch, otherwise its auto-dismiss (Alert.tsx clearDelay) clears the multicursors and + // closes the Command Center a few seconds after a formatting command (#3995 Issue B). + expect(store.getState().alert?.alertType).not.toBe(AlertType.MulticursorActive) + expect(store.getState().showCommandCenter).toBe(true) +}) + it('does not show the Command Center when undoing a multicursor delete while the Undo Slider is active', async () => { await initialize() diff --git a/src/redux-middleware/multicursorAlertMiddleware.ts b/src/redux-middleware/multicursorAlertMiddleware.ts index f78139ee8b1..d0c313010d0 100644 --- a/src/redux-middleware/multicursorAlertMiddleware.ts +++ b/src/redux-middleware/multicursorAlertMiddleware.ts @@ -26,13 +26,23 @@ const multicursorAlertMiddleware: ThunkMiddleware = ({ getState, dispatch // On mobile, show the Command Center when multicursor is active, and hide it when inactive. if (isTouch) { - if (numMulticursors === 0 && state.showCommandCenter) { - dispatch(toggleDropdown({ dropDownType: 'commandCenter', value: false })) - } else if (numMulticursors > 0 && !state.showCommandCenter && !state.showUndoSlider) { - // Do not open the Command Center while the Undo Slider session is active. - // Otherwise undoing/redoing a multicursor command (e.g. delete from the Command Center) restores the - // multicursor, which would re-open the Command Center and dismiss the Undo Slider being used. - dispatch(toggleDropdown({ dropDownType: 'commandCenter', value: true })) + // Skip while a multicursor command is executing, since executeCommandWithMulticursor transiently clears and + // restores the multicursors (per-thought setCursor), which would otherwise flicker showCommandCenter + // true → false → true. On iOS WebKit this re-animates the Command Center sliding in from the top. The terminal + // setIsMulticursorExecuting({ value: false }) dispatch flows through here with the settled multicursor count and + // reconciles the final state, preserving the close-on-clear behavior for commands like Delete. + // The guard is nested inside isTouch (rather than combined into the condition) so that a mobile multicursor + // command never falls through to the desktop alert branch below, which would show the "n thoughts selected" + // alert; that alert auto-dismisses after a few seconds and clears the multicursors, closing the Command Center. + if (!state.isMulticursorExecuting) { + if (numMulticursors === 0 && state.showCommandCenter) { + dispatch(toggleDropdown({ dropDownType: 'commandCenter', value: false })) + } else if (numMulticursors > 0 && !state.showCommandCenter && !state.showUndoSlider) { + // Do not open the Command Center while the Undo Slider session is active. + // Otherwise undoing/redoing a multicursor command (e.g. delete from the Command Center) restores the + // multicursor, which would re-open the Command Center and dismiss the Undo Slider being used. + dispatch(toggleDropdown({ dropDownType: 'commandCenter', value: true })) + } } } // on desktop, show a persistent alert diff --git a/src/redux-middleware/updateUrlHistory.ts b/src/redux-middleware/updateUrlHistory.ts index de3207c5a4a..a263da07433 100644 --- a/src/redux-middleware/updateUrlHistory.ts +++ b/src/redux-middleware/updateUrlHistory.ts @@ -136,8 +136,11 @@ const updateUrlHistoryMiddleware: ThunkMiddleware = ({ getState }) => { // Update the command state whenever the cursor changes. // Otherwise the command state will not update when the cursor is moved with no selection (mobile only, when the keyboard is down), since updateCommandState is otherwise only called on selection change. + // Skip while a multicursor command is executing: executeCommandWithMulticursor moves the cursor to each selected + // thought in turn and then restores the original cursor, which may be an unselected thought. Updating the command + // state from those transient/restored cursors would clobber the state that the command set for the selection (#3995). const cursor = getState().cursor - if (!equalPath(cursor, cursorPrev)) { + if (!equalPath(cursor, cursorPrev) && !getState().isMulticursorExecuting) { updateCommandState() } cursorPrev = cursor diff --git a/src/stores/commandStateStore.ts b/src/stores/commandStateStore.ts index 8ad2d05be9a..2c435559c42 100644 --- a/src/stores/commandStateStore.ts +++ b/src/stores/commandStateStore.ts @@ -29,12 +29,12 @@ export const resetCommandState = () => { }) } -/** Updates the command state to the current selection/thought. If there is an active selection, this uses document.queryCommandState to get the command state from the DOM. This detects a formatting style that has been enabled, but not yet entered (i.e. the next character typed will be bold). If there is no selection, this parses the cursor thought's value and sets a formatting state only if it applies to the entire thought. */ +/** Updates the command state to the current selection/thought. If there is a non-collapsed selection within a thought, this parses the selected HTML to detect a formatting style that applies to the selection. Otherwise (no selection or a collapsed caret) it parses the cursor thought's value and sets a formatting state only if it applies to the entire thought. A collapsed caret deliberately uses the cursor thought's value (from state) rather than the DOM, since the DOM may be stale immediately after a programmatic edit (e.g. formatWithTag) until React re-renders. The exception is when a note is focused: the note is a separate thought from the cursor thought, so its formatting can only be read from the DOM (selection.html()), not from the cursor thought's value. */ export const updateCommandState = () => { const state = store.getState() if (!state.cursor) return const action = - selection.isActive() && selection.isThought() + selection.isActive() && selection.isThought() && (!selection.isCollapsed() || state.noteFocus) ? getCommandState(selection.html() ?? '') : getCommandState(pathToThought(state, state.cursor)?.value ?? '') commandStateStore.update(action) diff --git a/src/stores/scrollMulticursorIntoView.ts b/src/stores/scrollMulticursorIntoView.ts new file mode 100644 index 00000000000..a8953cfbfef --- /dev/null +++ b/src/stores/scrollMulticursorIntoView.ts @@ -0,0 +1,17 @@ +import reactMinistore from './react-ministore' + +/** + * Pulse store that signals the layout to scroll the topmost selected thought into view. + * + * It is incremented after a multicursor formatting command completes (see the formatting commands' multicursor.onComplete). + * The actual scroll is performed in LayoutTree, which is the only place that has the positioned thoughts (y/height). + * See #3995. + */ +const scrollMulticursorIntoViewStore = reactMinistore(0) + +/** A multicursor onComplete handler that triggers a scroll of the topmost selected thought into view (only if it is offscreen). Used by the formatting commands. */ +export const scrollMulticursorIntoViewOnComplete = () => { + scrollMulticursorIntoViewStore.update(n => n + 1) +} + +export default scrollMulticursorIntoViewStore