From 4a6f7a38ef49a05423a4d9dc38d0659ee8899e3f Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Mon, 10 Feb 2025 15:57:45 +0100 Subject: [PATCH 01/20] (kb) allow classic keyboard nav when not viewing a document 1) Disable the clipboard catch-all element when there is no need to enable it (we conclude we don't need it when we see no copy/put/paste command is set). This means it is disabled on most pages except the document view. 2) Make sure interactive elements are highlighted when focusing them with the keyboard. Lots of elements have `outline: none` forced in the codebase to prevent showing unwanted outlines when clicking with the mouse. But it prevents keyboard users to see where they are. Have a tiny JS script checking for keyboard interactions to force showing the outline on actual kb focus. --- app/client/components/Clipboard.js | 11 ++++- .../components/KeyboardFocusHighlighter.ts | 41 +++++++++++++++++++ app/client/ui/App.ts | 3 ++ 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 app/client/components/KeyboardFocusHighlighter.ts diff --git a/app/client/components/Clipboard.js b/app/client/components/Clipboard.js index decd3f163a..664b222e61 100644 --- a/app/client/components/Clipboard.js +++ b/app/client/components/Clipboard.js @@ -49,6 +49,8 @@ var dom = require('../lib/dom'); var Base = require('./Base'); var tableUtil = require('../lib/tableUtil'); +var _ = require('underscore'); + const t = makeT('Clipboard'); function Clipboard(app) { @@ -71,7 +73,14 @@ function Clipboard(app) { FocusLayer.create(this, { defaultFocusElem: this.copypasteField, - allowFocus: allowFocus, + allowFocus: (element) => { + // We always allow focus if current screen doesn't have any clipboard events registered. + // This basically means the focus grab is disabled if no clipboard command is active. + const { copy, cut, paste } = commands.allCommands; + return (copy._activeFunc === _.noop && cut._activeFunc === _.noop && paste._activeFunc === _.noop) + ? true + : allowFocus(element); + }, onDefaultFocus: () => { this.copypasteField.value = ' '; this.copypasteField.select(); diff --git a/app/client/components/KeyboardFocusHighlighter.ts b/app/client/components/KeyboardFocusHighlighter.ts new file mode 100644 index 0000000000..9469cd7743 --- /dev/null +++ b/app/client/components/KeyboardFocusHighlighter.ts @@ -0,0 +1,41 @@ +/** + * KeyboardFocusHighlighter helps kb users view what interactive elements they have focus on. + * + * In an ideal world, this would not exist and our css styling would normally highlight + * focused elements. But for now, lots of css rules disable focus rings. + * + * This is done as a quick way to make sure focus rings are correctly visible when using a kb, + * without impacting touch/mouse users, and without having to change the whole codebase. + */ +import { Disposable, dom, styled } from "grainjs"; + +export class KeyboardFocusHighlighter extends Disposable { + constructor() { + super(); + this.autoDispose(dom.onElem(window, 'keydown', this._onKeyDown)); + this.autoDispose(dom.onElem(window, 'touchstart', this._clear)); + this.autoDispose(dom.onElem(window, 'mousedown', this._clear)); + } + + public isKeyboardUser = () => { + return document.documentElement.classList.contains(cssKeyboardUser.className); + }; + + private _onKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Tab') { + document.documentElement.classList.add(cssKeyboardUser.className); + } + }; + + private _clear = () => { + document.documentElement.classList.remove(cssKeyboardUser.className); + }; +} + +const cssKeyboardUser = styled('div', ` + & :is(a, input, textarea, select, button, [tabindex="0"]):focus-visible { + /* simulate default browser focus ring */ + outline: 2px solid Highlight !important; + outline: 2px solid -webkit-focus-ring-color !important; + } +`); diff --git a/app/client/ui/App.ts b/app/client/ui/App.ts index 8fbe1e7ade..996d5c3b5e 100644 --- a/app/client/ui/App.ts +++ b/app/client/ui/App.ts @@ -24,6 +24,7 @@ import {dom} from 'grainjs'; import * as ko from 'knockout'; import {makeT} from 'app/client/lib/localization'; import { onClickOutside } from 'app/client/lib/domUtils'; +import {KeyboardFocusHighlighter} from 'app/client/components/KeyboardFocusHighlighter'; const t = makeT('App'); @@ -75,6 +76,8 @@ export class AppImpl extends DisposableWithEvents implements App { this._settings = ko.observable({}); this.features = ko.computed(() => this._settings().features || {}); + this.autoDispose(new KeyboardFocusHighlighter()); + if (isDesktop()) { this.autoDispose(Clipboard.create(this)); } else { From 1a3715f6571b398173bb359ea9302a7de10dd7e2 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 11 Feb 2025 12:32:15 +0100 Subject: [PATCH 02/20] (kb) introduce "regions" keyboard navigation with RegionFocusSwitcher RegionFocusSwitcher makes it possible for the user to cycle through app panels and document widgets (named "regions" as a whole) with a couple keyboard shortcuts. 1) On a grist document page, pressing the Ctrl+o (or Ctrl+Shift+o) shortcut jumps your keyboard focus from widget to widget, but also panel to panel. You go to widget1, widget 2, then left panel, top panel, then widget 1, widget 2, etc. This shortcut is necessary to reach stuff outside widgets: because by default, usual kb shortcuts like tab or arrow keys are highjacked by widgets and you can't go out ot them. On other pages (that don't have widgets), pressing the shortcut jumps your keyboard focus from panel to panel, but this time it also includes the main content area as a "panel". The shortcut is not _that_ necessary on other pages since you can now use Tab normally. But I guess it's better to keep the shortcut always work whatever the page. 2) The new creatorPanel command allows to keyboard focus from/to the creator panel with Ctrl+Alt+o. This is a separate command from the regions-cycling, as the creator panel is tied to the currently focused viewLayout section. So we must be able to focus a section, and then directly go to the creator panel in order to configure it. If we notice the user just spammed the Ctrl+O shortcut too much, we assume he just tried accessing the creator panel and we show him the keyboard shortcut in a notification. 3) Pressing Esc while on a panel will first focus back the panel, then restore focus to the view layout. 4) The grist doc view now keeps track of this new stuff, in order to disable the view section commands when focusing panels. This is an important trick that lets us enable the Tab key and others when focusing the panels. 5) The new navigation shortcuts are now always on, even in inputs. Using ctrl+o is somewhat problematic because it is the default browser shortcut for opening files, so in cases where we don't catch the keypress, the filechooser appears. We want to avoid that as much as possible. 6) One main trick allowing us to easily allow Tab navigation on panels, while on a grist docs page, is the change in Clipboard.js. We can now set a 'clipboard_group_focus' class somewhere in the dom, and the clipboard will allow focusing elements inside the one having the class. 6) On a grist doc, mouse interactions also drastically impact keyboard navigation: they now silently toggle the view layout commands. If I click on the left panel, it wont actually force a focus change, but it will enable keyboard nav inside the panel. This is not done yet: - needs more actual usage testing to finetune the behavior and make sure it is what we want. - needs tests --- app/client/components/BaseView.js | 2 +- app/client/components/Clipboard.js | 55 +- app/client/components/Cursor.ts | 2 +- app/client/components/CustomView.ts | 2 +- app/client/components/DetailView.js | 4 +- app/client/components/Forms/FormView.ts | 2 +- app/client/components/GridView.js | 2 +- app/client/components/RegionFocusSwitcher.ts | 589 +++++++++++++++++++ app/client/components/ViewLayout.ts | 20 +- app/client/components/WidgetFrame.ts | 2 +- app/client/components/buildViewSectionDom.ts | 4 +- app/client/components/commandList.ts | 24 +- app/client/components/commands.ts | 47 +- app/client/lib/Mousetrap.js | 26 + app/client/lib/SafeBrowser.ts | 4 +- app/client/models/entities/ViewRec.ts | 7 + app/client/models/entities/ViewSectionRec.ts | 11 + app/client/ui/AppUI.ts | 2 + app/client/ui/PagePanels.ts | 41 +- app/client/widgets/DateEditor.ts | 2 +- 20 files changed, 792 insertions(+), 56 deletions(-) create mode 100644 app/client/components/RegionFocusSwitcher.ts diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index a432b65877..1c7263c124 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -128,7 +128,7 @@ function BaseView(gristDoc, viewSectionModel, options) { this.listenTo(this.viewSection.events, 'rowHeightChange', this.onResize ); // Create a command group for keyboard shortcuts common to all views. - this.autoDispose(commands.createGroup(BaseView.commonCommands, this, this.viewSection.hasFocus)); + this.autoDispose(commands.createGroup(BaseView.commonCommands, this, this.viewSection.enableCommands)); //-------------------------------------------------- // Prepare logic for linking with other sections. diff --git a/app/client/components/Clipboard.js b/app/client/components/Clipboard.js index 664b222e61..b3eeceaedf 100644 --- a/app/client/components/Clipboard.js +++ b/app/client/components/Clipboard.js @@ -73,14 +73,7 @@ function Clipboard(app) { FocusLayer.create(this, { defaultFocusElem: this.copypasteField, - allowFocus: (element) => { - // We always allow focus if current screen doesn't have any clipboard events registered. - // This basically means the focus grab is disabled if no clipboard command is active. - const { copy, cut, paste } = commands.allCommands; - return (copy._activeFunc === _.noop && cut._activeFunc === _.noop && paste._activeFunc === _.noop) - ? true - : allowFocus(element); - }, + allowFocus, onDefaultFocus: () => { this.copypasteField.value = ' '; this.copypasteField.select(); @@ -303,18 +296,54 @@ async function getTextFromClipboardItem(clipboardItem, type) { } /** - * Helper to determine if the currently active element deserves to keep its own focus, and capture - * copy-paste events. Besides inputs and textareas, any element can be marked to be a valid - * copy-paste target by adding 'clipboard_focus' class to it. + * Helper to determine if the currently active element deserves to keep its own focus, and capture copy-paste events. + * + * By default, focus is automatically allowed if: + * - there is no clipboard commands registered, + * - the element is an input, textarea, select or iframe, + * - the element has a tabindex attribute + * + * You can explicitly allow focus by setting different classes: + * - using the 'clipboard_allow_focus' class will allow focusing the element having the class, + * - using the 'clipboard_allow_group_focus' class will allow focusing any descendant element of the one having the class + * + * You can explicitly forbid focus by setting the 'clipboard_forbid_focus' class on a element. Forbidding wins over allowing + * if both are set. */ function allowFocus(elem) { - return elem && (FOCUS_TARGET_TAGS.hasOwnProperty(elem.tagName) || + const {copy, cut, paste} = commands.allCommands; + const noCopyPasteCommands = copy._activeFunc === _.noop && cut._activeFunc === _.noop && paste._activeFunc === _.noop; + if (elem && elem.classList.contains('clipboard_forbid_focus')) { + return false; + } + if (noCopyPasteCommands) { + return true; + } + if (elem && elem.closest('.clipboard_group_focus')) { + return true; + } + const allow = elem && ( + FOCUS_TARGET_TAGS.hasOwnProperty(elem.tagName) || elem.hasAttribute("tabindex") || - elem.classList.contains('clipboard_focus')); + elem.classList.contains('clipboard_allow_focus') + ); + return allow; } Clipboard.allowFocus = allowFocus; +/** + * Helper to manually refocus the main app focus grab element. + */ +function triggerFocusGrab() { + const elem = document.querySelector('textarea.copypaste.mousetrap'); + if (elem) { + elem.focus(); + } +} + +Clipboard.triggerFocusGrab = triggerFocusGrab; + function showUnavailableMenuCommandModal(action) { let keys; switch (action) { diff --git a/app/client/components/Cursor.ts b/app/client/components/Cursor.ts index 18b51183ca..e1b15a60f9 100644 --- a/app/client/components/Cursor.ts +++ b/app/client/components/Cursor.ts @@ -112,7 +112,7 @@ export class Cursor extends Disposable { this.fieldIndex = baseView.viewSection.viewFields().makeLiveIndex(optCursorPos.fieldIndex || 0); - this.autoDispose(commands.createGroup(Cursor.editorCommands, this, baseView.viewSection.hasFocus)); + this.autoDispose(commands.createGroup(Cursor.editorCommands, this, baseView.viewSection.enableCommands)); // RowId might diverge from the one stored in _rowId when the data changes (it is filtered out). So here // we will calculate rowId based on rowIndex (so in reverse order), to have a proper value. diff --git a/app/client/components/CustomView.ts b/app/client/components/CustomView.ts index 5ffa58bd00..ec5f4d9afe 100644 --- a/app/client/components/CustomView.ts +++ b/app/client/components/CustomView.ts @@ -129,7 +129,7 @@ export class CustomView extends Disposable { this.autoDispose(this.customDef.pluginId.subscribe(this._updatePluginInstance, this)); this.autoDispose(this.customDef.sectionId.subscribe(this._updateCustomSection, this)); - this.autoDispose(commands.createGroup(CustomView._commands, this, this.viewSection.hasFocus)); + this.autoDispose(commands.createGroup(CustomView._commands, this, this.viewSection.enableCommands)); this._unmappedColumns = this.autoDispose(ko.pureComputed(() => { const columns = this.viewSection.columnsToMap(); diff --git a/app/client/components/DetailView.js b/app/client/components/DetailView.js index 09f0b513dc..4b307a0d20 100644 --- a/app/client/components/DetailView.js +++ b/app/client/components/DetailView.js @@ -127,8 +127,8 @@ function DetailView(gristDoc, viewSectionModel) { //-------------------------------------------------- // Instantiate CommandGroups for the different modes. - this.autoDispose(commands.createGroup(DetailView.generalCommands, this, this.viewSection.hasFocus)); - this.autoDispose(commands.createGroup(DetailView.fieldCommands, this, this.viewSection.hasFocus)); + this.autoDispose(commands.createGroup(DetailView.generalCommands, this, this.viewSection.enableCommands)); + this.autoDispose(commands.createGroup(DetailView.fieldCommands, this, this.viewSection.enableCommands)); const hasSelection = this.autoDispose(ko.pureComputed(() => !this.cellSelector.isCurrentSelectType('') || this.copySelection())); this.autoDispose(commands.createGroup(DetailView.selectionCommands, this, hasSelection)); diff --git a/app/client/components/Forms/FormView.ts b/app/client/components/Forms/FormView.ts index 6f9844c744..3202115134 100644 --- a/app/client/components/Forms/FormView.ts +++ b/app/client/components/Forms/FormView.ts @@ -348,7 +348,7 @@ export class FormView extends Disposable { editField: keyboardActions.edit, deleteFields: keyboardActions.clearValues, hideFields: keyboardActions.hideFields, - }, this, this.viewSection.hasFocus)); + }, this, this.viewSection.enableCommands)); this._previewUrl = Computed.create(this, use => { const doc = use(this.gristDoc.docPageModel.currentDoc); diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index a45326b5dc..54114137d1 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -267,7 +267,7 @@ function GridView(gristDoc, viewSectionModel, isPreview = false) { //-------------------------------------------------- // Command group implementing all grid level commands (except cancel) - this.autoDispose(commands.createGroup(GridView.gridCommands, this, this.viewSection.hasFocus)); + this.autoDispose(commands.createGroup(GridView.gridCommands, this, this.viewSection.enableCommands)); // Cancel command is registered conditionally, only when there is an active // cell selection. This command is also used by Raw Data Views, to close the Grid popup. const hasSelection = this.autoDispose(ko.pureComputed(() => diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts new file mode 100644 index 0000000000..f1cb02e4c2 --- /dev/null +++ b/app/client/components/RegionFocusSwitcher.ts @@ -0,0 +1,589 @@ +import {Disposable, dom, Observable, styled} from 'grainjs'; +import {mod} from 'app/common/gutil'; +import isEqual from 'lodash/isEqual'; +import {makeT} from 'app/client/lib/localization'; +import * as commands from 'app/client/components/commands'; +import {triggerFocusGrab} from 'app/client/components/Clipboard'; +import {GristDoc} from 'app/client/components/GristDoc'; +import {theme} from 'app/client/ui2018/cssVars'; + +const t = makeT('RegionFocusSwitcher'); + +type Panel = 'left' | 'top' | 'right' | 'main'; +interface PanelRegion { + type: 'panel', + id: Panel // this matches a dom element id +} +interface SectionRegion { + type: 'section', + id: any // this matches a grist document view section id +} +type Region = PanelRegion | SectionRegion; + +/** @TODO: remove this when I'm done with the PR */ +const enableLog = true; +let batchLog: any[] = []; +const prepareBatchLog = () => { + batchLog = []; +}; +const bLog = (key: string, value?: any) => { + batchLog.push({key, value}); +}; +const outputBatchLog = (label: string) => { + if (!enableLog) { + return; + } + console.log('rfs', label, batchLog.reduce((acc, {key, value}, i) => { + acc[`${i}. ${key}`] = value; + return acc; + }, {} as Record)); + batchLog = []; +}; +const log = (...args: any[]) => { + if (!enableLog) { + return; + } + console.log('rfs', ...args); +}; + + +/** + * RegionFocusSwitcher enables keyboard navigation between app panels and doc widgets. + * + * It also follow mouse clicks to focus panels accordingly. + */ +export class RegionFocusSwitcher extends Disposable { + // Currently focused region + public readonly current: Observable; + private _currentUpdateInitiator: 'keyboard' | MouseEvent = 'keyboard'; + // Previously focused elements for each panel (not used for view section ids) + private _prevFocusedElements: Record = { + left: null, + top: null, + right: null, + main: null, + }; + + constructor(private readonly _gristDocObs?: Observable) { + super(); + this.current = Observable.create(this, undefined); + } + + public init() { + this.autoDispose(commands.createGroup({ + nextRegion: () => this._cycle('next'), + prevRegion: () => this._cycle('prev'), + creatorPanel: () => this._toggleCreatorPanel(), + cancel: this._onEscapeKeypress.bind(this), + }, this, true)); + + this.autoDispose(this.current.addListener(this._onCurrentUpdate.bind(this))); + + if (this._gristDocObs) { + const onClick = this._onClick.bind(this); + document.addEventListener('mouseup', onClick); + this.onDispose(() => document.removeEventListener('mouseup', onClick)); + this._dirtyClassesFix(); + } + } + + public focusRegion(region: Region | undefined, options: {initiator?: MouseEvent} = {}) { + if (region?.type === 'panel' && !getPanelElement(region.id)) { + console.log('RegionFocusSwitcher: skipping update (panel element not found)'); + return; + } + + const gristDoc = this._getGristDoc(); + if (gristDoc && region?.type === 'panel' && region?.id === 'main') { + throw new Error('main panel is not supported when a view layout is rendered'); + } + if (!gristDoc && region?.type === 'section') { + throw new Error('view section id is not supported when no view layout is rendered'); + } + + this._currentUpdateInitiator = options.initiator || 'keyboard'; + this.current.set(region); + } + + public reset() { + log('reset'); + this.focusRegion(undefined); + if (this._gristDocObs) { + this._dirtyClassesFix(); + } + } + + public panelAttrs(id: Panel, ariaLabel: string) { + return [ + dom.cls('clipboard_group_focus'), + dom.attr('tabindex', '-1'), + dom.attr('role', 'region'), + dom.attr('aria-label', ariaLabel), + dom.attr(ATTRS.regionId, id), + dom.cls(`${cssFocusedPanel.className}-focused`, use => { + const current = use(this.current); + return this._currentUpdateInitiator === 'keyboard' && current?.type === 'panel' && current.id === id; + }), + ]; + } + + private _cycle(direction: 'next' | 'prev') { + const gristDoc = this._getGristDoc(); + const cycleRegions = getCycleRegions(gristDoc); + this.focusRegion(getSibling( + this.current.get(), + cycleRegions, + direction, + gristDoc + )); + if (gristDoc) { + maybeNotifyAboutCreatorPanel(gristDoc, cycleRegions); + } + } + + // @TODO: fix this. This is dirty code to see if what I want to do works. + // My issue is when starting the regionFocusSwitcher, the gristDoc is not yet ready. + // I need to see how to correctly wait for this and be sure there are view layout sections or not. + private _dirtyClassesFix(tries = 0): any { + if (tries > 20) { + return; + } + const main = document.querySelector(`[${ATTRS.regionId}="main"]`); + if (!main) { + return setTimeout(() => this._dirtyClassesFix(tries + 1), 100); + } + const hasGristDoc = !!this._gristDocObs; + const gristDoc = this._getGristDoc(); + if (hasGristDoc && !gristDoc) { + return setTimeout(() => this._dirtyClassesFix(tries + 1), 100); + } + if (hasGristDoc) { + main?.classList.remove('clipboard_group_focus'); + main?.classList.add('clipboard_forbid_focus'); + } else { + main?.classList.remove('clipboard_forbid_focus'); + main?.classList.add('clipboard_group_focus'); + } + log('dirtyClassesFix, main classes:', main?.className); + } + + /** + * When clicking on a grist doc page: + * - if necessary, make it easier to tab through things inside panels by "unfocusing" the view section, + * - make sure the internal current region info is set when user clicks on the view layout. + */ + private _onClick(event: MouseEvent) { + const current = this.current.get(); + const gristDoc = this._getGristDoc(); + if (!gristDoc) { + return; + } + const closestRegion = (event.target as HTMLElement)?.closest(`[${ATTRS.regionId}]`); + if (!closestRegion) { + return; + } + const targetRegionId = closestRegion.getAttribute(ATTRS.regionId); + const targetsMain = targetRegionId === 'main'; + const currentlyInSection = current?.type === 'section'; + + if (targetsMain && !currentlyInSection) { + log('onClick: enable active section'); + this.focusRegion({ type: 'section', id: gristDoc.viewModel.activeSectionId() }, { initiator: event }); + return; + } + + // When not targeting the main panel, we don't always want to focus the given region _on click_. + // We only do it if clicking an empty area in the panel, or a focusable element like an input. + // Otherwise, we assume clicks are on elements like buttons or links, + // and we don't want to lose focus of current section in this case. + // For example I don't want to focus out current table if just click the "undo" button in the header. + if (!targetsMain + && ( + !currentlyInSection + || (isFocusableElement(event.target) || getPanelElement(targetRegionId as Panel) === event.target) + ) + ) { + log('onClick: disable active section'); + this.focusRegion({ type: 'panel', id: targetRegionId as Panel }, { initiator: event }); + return; + } + } + + private _onEscapeKeypress() { + log('Esc: pressed'); + const current = this.current.get(); + if (current?.type !== 'panel') { + log('Esc: not a panel, exiting', current); + return; + } + const panelElement = getPanelElement(current.id); + // Focus back the panel element itself if currently focused element is a child + if ( + (panelElement?.contains(document.activeElement) && document.activeElement !== panelElement) + // Specific case: when we escape inputs from panels, this isn't called and focus switches back to body. + // If user presses escape again, we also want to focus the panel. + || (document.activeElement === document.body && panelElement) + ) { + log('Esc: focus panel', panelElement?.className); + panelElement?.focus(); + return; + } + // …Reset region focus switch if already on the panel itself + if (document.activeElement === panelElement) { + log('Esc: reset'); + this.reset(); + } + } + + /** + * Save previous panel's focused element for later. Not necessary for view sections + */ + private _savePrevElementState(prev: Region | undefined) { + const prevIsPanel = prev?.type === 'panel'; + if (!prevIsPanel) { + return; + } + const prevPanelElement = getPanelElement(prev.id); + const isChildOfPanel = prevPanelElement?.contains(document.activeElement) + && document.activeElement !== prevPanelElement; + if (!isChildOfPanel) { + log('save prevFocusedElement: skip'); + return; + } + log('save prevFocusedElement', prev.id, document.activeElement?.className); + this._prevFocusedElements[prev.id] = document.activeElement; + } + + private _onCurrentUpdate(current: Region | undefined, prev: Region | undefined) { + if (isEqual(current, prev)) { + console.log('RegionFocusSwitcher: skipping update (no change)'); + return; + } + + prepareBatchLog(); + const gristDoc = this._getGristDoc(); + const mouseEvent = this._currentUpdateInitiator instanceof MouseEvent ? this._currentUpdateInitiator : undefined; + + removeFocusRings(); + if (!mouseEvent) { + this._savePrevElementState(prev); + if (prev?.type === 'panel') { + blurPanelChild(prev); + } + } + + const isPanel = current?.type === 'panel'; + const panelElement = isPanel && getPanelElement(current.id); + + // actually focus panel element if using keyboard + if (!mouseEvent && isPanel && panelElement) { + focusPanel(current, this._prevFocusedElements[current.id] as HTMLElement | null, gristDoc); + } + + // just make sure view layout commands are disabled if we click on a panel + if (mouseEvent && isPanel && panelElement && gristDoc) { + escapeViewLayout(gristDoc, !!(mouseEvent.target as Element)?.closest(`[${ATTRS.regionId}="right"]`)); + } + + if (current?.type === 'section' && gristDoc) { + focusSection(current, gristDoc); + } + + if (current === undefined && gristDoc) { + focusViewLayout(gristDoc); + } + + // if we reset the focus switch, clean all necessary state + if (current === undefined) { + bLog('reset, clear prevFocusedElements'); + this._prevFocusedElements = { + left: null, + top: null, + right: null, + main: null, + }; + } + bLog('activeElement', document.activeElement); + outputBatchLog('currentUpdate'); + } + + private _toggleCreatorPanel() { + const current = this.current.get(); + const gristDoc = this._getGristDoc(); + if (current?.type === 'panel' && current.id === 'right') { + return this.focusRegion(gristDoc + ? {type: 'section', id: gristDoc.viewModel.activeSectionId()} + : {type: 'panel', id: 'main'} + ); + } + commands.allCommands.rightPanelOpen.run(); + return this.focusRegion({type: 'panel', id: 'right'}); + } + + /** + * Returns the grist doc only if its has a view layout, meaning it has view sections. + * + * If there is a grist doc but no view sections, it certainly means we are on a grist-doc special page and + * we want to handle kb focus like non-docs pages. + */ + private _getGristDoc() { + const doc = !!this._gristDocObs && !this._gristDocObs.isDisposed() + ? this._gristDocObs.get() + : null; + if (hasViewLayout(doc)) { + return doc; + } + return null; + } +} + + +const ATTRS = { + regionId: 'data-grist-region-id', + focusedElement: 'data-grist-region-focused-el', +}; + +/** + * Focus the given panel (or the given element inside it, if any), and let the grist doc view know about it. + */ +const focusPanel = (panel: PanelRegion, child: HTMLElement | null, gristDoc: GristDoc | null) => { + const panelElement = getPanelElement(panel.id); + if (!panelElement) { + return; + } + // No child to focus found: just focus the panel + if (!child || child === panelElement || !child.isConnected) { + bLog('focusPanel', panelElement.className); + panelElement.focus(); + } + + // Child element found: focus it + if (child && child !== panelElement && child.isConnected) { + // Visually highlight the element with similar styles than panel focus, + // only for this time. This is here just to help the user better see the visual change when he switches panels. + child.setAttribute(ATTRS.focusedElement, 'true'); + child.addEventListener('blur', () => { + child.removeAttribute(ATTRS.focusedElement); + }, {once: true}); + bLog('focusPanel child', child.className); + child.focus?.(); + } + + if (gristDoc) { + // Creator panel is a special case "related to the view" + escapeViewLayout(gristDoc, panel.id === 'right'); + } +}; + +const focusViewLayout = (gristDoc: GristDoc) => { + triggerFocusGrab(); + gristDoc.viewModel.focusedRegionState('in'); + bLog('focusViewLayout focusedRegionState', 'in'); +}; + +// When going out of the view layout, default view state is 'out' to remove active session +// borders and disable the view kb commands. +// You can specific a special case 'related' to the view. It still disable commands, but keeps +// the active session borders, so that user understands what session the current panel is related to. +const escapeViewLayout = (gristDoc: GristDoc, isRelated = false) => { + gristDoc.viewModel.focusedRegionState(isRelated ? 'related' : 'out'); + bLog('escapeViewLayout focusedRegionState', isRelated ? 'related' : 'out'); +}; + +/** + * Focus the given doc view section id + */ +const focusSection = (section: SectionRegion, gristDoc: GristDoc) => { + focusViewLayout(gristDoc); + gristDoc.viewModel.activeSectionId(section.id); + bLog('focusSection activeSectionId', section.id); +}; + +/** + * Get all regions we can currently cycle through. + * + * Depending on whether a view layout is currently rendered, it returns only panels, or panels and sections. + */ +const getCycleRegions = (gristDoc: GristDoc | null): Region[] => { + const commonPanels = [ + getPanelElement('left') ? {type: 'panel', id: 'left'} as PanelRegion : null, + getPanelElement('top') ? {type: 'panel', id: 'top'} as PanelRegion : null, + ].filter((x): x is PanelRegion => Boolean(x)); + + // If there is no doc with layout, just cycle through panels + if (!gristDoc) { + return [ + ...commonPanels, + getPanelElement('main') ? {type: 'panel', id: 'main'} as PanelRegion : null, + ].filter((x): x is PanelRegion => Boolean(x)); + } + + // If there is a doc, also cycle through section ids + return [ + ...gristDoc.viewLayout?.layout.getAllLeafIds().map(id => ({ type: 'section', id } as SectionRegion)) ?? [], + ...commonPanels, + ]; +}; + +/** + * Get the sibling region to focus in the regions given, compared to the current region and the direction. + * + * Exceptions: + * - If we happen to be on the creator panel, focus back to the view layout active section, + * - If we don't find anything, focus the first region in the cycle. + */ +const getSibling = ( + current: Region | undefined, + regions: Region[], + direction: 'next' | 'prev', + gristDoc: GristDoc | null +): Region | undefined => { + const isCreatorPanel = current?.type === 'panel' && current.id === 'right'; + + // First normally try to get current region in the cycle + let currentIndexInCycle = findRegionIndex(regions, current); + + // If it's not found, it certainly means there is no current region set yet. + // In case of a grist doc, we can use the active section id as the "current index" + if ((currentIndexInCycle === -1 || isCreatorPanel) && gristDoc) { + currentIndexInCycle = findRegionIndex(regions, {type: 'section', id: gristDoc.viewModel.activeSectionId()}); + } + // If we still don't find anything, it means we never set the current region before on a non-doc page, + // or we didn't find any current doc section. Return the first region as default. + if (currentIndexInCycle === -1) { + bLog('sibling', regions[0]); + return regions[0]; + } + + // Normal case: just return the next or previous region in the cycle, wrapping around + const sibling = regions[mod(currentIndexInCycle + (direction === 'next' ? 1 : -1), regions.length)]; + bLog('sibling', sibling); + return sibling; +}; + +/** + * Blur the currently focused element in the given panel, if any. + */ +const blurPanelChild = (panel: PanelRegion) => { + const panelElement = getPanelElement(panel.id); + if (panelElement?.contains(document.activeElement) && document.activeElement !== panelElement) { + bLog('isPanel clear focus', { + activeElement: document.activeElement?.className, + panelElement: panelElement.className, + }); + (document.activeElement as HTMLElement).blur(); + } +}; + +const getPanelElement = (id: Panel): HTMLElement | null => { + return document.querySelector(getPanelElementId(id)); +}; + +const getPanelElementId = (id: Panel): string => { + return `[${ATTRS.regionId}="${id}"]`; +}; + +const isFocusableElement = (el: EventTarget | null): boolean => { + if (!el) { + return false; + } + if (el instanceof HTMLElement && ['input', 'textarea', 'select', 'iframe'].includes(el.tagName.toLocaleLowerCase())) { + return true; + } + if (el instanceof HTMLElement && el.getAttribute('tabindex') === "0") { + return true; + } + return false; +}; + +/** + * Remove the visual highlight on elements that are styled as focused elements of panels. + */ +const removeFocusRings = () => { + document.querySelectorAll(`[${ATTRS.focusedElement}]`).forEach(el => { + bLog(`remove ${ATTRS.focusedElement}`, el); + el.removeAttribute(ATTRS.focusedElement); + }); +}; + +const findRegionIndex = (regions: Region[], region: Region | undefined) => { + if (!region) { + return -1; + } + return regions.findIndex(r => isEqual(r, region)); +}; + +/** + * Whether the given grist doc has a view layout. + * + * This can be false if we are on a grist-doc special page, or if the grist doc is not yet ready. + */ +const hasViewLayout = (doc: GristDoc | null) => { + return doc + && !doc.viewModel.isDisposed() + && doc.viewLayout + && !doc.viewLayout.isDisposed() + && doc.viewLayout.layout; +}; + + +const maybeNotifyAboutCreatorPanel = (gristDoc: GristDoc, cycleRegions: Region[]) => { + // @TODO: have a better way to track if we already warned about the creator panel? + // Currently showing the warning every 15 days or until we showed it 3 times. + // Feels a bit convoluted… + const localStoreKey = 'grist-rfs-cp-warn'; + const lastWarning = localStorage.getItem(localStoreKey); + const lastWarningData = lastWarning ? JSON.parse(lastWarning) : { lastTime: 0, count: 0 }; + const toDay = (ms: number) => ms / 1000 / 60 / 60 / 24; + if (lastWarningData.count >= 3 || toDay(Date.now()) - toDay(lastWarningData.lastTime) < 15) { + return; + } + + // We warn the user about creator panel shortcut existing if + // all the commands he pressed in the last 10 seconds are only nextRegion and prevRegion, + // and they did a full cycle through the regions at least once. + const commandsHistory = commands.getCommandsHistory(Date.now() - (1000 * 10)); + const uniqueCommands = [...new Set(commandsHistory)]; + const regionsCount = cycleRegions.length > 10 ? 10 : cycleRegions.length; + + const warn = commandsHistory.length > regionsCount + && uniqueCommands.length <= 2 + && uniqueCommands.every(cmd => cmd === 'nextRegion' || cmd === 'prevRegion'); + + if (!warn) { + return; + } + gristDoc.appModel.notifier.createUserMessage( + t( + 'Trying to access the creator panel? Use {{key}}.', + {key: commands.allCommands.creatorPanel.humanKeys} + ), + { + level: 'info', + key: localStoreKey, + } + ); + // save warning info for next time + localStorage.setItem(localStoreKey, JSON.stringify({ + lastTime: Date.now(), + count: lastWarningData.count + 1, + })); +}; + +const cssFocusedPanel = styled('div', ` + &-focused:focus-within { + outline: 1px solid ${theme.widgetActiveBorder} !important; + outline-offset: -1px !important; + } + + &-focused:focus { + outline: 3px solid ${theme.widgetActiveBorder} !important; + outline-offset: -3px !important; + } + + /* the selector is intentionally heavy to apply more css weight than KeyboardFocusHighlighter styling… + * ideally we would not need KeyboardFocusHighlighter, but for now it's a good enough fallback */ + &-focused [${ATTRS.focusedElement}][${ATTRS.focusedElement}]:focus { + outline-width: 3px !important; + } +`); diff --git a/app/client/components/ViewLayout.ts b/app/client/components/ViewLayout.ts index f37ac57f46..4380398cf6 100644 --- a/app/client/components/ViewLayout.ts +++ b/app/client/components/ViewLayout.ts @@ -27,7 +27,6 @@ import {makeT} from 'app/client/lib/localization'; import {urlState} from 'app/client/models/gristUrlState'; import {cssRadioCheckboxOptions, radioCheckboxOption} from 'app/client/ui2018/checkbox'; import {cssLink} from 'app/client/ui2018/links'; -import {mod} from 'app/common/gutil'; import { Computed, computedArray, @@ -195,8 +194,6 @@ export class ViewLayout extends DisposableWithEvents implements IDomComponent { const commandGroup = { deleteSection: () => { this.removeViewSection(this.viewModel.activeSectionId()).catch(reportError); }, - nextSection: () => { this._otherSection(+1); }, - prevSection: () => { this._otherSection(-1); }, printSection: () => { printViewSection(this.layout, this.viewModel.activeSection()).catch(reportError); }, sortFilterMenuOpen: (sectionId?: number) => { this._openSortFilterMenu(sectionId); }, expandSection: () => { this._expandSection(); }, @@ -206,7 +203,11 @@ export class ViewLayout extends DisposableWithEvents implements IDomComponent { } } }; - this.autoDispose(commands.createGroup(commandGroup, this, true)); + this.autoDispose(commands.createGroup( + commandGroup, + this, + ko.pureComputed(() => this.viewModel.focusedRegionState() === 'in') + )); this.maximized = fromKo(this.layout.maximizedLeaf) as any; this.autoDispose(this.maximized.addListener((sectionId, prev) => { @@ -399,17 +400,6 @@ export class ViewLayout extends DisposableWithEvents implements IDomComponent { }); } - // Select another section in cyclic ordering of sections. Order is counter-clockwise if given a - // positive `delta`, clockwise otherwise. - private _otherSection(delta: number) { - const sectionIds = this.layout.getAllLeafIds(); - const sectionId = this.viewModel.activeSectionId.peek(); - const currentIndex = sectionIds.indexOf(sectionId); - const index = mod(currentIndex + delta, sectionIds.length); - // update the active section id - this.viewModel.activeSectionId(sectionIds[index]); - } - private _maybeFocusInSection() { // If the focused element is inside a view section, make that section active. const layoutBox = this.layout.getContainingBox(document.activeElement); diff --git a/app/client/components/WidgetFrame.ts b/app/client/components/WidgetFrame.ts index 604511f12c..7dd6dbff97 100644 --- a/app/client/components/WidgetFrame.ts +++ b/app/client/components/WidgetFrame.ts @@ -215,7 +215,7 @@ export class WidgetFrame extends DisposableWithEvents { this._iframe = dom( 'iframe', dom.style('visibility', use => use(this._visible) ? 'visible' : 'hidden'), - dom.cls('clipboard_focus'), + dom.cls('clipboard_allow_focus'), dom.cls('custom_view'), dom.attr('src', this._url), // Allow widgets to write to the clipboard via the Clipboard API. diff --git a/app/client/components/buildViewSectionDom.ts b/app/client/components/buildViewSectionDom.ts index 58e151972e..28e51f1a89 100644 --- a/app/client/components/buildViewSectionDom.ts +++ b/app/client/components/buildViewSectionDom.ts @@ -93,9 +93,9 @@ export function buildViewSectionDom(options: { dom.autoDispose(selectedBySectionTitle), !options.isResizing ? dom.autoDispose(isResizing) : null, cssViewLeaf.cls(''), - cssViewLeafInactive.cls('', (use) => !vs.isDisposed() && !use(vs.hasFocus)), + cssViewLeafInactive.cls('', (use) => !vs.isDisposed() && !use(vs.hasVisibleFocus)), dom.cls('active_section', vs.hasFocus), - dom.cls('active_section--no-indicator', !focusable), + dom.cls('active_section--no-indicator', (use) => !focusable || (!vs.isDisposed() && !use(vs.hasVisibleFocus))), dom.maybe((use) => use(vs.viewInstance), (viewInstance) => dom('div.viewsection_title.flexhbox', cssDragIcon('DragDrop', dom.cls("viewsection_drag_indicator"), diff --git a/app/client/components/commandList.ts b/app/client/components/commandList.ts index 3c3a6ce519..d262ac8516 100644 --- a/app/client/components/commandList.ts +++ b/app/client/components/commandList.ts @@ -43,8 +43,9 @@ export type CommandName = | 'openDocumentList' | 'nextPage' | 'prevPage' - | 'nextSection' - | 'prevSection' + | 'nextRegion' + | 'prevRegion' + | 'creatorPanel' | 'shiftDown' | 'shiftUp' | 'shiftRight' @@ -127,6 +128,10 @@ export interface CommandDef { keys: string[]; desc: string | null; bindKeys?: boolean; + /** + * When true, the command is always enabled, even in form inputs. + */ + alwaysOn?: boolean; deprecated?: boolean; } @@ -374,13 +379,20 @@ export const groups: CommendGroupDef[] = [{ keys: ['Alt+Up'], desc: 'Open previous page' }, { - name: 'nextSection', + name: 'nextRegion', keys: ['Mod+o'], - desc: 'Activate next page widget', + desc: 'Focus next page panel or widget', + alwaysOn: true, }, { - name: 'prevSection', + name: 'prevRegion', keys: ['Mod+Shift+O'], - desc: 'Activate previous page widget', + desc: 'Focus previous page panel or widget', + alwaysOn: true, + }, { + name: 'creatorPanel', + keys: ['Mod+Alt+o'], + desc: 'Toggle creator panel keyboard focus', + alwaysOn: true, } ], }, { diff --git a/app/client/components/commands.ts b/app/client/components/commands.ts index 4fa7daaf57..6a18ab3623 100644 --- a/app/client/components/commands.ts +++ b/app/client/components/commands.ts @@ -53,6 +53,32 @@ export const allCommands: { [key in CommandName]: Command } = {} as any; */ const _allKeys: Record = {}; +interface CommandHistoryEntry { + name: string; + timestamp: number; +} +let commandsHistory: CommandHistoryEntry[] = []; +const addToHistory = (commandName: string) => { + commandsHistory = [ + {name: commandName, timestamp: Date.now()}, + ...commandsHistory + ]; + if (commandsHistory.length > 15) { + commandsHistory.pop(); + } +}; +/** + * Get the (maximum 15) command names last keypressed by the user. + * + * Optionally pass a timestamp in milliseconds to filter commands only since that time. + */ +export const getCommandsHistory = (sinceMs?: number) => { + const filtered = sinceMs + ? commandsHistory.filter(entry => entry.timestamp > sinceMs) + : commandsHistory; + return [...filtered.map(entry => entry.name)]; +}; + /** * Populate allCommands from those provided, or listed in commandList.js. Also populates the * globally exposed `cmd` object whose properties invoke commands: e.g. typing `cmd.cursorDown` in @@ -76,6 +102,7 @@ export function init(optCommandGroups?: CommendGroupDef[]) { } else { allCommands[c.name] = new Command(c.name, c.desc, c.keys, { bindKeys: c.bindKeys, + alwaysOn: c.alwaysOn, deprecated: c.deprecated, }); } @@ -124,6 +151,10 @@ export function getHumanKey(key: string, mac: boolean): string { export interface CommandOptions { bindKeys?: boolean; deprecated?: boolean; + /** + * When true, the command is always enabled, even in form inputs. + */ + alwaysOn?: boolean; } /** @@ -140,6 +171,7 @@ export class Command implements CommandDef { public humanKeys: string[]; public keys: string[]; public bindKeys: boolean; + public alwaysOn: boolean; public isActive: ko.Observable; public deprecated: boolean; public run: (...args: any[]) => any; @@ -152,6 +184,7 @@ export class Command implements CommandDef { this.humanKeys = keys.map(key => getHumanKey(key, isMac)); this.keys = keys.map(function(k) { return k.trim().toLowerCase().replace(/ *\+ */g, '+'); }); this.bindKeys = options.bindKeys ?? true; + this.alwaysOn = options.alwaysOn ?? false; this.isActive = ko.observable(false); this._implGroupStack = []; this._activeFunc = _.noop; // The function to run when this command is invoked. @@ -216,14 +249,15 @@ export class Command implements CommandDef { if (this.bindKeys) { // Now bind or unbind the affected key combinations. - this.keys.forEach(function(key) { + this.keys.forEach((key) => { const keyGroups = _allKeys[key]; if (keyGroups && keyGroups.length > 0) { const commandGroup = _.last(keyGroups)!; // Command name might be different from this.name in case we are deactivating a command, and // the previous meaning of the key points to a different command. const commandName = commandGroup.knownKeys[key]; - Mousetrap.bind(key, wrapKeyCallback(commandGroup.commands[commandName])); + const bind = this.alwaysOn ? Mousetrap.bindAlwaysOn : Mousetrap.bind; + bind(key, wrapKeyCallback(commandGroup.commands[commandName], commandName)); } else { Mousetrap.unbind(key); } @@ -237,11 +271,14 @@ export class Command implements CommandDef { } /** - * Helper for mousetrap callbacks, which returns a version of the callback that by default stops - * the propagation of the keyboard event (unless the callback returns a true value). + * Helper for mousetrap callbacks: + * - returns a version of the callback that by default stops the propagation of the keyboard event + * (unless the callback returns a true value). + * - adds the command name in the command history stack. */ -function wrapKeyCallback(callback: Func) { +function wrapKeyCallback(callback: Func, commandName: string) { return function() { + addToHistory(commandName); return callback(...arguments) || false; }; } diff --git a/app/client/lib/Mousetrap.js b/app/client/lib/Mousetrap.js index 74fb471487..8590fadcfd 100644 --- a/app/client/lib/Mousetrap.js +++ b/app/client/lib/Mousetrap.js @@ -28,12 +28,19 @@ if (typeof window === 'undefined') { var MousetrapProtype = Mousetrap.prototype; var origStopCallback = MousetrapProtype.stopCallback; + var alwaysOnCallbacks = {}; + /** * Enhances Mousetrap's stopCallback filter. Normally, mousetrap ignores key events in input * fields and textareas. This replacement allows individual CommandGroups to be activated in such * elements. See also 'attach' method of commands.CommandGroup. */ MousetrapProtype.stopCallback = function(e, element, combo, sequence) { + // If the keyboard shortcut is meant to be always active, we never stop it. + if (alwaysOnCallbacks[combo] || alwaysOnCallbacks[sequence]) { + return false; + } + if (mousetrapBindingsPaused) { return true; } @@ -73,5 +80,24 @@ if (typeof window === 'undefined') { customStopCallbacks.set(element, callback); }; + /** + * Binds an "always-on" keyboard shortcut that triggers even when document.activeElement is an input or textarea. + * + * Alternative to having to handle a custom stop callback for specific elements. + */ + Mousetrap.prototype.bindAlwaysOn = function(keys, callback, action) { + var self = this; + self.bind(keys, callback, action); + if (keys instanceof Array) { + for (var i = 0; i < keys.length; i++) { + alwaysOnCallbacks[keys[i]] = true; + } + return; + } + alwaysOnCallbacks[keys] = true; + }; + + Mousetrap.init(); + module.exports = Mousetrap; } diff --git a/app/client/lib/SafeBrowser.ts b/app/client/lib/SafeBrowser.ts index 0306b92c90..60188590ea 100644 --- a/app/client/lib/SafeBrowser.ts +++ b/app/client/lib/SafeBrowser.ts @@ -302,7 +302,7 @@ class IframeProcess extends ViewProcess { super.create(safeBrowser, rpc, src); this._themeInitialized = Observable.create(this, false); const iframe = this.element = this.autoDispose( - grainjsDom(`iframe.safe_browser_process.clipboard_focus`, + grainjsDom(`iframe.safe_browser_process.clipboard_allow_focus`, {src}, grainjsDom.style('visibility', use => use(this._themeInitialized) ? 'visible' : 'hidden'), ) as HTMLIFrameElement @@ -344,7 +344,7 @@ class IframeProcess extends ViewProcess { class WebviewProcess extends ViewProcess { public create(safeBrowser: SafeBrowser, rpc: Rpc, src: string) { super.create(safeBrowser, rpc, src); - const webview = this.element = this.autoDispose(dom('webview.safe_browser_process.clipboard_focus', { + const webview = this.element = this.autoDispose(dom('webview.safe_browser_process.clipboard_allow_focus', { src, allowpopups: '', // Requests with this partition get an extra header (see main.js) to get access to plugin content. diff --git a/app/client/models/entities/ViewRec.ts b/app/client/models/entities/ViewRec.ts index f85ef8965d..3acdfefb86 100644 --- a/app/client/models/entities/ViewRec.ts +++ b/app/client/models/entities/ViewRec.ts @@ -19,6 +19,12 @@ export interface ViewRec extends IRowModel<"_grist_Views"> { // This is active collapsed section id. Set when the widget is clicked. activeCollapsedSectionId: ko.Observable; + // RegionFocusSwitcher updates this so that the view knows the current state of focused regions: + // - 'out' means the view region is not focused + // - 'in' means the view region is focused + // - 'related' means the currently focused region is not the view, but is something related to it + focusedRegionState: ko.Observable<'out' | 'in' | 'related'>; + // Saved collapsed sections. collapsedSections: ko.Computed; @@ -41,6 +47,7 @@ export function createViewRec(this: ViewRec, docModel: DocModel): void { this.layoutSpecObj = modelUtil.jsonObservable(this.layoutSpec); this.activeCollapsedSectionId = ko.observable(0); + this.focusedRegionState = ko.observable<'out' | 'in' | 'related'>('in'); this.collapsedSections = this.autoDispose(ko.pureComputed(() => { const allSections = new Set(this.viewSections().all().map(x => x.id())); diff --git a/app/client/models/entities/ViewSectionRec.ts b/app/client/models/entities/ViewSectionRec.ts index 8d5fb57f6d..91115f554e 100644 --- a/app/client/models/entities/ViewSectionRec.ts +++ b/app/client/models/entities/ViewSectionRec.ts @@ -163,6 +163,8 @@ export interface ViewSectionRec extends IRowModel<"_grist_Views_section">, RuleO hiddenColumns: ko.Computed; hasFocus: ko.Computed; + hasVisibleFocus: ko.Computed; + enableCommands: ko.Computed; // Section-linking affects table if linkSrcSection is set. The controller value of the // link is the value of srcCol at activeRowId of linkSrcSection, or activeRowId itself when @@ -723,6 +725,15 @@ export function createViewSectionRec(this: ViewSectionRec, docModel: DocModel): read: () => !this.isDisposed() && this.view().activeSectionId() === this.id(), write: (val) => { this.view().activeSectionId(val ? this.id() : 0); } }); + this.hasVisibleFocus = ko.pureComputed(() => { + if (this.isDisposed()) { + return false; + } + const region = this.view().focusedRegionState(); + return this.hasFocus() && (region === 'in' || region === 'related'); + }); + // we enable keyboard shortcuts only when the region is focused (see RegionFocusSwitcher) + this.enableCommands = ko.pureComputed(() => this.hasFocus() && this.view().focusedRegionState() === 'in'); // Section-linking affects this table if linkSrcSection is set. The controller value of the // link is the value of srcCol at activeRowId of linkSrcSection, or activeRowId itself when diff --git a/app/client/ui/AppUI.ts b/app/client/ui/AppUI.ts index 0ed6272f09..e358aad337 100644 --- a/app/client/ui/AppUI.ts +++ b/app/client/ui/AppUI.ts @@ -184,5 +184,7 @@ function pagePanelsDoc(owner: IDisposableOwner, appModel: AppModel, appObj: App) contentTop: buildDocumentBanners(pageModel), contentBottom: dom.create(createBottomBarDoc, pageModel, leftPanelOpen, rightPanelOpen), banner: dom.create(ViewAsBanner, pageModel), + }, { + gristDoc: pageModel.gristDoc, }); } diff --git a/app/client/ui/PagePanels.ts b/app/client/ui/PagePanels.ts index 5581c4c8e9..cca802b633 100644 --- a/app/client/ui/PagePanels.ts +++ b/app/client/ui/PagePanels.ts @@ -15,6 +15,8 @@ import noop from 'lodash/noop'; import once from 'lodash/once'; import {SessionObs} from 'app/client/lib/sessionObs'; import debounce from 'lodash/debounce'; +import {RegionFocusSwitcher} from 'app/client/components/RegionFocusSwitcher'; +import {GristDoc} from 'app/client/components/GristDoc'; const t = makeT('PagePanels'); @@ -47,7 +49,17 @@ export interface PageContents { contentBottom?: DomElementArg; } -export function pagePanels(page: PageContents) { +interface PagePanelsOptions { + /** + * If provided, the region kb focus switcher will also cycle through the given doc's widgets. + */ + gristDoc?: Observable; +} + +export function pagePanels( + page: PageContents, + options: PagePanelsOptions = { } +) { const testId = page.testId || noTestId; const left = page.leftPanel; const right = page.rightPanel; @@ -65,6 +77,9 @@ export function pagePanels(page: PageContents) { let contentTopDom: HTMLElement; let onLeftTransitionFinish = noop; + const regionFocusSwitcher = RegionFocusSwitcher.create(null, options.gristDoc); + regionFocusSwitcher.init(); + // When switching to mobile mode, close panels; when switching to desktop, restore the // last desktop state. const sub1 = subscribe(isNarrowScreenObs(), (use, narrow) => { @@ -79,12 +94,15 @@ export function pagePanels(page: PageContents) { leftOverlap.set(false); }); - // When url changes, we must have navigated; close the left panel since if it were open, it was - // the likely cause of the navigation (e.g. switch to another page or workspace). + // When url changes, we must have navigated; + // - close the left panel since if it were open, it was the likely cause of the navigation + // (e.g. switch to another page or workspace). + // - reset the focus switcher to behave like a normal browser navigation (lose focus). const sub2 = subscribe(isNarrowScreenObs(), urlState().state, (use, narrow, state) => { if (narrow) { left.panelOpen.set(false); } + regionFocusSwitcher.reset(); }); const pauseSavingLeft = (yesNo: boolean) => { @@ -133,6 +151,7 @@ export function pagePanels(page: PageContents) { cssContentMain( leftPaneDom = cssLeftPane( testId('left-panel'), + regionFocusSwitcher.panelAttrs('left', t('Main navigation and document settings (left panel)')), cssOverflowContainer( contentWrapper = cssLeftPanelContainer( cssLeftPaneHeader( @@ -269,6 +288,7 @@ export function pagePanels(page: PageContents) { cssMainPane( mainHeaderDom = cssTopHeader( testId('top-header'), + regionFocusSwitcher.panelAttrs('top', t('Document header')), (left.hideOpener ? null : cssPanelOpener('PanelRight', cssPanelOpener.cls('-open', left.panelOpen), testId('left-opener'), @@ -289,7 +309,12 @@ export function pagePanels(page: PageContents) { ), dom.style('margin-bottom', use => use(bannerHeight) + 'px'), ), - page.contentMain, + + cssContentMainPane( + regionFocusSwitcher.panelAttrs('main', t('Main content')), + page.contentMain, + ), + cssMainPane.cls('-left-overlap', leftOverlap), testId('main-pane'), ), @@ -303,6 +328,7 @@ export function pagePanels(page: PageContents) { rightPaneDom = cssRightPane( testId('right-panel'), + regionFocusSwitcher.panelAttrs('right', t('Creator panel (right panel)')), cssRightPaneHeader( right.header, dom.style('margin-bottom', use => use(bannerHeight) + 'px') @@ -399,6 +425,13 @@ const cssContentMain = styled(cssHBox, ` overflow: hidden; position: relative; `); + +// div wrapping the contentMain passed to pagePanels +const cssContentMainPane = styled(cssVBox, ` + flex-grow: 1; + overflow: auto; +`); + export const cssLeftPane = styled(cssVBox, ` position: relative; background-color: ${theme.leftPanelBg}; diff --git a/app/client/widgets/DateEditor.ts b/app/client/widgets/DateEditor.ts index 7da1710808..8ebe179ef9 100644 --- a/app/client/widgets/DateEditor.ts +++ b/app/client/widgets/DateEditor.ts @@ -136,7 +136,7 @@ export class DateEditor extends NTextEditor { if (datepickerElem) { dom.update(datepickerElem, dom.attr('tabIndex', '0'), // allows datepicker to gain focus - dom.cls('clipboard_focus') // tells clipboard to not steal focus from us + dom.cls('clipboard_allow_focus') // tells clipboard to not steal focus from us ); } From d3bd7ffdd79d9f97a285c03c3a5a20ecde3ce4d9 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 25 Mar 2025 17:27:21 +0100 Subject: [PATCH 03/20] (kb) cleaner way to toggle panel clipboard group/ignore classes we now check if the passed doc observer is actually ready before doing anything --- app/client/components/RegionFocusSwitcher.ts | 103 +++++++++++-------- 1 file changed, 59 insertions(+), 44 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index f1cb02e4c2..9714f0f2a4 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -1,5 +1,6 @@ import {Disposable, dom, Observable, styled} from 'grainjs'; import {mod} from 'app/common/gutil'; +import {SpecialDocPage} from 'app/common/gristUrls'; import isEqual from 'lodash/isEqual'; import {makeT} from 'app/client/lib/localization'; import * as commands from 'app/client/components/commands'; @@ -55,6 +56,7 @@ const log = (...args: any[]) => { export class RegionFocusSwitcher extends Disposable { // Currently focused region public readonly current: Observable; + private readonly _gristDocObs?: Observable; private _currentUpdateInitiator: 'keyboard' | MouseEvent = 'keyboard'; // Previously focused elements for each panel (not used for view section ids) private _prevFocusedElements: Record = { @@ -64,12 +66,25 @@ export class RegionFocusSwitcher extends Disposable { main: null, }; - constructor(private readonly _gristDocObs?: Observable) { + constructor(gristDocObs?: Observable) { super(); this.current = Observable.create(this, undefined); + if (gristDocObs) { + this._gristDocObs = gristDocObs; + } } public init() { + // if we have a grist doc, wait for the current grist doc to be ready before doing anything + if (this._gristDocObs && this._gristDocObs.get() === null) { + this._gristDocObs.addListener((doc, prevDoc) => { + if (doc && prevDoc === null) { + this.init(); + } + }); + return; + } + this.autoDispose(commands.createGroup({ nextRegion: () => this._cycle('next'), prevRegion: () => this._cycle('prev'), @@ -83,7 +98,6 @@ export class RegionFocusSwitcher extends Disposable { const onClick = this._onClick.bind(this); document.addEventListener('mouseup', onClick); this.onDispose(() => document.removeEventListener('mouseup', onClick)); - this._dirtyClassesFix(); } } @@ -108,18 +122,47 @@ export class RegionFocusSwitcher extends Disposable { public reset() { log('reset'); this.focusRegion(undefined); - if (this._gristDocObs) { - this._dirtyClassesFix(); - } } public panelAttrs(id: Panel, ariaLabel: string) { return [ - dom.cls('clipboard_group_focus'), dom.attr('tabindex', '-1'), dom.attr('role', 'region'), dom.attr('aria-label', ariaLabel), dom.attr(ATTRS.regionId, id), + dom.cls('clipboard_group_focus', use => { + // top/left/right panels are always focusable + if (id !== 'main') { + return true; + } + const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; + // if we are not on a grist doc, main panel is always focusable + if (!gristDoc) { + return true; + } + if (gristDoc) { + use(gristDoc.activeViewId); + } + // if we are on a grist doc, main panel is focusable only if we are not the actual document view + return isSpecialPage(gristDoc); + }), + dom.cls('clipboard_forbid_focus', use => { + // forbid focus only on the main panel when on an actual document view (and not a special page) + if (id !== 'main') { + return false; + } + const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; + if (!gristDoc) { + return false; + } + if (gristDoc) { + use(gristDoc.activeViewId); + } + if (isSpecialPage(gristDoc)) { + return false; + } + return id === 'main'; + }), dom.cls(`${cssFocusedPanel.className}-focused`, use => { const current = use(this.current); return this._currentUpdateInitiator === 'keyboard' && current?.type === 'panel' && current.id === id; @@ -141,32 +184,6 @@ export class RegionFocusSwitcher extends Disposable { } } - // @TODO: fix this. This is dirty code to see if what I want to do works. - // My issue is when starting the regionFocusSwitcher, the gristDoc is not yet ready. - // I need to see how to correctly wait for this and be sure there are view layout sections or not. - private _dirtyClassesFix(tries = 0): any { - if (tries > 20) { - return; - } - const main = document.querySelector(`[${ATTRS.regionId}="main"]`); - if (!main) { - return setTimeout(() => this._dirtyClassesFix(tries + 1), 100); - } - const hasGristDoc = !!this._gristDocObs; - const gristDoc = this._getGristDoc(); - if (hasGristDoc && !gristDoc) { - return setTimeout(() => this._dirtyClassesFix(tries + 1), 100); - } - if (hasGristDoc) { - main?.classList.remove('clipboard_group_focus'); - main?.classList.add('clipboard_forbid_focus'); - } else { - main?.classList.remove('clipboard_forbid_focus'); - main?.classList.add('clipboard_group_focus'); - } - log('dirtyClassesFix, main classes:', main?.className); - } - /** * When clicking on a grist doc page: * - if necessary, make it easier to tab through things inside panels by "unfocusing" the view section, @@ -330,7 +347,7 @@ export class RegionFocusSwitcher extends Disposable { const doc = !!this._gristDocObs && !this._gristDocObs.isDisposed() ? this._gristDocObs.get() : null; - if (hasViewLayout(doc)) { + if (!isSpecialPage(doc)) { return doc; } return null; @@ -513,17 +530,15 @@ const findRegionIndex = (regions: Region[], region: Region | undefined) => { return regions.findIndex(r => isEqual(r, region)); }; -/** - * Whether the given grist doc has a view layout. - * - * This can be false if we are on a grist-doc special page, or if the grist doc is not yet ready. - */ -const hasViewLayout = (doc: GristDoc | null) => { - return doc - && !doc.viewModel.isDisposed() - && doc.viewLayout - && !doc.viewLayout.isDisposed() - && doc.viewLayout.layout; +const isSpecialPage = (doc: GristDoc | null) => { + if (!doc) { + return false; + } + const activeViewId = doc.activeViewId.get(); + if (typeof activeViewId === 'string' && SpecialDocPage.guard(activeViewId)) { + return true; + } + return false; }; From b49db5db39bd2fb6bd1a31f7a7e8c3209cffa266 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 25 Mar 2025 17:27:32 +0100 Subject: [PATCH 04/20] (kb) remove logs --- app/client/components/RegionFocusSwitcher.ts | 54 -------------------- 1 file changed, 54 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 9714f0f2a4..eb2d7c600e 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -21,33 +21,6 @@ interface SectionRegion { } type Region = PanelRegion | SectionRegion; -/** @TODO: remove this when I'm done with the PR */ -const enableLog = true; -let batchLog: any[] = []; -const prepareBatchLog = () => { - batchLog = []; -}; -const bLog = (key: string, value?: any) => { - batchLog.push({key, value}); -}; -const outputBatchLog = (label: string) => { - if (!enableLog) { - return; - } - console.log('rfs', label, batchLog.reduce((acc, {key, value}, i) => { - acc[`${i}. ${key}`] = value; - return acc; - }, {} as Record)); - batchLog = []; -}; -const log = (...args: any[]) => { - if (!enableLog) { - return; - } - console.log('rfs', ...args); -}; - - /** * RegionFocusSwitcher enables keyboard navigation between app panels and doc widgets. * @@ -103,7 +76,6 @@ export class RegionFocusSwitcher extends Disposable { public focusRegion(region: Region | undefined, options: {initiator?: MouseEvent} = {}) { if (region?.type === 'panel' && !getPanelElement(region.id)) { - console.log('RegionFocusSwitcher: skipping update (panel element not found)'); return; } @@ -120,7 +92,6 @@ export class RegionFocusSwitcher extends Disposable { } public reset() { - log('reset'); this.focusRegion(undefined); } @@ -204,7 +175,6 @@ export class RegionFocusSwitcher extends Disposable { const currentlyInSection = current?.type === 'section'; if (targetsMain && !currentlyInSection) { - log('onClick: enable active section'); this.focusRegion({ type: 'section', id: gristDoc.viewModel.activeSectionId() }, { initiator: event }); return; } @@ -220,17 +190,14 @@ export class RegionFocusSwitcher extends Disposable { || (isFocusableElement(event.target) || getPanelElement(targetRegionId as Panel) === event.target) ) ) { - log('onClick: disable active section'); this.focusRegion({ type: 'panel', id: targetRegionId as Panel }, { initiator: event }); return; } } private _onEscapeKeypress() { - log('Esc: pressed'); const current = this.current.get(); if (current?.type !== 'panel') { - log('Esc: not a panel, exiting', current); return; } const panelElement = getPanelElement(current.id); @@ -241,13 +208,11 @@ export class RegionFocusSwitcher extends Disposable { // If user presses escape again, we also want to focus the panel. || (document.activeElement === document.body && panelElement) ) { - log('Esc: focus panel', panelElement?.className); panelElement?.focus(); return; } // …Reset region focus switch if already on the panel itself if (document.activeElement === panelElement) { - log('Esc: reset'); this.reset(); } } @@ -264,20 +229,16 @@ export class RegionFocusSwitcher extends Disposable { const isChildOfPanel = prevPanelElement?.contains(document.activeElement) && document.activeElement !== prevPanelElement; if (!isChildOfPanel) { - log('save prevFocusedElement: skip'); return; } - log('save prevFocusedElement', prev.id, document.activeElement?.className); this._prevFocusedElements[prev.id] = document.activeElement; } private _onCurrentUpdate(current: Region | undefined, prev: Region | undefined) { if (isEqual(current, prev)) { - console.log('RegionFocusSwitcher: skipping update (no change)'); return; } - prepareBatchLog(); const gristDoc = this._getGristDoc(); const mouseEvent = this._currentUpdateInitiator instanceof MouseEvent ? this._currentUpdateInitiator : undefined; @@ -312,7 +273,6 @@ export class RegionFocusSwitcher extends Disposable { // if we reset the focus switch, clean all necessary state if (current === undefined) { - bLog('reset, clear prevFocusedElements'); this._prevFocusedElements = { left: null, top: null, @@ -320,8 +280,6 @@ export class RegionFocusSwitcher extends Disposable { main: null, }; } - bLog('activeElement', document.activeElement); - outputBatchLog('currentUpdate'); } private _toggleCreatorPanel() { @@ -370,7 +328,6 @@ const focusPanel = (panel: PanelRegion, child: HTMLElement | null, gristDoc: Gri } // No child to focus found: just focus the panel if (!child || child === panelElement || !child.isConnected) { - bLog('focusPanel', panelElement.className); panelElement.focus(); } @@ -382,7 +339,6 @@ const focusPanel = (panel: PanelRegion, child: HTMLElement | null, gristDoc: Gri child.addEventListener('blur', () => { child.removeAttribute(ATTRS.focusedElement); }, {once: true}); - bLog('focusPanel child', child.className); child.focus?.(); } @@ -395,7 +351,6 @@ const focusPanel = (panel: PanelRegion, child: HTMLElement | null, gristDoc: Gri const focusViewLayout = (gristDoc: GristDoc) => { triggerFocusGrab(); gristDoc.viewModel.focusedRegionState('in'); - bLog('focusViewLayout focusedRegionState', 'in'); }; // When going out of the view layout, default view state is 'out' to remove active session @@ -404,7 +359,6 @@ const focusViewLayout = (gristDoc: GristDoc) => { // the active session borders, so that user understands what session the current panel is related to. const escapeViewLayout = (gristDoc: GristDoc, isRelated = false) => { gristDoc.viewModel.focusedRegionState(isRelated ? 'related' : 'out'); - bLog('escapeViewLayout focusedRegionState', isRelated ? 'related' : 'out'); }; /** @@ -413,7 +367,6 @@ const escapeViewLayout = (gristDoc: GristDoc, isRelated = false) => { const focusSection = (section: SectionRegion, gristDoc: GristDoc) => { focusViewLayout(gristDoc); gristDoc.viewModel.activeSectionId(section.id); - bLog('focusSection activeSectionId', section.id); }; /** @@ -468,13 +421,11 @@ const getSibling = ( // If we still don't find anything, it means we never set the current region before on a non-doc page, // or we didn't find any current doc section. Return the first region as default. if (currentIndexInCycle === -1) { - bLog('sibling', regions[0]); return regions[0]; } // Normal case: just return the next or previous region in the cycle, wrapping around const sibling = regions[mod(currentIndexInCycle + (direction === 'next' ? 1 : -1), regions.length)]; - bLog('sibling', sibling); return sibling; }; @@ -484,10 +435,6 @@ const getSibling = ( const blurPanelChild = (panel: PanelRegion) => { const panelElement = getPanelElement(panel.id); if (panelElement?.contains(document.activeElement) && document.activeElement !== panelElement) { - bLog('isPanel clear focus', { - activeElement: document.activeElement?.className, - panelElement: panelElement.className, - }); (document.activeElement as HTMLElement).blur(); } }; @@ -518,7 +465,6 @@ const isFocusableElement = (el: EventTarget | null): boolean => { */ const removeFocusRings = () => { document.querySelectorAll(`[${ATTRS.focusedElement}]`).forEach(el => { - bLog(`remove ${ATTRS.focusedElement}`, el); el.removeAttribute(ATTRS.focusedElement); }); }; From 68de731b038af0630a7e809af87ba91ed99d2860 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 28 Mar 2025 12:34:46 +0100 Subject: [PATCH 05/20] (kb) fix issue when clicking a non-focusable item inside a region MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit i actually don't understand why I didn't stumble on this sooner but oh well… --- app/client/components/RegionFocusSwitcher.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index eb2d7c600e..56b2e942eb 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -97,7 +97,6 @@ export class RegionFocusSwitcher extends Disposable { public panelAttrs(id: Panel, ariaLabel: string) { return [ - dom.attr('tabindex', '-1'), dom.attr('role', 'region'), dom.attr('aria-label', ariaLabel), dom.attr(ATTRS.regionId, id), @@ -132,7 +131,7 @@ export class RegionFocusSwitcher extends Disposable { if (isSpecialPage(gristDoc)) { return false; } - return id === 'main'; + return true; }), dom.cls(`${cssFocusedPanel.className}-focused`, use => { const current = use(this.current); @@ -208,6 +207,7 @@ export class RegionFocusSwitcher extends Disposable { // If user presses escape again, we also want to focus the panel. || (document.activeElement === document.body && panelElement) ) { + panelElement?.setAttribute('tabindex', '-1'); panelElement?.focus(); return; } @@ -243,6 +243,7 @@ export class RegionFocusSwitcher extends Disposable { const mouseEvent = this._currentUpdateInitiator instanceof MouseEvent ? this._currentUpdateInitiator : undefined; removeFocusRings(); + removeTabIndexes(); if (!mouseEvent) { this._savePrevElementState(prev); if (prev?.type === 'panel') { @@ -328,6 +329,11 @@ const focusPanel = (panel: PanelRegion, child: HTMLElement | null, gristDoc: Gri } // No child to focus found: just focus the panel if (!child || child === panelElement || !child.isConnected) { + // tabindex is dynamically set instead of always there for a reason: + // if we happen to just click on a non-focusable element inside the panel, + // browser default behavior is to make document.activeElement the closest focusable parent (the panel). + // We don't want this behavior, so we add/remove the tabindex attribute as needed. + panelElement.setAttribute('tabindex', '-1'); panelElement.focus(); } @@ -469,6 +475,12 @@ const removeFocusRings = () => { }); }; +const removeTabIndexes = () => { + document.querySelectorAll(`[${ATTRS.regionId}]`).forEach(el => { + el.removeAttribute('tabindex'); + }); +}; + const findRegionIndex = (regions: Region[], region: Region | undefined) => { if (!region) { return -1; From 523dc78b2524593778db1208e73abdee8c102be8 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 28 Mar 2025 15:46:41 +0100 Subject: [PATCH 06/20] (kb) handle esc key differently whether we use kb or mouse - we basically dont want to focus back the current panel with esc, if we didn't go on it via the cycle commands before. just directly reset when we dont come from cycle commands. --- app/client/components/RegionFocusSwitcher.ts | 61 ++++++++++++++------ 1 file changed, 42 insertions(+), 19 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 56b2e942eb..93873094d5 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -20,6 +20,7 @@ interface SectionRegion { id: any // this matches a grist document view section id } type Region = PanelRegion | SectionRegion; +type UpdateInitiator = {type: 'cycle'} | {type: 'mouse', event?: MouseEvent}; /** * RegionFocusSwitcher enables keyboard navigation between app panels and doc widgets. @@ -30,7 +31,7 @@ export class RegionFocusSwitcher extends Disposable { // Currently focused region public readonly current: Observable; private readonly _gristDocObs?: Observable; - private _currentUpdateInitiator: 'keyboard' | MouseEvent = 'keyboard'; + private _currentUpdateInitiator?: UpdateInitiator; // Previously focused elements for each panel (not used for view section ids) private _prevFocusedElements: Record = { left: null, @@ -74,7 +75,10 @@ export class RegionFocusSwitcher extends Disposable { } } - public focusRegion(region: Region | undefined, options: {initiator?: MouseEvent} = {}) { + public focusRegion( + region: Region | undefined, + options: {initiator?: UpdateInitiator} = {} + ) { if (region?.type === 'panel' && !getPanelElement(region.id)) { return; } @@ -87,12 +91,12 @@ export class RegionFocusSwitcher extends Disposable { throw new Error('view section id is not supported when no view layout is rendered'); } - this._currentUpdateInitiator = options.initiator || 'keyboard'; + this._currentUpdateInitiator = options.initiator; this.current.set(region); } - public reset() { - this.focusRegion(undefined); + public reset(initiator?: UpdateInitiator) { + this.focusRegion(undefined, {initiator}); } public panelAttrs(id: Panel, ariaLabel: string) { @@ -135,7 +139,7 @@ export class RegionFocusSwitcher extends Disposable { }), dom.cls(`${cssFocusedPanel.className}-focused`, use => { const current = use(this.current); - return this._currentUpdateInitiator === 'keyboard' && current?.type === 'panel' && current.id === id; + return this._currentUpdateInitiator?.type === 'cycle' && current?.type === 'panel' && current.id === id; }), ]; } @@ -148,7 +152,7 @@ export class RegionFocusSwitcher extends Disposable { cycleRegions, direction, gristDoc - )); + ), {initiator: {type: 'cycle'}}); if (gristDoc) { maybeNotifyAboutCreatorPanel(gristDoc, cycleRegions); } @@ -174,7 +178,10 @@ export class RegionFocusSwitcher extends Disposable { const currentlyInSection = current?.type === 'section'; if (targetsMain && !currentlyInSection) { - this.focusRegion({ type: 'section', id: gristDoc.viewModel.activeSectionId() }, { initiator: event }); + this.focusRegion( + {type: 'section', id: gristDoc.viewModel.activeSectionId()}, + {initiator: {type: 'mouse', event}} + ); return; } @@ -189,28 +196,41 @@ export class RegionFocusSwitcher extends Disposable { || (isFocusableElement(event.target) || getPanelElement(targetRegionId as Panel) === event.target) ) ) { - this.focusRegion({ type: 'panel', id: targetRegionId as Panel }, { initiator: event }); + this.focusRegion( + {type: 'panel', id: targetRegionId as Panel}, + {initiator: {type: 'mouse', event}} + ); return; } } private _onEscapeKeypress() { const current = this.current.get(); + // Do nothing if we are not focused on a panel if (current?.type !== 'panel') { return; } + const comesFromKeyboard = this._currentUpdateInitiator?.type === 'cycle'; const panelElement = getPanelElement(current.id); - // Focus back the panel element itself if currently focused element is a child + const activeElement = document.activeElement; + const activeElementIsInPanel = panelElement?.contains(activeElement) && activeElement !== panelElement; + if ( - (panelElement?.contains(document.activeElement) && document.activeElement !== panelElement) - // Specific case: when we escape inputs from panels, this isn't called and focus switches back to body. + // Focus back the panel element itself if currently focused element is a child + activeElementIsInPanel + // Specific case: when we escape inputs from panels, this isn't called, and focus switches back to body. // If user presses escape again, we also want to focus the panel. - || (document.activeElement === document.body && panelElement) + || (activeElement === document.body && panelElement) ) { - panelElement?.setAttribute('tabindex', '-1'); - panelElement?.focus(); + if (comesFromKeyboard) { + panelElement?.setAttribute('tabindex', '-1'); + panelElement?.focus(); + } else { + this.reset(); + } return; } + // …Reset region focus switch if already on the panel itself if (document.activeElement === panelElement) { this.reset(); @@ -240,7 +260,9 @@ export class RegionFocusSwitcher extends Disposable { } const gristDoc = this._getGristDoc(); - const mouseEvent = this._currentUpdateInitiator instanceof MouseEvent ? this._currentUpdateInitiator : undefined; + const mouseEvent = this._currentUpdateInitiator?.type === 'mouse' + ? this._currentUpdateInitiator.event + : undefined; removeFocusRings(); removeTabIndexes(); @@ -289,11 +311,12 @@ export class RegionFocusSwitcher extends Disposable { if (current?.type === 'panel' && current.id === 'right') { return this.focusRegion(gristDoc ? {type: 'section', id: gristDoc.viewModel.activeSectionId()} - : {type: 'panel', id: 'main'} + : {type: 'panel', id: 'main'}, + {initiator: {type: 'cycle'}} ); } commands.allCommands.rightPanelOpen.run(); - return this.focusRegion({type: 'panel', id: 'right'}); + return this.focusRegion({type: 'panel', id: 'right'}, {initiator: {type: 'cycle'}}); } /** @@ -396,7 +419,7 @@ const getCycleRegions = (gristDoc: GristDoc | null): Region[] => { // If there is a doc, also cycle through section ids return [ - ...gristDoc.viewLayout?.layout.getAllLeafIds().map(id => ({ type: 'section', id } as SectionRegion)) ?? [], + ...gristDoc.viewLayout?.layout.getAllLeafIds().map(id => ({type: 'section', id} as SectionRegion)) ?? [], ...commonPanels, ]; }; From c7333ca44512da71dbe34bb8910f2d4c213d3952 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 16 Apr 2025 17:53:41 +0200 Subject: [PATCH 07/20] (kb) panels focus: change how we handle mouse clicks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit before, we listened for clicks anywhere in the document. It seems that some e2e tests do some trickery by specifically clicking on out-of-the-app-ui stuff… Do not interfer when doing that. This is more of a precaution in order to make sure we only do things on purpose. --- app/client/components/RegionFocusSwitcher.ts | 32 +++++++++++--------- app/client/ui/PagePanels.ts | 4 ++- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 93873094d5..9776d43d4d 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -48,12 +48,12 @@ export class RegionFocusSwitcher extends Disposable { } } - public init() { + public init(pageContainer: HTMLElement) { // if we have a grist doc, wait for the current grist doc to be ready before doing anything if (this._gristDocObs && this._gristDocObs.get() === null) { this._gristDocObs.addListener((doc, prevDoc) => { if (doc && prevDoc === null) { - this.init(); + this.init(pageContainer); } }); return; @@ -70,8 +70,8 @@ export class RegionFocusSwitcher extends Disposable { if (this._gristDocObs) { const onClick = this._onClick.bind(this); - document.addEventListener('mouseup', onClick); - this.onDispose(() => document.removeEventListener('mouseup', onClick)); + pageContainer.addEventListener('mouseup', onClick); + this.onDispose(() => pageContainer.removeEventListener('mouseup', onClick)); } } @@ -185,22 +185,24 @@ export class RegionFocusSwitcher extends Disposable { return; } + const focusPanel = () => { + this.focusRegion( + {type: 'panel', id: targetRegionId as Panel}, + {initiator: {type: 'mouse', event}} + ); + }; + // When not targeting the main panel, we don't always want to focus the given region _on click_. // We only do it if clicking an empty area in the panel, or a focusable element like an input. // Otherwise, we assume clicks are on elements like buttons or links, // and we don't want to lose focus of current section in this case. // For example I don't want to focus out current table if just click the "undo" button in the header. - if (!targetsMain - && ( - !currentlyInSection - || (isFocusableElement(event.target) || getPanelElement(targetRegionId as Panel) === event.target) - ) - ) { - this.focusRegion( - {type: 'panel', id: targetRegionId as Panel}, - {initiator: {type: 'mouse', event}} - ); - return; + if (!targetsMain && isFocusableElement(event.target)) { + focusPanel(); + } + + if (!targetsMain && getPanelElement(targetRegionId as Panel) === event.target) { + focusPanel(); } } diff --git a/app/client/ui/PagePanels.ts b/app/client/ui/PagePanels.ts index cca802b633..cef8e1fdba 100644 --- a/app/client/ui/PagePanels.ts +++ b/app/client/ui/PagePanels.ts @@ -78,7 +78,6 @@ export function pagePanels( let onLeftTransitionFinish = noop; const regionFocusSwitcher = RegionFocusSwitcher.create(null, options.gristDoc); - regionFocusSwitcher.init(); // When switching to mobile mode, close panels; when switching to desktop, restore the // last desktop state. @@ -149,6 +148,9 @@ export function pagePanels( ); }), cssContentMain( + (el) => { + regionFocusSwitcher.init(el); + }, leftPaneDom = cssLeftPane( testId('left-panel'), regionFocusSwitcher.panelAttrs('left', t('Main navigation and document settings (left panel)')), From e9ff098dfbecc818a897676167c59c1604842bca Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 18 Apr 2025 17:44:00 +0200 Subject: [PATCH 08/20] (kb) make sure panel content is not focusable until we want to before, panel content was always focusable. This made some unintended behavior, like clicking a "a" element would keep the focus, and make the `input` command stop working. now if we just click a "a" element inside a non-focused panel, it wont grab focus. --- app/client/components/RegionFocusSwitcher.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 9776d43d4d..9335d9795b 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -105,19 +105,20 @@ export class RegionFocusSwitcher extends Disposable { dom.attr('aria-label', ariaLabel), dom.attr(ATTRS.regionId, id), dom.cls('clipboard_group_focus', use => { - // top/left/right panels are always focusable - if (id !== 'main') { - return true; - } const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; - // if we are not on a grist doc, main panel is always focusable + // if we are not on a grist doc, whole page is always focusable if (!gristDoc) { return true; } + const current = use(this.current); + // on a grist doc, panel content is focusable only if it's the current region + if (current?.type === 'panel' && current.id === id) { + return true; + } if (gristDoc) { use(gristDoc.activeViewId); } - // if we are on a grist doc, main panel is focusable only if we are not the actual document view + // on a grist doc, main panel is focusable only if we are not the actual document view return isSpecialPage(gristDoc); }), dom.cls('clipboard_forbid_focus', use => { From d2b1dc77283c8e47d541880ec6912ebd789accd6 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 16 Apr 2025 17:51:06 +0200 Subject: [PATCH 09/20] (kb) differenciate global view commands from focused-only commands sometimes, we want to run "current view"-related commands via keyboard shortcuts, while we are focusing ie. the creator panel. this commit makes it possible, by distinguishing "globally active" view commands from "only user-focused" view commands. in order to have the "globally active" commands more reliably work, we make sure the active section is focused by the region switcher automatically. it's not an ideal behavior for the user, as he will then need to manually go focus back the panel he was on ; but this is more done as a safety measure to not end up in a state where `body` is focused while we'd expect otherwise. --- app/client/components/BaseView.js | 30 ++++-- app/client/components/Cursor.ts | 2 +- app/client/components/CustomView.ts | 2 +- app/client/components/DetailView.js | 37 ++++--- app/client/components/Forms/FormView.ts | 2 +- app/client/components/GridView.js | 105 ++++++++++--------- app/client/components/GristDoc.ts | 3 + app/client/components/RegionFocusSwitcher.ts | 24 +++++ app/client/models/entities/ViewSectionRec.ts | 8 +- 9 files changed, 131 insertions(+), 82 deletions(-) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 1c7263c124..3ca824743a 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -34,6 +34,7 @@ const {markAsSeen} = require('app/client/models/UserPrefs'); const {buildConfirmDelete, reportUndo} = require('app/client/components/modals'); const {buildReassignModal} = require('app/client/ui/buildReassignModal'); const {MutedError} = require('app/client/models/errors'); +const {viewCommands} = require('app/client/components/RegionFocusSwitcher'); /** @@ -128,7 +129,8 @@ function BaseView(gristDoc, viewSectionModel, options) { this.listenTo(this.viewSection.events, 'rowHeightChange', this.onResize ); // Create a command group for keyboard shortcuts common to all views. - this.autoDispose(commands.createGroup(BaseView.commonCommands, this, this.viewSection.enableCommands)); + this.autoDispose(commands.createGroup(viewCommands(BaseView.commonCommands, this), this, this.viewSection.hasFocus)); + this.autoDispose(commands.createGroup(BaseView.commonFocusedCommands, this, this.viewSection.hasRegionFocus)); //-------------------------------------------------- // Prepare logic for linking with other sections. @@ -233,34 +235,43 @@ _.extend(Base.prototype, BackboneEvents); /** * These commands are common to GridView and DetailView. + * + * They work when the view is the currently active one. */ BaseView.commonCommands = { input: function(init) { this.scrollToCursor(true).catch(reportError); this.activateEditorAtCursor({init}); }, - editField: function(event) { closeRegisteredMenu(); this.scrollToCursor(true); this.activateEditorAtCursor({event}); }, - + copyLink: function() { this.copyLink().catch(reportError); }, + filterByThisCellValue: function() { this.filterByThisCellValue(); }, + duplicateRows: function() { this._duplicateRows().catch(reportError); }, + openDiscussion: function() { this.openDiscussionAtCursor(); }, insertRecordBefore: function() { this.insertRow(this.cursor.rowIndex()); }, insertRecordAfter: function() { this.insertRow(this.cursor.rowIndex() + 1); }, +}; + +/** + * These commands are common to GridView and DetailView. + * + * They are enabled only when the user is actually focusing the view, meaning + * they don't work when the view is the active one but the user is focused on the creator panel. + */ +BaseView.commonFocusedCommands = { + editField: function(event) { closeRegisteredMenu(); this.scrollToCursor(true); this.activateEditorAtCursor({event}); }, insertCurrentDate: function() { this.insertCurrentDate(false); }, insertCurrentDateTime: function() { this.insertCurrentDate(true); }, - copyLink: function() { this.copyLink().catch(reportError); }, - deleteRecords: function(source) { this.deleteRecords(source); }, - filterByThisCellValue: function() { this.filterByThisCellValue(); }, - duplicateRows: function() { this._duplicateRows().catch(reportError); }, - openDiscussion: function() { this.openDiscussionAtCursor(); }, viewAsCard: function() { /* Overridden by subclasses. * * This is still needed so that doesn't trigger the `input` command * if a subclass doesn't support opening the current record as a card. */ }, -}; +} BaseView.prototype.selectedRows = function() { return []; @@ -860,4 +871,5 @@ BaseView.prototype.isRecordCardDisabled = function() { return this.viewSection.isTableRecordCardDisabled(); } + module.exports = BaseView; diff --git a/app/client/components/Cursor.ts b/app/client/components/Cursor.ts index e1b15a60f9..11662b75e4 100644 --- a/app/client/components/Cursor.ts +++ b/app/client/components/Cursor.ts @@ -112,7 +112,7 @@ export class Cursor extends Disposable { this.fieldIndex = baseView.viewSection.viewFields().makeLiveIndex(optCursorPos.fieldIndex || 0); - this.autoDispose(commands.createGroup(Cursor.editorCommands, this, baseView.viewSection.enableCommands)); + this.autoDispose(commands.createGroup(Cursor.editorCommands, this, baseView.viewSection.hasRegionFocus)); // RowId might diverge from the one stored in _rowId when the data changes (it is filtered out). So here // we will calculate rowId based on rowIndex (so in reverse order), to have a proper value. diff --git a/app/client/components/CustomView.ts b/app/client/components/CustomView.ts index ec5f4d9afe..9875711aca 100644 --- a/app/client/components/CustomView.ts +++ b/app/client/components/CustomView.ts @@ -129,7 +129,7 @@ export class CustomView extends Disposable { this.autoDispose(this.customDef.pluginId.subscribe(this._updatePluginInstance, this)); this.autoDispose(this.customDef.sectionId.subscribe(this._updateCustomSection, this)); - this.autoDispose(commands.createGroup(CustomView._commands, this, this.viewSection.enableCommands)); + this.autoDispose(commands.createGroup(CustomView._commands, this, this.viewSection.hasRegionFocus)); this._unmappedColumns = this.autoDispose(ko.pureComputed(() => { const columns = this.viewSection.columnsToMap(); diff --git a/app/client/components/DetailView.js b/app/client/components/DetailView.js index 4b307a0d20..0840685f4d 100644 --- a/app/client/components/DetailView.js +++ b/app/client/components/DetailView.js @@ -18,6 +18,7 @@ const selector = require('./CellSelector'); const {CopySelection} = require('./CopySelection'); const RecordLayout = require('./RecordLayout'); const commands = require('./commands'); +const {viewCommands} = require('./RegionFocusSwitcher'); const tableUtil = require('../lib/tableUtil'); const {CardContextMenu} = require('../ui/CardContextMenu'); const {FieldContextMenu} = require('../ui/FieldContextMenu'); @@ -127,8 +128,8 @@ function DetailView(gristDoc, viewSectionModel) { //-------------------------------------------------- // Instantiate CommandGroups for the different modes. - this.autoDispose(commands.createGroup(DetailView.generalCommands, this, this.viewSection.enableCommands)); - this.autoDispose(commands.createGroup(DetailView.fieldCommands, this, this.viewSection.enableCommands)); + this.autoDispose(commands.createGroup(viewCommands(DetailView.detailCommands, this), this, this.viewSection.hasFocus)); + this.autoDispose(commands.createGroup(DetailView.detailFocusedCommands, this, this.viewSection.hasRegionFocus)); const hasSelection = this.autoDispose(ko.pureComputed(() => !this.cellSelector.isCurrentSelectType('') || this.copySelection())); this.autoDispose(commands.createGroup(DetailView.selectionCommands, this, hasSelection)); @@ -154,31 +155,33 @@ DetailView.prototype._updateFloatingRow = function() { }; /** - * DetailView commands. + * DetailView commands, enabled when the view is the active one. */ -DetailView.generalCommands = { - cursorUp: function() { this.cursor.fieldIndex(this.cursor.fieldIndex() - 1); }, - cursorDown: function() { this.cursor.fieldIndex(this.cursor.fieldIndex() + 1); }, - pageUp: function() { this.cursor.rowIndex(this.cursor.rowIndex() - 1); }, - pageDown: function() { this.cursor.rowIndex(this.cursor.rowIndex() + 1); }, - copy: function() { return this.copy(this.getSelection()); }, - cut: function() { return this.cut(this.getSelection()); }, - paste: function(pasteObj, cutCallback) { - return this.gristDoc.docData.bundleActions(null, () => this.paste(pasteObj, cutCallback)); - }, - +DetailView.detailCommands = { editLayout: function() { if (this.scrolly()) { this.scrolly().scrollRowIntoView(this.cursor.rowIndex()); } this.recordLayout.editLayout(this.cursor.rowIndex()); }, + hideCardFields: function() { this._hideCardFields(); }, + copy: function() { return this.copy(this.getSelection()); }, + cut: function() { return this.cut(this.getSelection()); }, + paste: function(pasteObj, cutCallback) { + return this.gristDoc.docData.bundleActions(null, () => this.paste(pasteObj, cutCallback)); + }, }; -DetailView.fieldCommands = { +/** + * DetailView commands, enabled when the view is user-focused. + */ +DetailView.detailFocusedCommands = { + cursorUp: function() { this.cursor.fieldIndex(this.cursor.fieldIndex() - 1); }, + cursorDown: function() { this.cursor.fieldIndex(this.cursor.fieldIndex() + 1); }, + pageUp: function() { this.cursor.rowIndex(this.cursor.rowIndex() - 1); }, + pageDown: function() { this.cursor.rowIndex(this.cursor.rowIndex() + 1); }, clearValues: function() { this._clearCardFields(); }, - hideCardFields: function() { this._hideCardFields(); }, -}; +} DetailView.selectionCommands = { clearCopySelection: function() { this._clearCopySelection(); }, diff --git a/app/client/components/Forms/FormView.ts b/app/client/components/Forms/FormView.ts index 3202115134..8872cf3b9d 100644 --- a/app/client/components/Forms/FormView.ts +++ b/app/client/components/Forms/FormView.ts @@ -348,7 +348,7 @@ export class FormView extends Disposable { editField: keyboardActions.edit, deleteFields: keyboardActions.clearValues, hideFields: keyboardActions.hideFields, - }, this, this.viewSection.enableCommands)); + }, this, this.viewSection.hasRegionFocus)); this._previewUrl = Computed.create(this, use => { const doc = use(this.gristDoc.docPageModel.currentDoc); diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index 54114137d1..3b0e06aa95 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -16,6 +16,7 @@ const tableUtil = require('../lib/tableUtil'); const {addToSort, sortBy} = require('../lib/sortUtil'); const commands = require('./commands'); +const {viewCommands} = require('./RegionFocusSwitcher'); const viewCommon = require('./viewCommon'); const Base = require('./Base'); const BaseView = require('./BaseView'); @@ -266,8 +267,10 @@ function GridView(gristDoc, viewSectionModel, isPreview = false) { this.onEvent(this.scrollPane, 'scroll', this.onScroll); //-------------------------------------------------- - // Command group implementing all grid level commands (except cancel) - this.autoDispose(commands.createGroup(GridView.gridCommands, this, this.viewSection.enableCommands)); + // Command groups implementing all grid level commands (except cancel) + this.autoDispose(commands.createGroup(viewCommands(GridView.gridCommands, this), this, this.viewSection.hasFocus)); + this.autoDispose(commands.createGroup(GridView.gridFocusedCommands, this, this.viewSection.hasRegionFocus)); + // Cancel command is registered conditionally, only when there is an active // cell selection. This command is also used by Raw Data Views, to close the Grid popup. const hasSelection = this.autoDispose(ko.pureComputed(() => @@ -295,6 +298,56 @@ GridView.selectionCommands = { // TODO: move commands with modifications to gridEditCommands and use a single guard for // readonly state. GridView.gridCommands = { + fillSelectionDown: function() { tableUtil.fillSelectionDown(this.getSelection(), this.tableModel); }, + selectAll: function() { this.selectAll(); }, + insertFieldBefore: function(event) { this._insertField(event, this.cursor.fieldIndex()); }, + insertFieldAfter: function(event) { this._insertField(event, this.cursor.fieldIndex() + 1); }, + makeHeadersFromRow: function() { this.makeHeadersFromRow(this.getSelection()); }, + renameField: function() { this.renameColumn(this.cursor.fieldIndex()); }, + hideFields: function() { this.hideFields(this.getSelection()); }, + deleteFields: function() { this._deleteFields(); }, + clearColumns: function() { this._clearColumns(this.getSelection()); }, + convertFormulasToData: function() { this._convertFormulasToData(this.getSelection()); }, + sortAsc: function() { + sortBy(this.viewSection.activeSortSpec, this.currentColumn().getRowId(), Sort.ASC); + }, + sortDesc: function() { + sortBy(this.viewSection.activeSortSpec, this.currentColumn().getRowId(), Sort.DESC); + }, + addSortAsc: function() { + addToSort(this.viewSection.activeSortSpec, this.currentColumn().getRowId(), Sort.ASC); + }, + addSortDesc: function() { + addToSort(this.viewSection.activeSortSpec, this.currentColumn().getRowId(), Sort.DESC); + }, + toggleFreeze: function() { + // get column selection + const selection = this.getSelection(); + // convert it menu option + const options = this._getColumnMenuOptions(selection); + // generate action that is available for freeze toggle + const action = freezeAction(options); + // if no action, do nothing + if (!action) { return; } + // if grist document is in readonly - simply change the value + // without saving + if (this.isReadonly) { + this.viewSection.rawNumFrozen(action.numFrozen); + return; + } + this.viewSection.rawNumFrozen.setAndSave(action.numFrozen); + }, + copy: function() { console.log(this.getSelection()); return this.copy(this.getSelection()); }, + cut: function() { return this.cut(this.getSelection()); }, + paste: async function(pasteObj, cutCallback) { + if (this.gristDoc.isReadonly.get()) { return; } + await this.gristDoc.docData.bundleActions(null, () => this.paste(pasteObj, cutCallback)); + await this.scrollToCursor(false); + }, +}; + +// These commands are enabled only when the grid is the user-focused region. +GridView.gridFocusedCommands = { cursorDown: function() { if (this.cursor.rowIndex() === this.viewData.peekLength - 1) { // When the cursor is in the bottom row, the view may not be scrolled all the way to @@ -335,58 +388,10 @@ GridView.gridCommands = { ctrlShiftUp: function () { this._shiftSelectUntilFirstOrLastNonEmptyCell({direction: 'up'}); }, ctrlShiftRight: function () { this._shiftSelectUntilFirstOrLastNonEmptyCell({direction: 'right'}); }, ctrlShiftLeft: function () { this._shiftSelectUntilFirstOrLastNonEmptyCell({direction: 'left'}); }, - fillSelectionDown: function() { tableUtil.fillSelectionDown(this.getSelection(), this.tableModel); }, - - selectAll: function() { this.selectAll(); }, - fieldEditSave: function() { this.cursor.rowIndex(this.cursor.rowIndex() + 1); }, // Re-define editField after fieldEditSave to make it take precedence for the Enter key. editField: function(event) { closeRegisteredMenu(); this.scrollToCursor(true); this.activateEditorAtCursor({event}); }, - insertFieldBefore: function(event) { this._insertField(event, this.cursor.fieldIndex()); }, - insertFieldAfter: function(event) { this._insertField(event, this.cursor.fieldIndex() + 1); }, - makeHeadersFromRow: function() { this.makeHeadersFromRow(this.getSelection()); }, - renameField: function() { this.renameColumn(this.cursor.fieldIndex()); }, - hideFields: function() { this.hideFields(this.getSelection()); }, - deleteFields: function() { this._deleteFields(); }, clearValues: function() { this.clearValues(this.getSelection()); }, - clearColumns: function() { this._clearColumns(this.getSelection()); }, - convertFormulasToData: function() { this._convertFormulasToData(this.getSelection()); }, - copy: function() { return this.copy(this.getSelection()); }, - cut: function() { return this.cut(this.getSelection()); }, - paste: async function(pasteObj, cutCallback) { - if (this.gristDoc.isReadonly.get()) { return; } - await this.gristDoc.docData.bundleActions(null, () => this.paste(pasteObj, cutCallback)); - await this.scrollToCursor(false); - }, - sortAsc: function() { - sortBy(this.viewSection.activeSortSpec, this.currentColumn().getRowId(), Sort.ASC); - }, - sortDesc: function() { - sortBy(this.viewSection.activeSortSpec, this.currentColumn().getRowId(), Sort.DESC); - }, - addSortAsc: function() { - addToSort(this.viewSection.activeSortSpec, this.currentColumn().getRowId(), Sort.ASC); - }, - addSortDesc: function() { - addToSort(this.viewSection.activeSortSpec, this.currentColumn().getRowId(), Sort.DESC); - }, - toggleFreeze: function() { - // get column selection - const selection = this.getSelection(); - // convert it menu option - const options = this._getColumnMenuOptions(selection); - // generate action that is available for freeze toggle - const action = freezeAction(options); - // if no action, do nothing - if (!action) { return; } - // if grist document is in readonly - simply change the value - // without saving - if (this.isReadonly) { - this.viewSection.rawNumFrozen(action.numFrozen); - return; - } - this.viewSection.rawNumFrozen.setAndSave(action.numFrozen); - }, viewAsCard() { const selectedRows = this.selectedRows(); if (selectedRows.length !== 1) { return; } diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index d309212845..f3e1761ae8 100644 --- a/app/client/components/GristDoc.ts +++ b/app/client/components/GristDoc.ts @@ -21,6 +21,7 @@ import {RawDataPage, RawDataPopup} from 'app/client/components/RawDataPage'; import {RecordCardPopup} from 'app/client/components/RecordCardPopup'; import {ActionGroupWithCursorPos, UndoStack} from 'app/client/components/UndoStack'; import {ViewLayout} from 'app/client/components/ViewLayout'; +import {RegionFocusSwitcher} from 'app/client/components/RegionFocusSwitcher'; import {get as getBrowserGlobals} from 'app/client/lib/browserGlobals'; import {DocPluginManager} from 'app/client/lib/DocPluginManager'; import {ImportSourceElement} from 'app/client/lib/ImportSourceElement'; @@ -177,6 +178,7 @@ export interface GristDoc extends DisposableWithEvents { isTimingOn: Observable; attachmentTransfer: Observable; canShowRawData: Observable; + regionFocusSwitcher?: RegionFocusSwitcher; docId(): string; openDocPage(viewId: IDocPage): Promise; @@ -220,6 +222,7 @@ export class GristDocImpl extends DisposableWithEvents implements GristDoc { public isReadonly = this.docPageModel.isReadonly; public isReadonlyKo = toKo(ko, this.isReadonly); public comparison: DocStateComparison | null; + public regionFocusSwitcher?: RegionFocusSwitcher; // component for keeping track of latest cursor position public cursorMonitor: CursorMonitor; // component for keeping track of a cell that is being edited diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 9335d9795b..c2e0b6cd6b 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -7,6 +7,7 @@ import * as commands from 'app/client/components/commands'; import {triggerFocusGrab} from 'app/client/components/Clipboard'; import {GristDoc} from 'app/client/components/GristDoc'; import {theme} from 'app/client/ui2018/cssVars'; +import BaseView from 'app/client/components/BaseView'; const t = makeT('RegionFocusSwitcher'); @@ -53,6 +54,7 @@ export class RegionFocusSwitcher extends Disposable { if (this._gristDocObs && this._gristDocObs.get() === null) { this._gristDocObs.addListener((doc, prevDoc) => { if (doc && prevDoc === null) { + doc.regionFocusSwitcher = this; this.init(pageContainer); } }); @@ -95,6 +97,13 @@ export class RegionFocusSwitcher extends Disposable { this.current.set(region); } + public focusActiveSection() { + const gristDoc = this._getGristDoc(); + if (gristDoc) { + this.focusRegion({type: 'section', id: gristDoc.viewModel.activeSectionId()}); + } + } + public reset(initiator?: UpdateInitiator) { this.focusRegion(undefined, {initiator}); } @@ -339,6 +348,21 @@ export class RegionFocusSwitcher extends Disposable { } } +/** + * Helper to declare view commands that should also focus current view. + * + * Used by a view when registering command groups. + */ +export const viewCommands = (commandsObject: Record, context: BaseView) => { + return Object.keys(commandsObject).reduce>((acc, key) => { + const originalCommand = commandsObject[key]; + acc[key] = function(...args: any[]) { + context.gristDoc.regionFocusSwitcher?.focusActiveSection(); + return originalCommand.apply(context, args); + }; + return acc; + }, {}); +}; const ATTRS = { regionId: 'data-grist-region-id', diff --git a/app/client/models/entities/ViewSectionRec.ts b/app/client/models/entities/ViewSectionRec.ts index 91115f554e..43163c26d9 100644 --- a/app/client/models/entities/ViewSectionRec.ts +++ b/app/client/models/entities/ViewSectionRec.ts @@ -162,9 +162,12 @@ export interface ViewSectionRec extends IRowModel<"_grist_Views_section">, RuleO // Evaluates to an array of column models, which are not referenced by anything in viewFields. hiddenColumns: ko.Computed; + // True if the section is the active section. hasFocus: ko.Computed; + // True if the section is the active section and if the user-focused page panel is the section or the creator panel. hasVisibleFocus: ko.Computed; - enableCommands: ko.Computed; + // True if the section is the active section and if the user-focused page panel is the section. + hasRegionFocus: ko.Computed; // Section-linking affects table if linkSrcSection is set. The controller value of the // link is the value of srcCol at activeRowId of linkSrcSection, or activeRowId itself when @@ -732,8 +735,7 @@ export function createViewSectionRec(this: ViewSectionRec, docModel: DocModel): const region = this.view().focusedRegionState(); return this.hasFocus() && (region === 'in' || region === 'related'); }); - // we enable keyboard shortcuts only when the region is focused (see RegionFocusSwitcher) - this.enableCommands = ko.pureComputed(() => this.hasFocus() && this.view().focusedRegionState() === 'in'); + this.hasRegionFocus = ko.pureComputed(() => this.hasFocus() && this.view().focusedRegionState() === 'in'); // Section-linking affects this table if linkSrcSection is set. The controller value of the // link is the value of srcCol at activeRowId of linkSrcSection, or activeRowId itself when From eac1a6d88d10d36b5dff0a95314e962aa7716399 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 18 Apr 2025 18:11:16 +0200 Subject: [PATCH 10/20] (kb) highlight focused elements only out of grist doc views since most kb-related things in grist views are handled manually with specific js, we dont want a default focus ring on stuff that might get focus inside them. before that, a focus ring was added on celleditor_text_editor for example. --- app/client/components/KeyboardFocusHighlighter.ts | 2 +- app/client/components/RegionFocusSwitcher.ts | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/app/client/components/KeyboardFocusHighlighter.ts b/app/client/components/KeyboardFocusHighlighter.ts index 9469cd7743..ff0f72240e 100644 --- a/app/client/components/KeyboardFocusHighlighter.ts +++ b/app/client/components/KeyboardFocusHighlighter.ts @@ -33,7 +33,7 @@ export class KeyboardFocusHighlighter extends Disposable { } const cssKeyboardUser = styled('div', ` - & :is(a, input, textarea, select, button, [tabindex="0"]):focus-visible { + & .kb-focus-highlighter-group :is(a, input, textarea, select, button, [tabindex="0"]):focus-visible { /* simulate default browser focus ring */ outline: 2px solid Highlight !important; outline: 2px solid -webkit-focus-ring-color !important; diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index c2e0b6cd6b..2b90f2ced5 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -113,6 +113,20 @@ export class RegionFocusSwitcher extends Disposable { dom.attr('role', 'region'), dom.attr('aria-label', ariaLabel), dom.attr(ATTRS.regionId, id), + dom.cls('kb-focus-highlighter-group', use => { + // highlight focused elements everywhere except in the grist doc views + if (id !== 'main') { + return true; + } + const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; + if (!gristDoc) { + return true; + } + if (gristDoc) { + use(gristDoc.activeViewId); + } + return isSpecialPage(gristDoc); + }), dom.cls('clipboard_group_focus', use => { const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; // if we are not on a grist doc, whole page is always focusable From b3bd850d9f4967582347539377b7fbc6021b3651 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 18 Apr 2025 20:28:29 +0200 Subject: [PATCH 11/20] (kb) trying out something, this needs a followup the idea is to fix all the interactions where we start focused on the creator panel and the app ends up focusing the clipboard element by itself, for example when transforming a column type (after applying the transform). In those cases for now we end up in a weird state where the regionFocusSwitcher says we are on the creator panel, but the document.activeElement is the hidden textarea (clipboard el). --- app/client/components/Clipboard.js | 3 +++ app/client/ui/PagePanels.ts | 2 ++ 2 files changed, 5 insertions(+) diff --git a/app/client/components/Clipboard.js b/app/client/components/Clipboard.js index b3eeceaedf..90a057b1c3 100644 --- a/app/client/components/Clipboard.js +++ b/app/client/components/Clipboard.js @@ -77,6 +77,9 @@ function Clipboard(app) { onDefaultFocus: () => { this.copypasteField.value = ' '; this.copypasteField.select(); + if (window.gristRegionFocusSwitcher) { + window.gristRegionFocusSwitcher.focusActiveSection(); + } this._app.trigger('clipboard_focus'); }, onDefaultBlur: () => { diff --git a/app/client/ui/PagePanels.ts b/app/client/ui/PagePanels.ts index cef8e1fdba..c3e95e4708 100644 --- a/app/client/ui/PagePanels.ts +++ b/app/client/ui/PagePanels.ts @@ -78,6 +78,8 @@ export function pagePanels( let onLeftTransitionFinish = noop; const regionFocusSwitcher = RegionFocusSwitcher.create(null, options.gristDoc); + // @ts-expect-error just dirty code to check an idea. To remove later. + window.gristRegionFocusSwitcher = regionFocusSwitcher; // When switching to mobile mode, close panels; when switching to desktop, restore the // last desktop state. From 83a049639074ced99d158c1a2ca63cc19750b590 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 18 Feb 2025 16:11:02 +0100 Subject: [PATCH 12/20] (kb) update tests after keyboard region switching work - two interactions now need the user to first press esc to get out of current focus before continuing (I'd say an acceptable change?) - sometimes when we want to return on the "app focus", it won't mean the clipboard element anymore --- test/nbrowser/AccessRules1.ts | 2 +- test/nbrowser/RefTransforms.ts | 1 + test/nbrowser/TokenField.ts | 1 + test/nbrowser/gristUtils.ts | 9 +++++++-- 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/test/nbrowser/AccessRules1.ts b/test/nbrowser/AccessRules1.ts index 43d1b4ac64..268d1e1849 100644 --- a/test/nbrowser/AccessRules1.ts +++ b/test/nbrowser/AccessRules1.ts @@ -451,7 +451,7 @@ describe('AccessRules1', function() { "Add Widget to Page", ]); await driver.sendKeys(Key.ESCAPE); - await gu.waitAppFocus(); + await gu.waitForFocus('body'); }); it('should support dollar syntax in the editor', async function() { diff --git a/test/nbrowser/RefTransforms.ts b/test/nbrowser/RefTransforms.ts index c6c812bea8..ebc5c5d2d8 100644 --- a/test/nbrowser/RefTransforms.ts +++ b/test/nbrowser/RefTransforms.ts @@ -91,6 +91,7 @@ describe('RefTransforms', function() { await gu.waitForServer(); // Add some references to values in the same table. + await gu.sendKeys(Key.ESCAPE); // First ESCAPE to get out of select focus. await gu.sendKeys(Key.ENTER, 'foo', Key.ENTER, 'bar', Key.ENTER, Key.ENTER); await gu.waitForServer(); diff --git a/test/nbrowser/TokenField.ts b/test/nbrowser/TokenField.ts index cce168828c..bb14d8d818 100644 --- a/test/nbrowser/TokenField.ts +++ b/test/nbrowser/TokenField.ts @@ -91,6 +91,7 @@ describe('TokenField', function() { await gu.waitForServer(); // Add two films. + await gu.sendKeys(Key.ESCAPE); // First ESCAPE to get out of select/panel focus. await gu.sendKeys(Key.ENTER); await gu.sendKeys('Toy', Key.ENTER); await gu.sendKeys('Alien', Key.ENTER); diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 102960e007..322eb24d40 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -795,7 +795,6 @@ export async function enterCell(...keys: string[]) { } await driver.sendKeys(...keys); await waitForServer(); // Wait for the value to be saved - await waitAppFocus(); // Wait for the cell editor to be closed (maybe unnecessary) } /** @@ -1163,7 +1162,6 @@ export async function docMenuImport(filePath: string) { }); } - /** * Wait for the focus to return to the main application, i.e. the special .copypaste element that * normally has it (as opposed to an open cell editor, or a focus in some input or menu). Specify @@ -1173,6 +1171,13 @@ export async function waitAppFocus(yesNo: boolean = true): Promise { await driver.wait(async () => (await driver.find('.copypaste').hasFocus()) === yesNo, 5000); } +/** + * Wait for the focus to be on the first element matching given selector. + */ +export async function waitForFocus(selector: string): Promise { + await driver.wait(async () => (await driver.find(selector).hasFocus()), 1000); +} + export async function waitForLabelInput(): Promise { await driver.wait(async () => (await driver.findWait('.test-column-title-label', 100).hasFocus()), 300); } From 59da755272d051a584d67a71132d361499ce2092 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 18 Apr 2025 17:36:07 +0200 Subject: [PATCH 13/20] (tests) normalize some function calls to find them quicker in codebase --- test/nbrowser/CellColor.ts | 2 +- test/nbrowser/ChoiceList.ts | 2 +- test/nbrowser/ColumnFilterMenu.ts | 6 +++--- test/nbrowser/ColumnOps.ntest.js | 8 ++++---- test/nbrowser/Dates.ntest.js | 4 ++-- test/nbrowser/Formulas.ts | 10 +++++----- test/nbrowser/NumericEditor.ts | 2 +- test/nbrowser/TextEditor.ntest.js | 2 +- test/nbrowser/TypeChange.ntest.js | 2 +- test/nbrowser/gristUtil-nbrowser.js | 2 +- 10 files changed, 20 insertions(+), 20 deletions(-) diff --git a/test/nbrowser/CellColor.ts b/test/nbrowser/CellColor.ts index 84e0cf449a..62bc242a1f 100644 --- a/test/nbrowser/CellColor.ts +++ b/test/nbrowser/CellColor.ts @@ -305,7 +305,7 @@ describe('CellColor', function() { // Empty cell to clear error from converting toggle to date await cell.click(); await driver.sendKeys(Key.DELETE); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); const clip = cell.find('.field_clip'); diff --git a/test/nbrowser/ChoiceList.ts b/test/nbrowser/ChoiceList.ts index 442e0c10b7..cb8e389caa 100644 --- a/test/nbrowser/ChoiceList.ts +++ b/test/nbrowser/ChoiceList.ts @@ -771,7 +771,7 @@ describe('ChoiceList', function() { it('should save and close the choice config editor on focusout', async function() { // Click outside of the editor. await driver.find('.test-gristdoc').click(); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); // Check that the changes were saved. assert.deepEqual(await getChoiceLabels(), ['Choice 1', 'Choice 2', 'Choice 3']); diff --git a/test/nbrowser/ColumnFilterMenu.ts b/test/nbrowser/ColumnFilterMenu.ts index 0bd5770b63..7cb160871d 100644 --- a/test/nbrowser/ColumnFilterMenu.ts +++ b/test/nbrowser/ColumnFilterMenu.ts @@ -523,7 +523,7 @@ describe('ColumnFilterMenu', function() { // set min to '2019-07-16' await gu.setRangeFilterBound('min', '2019-07-16'); await driver.find('.test-filter-menu-apply-btn').click(); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); // check values assert.deepEqual( @@ -541,7 +541,7 @@ describe('ColumnFilterMenu', function() { // set max to '2019-07-17' await gu.setRangeFilterBound('max', '2019-07-17'); await driver.find('.test-filter-menu-apply-btn').click(); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); assert.deepEqual( await gu.getVisibleGridCells({cols: ['Name', colId], rowNums: [1, 2, 3, 4]}), @@ -557,7 +557,7 @@ describe('ColumnFilterMenu', function() { await gu.setRangeFilterBound('min', null); await gu.setRangeFilterBound('max', null); await driver.find('.test-filter-menu-apply-btn').click(); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); // check all values are there assert.deepEqual( diff --git a/test/nbrowser/ColumnOps.ntest.js b/test/nbrowser/ColumnOps.ntest.js index 8ebdcc0111..21fd78b1e3 100644 --- a/test/nbrowser/ColumnOps.ntest.js +++ b/test/nbrowser/ColumnOps.ntest.js @@ -30,7 +30,7 @@ describe('ColumnOps.ntest', function() { await gu.waitForServer(); await gu.userActionsVerify([]); await gu.userActionsCollect(false); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); await shouldHaveColumnHeader('A'); await assert.isPresent(gu.getOpenEditingLabel(gu.getColumnHeader('A')), false); @@ -53,7 +53,7 @@ describe('ColumnOps.ntest', function() { ["AddRecord", "_grist_Views_section_field", null, {"colRef":43, "parentId":4, "parentPos":null}]]); await gu.userActionsCollect(false); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); await shouldHaveColumnHeader('A'); await assert.isPresent(gu.getOpenEditingLabel(gu.getColumnHeader('A')), false); assert.deepEqual(await gu.getGridLabels('City'), @@ -102,7 +102,7 @@ describe('ColumnOps.ntest', function() { await gu.waitToPass(() => gu.getColumnHeader('B')); await gu.getOpenEditingLabel(await gu.getColumnHeader('B')).wait().sendKeys($.ENTER); await gu.waitForServer(); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); await shouldHaveColumnHeader('B'); }); @@ -169,7 +169,7 @@ describe('ColumnOps.ntest', function() { await gu.userActionsVerify([]); await gu.userActionsCollect(false); await assert.isPresent(gu.getOpenEditingLabel(gu.getColumnHeader('foo')), false); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); // Bug T384: Should save the column name after cancelling a rename earlier. await shouldHaveColumnHeader('A'); diff --git a/test/nbrowser/Dates.ntest.js b/test/nbrowser/Dates.ntest.js index 54f1333529..f5da18a42a 100644 --- a/test/nbrowser/Dates.ntest.js +++ b/test/nbrowser/Dates.ntest.js @@ -179,12 +179,12 @@ describe('Dates.ntest', function() { await gu.sendKeys($.DELETE); await gu.waitForServer(); await gu.getCellRC(0, 2).click(); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); await gu.sendKeys('='); await $('.test-editor-tooltip-convert').click(); await gu.sendKeys('$A', $.ENTER); await gu.waitForServer(); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); await gu.getCellRC(0, 3).click(); await gu.sendKeys('='); await gu.waitAppFocus(false); diff --git a/test/nbrowser/Formulas.ts b/test/nbrowser/Formulas.ts index cb56f515a3..22d6cd0967 100644 --- a/test/nbrowser/Formulas.ts +++ b/test/nbrowser/Formulas.ts @@ -270,7 +270,7 @@ return [ await driver.sendKeys(Key.ENTER); await driver.findContentWait('.ace_content', /^MEDIAN\($/, 1000); await driver.sendKeys(Key.ESCAPE); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); // Check that this works also for table names ("fri" finds "Friends") await driver.sendKeys('='); @@ -301,7 +301,7 @@ return [ await driver.sendKeys(Key.DOWN, Key.DOWN, Key.ENTER); await driver.findContentWait('.ace_content', /^Friends\.lookupOne\($/, 1000); await driver.sendKeys(Key.ESCAPE, Key.ESCAPE); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); // Check that some built-in values are recognized in lowercase. async function testBuiltin(typedText: string, expectedCompletion: string) { @@ -311,7 +311,7 @@ return [ await gu.waitToPass(async () => assert.include(await driver.findAll('.ace_autocomplete .ace_line', el => el.getText()), expectedCompletion)); await driver.sendKeys(Key.ESCAPE, Key.ESCAPE); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); } await testBuiltin('tr', 'Tr\nue\n '); await testBuiltin('fa', 'Fa\nlse\n '); @@ -349,7 +349,7 @@ return [ await driver.findContent('.ace_autocomplete .ace_line span', /value/).click(); await driver.findContentWait('.ace_content', /^MEDIAN\($/, 1000); await driver.sendKeys(Key.ESCAPE); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); // Check that this works also for table names ("fri" finds "Friends") await driver.sendKeys('='); @@ -391,6 +391,6 @@ return [ await driver.findContent('.ace_autocomplete .ace_line', /lookupRecords/).findContent('span', /Friends/).click(); await driver.findContentWait('.ace_content', /^Friends\.lookupRecords\($/, 1000); await driver.sendKeys(Key.ESCAPE, Key.ESCAPE); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); }); }); diff --git a/test/nbrowser/NumericEditor.ts b/test/nbrowser/NumericEditor.ts index 9db36c2054..67831499ea 100644 --- a/test/nbrowser/NumericEditor.ts +++ b/test/nbrowser/NumericEditor.ts @@ -140,7 +140,7 @@ describe('NumericEditor', function() { // separator would open with a "." decimal separator, and get saved back incorrectly). await gu.sendKeys(Key.ENTER); await gu.waitForServer(); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); assert.equal(await gu.getCell({rowNum: 1, col: Number(i)}).getText(), entry.expect); } }); diff --git a/test/nbrowser/TextEditor.ntest.js b/test/nbrowser/TextEditor.ntest.js index ec42547d17..ad418e4811 100644 --- a/test/nbrowser/TextEditor.ntest.js +++ b/test/nbrowser/TextEditor.ntest.js @@ -111,7 +111,7 @@ describe('TextEditor.ntest', function() { await autoCompleteSelect({input: 'foobar3', keys: [$.UP]}); await gu.sendKeys($.ESCAPE); await gu.waitForServer(); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); await $(".viewsection_title:contains(TABLE1)").click(); assert.equal(await gu.getCellRC(6, 0).text(), ""); diff --git a/test/nbrowser/TypeChange.ntest.js b/test/nbrowser/TypeChange.ntest.js index c7d39e6014..82d03034a8 100644 --- a/test/nbrowser/TypeChange.ntest.js +++ b/test/nbrowser/TypeChange.ntest.js @@ -148,7 +148,7 @@ describe('TypeChange.ntest', function() { // Change type to reference column await gu.actions.viewSection('Table1').selectSection(); await gu.getCellRC(0, 3).click(); - await gu.waitAppFocus(true); + await gu.waitAppFocus(); await gu.sendKeys('blue', $.ENTER); await gu.getCellRC(1, 3).click(); await gu.sendKeys('green', $.ENTER); diff --git a/test/nbrowser/gristUtil-nbrowser.js b/test/nbrowser/gristUtil-nbrowser.js index 8f535f6fcb..2630e69081 100644 --- a/test/nbrowser/gristUtil-nbrowser.js +++ b/test/nbrowser/gristUtil-nbrowser.js @@ -373,7 +373,7 @@ const gu = { await driver.sleep(1000); // For each value, type it, followed by Tab. for (const [i, value] of values.entries()) { - await gu.waitAppFocus(true); + await gu.waitAppFocus(); await gu.sendKeys(value, $.TAB); await gu.waitForServer(); if (i === 0) { From a7d68f83d1ab554bd82df4c1243720b74392f506 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Thu, 27 Mar 2025 11:20:51 +0100 Subject: [PATCH 14/20] (tests) fix flaky tests when adding columns or pages waits were already done here and there in the code but not everywhere, making tests breaking randomly when opening dropdowns --- test/nbrowser/CellColor.ts | 4 ++-- test/nbrowser/CustomWidgetsConfig.ts | 2 +- test/nbrowser/DescriptionColumn.ts | 2 +- test/nbrowser/Importer.ts | 2 +- test/nbrowser/TextEditor.ntest.js | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/nbrowser/CellColor.ts b/test/nbrowser/CellColor.ts index 62bc242a1f..f21f3541af 100644 --- a/test/nbrowser/CellColor.ts +++ b/test/nbrowser/CellColor.ts @@ -463,7 +463,7 @@ describe('CellColor', function() { it('should handle correctly default text color', async function() { // Create new checkbox column await driver.find('.mod-add-column').click(); - await driver.find('.test-new-columns-menu-add-new').click(); + await driver.findWait('.test-new-columns-menu-add-new', 100).click(); await gu.waitForServer(); await gu.setType(/Toggle/); @@ -527,7 +527,7 @@ describe('CellColor', function() { // create a new checkbox column await driver.find('.mod-add-column').click(); - await driver.find('.test-new-columns-menu-add-new').click(); + await driver.findWait('.test-new-columns-menu-add-new', 100).click(); await gu.waitForServer(); await gu.setType(/Toggle/); diff --git a/test/nbrowser/CustomWidgetsConfig.ts b/test/nbrowser/CustomWidgetsConfig.ts index 00f6017499..72e451540a 100644 --- a/test/nbrowser/CustomWidgetsConfig.ts +++ b/test/nbrowser/CustomWidgetsConfig.ts @@ -298,7 +298,7 @@ describe('CustomWidgetsConfig', function () { }); // Make sure the widget is now visible. - assert.isTrue(await driver.find('.test-custom-widget-ready').isDisplayed()); + assert.isTrue(await driver.findWait('.test-custom-widget-ready', 1000).isDisplayed()); // And we see widget with info about mapped columns, Column to A. assert.deepEqual(await widget.onRecordsMappings(), {Column: 'A'}); diff --git a/test/nbrowser/DescriptionColumn.ts b/test/nbrowser/DescriptionColumn.ts index 77c2298e8c..3901241b66 100644 --- a/test/nbrowser/DescriptionColumn.ts +++ b/test/nbrowser/DescriptionColumn.ts @@ -649,7 +649,7 @@ async function clickAddDescription() { async function addColumn() { await driver.find(".mod-add-column").click(); - await driver.find('.test-new-columns-menu-add-new').click(); + await driver.findWait('.test-new-columns-menu-add-new', 100).click(); await gu.waitForServer(); } diff --git a/test/nbrowser/Importer.ts b/test/nbrowser/Importer.ts index 282348d722..73a7fbadc7 100644 --- a/test/nbrowser/Importer.ts +++ b/test/nbrowser/Importer.ts @@ -325,7 +325,7 @@ describe('Importer', function() { // Add a new column, with a formula to examine the first. await gu.openColumnMenu('Birthday', 'Insert column to the right'); - await driver.find('.test-new-columns-menu-add-new').click(); + await driver.findWait('.test-new-columns-menu-add-new', 100).click(); await gu.waitForServer(); await driver.sendKeys(Key.ESCAPE); await gu.getCell({col: 2, rowNum: 1}).click(); diff --git a/test/nbrowser/TextEditor.ntest.js b/test/nbrowser/TextEditor.ntest.js index ad418e4811..3973787da4 100644 --- a/test/nbrowser/TextEditor.ntest.js +++ b/test/nbrowser/TextEditor.ntest.js @@ -133,7 +133,7 @@ describe('TextEditor.ntest', function() { async function addColumnRightOf(index) { // Add a column. We have to hover over the column header first. await gu.openColumnMenu({col: index}, 'Insert column to the right'); - await driver.find('.test-new-columns-menu-add-new').click(); + await driver.findWait('.test-new-columns-menu-add-new', 100).click(); await gu.waitForServer(); await gu.sendKeys($.ESCAPE); } From 4dae191040d84869b6e9219d84656ada6d7a22c5 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Thu, 27 Mar 2025 11:41:57 +0100 Subject: [PATCH 15/20] (tests) fix import flaky tests (dropdown menus) some tests break because we want to click too early on elements not there yet, because they appear in dropdown menus right after clicking on buttons showing them. just wait a bit --- test/nbrowser/ImportReferences.ts | 4 +-- test/nbrowser/Importer.ts | 47 ++++++++++++++++++------------- test/nbrowser/Importer2.ts | 28 +++++++++--------- 3 files changed, 45 insertions(+), 34 deletions(-) diff --git a/test/nbrowser/ImportReferences.ts b/test/nbrowser/ImportReferences.ts index 172d9e431a..664bc37704 100644 --- a/test/nbrowser/ImportReferences.ts +++ b/test/nbrowser/ImportReferences.ts @@ -124,7 +124,7 @@ describe('ImportReferences', function() { // PName (a reference column) does. await openSourceMenu('Label'); assert.equal(await findColumnMenuItem('PIndex').isPresent(), true); - assert.equal(await findColumnMenuItem(/as row ID/).isPresent(), false); + assert.equal(await driver.findContent('.test-importer-column-match-menu-item', /as row ID/).isPresent(), false); await driver.sendKeys(Key.ESCAPE); await openSourceMenu('PName'); @@ -208,5 +208,5 @@ async function mapper(el: WebElement) { } function findColumnMenuItem(label: RegExp|string) { - return driver.findContent('.test-importer-column-match-menu-item', label); + return driver.findContentWait('.test-importer-column-match-menu-item', label, 100); } diff --git a/test/nbrowser/Importer.ts b/test/nbrowser/Importer.ts index 73a7fbadc7..40837f0dcc 100644 --- a/test/nbrowser/Importer.ts +++ b/test/nbrowser/Importer.ts @@ -461,9 +461,10 @@ describe('Importer', function() { // Select a merge field, and check that the red outline is gone. await driver.find('.test-importer-merge-fields-select').click(); - await driver.findContent( + await driver.findContentWait( '.test-multi-select-menu .test-multi-select-menu-option', - /Name/ + /Name/, + 100 ).click(); assert.match( await driver.find('.test-importer-merge-fields-select').getCssValue('border'), @@ -518,9 +519,10 @@ describe('Importer', function() { await waitForColumnMapping(); await driver.find('.test-importer-update-existing-records').click(); await driver.find('.test-importer-merge-fields-select').click(); - await driver.findContent( + await driver.findContentWait( '.test-multi-select-menu .test-multi-select-menu-option', - /Name/ + /Name/, + 100 ).click(); await driver.findContent( '.test-multi-select-menu .test-multi-select-menu-option', @@ -599,13 +601,15 @@ describe('Importer', function() { // Now pick the merge fields, and check that the preview diff looks correct. await driver.find('.test-importer-merge-fields-select').click(); - await driver.findContent( + await driver.findContentWait( '.test-multi-select-menu .test-multi-select-menu-option', - /Name/ + /Name/, + 100 ).click(); - await driver.findContent( + await driver.findContentWait( '.test-multi-select-menu .test-multi-select-menu-option', - /Phone/ + /Phone/, + 100 ).click(); await gu.sendKeys(Key.ESCAPE); @@ -634,9 +638,10 @@ describe('Importer', function() { await waitForColumnMapping(); await driver.find('.test-importer-update-existing-records').click(); await driver.find('.test-importer-merge-fields-select').click(); - await driver.findContent( + await driver.findContentWait( '.test-multi-select-menu .test-multi-select-menu-option', - /CourseId/ + /CourseId/, + 100 ).click(); // Close the merge fields menu. @@ -724,9 +729,10 @@ describe('Importer', function() { await waitForColumnMapping(); await driver.find('.test-importer-update-existing-records').click(); await driver.find('.test-importer-merge-fields-select').click(); - await driver.findContent( + await driver.findContentWait( '.test-multi-select-menu .test-multi-select-menu-option', - /Name/ + /Name/, + 100 ).click(); await driver.findContent( '.test-multi-select-menu .test-multi-select-menu-option', @@ -758,9 +764,10 @@ describe('Importer', function() { await waitForColumnMapping(); await driver.find('.test-importer-update-existing-records').click(); await driver.find('.test-importer-merge-fields-select').click(); - await driver.findContent( + await driver.findContentWait( '.test-multi-select-menu .test-multi-select-menu-option', - /Code/ + /Code/, + 100 ).click(); await gu.sendKeys(Key.ESCAPE); @@ -788,9 +795,10 @@ describe('Importer', function() { await waitForColumnMapping(); await driver.find('.test-importer-update-existing-records').click(); await driver.find('.test-importer-merge-fields-select').click(); - await driver.findContent( + await driver.findContentWait( '.test-multi-select-menu .test-multi-select-menu-option', - /Country/ + /Country/, + 100 ).click(); await driver.findContent( '.test-multi-select-menu .test-multi-select-menu-option', @@ -908,9 +916,10 @@ describe('Importer', function() { // Select 'CourseId' as the merge column, and check that the preview now contains a diff of old/new values. await driver.find('.test-importer-merge-fields-select').click(); - await driver.findContent( + await driver.findContentWait( '.test-multi-select-menu .test-multi-select-menu-option', - /CourseId/ + /CourseId/, + 100 ).click(); await gu.sendKeys(Key.ESCAPE); await gu.waitForServer(); @@ -950,7 +959,7 @@ describe('Importer', function() { await waitForDiffPreviewToLoad(); await driver.findContent('.test-importer-column-match-source-destination', /CourseId/) .find('.test-importer-column-match-formula').click(); - await driver.find('.test-importer-apply-formula').click(); + await driver.findWait('.test-importer-apply-formula', 100).click(); await gu.sendKeys(' + "-NEW"'); // Before saving the formula, check that the preview isn't showing the hidden helper column ids. diff --git a/test/nbrowser/Importer2.ts b/test/nbrowser/Importer2.ts index 43a74d396e..5dccee396f 100644 --- a/test/nbrowser/Importer2.ts +++ b/test/nbrowser/Importer2.ts @@ -223,9 +223,10 @@ describe('Importer2', function() { await waitForColumnMapping(); await driver.find('.test-importer-update-existing-records').click(); await driver.find('.test-importer-merge-fields-select').click(); - await driver.findContent( + await driver.findContentWait( '.test-multi-select-menu .test-multi-select-menu-option', - /CourseId/ + /CourseId/, + 100 ).click(); await gu.sendKeys(Key.ESCAPE); await gu.waitForServer(); @@ -458,7 +459,7 @@ describe('Importer2', function() { ); // Click 'Skip', and check that the column mapping section and preview both updated. - await menu.findContent('.test-importer-column-match-menu-item', /Skip/).click(); + await menu.findContentWait('.test-importer-column-match-menu-item', /Skip/, 100).click(); await gu.waitForServer(); assert.deepEqual(await getColumnMatchingRows(), [ { destination: 'Name', source: 'Name' }, @@ -484,7 +485,7 @@ describe('Importer2', function() { // Click Country in the column mapping section, and clear the formula. await driver.findContent('.test-importer-column-match-source', /Country/).click(); - await driver.find('.test-importer-apply-formula').click(); + await driver.findWait('.test-importer-apply-formula', 100).click(); await gu.sendKeys(await gu.selectAllKey(), Key.DELETE, Key.ENTER); await gu.waitForServer(); @@ -516,7 +517,7 @@ describe('Importer2', function() { // We want to map the same column twice, which is not possible through the menu, so we will // use the formula. - await driver.find('.test-importer-apply-formula').click(); + await driver.findWait('.test-importer-apply-formula', 100).click(); await gu.sendKeys(await gu.selectAllKey(), Key.DELETE, '$Population', Key.ENTER); await gu.waitForServer(); assert.deepEqual(await getColumnMatchingRows(), [ @@ -541,7 +542,7 @@ describe('Importer2', function() { // Click Country (with formula 'Skip') in the column mapping section, and start typing a formula. await openSourceFor(/Country/); - await driver.find('.test-importer-apply-formula').click(); + await driver.findWait('.test-importer-apply-formula', 100).click(); await gu.sendKeys('$'); await gu.waitForServer(); @@ -593,7 +594,7 @@ describe('Importer2', function() { it('should reflect mappings when import to new table is finished', async function() { // Skip 'Population', so that we can test imports with skipped columns. await openSourceFor(/Population/); - await driver.findContent('.test-importer-column-match-menu-item', 'Skip').click(); + await driver.findContentWait('.test-importer-column-match-menu-item', 'Skip', 100).click(); await gu.waitForServer(); // Finish importing, and check that the destination tables have the correct data. @@ -681,14 +682,14 @@ describe('Importer2', function() { // Set formula for 'Name' to 'city_name' by typing in the formula. await openSourceFor(/Name/); - await driver.find('.test-importer-apply-formula').click(); + await driver.findWait('.test-importer-apply-formula', 100).click(); await gu.sendKeys('$city_name', Key.ENTER); await gu.waitForServer(); // Map 'District' to 'city_district' via the column mapping menu. await openSourceFor('District'); const menu = gu.findOpenMenu(); - await menu.findContent('.test-importer-column-match-menu-item', /city_district/).click(); + await menu.findContentWait('.test-importer-column-match-menu-item', /city_district/, 100).click(); await gu.waitForServer(); // Check the column mapping section and preview both updated correctly. @@ -715,9 +716,10 @@ describe('Importer2', function() { // Now toggle 'Update existing records', and merge on 'Name' and 'District'. await driver.find('.test-importer-update-existing-records').click(); await driver.find('.test-importer-merge-fields-select').click(); - await driver.findContent( + await driver.findContentWait( '.test-multi-select-menu .test-multi-select-menu-option', - /Name/ + /Name/, + 100 ).click(); await driver.findContent( '.test-multi-select-menu .test-multi-select-menu-option', @@ -764,13 +766,13 @@ describe('Importer2', function() { // we don't overwrite any values in the destination table. (A previous bug caused non-text // skipped columns to overwrite data with default values, like 0.) await openSourceFor(/Population/); - await driver.find('.test-importer-apply-formula').click(); + await driver.findWait('.test-importer-apply-formula', 100).click(); await gu.sendKeys('$city_pop', Key.ENTER); await gu.waitForServer(); // For "Pop. '000", deliberately map a duplicate column (so we can later check if import succeeded). await openSourceFor(/Pop\. '000/); - await driver.find('.test-importer-apply-formula').click(); + await driver.findWait('.test-importer-apply-formula', 100).click(); await gu.sendKeys('$city_pop', Key.ENTER); await gu.waitForServer(); From a2b8a6da7af3b12d06948000bda4b4d6e73cdc8d Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 23 Apr 2025 13:34:53 +0200 Subject: [PATCH 16/20] (kb) change where we initialize RegionFocusSwitcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - now the RFS is created in the App, I guess it makes more sense here, because it's also where the Clipboard is, and now we can more easilly access it across the app if needed… - …but I have a whole mess when initializing now (more than before), to make sure panelAttrs are applied correctly after the grist doc loaded… i'm sure this can be improved somehow? - no more dirty window.regionFocusSwitcher --- app/client/components/Clipboard.js | 3 -- app/client/components/RegionFocusSwitcher.ts | 40 ++++++++++++++++---- app/client/ui/AccountPage.ts | 5 ++- app/client/ui/AdminPanel.ts | 5 ++- app/client/ui/App.ts | 9 ++++- app/client/ui/AppUI.ts | 12 +++--- app/client/ui/AuditLogsPage.ts | 5 ++- app/client/ui/PagePanels.ts | 24 +++++------- app/client/ui/WelcomePage.ts | 5 ++- 9 files changed, 71 insertions(+), 37 deletions(-) diff --git a/app/client/components/Clipboard.js b/app/client/components/Clipboard.js index 90a057b1c3..b3eeceaedf 100644 --- a/app/client/components/Clipboard.js +++ b/app/client/components/Clipboard.js @@ -77,9 +77,6 @@ function Clipboard(app) { onDefaultFocus: () => { this.copypasteField.value = ' '; this.copypasteField.select(); - if (window.gristRegionFocusSwitcher) { - window.gristRegionFocusSwitcher.focusActiveSection(); - } this._app.trigger('clipboard_focus'); }, onDefaultBlur: () => { diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 2b90f2ced5..99aade7927 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -1,10 +1,11 @@ -import {Disposable, dom, Observable, styled} from 'grainjs'; +import {Disposable, dom, Listener, Observable, styled} from 'grainjs'; import {mod} from 'app/common/gutil'; import {SpecialDocPage} from 'app/common/gristUrls'; import isEqual from 'lodash/isEqual'; import {makeT} from 'app/client/lib/localization'; import * as commands from 'app/client/components/commands'; import {triggerFocusGrab} from 'app/client/components/Clipboard'; +import {App} from 'app/client/ui/App'; import {GristDoc} from 'app/client/components/GristDoc'; import {theme} from 'app/client/ui2018/cssVars'; import BaseView from 'app/client/components/BaseView'; @@ -31,7 +32,8 @@ type UpdateInitiator = {type: 'cycle'} | {type: 'mouse', event?: MouseEvent}; export class RegionFocusSwitcher extends Disposable { // Currently focused region public readonly current: Observable; - private readonly _gristDocObs?: Observable; + + private _gristDocObs: Observable; private _currentUpdateInitiator?: UpdateInitiator; // Previously focused elements for each panel (not used for view section ids) private _prevFocusedElements: Record = { @@ -41,18 +43,30 @@ export class RegionFocusSwitcher extends Disposable { main: null, }; - constructor(gristDocObs?: Observable) { + private _initiated: Observable; + private _initListener?: Listener; + + constructor(private _app: App) { super(); this.current = Observable.create(this, undefined); - if (gristDocObs) { - this._gristDocObs = gristDocObs; - } + this._initiated = Observable.create(this, false); } public init(pageContainer: HTMLElement) { - // if we have a grist doc, wait for the current grist doc to be ready before doing anything + if (this._initiated.get()) { + if (this._initListener && !this._initListener.isDisposed()) { + this._initListener.dispose(); + } + return; + } + + if (this._app.pageModel?.gristDoc) { + this._gristDocObs = this._app.pageModel.gristDoc; + } + + // if we have a grist doc, wait for it to be ready before doing anything if (this._gristDocObs && this._gristDocObs.get() === null) { - this._gristDocObs.addListener((doc, prevDoc) => { + this._initListener = this._gristDocObs.addListener((doc, prevDoc) => { if (doc && prevDoc === null) { doc.regionFocusSwitcher = this; this.init(pageContainer); @@ -70,11 +84,17 @@ export class RegionFocusSwitcher extends Disposable { this.autoDispose(this.current.addListener(this._onCurrentUpdate.bind(this))); + const focusActiveSection = () => this.focusActiveSection(); + this._app.on('clipboard_focus', focusActiveSection); + this.onDispose(() => this._app.off('clipboard_focus', focusActiveSection)); + if (this._gristDocObs) { const onClick = this._onClick.bind(this); pageContainer.addEventListener('mouseup', onClick); this.onDispose(() => pageContainer.removeEventListener('mouseup', onClick)); } + + this._initiated.set(true); } public focusRegion( @@ -114,6 +134,7 @@ export class RegionFocusSwitcher extends Disposable { dom.attr('aria-label', ariaLabel), dom.attr(ATTRS.regionId, id), dom.cls('kb-focus-highlighter-group', use => { + use(this._initiated); // highlight focused elements everywhere except in the grist doc views if (id !== 'main') { return true; @@ -128,6 +149,7 @@ export class RegionFocusSwitcher extends Disposable { return isSpecialPage(gristDoc); }), dom.cls('clipboard_group_focus', use => { + use(this._initiated); const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; // if we are not on a grist doc, whole page is always focusable if (!gristDoc) { @@ -145,6 +167,7 @@ export class RegionFocusSwitcher extends Disposable { return isSpecialPage(gristDoc); }), dom.cls('clipboard_forbid_focus', use => { + use(this._initiated); // forbid focus only on the main panel when on an actual document view (and not a special page) if (id !== 'main') { return false; @@ -162,6 +185,7 @@ export class RegionFocusSwitcher extends Disposable { return true; }), dom.cls(`${cssFocusedPanel.className}-focused`, use => { + use(this._initiated); const current = use(this.current); return this._currentUpdateInitiator?.type === 'cycle' && current?.type === 'panel' && current.id === id; }), diff --git a/app/client/ui/AccountPage.ts b/app/client/ui/AccountPage.ts index e68be43e08..710165f218 100644 --- a/app/client/ui/AccountPage.ts +++ b/app/client/ui/AccountPage.ts @@ -1,6 +1,7 @@ import {detectCurrentLang, makeT} from 'app/client/lib/localization'; import {checkName} from 'app/client/lib/nameUtils'; import {AppModel, reportError} from 'app/client/models/AppModel'; +import {App} from 'app/client/ui/App'; import {urlState} from 'app/client/models/gristUrlState'; import * as css from 'app/client/ui/AccountPageCss'; import {ApiKey} from 'app/client/ui/ApiKey'; @@ -39,7 +40,7 @@ export class AccountPage extends Disposable { private _allowGoogleLogin = Computed.create(this, (use) => use(this._userObs)?.allowGoogleLogin ?? false) .onWrite((val) => this._updateAllowGooglelogin(val)); - constructor(private _appModel: AppModel) { + constructor(private _appModel: AppModel, private _appObj: App) { super(); this._setPageTitle(); this._fetchAll().catch(reportError); @@ -58,6 +59,8 @@ export class AccountPage extends Disposable { headerMain: this._buildHeaderMain(), contentMain: this._buildContentMain(), testId, + }, { + regionFocusSwitcher: this._appObj.regionFocusSwitcher, }); } diff --git a/app/client/ui/AdminPanel.ts b/app/client/ui/AdminPanel.ts index 8ea49fa378..ff86d4cdc7 100644 --- a/app/client/ui/AdminPanel.ts +++ b/app/client/ui/AdminPanel.ts @@ -4,6 +4,7 @@ import {localStorageJsonObs} from 'app/client/lib/localStorageObs'; import {getTimeFromNow} from 'app/client/lib/timeUtils'; import {AdminChecks, probeDetails, ProbeDetails} from 'app/client/models/AdminChecks'; import {AppModel, getHomeUrl, reportError} from 'app/client/models/AppModel'; +import {App} from 'app/client/ui/App'; import {AuditLogsModel} from 'app/client/models/AuditLogsModel'; import {urlState} from 'app/client/models/gristUrlState'; import {showEnterpriseToggle} from 'app/client/ui/ActivationPage'; @@ -35,7 +36,7 @@ const t = makeT('AdminPanel'); export class AdminPanel extends Disposable { private _page = Computed.create(this, (use) => use(urlState().state).adminPanel || 'admin'); - constructor(private _appModel: AppModel) { + constructor(private _appModel: AppModel, private _appObj: App) { super(); document.title = getAdminPanelName() + getPageTitleSuffix(getGristConfig()); } @@ -47,6 +48,8 @@ export class AdminPanel extends Disposable { headerMain: this._buildMainHeader(pageObs), contentTop: buildHomeBanners(this._appModel), contentMain: this._buildMainContent(), + }, { + regionFocusSwitcher: this._appObj.regionFocusSwitcher, }); } diff --git a/app/client/ui/App.ts b/app/client/ui/App.ts index 996d5c3b5e..3eb0f8b4b7 100644 --- a/app/client/ui/App.ts +++ b/app/client/ui/App.ts @@ -1,5 +1,7 @@ import {ClientScope} from 'app/client/components/ClientScope'; import * as Clipboard from 'app/client/components/Clipboard'; +import {RegionFocusSwitcher} from 'app/client/components/RegionFocusSwitcher'; +import {KeyboardFocusHighlighter} from 'app/client/components/KeyboardFocusHighlighter'; import {Comm} from 'app/client/components/Comm'; import * as commandList from 'app/client/components/commandList'; import * as commands from 'app/client/components/commands'; @@ -24,7 +26,6 @@ import {dom} from 'grainjs'; import * as ko from 'knockout'; import {makeT} from 'app/client/lib/localization'; import { onClickOutside } from 'app/client/lib/domUtils'; -import {KeyboardFocusHighlighter} from 'app/client/components/KeyboardFocusHighlighter'; const t = makeT('App'); @@ -39,6 +40,7 @@ export interface App extends DisposableWithEvents { features: ko.Computed; topAppModel: TopAppModel; pageModel?: DocPageModel; + regionFocusSwitcher?: RegionFocusSwitcher; } /** @@ -56,6 +58,8 @@ export class AppImpl extends DisposableWithEvents implements App { // Track the most recently created DocPageModel, for some error handling. public pageModel?: DocPageModel; + public regionFocusSwitcher?: RegionFocusSwitcher; + private _settings: ko.Observable<{features?: ISupportedFeatures}>; // Track the version of the server we are communicating with, so that if it changes @@ -76,9 +80,10 @@ export class AppImpl extends DisposableWithEvents implements App { this._settings = ko.observable({}); this.features = ko.computed(() => this._settings().features || {}); - this.autoDispose(new KeyboardFocusHighlighter()); + KeyboardFocusHighlighter.create(this); if (isDesktop()) { + this.regionFocusSwitcher = RegionFocusSwitcher.create(this, this); this.autoDispose(Clipboard.create(this)); } else { // On mobile, we do not want to keep focus on a special textarea (which would cause unwanted diff --git a/app/client/ui/AppUI.ts b/app/client/ui/AppUI.ts index e358aad337..5773e4280b 100644 --- a/app/client/ui/AppUI.ts +++ b/app/client/ui/AppUI.ts @@ -83,15 +83,15 @@ function createMainPage(appModel: AppModel, appObj: App) { } else if (pageType === 'billing') { return domAsync(loadBillingPage().then(bp => dom.create(bp.BillingPage, appModel))); } else if (pageType === 'welcome') { - return dom.create(WelcomePage, appModel); + return dom.create(WelcomePage, appModel, appObj); } else if (pageType === 'account') { - return domAsync(loadAccountPage().then(ap => dom.create(ap.AccountPage, appModel))); + return domAsync(loadAccountPage().then(ap => dom.create(ap.AccountPage, appModel, appObj))); } else if (pageType === 'admin') { - return domAsync(loadAdminPanel().then(m => dom.create(m.AdminPanel, appModel))); + return domAsync(loadAdminPanel().then(m => dom.create(m.AdminPanel, appModel, appObj))); } else if (pageType === 'activation') { return domAsync(loadActivationPage().then(ap => dom.create(ap.getActivationPage(), appModel))); } else if (pageType === 'audit-logs') { - return domAsync(loadAuditLogsPage().then(m => dom.create(m.AuditLogsPage, appModel))); + return domAsync(loadAuditLogsPage().then(m => dom.create(m.AuditLogsPage, appModel, appObj))); } else { return dom.create(pagePanelsDoc, appModel, appObj); } @@ -128,6 +128,8 @@ function pagePanelsHome(owner: IDisposableOwner, appModel: AppModel, app: App) { contentMain: createDocMenu(pageModel), contentTop: buildHomeBanners(appModel), testId, + }, { + regionFocusSwitcher: app.regionFocusSwitcher, }); } @@ -185,6 +187,6 @@ function pagePanelsDoc(owner: IDisposableOwner, appModel: AppModel, appObj: App) contentBottom: dom.create(createBottomBarDoc, pageModel, leftPanelOpen, rightPanelOpen), banner: dom.create(ViewAsBanner, pageModel), }, { - gristDoc: pageModel.gristDoc, + regionFocusSwitcher: appObj.regionFocusSwitcher, }); } diff --git a/app/client/ui/AuditLogsPage.ts b/app/client/ui/AuditLogsPage.ts index dee8fc6083..1116ca539c 100644 --- a/app/client/ui/AuditLogsPage.ts +++ b/app/client/ui/AuditLogsPage.ts @@ -1,6 +1,7 @@ import { buildHomeBanners } from "app/client/components/Banners"; import { makeT } from "app/client/lib/localization"; import { AppModel, reportError } from "app/client/models/AppModel"; +import { App } from "app/client/ui/App"; import { AuditLogsModel } from "app/client/models/AuditLogsModel"; import { urlState } from "app/client/models/gristUrlState"; import { AppHeader } from "app/client/ui/AppHeader"; @@ -40,7 +41,7 @@ export class AuditLogsPage extends Disposable { (_use, s) => s.auditLogs ); - constructor(private _appModel: AppModel) { + constructor(private _appModel: AppModel, private _appObj: App) { super(); this._setTitle(); } @@ -73,6 +74,8 @@ export class AuditLogsPage extends Disposable { headerMain: this._buildHeader(), contentTop: buildHomeBanners(this._appModel), contentMain: this._buildContent(), + }, { + regionFocusSwitcher: this._appObj.regionFocusSwitcher, }); } diff --git a/app/client/ui/PagePanels.ts b/app/client/ui/PagePanels.ts index c3e95e4708..f05757f48a 100644 --- a/app/client/ui/PagePanels.ts +++ b/app/client/ui/PagePanels.ts @@ -16,7 +16,6 @@ import once from 'lodash/once'; import {SessionObs} from 'app/client/lib/sessionObs'; import debounce from 'lodash/debounce'; import {RegionFocusSwitcher} from 'app/client/components/RegionFocusSwitcher'; -import {GristDoc} from 'app/client/components/GristDoc'; const t = makeT('PagePanels'); @@ -50,10 +49,7 @@ export interface PageContents { } interface PagePanelsOptions { - /** - * If provided, the region kb focus switcher will also cycle through the given doc's widgets. - */ - gristDoc?: Observable; + regionFocusSwitcher?: RegionFocusSwitcher; } export function pagePanels( @@ -69,6 +65,8 @@ export function pagePanels( const bannerHeight = Observable.create(null, 0); const isScreenResizingObs = isScreenResizing(); + const regionFocusSwitcher = options.regionFocusSwitcher; + let lastLeftOpen = left.panelOpen.get(); let lastRightOpen = right?.panelOpen.get() || false; let leftPaneDom: HTMLElement; @@ -77,10 +75,6 @@ export function pagePanels( let contentTopDom: HTMLElement; let onLeftTransitionFinish = noop; - const regionFocusSwitcher = RegionFocusSwitcher.create(null, options.gristDoc); - // @ts-expect-error just dirty code to check an idea. To remove later. - window.gristRegionFocusSwitcher = regionFocusSwitcher; - // When switching to mobile mode, close panels; when switching to desktop, restore the // last desktop state. const sub1 = subscribe(isNarrowScreenObs(), (use, narrow) => { @@ -103,7 +97,7 @@ export function pagePanels( if (narrow) { left.panelOpen.set(false); } - regionFocusSwitcher.reset(); + regionFocusSwitcher?.reset(); }); const pauseSavingLeft = (yesNo: boolean) => { @@ -151,11 +145,11 @@ export function pagePanels( }), cssContentMain( (el) => { - regionFocusSwitcher.init(el); + regionFocusSwitcher?.init(el); }, leftPaneDom = cssLeftPane( testId('left-panel'), - regionFocusSwitcher.panelAttrs('left', t('Main navigation and document settings (left panel)')), + regionFocusSwitcher?.panelAttrs('left', t('Main navigation and document settings (left panel)')), cssOverflowContainer( contentWrapper = cssLeftPanelContainer( cssLeftPaneHeader( @@ -292,7 +286,7 @@ export function pagePanels( cssMainPane( mainHeaderDom = cssTopHeader( testId('top-header'), - regionFocusSwitcher.panelAttrs('top', t('Document header')), + regionFocusSwitcher?.panelAttrs('top', t('Document header')), (left.hideOpener ? null : cssPanelOpener('PanelRight', cssPanelOpener.cls('-open', left.panelOpen), testId('left-opener'), @@ -315,7 +309,7 @@ export function pagePanels( ), cssContentMainPane( - regionFocusSwitcher.panelAttrs('main', t('Main content')), + regionFocusSwitcher?.panelAttrs('main', t('Main content')), page.contentMain, ), @@ -332,7 +326,7 @@ export function pagePanels( rightPaneDom = cssRightPane( testId('right-panel'), - regionFocusSwitcher.panelAttrs('right', t('Creator panel (right panel)')), + regionFocusSwitcher?.panelAttrs('right', t('Creator panel (right panel)')), cssRightPaneHeader( right.header, dom.style('margin-bottom', use => use(bannerHeight) + 'px') diff --git a/app/client/ui/WelcomePage.ts b/app/client/ui/WelcomePage.ts index b56c7fd2cf..3a017fba7d 100644 --- a/app/client/ui/WelcomePage.ts +++ b/app/client/ui/WelcomePage.ts @@ -2,6 +2,7 @@ import { Disposable, dom, domComputed, DomContents, MultiHolder, Observable, sty import { handleSubmit } from "app/client/lib/formUtils"; import { AppModel } from "app/client/models/AppModel"; +import { App } from 'app/client/ui/App'; import { getLoginUrl, getSignupUrl, urlState } from "app/client/models/gristUrlState"; import { AccountWidget } from "app/client/ui/AccountWidget"; import { AppHeader } from 'app/client/ui/AppHeader'; @@ -36,7 +37,7 @@ function handleSubmitForm( export class WelcomePage extends Disposable { - constructor(private _appModel: AppModel) { + constructor(private _appModel: AppModel, private _appObj: App) { super(); } @@ -58,6 +59,8 @@ export class WelcomePage extends Disposable { page === 'teams' ? dom.create(buildWelcomeSitePicker, this._appModel) : this._buildPageContent(page) ), + }, { + regionFocusSwitcher: this._appObj.regionFocusSwitcher, }); } From defe10928898866652c8a6a61820009dfd3c7aa8 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 23 Apr 2025 13:51:20 +0200 Subject: [PATCH 17/20] (kb) regionFocusSwitcher internals: keep everything in state not sure this is a good way to handle that, but it was a bit convoluted to keep track of state change initiator in "_currentUpdateInitiator". Now that it's in the state object it's more logical I think. But i'm not sure of the implications of putting a mouse event object in an observable? --- app/client/components/RegionFocusSwitcher.ts | 72 ++++++++++++-------- 1 file changed, 42 insertions(+), 30 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 99aade7927..ee122e1987 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -22,7 +22,11 @@ interface SectionRegion { id: any // this matches a grist document view section id } type Region = PanelRegion | SectionRegion; -type UpdateInitiator = {type: 'cycle'} | {type: 'mouse', event?: MouseEvent}; +type StateUpdateInitiator = {type: 'cycle'} | {type: 'mouse', event?: MouseEvent}; +interface State { + region?: Region; + initiator?: StateUpdateInitiator; +} /** * RegionFocusSwitcher enables keyboard navigation between app panels and doc widgets. @@ -30,11 +34,10 @@ type UpdateInitiator = {type: 'cycle'} | {type: 'mouse', event?: MouseEvent}; * It also follow mouse clicks to focus panels accordingly. */ export class RegionFocusSwitcher extends Disposable { - // Currently focused region - public readonly current: Observable; + // State with currently focused region + private readonly _state: Observable; private _gristDocObs: Observable; - private _currentUpdateInitiator?: UpdateInitiator; // Previously focused elements for each panel (not used for view section ids) private _prevFocusedElements: Record = { left: null, @@ -48,7 +51,10 @@ export class RegionFocusSwitcher extends Disposable { constructor(private _app: App) { super(); - this.current = Observable.create(this, undefined); + this._state = Observable.create(this, { + region: undefined, + initiator: undefined, + }); this._initiated = Observable.create(this, false); } @@ -82,7 +88,7 @@ export class RegionFocusSwitcher extends Disposable { cancel: this._onEscapeKeypress.bind(this), }, this, true)); - this.autoDispose(this.current.addListener(this._onCurrentUpdate.bind(this))); + this.autoDispose(this._state.addListener(this._onStateChange.bind(this))); const focusActiveSection = () => this.focusActiveSection(); this._app.on('clipboard_focus', focusActiveSection); @@ -99,7 +105,7 @@ export class RegionFocusSwitcher extends Disposable { public focusRegion( region: Region | undefined, - options: {initiator?: UpdateInitiator} = {} + options: {initiator?: StateUpdateInitiator} = {} ) { if (region?.type === 'panel' && !getPanelElement(region.id)) { return; @@ -113,8 +119,7 @@ export class RegionFocusSwitcher extends Disposable { throw new Error('view section id is not supported when no view layout is rendered'); } - this._currentUpdateInitiator = options.initiator; - this.current.set(region); + this._state.set({region, initiator: options.initiator}); } public focusActiveSection() { @@ -124,7 +129,7 @@ export class RegionFocusSwitcher extends Disposable { } } - public reset(initiator?: UpdateInitiator) { + public reset(initiator?: StateUpdateInitiator) { this.focusRegion(undefined, {initiator}); } @@ -155,7 +160,7 @@ export class RegionFocusSwitcher extends Disposable { if (!gristDoc) { return true; } - const current = use(this.current); + const current = use(this._state).region; // on a grist doc, panel content is focusable only if it's the current region if (current?.type === 'panel' && current.id === id) { return true; @@ -186,8 +191,8 @@ export class RegionFocusSwitcher extends Disposable { }), dom.cls(`${cssFocusedPanel.className}-focused`, use => { use(this._initiated); - const current = use(this.current); - return this._currentUpdateInitiator?.type === 'cycle' && current?.type === 'panel' && current.id === id; + const current = use(this._state); + return current.initiator?.type === 'cycle' && current.region?.type === 'panel' && current.region.id === id; }), ]; } @@ -196,7 +201,7 @@ export class RegionFocusSwitcher extends Disposable { const gristDoc = this._getGristDoc(); const cycleRegions = getCycleRegions(gristDoc); this.focusRegion(getSibling( - this.current.get(), + this._state.get().region, cycleRegions, direction, gristDoc @@ -212,7 +217,7 @@ export class RegionFocusSwitcher extends Disposable { * - make sure the internal current region info is set when user clicks on the view layout. */ private _onClick(event: MouseEvent) { - const current = this.current.get(); + const current = this._state.get().region; const gristDoc = this._getGristDoc(); if (!gristDoc) { return; @@ -255,12 +260,12 @@ export class RegionFocusSwitcher extends Disposable { } private _onEscapeKeypress() { - const current = this.current.get(); + const {region: current, initiator} = this._state.get(); // Do nothing if we are not focused on a panel if (current?.type !== 'panel') { return; } - const comesFromKeyboard = this._currentUpdateInitiator?.type === 'cycle'; + const comesFromKeyboard = initiator?.type === 'cycle'; const panelElement = getPanelElement(current.id); const activeElement = document.activeElement; const activeElementIsInPanel = panelElement?.contains(activeElement) && activeElement !== panelElement; @@ -304,31 +309,38 @@ export class RegionFocusSwitcher extends Disposable { this._prevFocusedElements[prev.id] = document.activeElement; } - private _onCurrentUpdate(current: Region | undefined, prev: Region | undefined) { + private _onStateChange(current: State | undefined, prev: State | undefined) { if (isEqual(current, prev)) { return; } + console.log('mhl current update', current); + + const gristDoc = this._getGristDoc(); - const mouseEvent = this._currentUpdateInitiator?.type === 'mouse' - ? this._currentUpdateInitiator.event + const mouseEvent = current?.initiator?.type === 'mouse' + ? current.initiator.event : undefined; removeFocusRings(); removeTabIndexes(); if (!mouseEvent) { - this._savePrevElementState(prev); - if (prev?.type === 'panel') { - blurPanelChild(prev); + this._savePrevElementState(prev?.region); + if (prev?.region?.type === 'panel') { + blurPanelChild(prev.region); } } - const isPanel = current?.type === 'panel'; - const panelElement = isPanel && getPanelElement(current.id); + const isPanel = current?.region?.type === 'panel'; + const panelElement = isPanel && current.region?.id && getPanelElement(current.region.id); // actually focus panel element if using keyboard - if (!mouseEvent && isPanel && panelElement) { - focusPanel(current, this._prevFocusedElements[current.id] as HTMLElement | null, gristDoc); + if (!mouseEvent && isPanel && panelElement && current.region) { + focusPanel( + current.region as PanelRegion, + this._prevFocusedElements[current.region.id as Panel] as HTMLElement | null, + gristDoc + ); } // just make sure view layout commands are disabled if we click on a panel @@ -336,8 +348,8 @@ export class RegionFocusSwitcher extends Disposable { escapeViewLayout(gristDoc, !!(mouseEvent.target as Element)?.closest(`[${ATTRS.regionId}="right"]`)); } - if (current?.type === 'section' && gristDoc) { - focusSection(current, gristDoc); + if (current?.region?.type === 'section' && gristDoc) { + focusSection(current.region, gristDoc); } if (current === undefined && gristDoc) { @@ -356,7 +368,7 @@ export class RegionFocusSwitcher extends Disposable { } private _toggleCreatorPanel() { - const current = this.current.get(); + const current = this._state.get().region; const gristDoc = this._getGristDoc(); if (current?.type === 'panel' && current.id === 'right') { return this.focusRegion(gristDoc From dd5b990bf29c3b5160bcc266c8c35d559c75cce1 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Thu, 24 Apr 2025 15:58:07 +0200 Subject: [PATCH 18/20] (kb) add a first batch of simple tests for the RegionFocusSwitcher more advanced tests are missing, but this is a good start --- app/client/components/RegionFocusSwitcher.ts | 7 +- app/client/ui/PagePanels.ts | 1 + test/nbrowser/RegionFocusSwitcher.ts | 296 +++++++++++++++++++ test/nbrowser/gristUtils.ts | 10 +- 4 files changed, 312 insertions(+), 2 deletions(-) create mode 100644 test/nbrowser/RegionFocusSwitcher.ts diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index ee122e1987..6d8fc4a84e 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -217,7 +217,6 @@ export class RegionFocusSwitcher extends Disposable { * - make sure the internal current region info is set when user clicks on the view layout. */ private _onClick(event: MouseEvent) { - const current = this._state.get().region; const gristDoc = this._getGristDoc(); if (!gristDoc) { return; @@ -228,8 +227,11 @@ export class RegionFocusSwitcher extends Disposable { } const targetRegionId = closestRegion.getAttribute(ATTRS.regionId); const targetsMain = targetRegionId === 'main'; + const current = this._state.get().region; const currentlyInSection = current?.type === 'section'; + console.log('mhl onClick', {event, closestRegion, targetRegionId, current, currentlyInSection}); + if (targetsMain && !currentlyInSection) { this.focusRegion( {type: 'section', id: gristDoc.viewModel.activeSectionId()}, @@ -280,6 +282,9 @@ export class RegionFocusSwitcher extends Disposable { if (comesFromKeyboard) { panelElement?.setAttribute('tabindex', '-1'); panelElement?.focus(); + if (activeElementIsInPanel) { + this._prevFocusedElements[current.id] = null; + } } else { this.reset(); } diff --git a/app/client/ui/PagePanels.ts b/app/client/ui/PagePanels.ts index f05757f48a..fe9ffe3385 100644 --- a/app/client/ui/PagePanels.ts +++ b/app/client/ui/PagePanels.ts @@ -309,6 +309,7 @@ export function pagePanels( ), cssContentMainPane( + testId('main-content'), regionFocusSwitcher?.panelAttrs('main', t('Main content')), page.contentMain, ), diff --git a/test/nbrowser/RegionFocusSwitcher.ts b/test/nbrowser/RegionFocusSwitcher.ts new file mode 100644 index 0000000000..68f3441114 --- /dev/null +++ b/test/nbrowser/RegionFocusSwitcher.ts @@ -0,0 +1,296 @@ +import { assert, driver, Key } from "mocha-webdriver"; +import { describe } from "mocha"; +import * as gu from "test/nbrowser/gristUtils"; +import { setupTestSuite } from "test/nbrowser/testUtils"; + +const isClipboardFocused = () => { + return gu.hasFocus('textarea.copypaste.mousetrap'); +}; + +const isNormalElementFocused = async (containerSelector?: string) => { + const activeElement = await gu.getActiveElement(); + const isException = await activeElement.matches( + '.test-left-panel, .test-top-header, .test-right-panel, .test-main-content, body, textarea.copypaste.mousetrap' + ); + const isInContainer = containerSelector + ? await activeElement.matches(`${containerSelector} *`) + : true; + return !isException && isInContainer; +}; + +/** + * tab twice: if we managed to focus things we consider "normal elements", we assume we can use tab to navigate + */ +const assertTabToNavigate = async (containerSelector?: string) => { + await driver.sendKeys(Key.TAB); + assert.isTrue(await isNormalElementFocused(containerSelector)); + + await driver.sendKeys(Key.TAB); + assert.isTrue(await isNormalElementFocused(containerSelector)); +}; + +const cycle = async (dir: 'forward' | 'backward' = 'forward') => { + const modKey = await gu.modKey(); + const shortcut = dir === 'forward' + ? Key.chord(modKey, 'o') + : Key.chord(modKey, Key.SHIFT, 'O'); + + await gu.sendKeys(shortcut); +}; + +const toggleCreatorPanelFocus = async () => { + const modKey = await gu.modKey(); + await gu.sendKeys(Key.chord(modKey, Key.ALT, 'o')); +}; + +const panelMatchs = { + left: '.test-left-panel', + top: '.test-top-header', + right: '.test-right-panel', + main: '.test-main-content', +}; +const assertPanelFocus = async (panel: 'left' | 'top' | 'right' | 'main', expected: boolean = true) => { + assert.equal(await gu.hasFocus(panelMatchs[panel]), expected); +}; + +const assertSectionFocus = async (sectionId: number, expected: boolean = true) => { + assert.equal(await isClipboardFocused(), expected); + assert.equal(await gu.getSectionId() === sectionId, expected); +}; + +/** + * check if we can do a full cycle through regions with nextRegion/prevRegion commands + * + * `sections` is the number of view sections currently on the page. + */ +const assertCycleThroughRegions = async ({ sections = 1 }: { sections?: number } = {}) => { + await cycle(); + await assertPanelFocus('left'); + + await cycle(); + await assertPanelFocus('top'); + + if (sections) { + let sectionsCount = 0; + while (sectionsCount < sections) { + await cycle(); + assert.isTrue(await isClipboardFocused()); + sectionsCount++; + } + } else { + await cycle(); + await assertPanelFocus('main'); + } + + await cycle(); + await assertPanelFocus('left'); + + if (sections) { + let sectionsCount = 0; + while (sectionsCount < sections) { + await cycle('backward'); + assert.isTrue(await isClipboardFocused()); + sectionsCount++; + } + } else { + await cycle('backward'); + await assertPanelFocus('main'); + } + + await cycle('backward'); + await assertPanelFocus('top'); + + await cycle('backward'); + await assertPanelFocus('left'); +}; + +describe("RegionFocusSwitcher", function () { + this.timeout(60000); + const cleanup = setupTestSuite(); + + it("should tab though elements in non-document pages", async () => { + const session = await gu.session().teamSite.login(); + + await session.loadDocMenu("/"); + await assertTabToNavigate(); + + await gu.openProfileSettingsPage(); + await assertTabToNavigate(); + }); + + it("should keep the active section focused at document page load", async () => { + const session = await gu.session().teamSite.login(); + await session.tempDoc(cleanup, 'Hello.grist'); + + assert.isTrue(await isClipboardFocused()); + assert.equal(await gu.getActiveCell().getText(), 'hello'); + await driver.sendKeys(Key.TAB); + // after pressing tab once, we should be on the [first row, second column]-cell + const secondCellText = await gu.getCell(1, 1).getText(); + const activeCellText = await gu.getActiveCell().getText(); + assert.equal(activeCellText, secondCellText); + assert.isTrue(await isClipboardFocused()); + }); + + it("should cycle through regions with (Shift+)Ctrl+O", async () => { + const session = await gu.session().teamSite.login(); + await session.loadDocMenu("/"); + + await assertCycleThroughRegions({ sections: 0 }); + + await session.tempNewDoc(cleanup); + await assertCycleThroughRegions({ sections: 1 }); + + await gu.addNewSection(/Card List/, /Table1/); + await gu.reloadDoc(); + await assertCycleThroughRegions({ sections: 2 }); + }); + + it("should toggle creator panel with Alt+Ctrl+O", async () => { + const session = await gu.session().teamSite.login(); + await session.tempNewDoc(cleanup); + + const firstSectionId = await gu.getSectionId(); + + // test if shortcut works with one view section: + // press the shortcut two times to focus creator panel, then focus back the view section + await toggleCreatorPanelFocus(); + await assertPanelFocus('right'); + + await toggleCreatorPanelFocus(); + await assertSectionFocus(firstSectionId); + + // add a new section, make sure it's the active section/focus after creation + await gu.addNewSection(/Card List/, /Table1/); + const secondSectionId = await gu.getSectionId(); + await assertSectionFocus(secondSectionId); + + // toggle creator panel again: make sure it goes back to the new section + await toggleCreatorPanelFocus(); + await assertPanelFocus('right'); + + await toggleCreatorPanelFocus(); + await assertSectionFocus(secondSectionId); + + // combine with cycle shortcut: when focus is on a panel, toggling creator panel focuses back the current view + await cycle(); + await assertPanelFocus('left'); + + await toggleCreatorPanelFocus(); + await assertPanelFocus('right'); + + await toggleCreatorPanelFocus(); + await assertSectionFocus(secondSectionId); + + // cycle to previous section and make sure all focus is good + await cycle('backward'); + await assertSectionFocus(firstSectionId); + + await toggleCreatorPanelFocus(); + await assertPanelFocus('right'); + + await toggleCreatorPanelFocus(); + await assertSectionFocus(firstSectionId); + + await toggleCreatorPanelFocus(); + await assertPanelFocus('right'); + + await cycle(); + await assertSectionFocus(secondSectionId); + + await toggleCreatorPanelFocus(); + await toggleCreatorPanelFocus(); + await assertSectionFocus(secondSectionId); + }); + + it("should tab through elements when inside a region", async function() { + const session = await gu.session().teamSite.login(); + await session.tempNewDoc(cleanup); + + await cycle(); + await assertTabToNavigate('.test-left-panel'); + + await cycle(); + await assertTabToNavigate('.test-top-header'); + + await toggleCreatorPanelFocus(); + await assertTabToNavigate('.test-right-panel'); + + await toggleCreatorPanelFocus(); + await driver.sendKeys(Key.TAB); + assert.isTrue(await isClipboardFocused()); + }); + + it("should exit from a region when pressing Esc", async function() { + const session = await gu.session().teamSite.login(); + await session.tempNewDoc(cleanup); + + await cycle(); + await driver.sendKeys(Key.ESCAPE); + await assertPanelFocus('left', false); + assert.isTrue(await isClipboardFocused()); + }); + + it("should remember the last focused element in a panel", async function() { + const session = await gu.session().teamSite.login(); + await session.tempNewDoc(cleanup); + + await cycle(); + await driver.sendKeys(Key.TAB); + assert.isTrue(await isNormalElementFocused('.test-left-panel')); + + await cycle(); // top + await cycle(); // main + await cycle(); // back to left + assert.isTrue(await isNormalElementFocused('.test-left-panel')); + + // when pressing escape in that case, first focus back to the panel… + await driver.sendKeys(Key.ESCAPE); + await assertPanelFocus('left'); + + // … then reset the kb focus as usual + await driver.sendKeys(Key.ESCAPE); + assert.isTrue(await isClipboardFocused()); + }); + + it("should focus a panel-region when clicking an input child element", async function() { + const session = await gu.session().teamSite.login(); + await session.tempNewDoc(cleanup); + + // Click on an input on the top panel + await driver.find('.test-bc-doc').click(); + await driver.sendKeys(Key.TAB); + assert.isTrue(await isNormalElementFocused('.test-top-header')); + + // in that case (mouse click) when pressing esc, we directly focus back to view section + await driver.sendKeys(Key.ESCAPE); + await assertPanelFocus('top', false); + assert.isTrue(await isClipboardFocused()); + }); + + it("should focus a section-region when clicking on it", async function() { + const session = await gu.session().teamSite.login(); + await session.tempNewDoc(cleanup); + + await cycle(); // left + await driver.sendKeys(Key.TAB); + assert.isTrue(await isNormalElementFocused('.test-left-panel')); + + await gu.getActiveCell().click(); + + await assertPanelFocus('left', false); + assert.isTrue(await isClipboardFocused()); + }); + + it("should keep the active section focused when clicking a link or button of a panel-region", async function() { + const session = await gu.session().teamSite.login(); + await session.tempNewDoc(cleanup); + + await gu.enterCell('test'); + await driver.find('.test-undo').click(); + await assertPanelFocus('top', false); + assert.isTrue(await isClipboardFocused()); + }); + + afterEach(() => gu.checkForErrors()); +}); diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 322eb24d40..3e9c3c5de9 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -255,6 +255,10 @@ export async function selectAll() { await driver.executeScript('document.activeElement.select()'); } +export async function getActiveElement() { + return await driver.executeScript('return document.activeElement'); +} + /** * Returns a WebElementPromise for the .viewsection_content element for the section which contains * the given text (case insensitive) content. @@ -1162,6 +1166,10 @@ export async function docMenuImport(filePath: string) { }); } +export async function hasFocus(selector: string): Promise { + return await driver.find(selector).hasFocus(); +} + /** * Wait for the focus to return to the main application, i.e. the special .copypaste element that * normally has it (as opposed to an open cell editor, or a focus in some input or menu). Specify @@ -1175,7 +1183,7 @@ export async function waitAppFocus(yesNo: boolean = true): Promise { * Wait for the focus to be on the first element matching given selector. */ export async function waitForFocus(selector: string): Promise { - await driver.wait(async () => (await driver.find(selector).hasFocus()), 1000); + await driver.wait(async () => (await hasFocus(selector)), 1000); } export async function waitForLabelInput(): Promise { From fa075c2ea18fb3446567f89624b01ec205ceda37 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 13 May 2025 17:06:51 +0200 Subject: [PATCH 19/20] (kb) only activate kb nav when actually using the keyboard Just trying to disable the mouse-specific stuff to see if tests pass or not right now. --- app/client/components/RegionFocusSwitcher.ts | 12 ++++++------ test/nbrowser/RegionFocusSwitcher.ts | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 6d8fc4a84e..31f6395bfd 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -94,11 +94,11 @@ export class RegionFocusSwitcher extends Disposable { this._app.on('clipboard_focus', focusActiveSection); this.onDispose(() => this._app.off('clipboard_focus', focusActiveSection)); - if (this._gristDocObs) { + /*if (this._gristDocObs) { const onClick = this._onClick.bind(this); pageContainer.addEventListener('mouseup', onClick); this.onDispose(() => pageContainer.removeEventListener('mouseup', onClick)); - } + }*/ this._initiated.set(true); } @@ -216,7 +216,7 @@ export class RegionFocusSwitcher extends Disposable { * - if necessary, make it easier to tab through things inside panels by "unfocusing" the view section, * - make sure the internal current region info is set when user clicks on the view layout. */ - private _onClick(event: MouseEvent) { + /*private _onClick(event: MouseEvent) { const gristDoc = this._getGristDoc(); if (!gristDoc) { return; @@ -259,7 +259,7 @@ export class RegionFocusSwitcher extends Disposable { if (!targetsMain && getPanelElement(targetRegionId as Panel) === event.target) { focusPanel(); } - } + }*/ private _onEscapeKeypress() { const {region: current, initiator} = this._state.get(); @@ -558,7 +558,7 @@ const getPanelElementId = (id: Panel): string => { return `[${ATTRS.regionId}="${id}"]`; }; -const isFocusableElement = (el: EventTarget | null): boolean => { +/*const isFocusableElement = (el: EventTarget | null): boolean => { if (!el) { return false; } @@ -569,7 +569,7 @@ const isFocusableElement = (el: EventTarget | null): boolean => { return true; } return false; -}; +};*/ /** * Remove the visual highlight on elements that are styled as focused elements of panels. diff --git a/test/nbrowser/RegionFocusSwitcher.ts b/test/nbrowser/RegionFocusSwitcher.ts index 68f3441114..ff0f941482 100644 --- a/test/nbrowser/RegionFocusSwitcher.ts +++ b/test/nbrowser/RegionFocusSwitcher.ts @@ -253,7 +253,7 @@ describe("RegionFocusSwitcher", function () { assert.isTrue(await isClipboardFocused()); }); - it("should focus a panel-region when clicking an input child element", async function() { + it.skip("should focus a panel-region when clicking an input child element", async function() { const session = await gu.session().teamSite.login(); await session.tempNewDoc(cleanup); @@ -268,7 +268,7 @@ describe("RegionFocusSwitcher", function () { assert.isTrue(await isClipboardFocused()); }); - it("should focus a section-region when clicking on it", async function() { + it.skip("should focus a section-region when clicking on it", async function() { const session = await gu.session().teamSite.login(); await session.tempNewDoc(cleanup); From 27d5180a8688661a45a453b01767d42f7177394d Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 14 May 2025 10:51:57 +0200 Subject: [PATCH 20/20] (kb) fix crash when the topAppModel is reloaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit this is yet another thing showing I don't initialize stuff where I should… help appreciated --- app/client/components/RegionFocusSwitcher.ts | 40 +++++++++++++++----- app/client/ui/AppUI.ts | 2 + 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 31f6395bfd..65f2c6a4b3 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -129,8 +129,8 @@ export class RegionFocusSwitcher extends Disposable { } } - public reset(initiator?: StateUpdateInitiator) { - this.focusRegion(undefined, {initiator}); + public reset() { + this.focusRegion(undefined); } public panelAttrs(id: Panel, ariaLabel: string) { @@ -139,7 +139,11 @@ export class RegionFocusSwitcher extends Disposable { dom.attr('aria-label', ariaLabel), dom.attr(ATTRS.regionId, id), dom.cls('kb-focus-highlighter-group', use => { - use(this._initiated); + const initiated = use(this._initiated); + if (!initiated) { + return false; + } + // highlight focused elements everywhere except in the grist doc views if (id !== 'main') { return true; @@ -154,7 +158,11 @@ export class RegionFocusSwitcher extends Disposable { return isSpecialPage(gristDoc); }), dom.cls('clipboard_group_focus', use => { - use(this._initiated); + const initiated = use(this._initiated); + if (!initiated) { + return false; + } + const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; // if we are not on a grist doc, whole page is always focusable if (!gristDoc) { @@ -172,7 +180,11 @@ export class RegionFocusSwitcher extends Disposable { return isSpecialPage(gristDoc); }), dom.cls('clipboard_forbid_focus', use => { - use(this._initiated); + const initiated = use(this._initiated); + if (!initiated) { + return false; + } + // forbid focus only on the main panel when on an actual document view (and not a special page) if (id !== 'main') { return false; @@ -190,13 +202,26 @@ export class RegionFocusSwitcher extends Disposable { return true; }), dom.cls(`${cssFocusedPanel.className}-focused`, use => { - use(this._initiated); + const initiated = use(this._initiated); + if (!initiated) { + return false; + } + const current = use(this._state); return current.initiator?.type === 'cycle' && current.region?.type === 'panel' && current.region.id === id; }), ]; } + /** + * this is smelly code that is just here because I don't initialize the region focus switcher correctly… + */ + public onAppPageModelUpdate() { + if (this._app.pageModel?.gristDoc) { + this._gristDocObs = this._app.pageModel.gristDoc; + } + } + private _cycle(direction: 'next' | 'prev') { const gristDoc = this._getGristDoc(); const cycleRegions = getCycleRegions(gristDoc); @@ -319,9 +344,6 @@ export class RegionFocusSwitcher extends Disposable { return; } - console.log('mhl current update', current); - - const gristDoc = this._getGristDoc(); const mouseEvent = current?.initiator?.type === 'mouse' ? current.initiator.event diff --git a/app/client/ui/AppUI.ts b/app/client/ui/AppUI.ts index 5773e4280b..46b070bb09 100644 --- a/app/client/ui/AppUI.ts +++ b/app/client/ui/AppUI.ts @@ -139,6 +139,8 @@ function pagePanelsDoc(owner: IDisposableOwner, appModel: AppModel, appObj: App) // DocPageModel available as a global variable. (window as any).gristDocPageModel = pageModel; appObj.pageModel = pageModel; + appObj.regionFocusSwitcher?.onAppPageModelUpdate(); + const leftPanelOpen = createSessionObs(owner, "leftPanelOpen", true, isBoolean); const rightPanelOpen = createSessionObs(owner, "rightPanelOpen", false, isBoolean); const leftPanelWidth = createSessionObs(owner, "leftPanelWidth", 240, isNumber);