From 7ac32892be41aeb3d2e36348ddd08792da3ed13b Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Mon, 10 Feb 2025 15:57:45 +0100 Subject: [PATCH 01/44] (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 efe8f7d1f96d0150901b60d17d26543750e743d3 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 11 Feb 2025 12:32:15 +0100 Subject: [PATCH 02/44] (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 5397a154d8..b17d0dec10 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -266,7 +266,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 eb25ec6fe9..752f3e2279 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 f57f83ee93..5369d4642e 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 @@ -739,6 +741,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 b979342676..bc2b544963 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 c741df87c0..c8ebe54b69 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 535f42dd23537bef8d4719c868eec104ccf398a3 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 25 Mar 2025 17:27:21 +0100 Subject: [PATCH 03/44] (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 60e40617b49b4fea3a92d9a7b655f01cd99c5ac4 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 25 Mar 2025 17:27:32 +0100 Subject: [PATCH 04/44] (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 a2c3ca4b69647773b4e68d68a56c3436a97afd93 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 28 Mar 2025 12:34:46 +0100 Subject: [PATCH 05/44] (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 31e71bd246a0c721db9aa002112336723bc4b529 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 28 Mar 2025 15:46:41 +0100 Subject: [PATCH 06/44] (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 df9f27d3907eef298dd4bb11cccba13d733d28c5 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 16 Apr 2025 17:53:41 +0200 Subject: [PATCH 07/44] (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 c8ebe54b69..5519c94026 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 c28ca5b78c08bc3066ad0ed09a96f1775e321d74 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 18 Apr 2025 17:44:00 +0200 Subject: [PATCH 08/44] (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 8477d87ab6b80de84af75788812616557a014861 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 16 Apr 2025 17:51:06 +0200 Subject: [PATCH 09/44] (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 | 29 +++-- 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, 130 insertions(+), 82 deletions(-) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 1c7263c124..db253a4e8d 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 []; 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 b17d0dec10..5d7ef8605b 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -15,6 +15,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'); @@ -265,8 +266,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(() => @@ -294,6 +297,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 @@ -334,58 +387,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 4734d33211..94fe84f23e 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'; @@ -178,6 +179,7 @@ export interface GristDoc extends DisposableWithEvents { isTimingOn: Observable; attachmentTransfer: Observable; canShowRawData: Observable; + regionFocusSwitcher?: RegionFocusSwitcher; docId(): string; openDocPage(viewId: IDocPage): Promise; @@ -221,6 +223,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 5369d4642e..a3207307b3 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 @@ -748,8 +751,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 d93523fc5283628a7d6b84c4e502e8d2f85ace7a Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 18 Apr 2025 18:11:16 +0200 Subject: [PATCH 10/44] (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 39b751f27de6c14faf5a83617a169528b1ca078f Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 18 Apr 2025 20:28:29 +0200 Subject: [PATCH 11/44] (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 5519c94026..c9a47ae5fe 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 5e82f9c258cdc04e7988f4c0883f6c839cf5463f Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 18 Feb 2025 16:11:02 +0100 Subject: [PATCH 12/44] (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 3105565c14..a15020c6d4 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -686,7 +686,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) } /** @@ -1090,7 +1089,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 @@ -1100,6 +1098,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 e28a9561fd230d531599d84ac47c487bba7c3845 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 18 Apr 2025 17:36:07 +0200 Subject: [PATCH 13/44] (tests) normalize some function calls to find them quicker in codebase --- test/nbrowser/AccessRules1.ts | 2 +- 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 +- 11 files changed, 21 insertions(+), 21 deletions(-) diff --git a/test/nbrowser/AccessRules1.ts b/test/nbrowser/AccessRules1.ts index 268d1e1849..43d1b4ac64 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.waitForFocus('body'); + await gu.waitAppFocus(); }); it('should support dollar syntax in the editor', async function() { 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 8c20c29d02..9beacff1e4 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 59859806416e14620f5d59284319240cf6d4ee30 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Thu, 27 Mar 2025 11:20:51 +0100 Subject: [PATCH 14/44] (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/TextEditor.ntest.js | 2 +- 4 files changed, 5 insertions(+), 5 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/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 2aba22992b0ab40fcfb10b21ce462319aa3e433c Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 27 May 2025 19:42:28 +0200 Subject: [PATCH 15/44] (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/Importer2.ts | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 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/Importer2.ts b/test/nbrowser/Importer2.ts index 48356bd432..3180bda43e 100644 --- a/test/nbrowser/Importer2.ts +++ b/test/nbrowser/Importer2.ts @@ -459,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' }, @@ -485,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(); @@ -517,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(), [ @@ -542,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(); @@ -594,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. @@ -682,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. @@ -758,7 +758,7 @@ 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(); From ab1c7b637b30a10a118a10fd056ee66aa20a2252 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 23 Apr 2025 13:34:53 +0200 Subject: [PATCH 16/44] (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 9b5dc00989..be5dcd4092 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 bc2b544963..4c8bdc2485 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 c9a47ae5fe..3e21eb314e 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 8ac86b16e328fa5d279a7a3bd931b22b1fc35f0d Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 23 Apr 2025 13:51:20 +0200 Subject: [PATCH 17/44] (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 4c4f41ea32549147fecf87e6c57bf650c16a3257 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Thu, 24 Apr 2025 15:58:07 +0200 Subject: [PATCH 18/44] (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 3e21eb314e..53084c3078 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 a15020c6d4..f0c48f9424 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -237,6 +237,10 @@ export async function selectAll() { await driver.executeScript('document.activeElement.select()'); } +export async function getActiveElement() { + return await driver.executeScript('return document.activeElement'); +} + /** * Detaches section from the layout. Used for manipulating sections in the layout. */ @@ -1089,6 +1093,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 @@ -1102,7 +1110,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 1df3e76a7acfeb71b42e122a38979138e2f76725 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 13 May 2025 17:06:51 +0200 Subject: [PATCH 19/44] (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 5f249570afe5d1c9ed7ecf619a845621f44724a9 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 14 May 2025 10:51:57 +0200 Subject: [PATCH 20/44] (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 4c8bdc2485..293cd4a142 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); From 214be2620a67b7534a2ab0485fa6feff9d1c5f8f Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Mon, 26 May 2025 17:04:37 +0200 Subject: [PATCH 21/44] clipboard: no need for the copy/cut/paste commands check This was a bit overkill and even something that made stuff break. The assumption that the clipboard was needed only when copy/cut/paste commands were registered was false. This made the app behave weirdly in some cases. One use case where we understand the assumption is not great: in a grist doc, switch from a grid view to a chart view, click on the empty space of a panel, then go back to a grid view. The chart view has no copy/cut/paste commands registered, so the focus is set back to body when clicking the empty space. Then going back to a grid view and using the kb directly doesn't trigger necessarily the `allowFocus` function. This makes the `input` command break. It's actually not even needed because the RegionFocusSwitcher adds the clipboard_group_focus as needed. --- app/client/components/Clipboard.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/app/client/components/Clipboard.js b/app/client/components/Clipboard.js index b3eeceaedf..e4a76c4db1 100644 --- a/app/client/components/Clipboard.js +++ b/app/client/components/Clipboard.js @@ -49,8 +49,6 @@ var dom = require('../lib/dom'); var Base = require('./Base'); var tableUtil = require('../lib/tableUtil'); -var _ = require('underscore'); - const t = makeT('Clipboard'); function Clipboard(app) { @@ -305,20 +303,15 @@ async function getTextFromClipboardItem(clipboardItem, type) { * * 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 + * - using the 'clipboard_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) { - 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; } From 2c617551c9ebab2f87740b14a3a464f53d204a1b Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Mon, 26 May 2025 17:06:26 +0200 Subject: [PATCH 22/44] regionFocusSwitcher: use one less dom.cls call the clipboard_group_focus and clipboard_forbid_focus dom.cls calls were basically the same thing by reversed; use the dom.cls with a callback directly to apply one or the other --- app/client/components/RegionFocusSwitcher.ts | 38 ++++++-------------- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 65f2c6a4b3..d3a60a713a 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -157,49 +157,33 @@ export class RegionFocusSwitcher extends Disposable { } return isSpecialPage(gristDoc); }), - dom.cls('clipboard_group_focus', use => { + dom.cls(use => { const initiated = use(this._initiated); if (!initiated) { - return false; + return ''; } const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; // if we are not on a grist doc, whole page is always focusable if (!gristDoc) { - return true; + return 'clipboard_group_focus'; } - const current = use(this._state).region; + // on a grist doc, panel content is focusable only if it's the current region + const current = use(this._state).region; if (current?.type === 'panel' && current.id === id) { - return true; + return 'clipboard_group_focus'; } if (gristDoc) { use(gristDoc.activeViewId); } // 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 => { - 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; - } - const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; - if (!gristDoc) { - return false; - } - if (gristDoc) { - use(gristDoc.activeViewId); - } - if (isSpecialPage(gristDoc)) { - return false; + if (id === "main") { + return isSpecialPage(gristDoc) + ? 'clipboard_group_focus' + : 'clipboard_forbid_focus'; } - return true; + return ''; }), dom.cls(`${cssFocusedPanel.className}-focused`, use => { const initiated = use(this._initiated); From 6740eb78819bd15e4bcdc9a717827cc2efae64f7 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Mon, 26 May 2025 17:15:42 +0200 Subject: [PATCH 23/44] views keyboard shortcuts: better comment on active vs focused commands --- app/client/components/BaseView.js | 21 ++++++++++++++++++--- app/client/components/DetailView.js | 4 ++++ app/client/components/GridView.js | 3 +++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index db253a4e8d..13a1e76e56 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -236,7 +236,22 @@ _.extend(Base.prototype, BackboneEvents); /** * These commands are common to GridView and DetailView. * - * They work when the view is the currently active one. + * They work when the view is the currently active one, but not necessarily user-focused. + * + * That means the user can be focusing a button in the creator panel and run these commands: + * they will apply to the active view. + * When a command from here is executed, keyboard focus is set back to the view. + * + * There is no strict rule for which command goes here and which goes in the commonFocusedCommands list. + * The goal of the distinction is to: + * 1) allow users to run most commands easily, without having to think about actually focusing an active view, + * 2) make sure command keyboard shortcuts don't interfere with user keyboard navigation when the user is + * focused on something else. + * The main thing to watch out for is the 2) point. When adding a command, ask yourself if "blocking" the kb shortcut + * when not focusing the view is risky: is the shortcut so generic that it's likely to be used outside of the view, + * for example for navigation? If so, the command should go in the "focused" list. + * Most commands triggered by arrow keys, Tab, Enter, pagination keys, should usually go in the focused list. + * Most commands with relatively hard or specific triggers should usually go in the normal list. */ BaseView.commonCommands = { input: function(init) { @@ -255,7 +270,7 @@ BaseView.commonCommands = { * 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. + * they don't work when the view is the active one but the user is focused on something else, like the creator panel. */ BaseView.commonFocusedCommands = { editField: function(event) { closeRegisteredMenu(); this.scrollToCursor(true); this.activateEditorAtCursor({event}); }, @@ -271,7 +286,7 @@ BaseView.commonFocusedCommands = { * 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 []; diff --git a/app/client/components/DetailView.js b/app/client/components/DetailView.js index 0840685f4d..1f76796131 100644 --- a/app/client/components/DetailView.js +++ b/app/client/components/DetailView.js @@ -156,6 +156,8 @@ DetailView.prototype._updateFloatingRow = function() { /** * DetailView commands, enabled when the view is the active one. + * + * See BaseView.commonCommands for more details. */ DetailView.detailCommands = { editLayout: function() { @@ -174,6 +176,8 @@ DetailView.detailCommands = { /** * DetailView commands, enabled when the view is user-focused. + * + * See BaseView.commonCommands and BaseView.commonFocusedCommands for more details. */ DetailView.detailFocusedCommands = { cursorUp: function() { this.cursor.fieldIndex(this.cursor.fieldIndex() - 1); }, diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index 5d7ef8605b..9584e39949 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -296,6 +296,8 @@ GridView.selectionCommands = { // TODO: move commands with modifications to gridEditCommands and use a single guard for // readonly state. +// GridView commands, enabled when the view is the active one. +// See BaseView.commonCommands for more details. GridView.gridCommands = { fillSelectionDown: function() { tableUtil.fillSelectionDown(this.getSelection(), this.tableModel); }, selectAll: function() { this.selectAll(); }, @@ -346,6 +348,7 @@ GridView.gridCommands = { }; // These commands are enabled only when the grid is the user-focused region. +// See BaseView.commonCommands and BaseView.commonFocusedCommands for more details. GridView.gridFocusedCommands = { cursorDown: function() { if (this.cursor.rowIndex() === this.viewData.peekLength - 1) { From 0e11ba40b704bee647a6b15f78976f890cb4fe9f Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Mon, 26 May 2025 17:46:58 +0200 Subject: [PATCH 24/44] removing testing console log --- app/client/components/GridView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index 9584e39949..902368c57d 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -338,7 +338,7 @@ GridView.gridCommands = { } this.viewSection.rawNumFrozen.setAndSave(action.numFrozen); }, - copy: function() { console.log(this.getSelection()); return this.copy(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; } From da85c9d94931116b45b593d66cf47d284e4d4cb5 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 27 May 2025 15:43:53 +0200 Subject: [PATCH 25/44] (kb) enabling back mouse handling on region focus switcher --- app/client/components/RegionFocusSwitcher.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index d3a60a713a..6333fa2215 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); } @@ -225,7 +225,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; @@ -268,7 +268,7 @@ export class RegionFocusSwitcher extends Disposable { if (!targetsMain && getPanelElement(targetRegionId as Panel) === event.target) { focusPanel(); } - }*/ + } private _onEscapeKeypress() { const {region: current, initiator} = this._state.get(); @@ -564,7 +564,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; } @@ -575,7 +575,7 @@ const getPanelElementId = (id: Panel): string => { return true; } return false; -};*/ +}; /** * Remove the visual highlight on elements that are styled as focused elements of panels. From 5e30d74c14738b7297b5ae1696a3edfa100e0864 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 27 May 2025 15:43:40 +0200 Subject: [PATCH 26/44] (kb) refactoring stuff here and there after dmitry's feedback these are just small changes for now, logic doesn't move much --- app/client/components/RegionFocusSwitcher.ts | 91 ++++++++++++-------- app/client/models/entities/ViewRec.ts | 2 +- 2 files changed, 55 insertions(+), 38 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 6333fa2215..b2f3e6dc79 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -19,7 +19,7 @@ interface PanelRegion { } interface SectionRegion { type: 'section', - id: any // this matches a grist document view section id + id: number // this matches a grist document view section id } type Region = PanelRegion | SectionRegion; type StateUpdateInitiator = {type: 'cycle'} | {type: 'mouse', event?: MouseEvent}; @@ -37,7 +37,7 @@ export class RegionFocusSwitcher extends Disposable { // State with currently focused region private readonly _state: Observable; - private _gristDocObs: Observable; + private _gristDocObs?: Observable; // Previously focused elements for each panel (not used for view section ids) private _prevFocusedElements: Record = { left: null, @@ -185,7 +185,7 @@ export class RegionFocusSwitcher extends Disposable { } return ''; }), - dom.cls(`${cssFocusedPanel.className}-focused`, use => { + cssFocusedPanel.cls('-focused', use => { const initiated = use(this._initiated); if (!initiated) { return false; @@ -239,8 +239,6 @@ export class RegionFocusSwitcher extends Disposable { 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()}, @@ -270,6 +268,13 @@ export class RegionFocusSwitcher extends Disposable { } } + /** + * This is registered as a `cancel` command when the RegionFocusSwitcher is created. + * + * That means this is called when pressing Escape in no particular setting. + * Any `cancel` command registered by other code after loading the page will take precedence over this one. + * So, this doesn't get called when in a modal, a popup menu, etc., as those have their own cancel callback. + */ private _onEscapeKeypress() { const {region: current, initiator} = this._state.get(); // Do nothing if we are not focused on a panel @@ -278,19 +283,28 @@ export class RegionFocusSwitcher extends Disposable { } const comesFromKeyboard = initiator?.type === 'cycle'; const panelElement = getPanelElement(current.id); + if (!panelElement) { + return; + } + + // …Reset region focus switch if already on the panel itself + if (document.activeElement === panelElement) { + this.reset(); + return; + } + const activeElement = document.activeElement; - const activeElementIsInPanel = panelElement?.contains(activeElement) && activeElement !== panelElement; + const activeElementIsInPanel = containsActiveElement(panelElement); if ( // 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. - || (activeElement === document.body && panelElement) + || (activeElement === document.body) ) { if (comesFromKeyboard) { - panelElement?.setAttribute('tabindex', '-1'); - panelElement?.focus(); + focusPanelElement(panelElement); if (activeElementIsInPanel) { this._prevFocusedElements[current.id] = null; } @@ -299,11 +313,6 @@ export class RegionFocusSwitcher extends Disposable { } return; } - - // …Reset region focus switch if already on the panel itself - if (document.activeElement === panelElement) { - this.reset(); - } } /** @@ -315,8 +324,7 @@ export class RegionFocusSwitcher extends Disposable { return; } const prevPanelElement = getPanelElement(prev.id); - const isChildOfPanel = prevPanelElement?.contains(document.activeElement) - && document.activeElement !== prevPanelElement; + const isChildOfPanel = containsActiveElement(prevPanelElement); if (!isChildOfPanel) { return; } @@ -343,32 +351,30 @@ export class RegionFocusSwitcher extends Disposable { } const isPanel = current?.region?.type === 'panel'; - const panelElement = isPanel && current.region?.id && getPanelElement(current.region.id); + const panelElement = isPanel && current.region?.id && getPanelElement((current.region as PanelRegion).id); - // actually focus panel element if using keyboard + // if kb-focusing a panel: actually focus panel element 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 - if (mouseEvent && isPanel && panelElement && gristDoc) { + // if clicking on a panel: just make sure view layout commands are disabled + } else if (mouseEvent && isPanel && panelElement && gristDoc) { escapeViewLayout(gristDoc, !!(mouseEvent.target as Element)?.closest(`[${ATTRS.regionId}="right"]`)); - } - if (current?.region?.type === 'section' && gristDoc) { + // if clicking or kb-focusing a section: focus the section + } else if (current?.region?.type === 'section' && gristDoc) { focusSection(current.region, gristDoc); } - if (current === undefined && gristDoc) { - focusViewLayout(gristDoc); - } - // if we reset the focus switch, clean all necessary state if (current === undefined) { + if (gristDoc) { + focusViewLayout(gristDoc); + } this._prevFocusedElements = { left: null, top: null, @@ -438,15 +444,6 @@ const focusPanel = (panel: PanelRegion, child: HTMLElement | null, gristDoc: Gri if (!panelElement) { return; } - // 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(); - } // Child element found: focus it if (child && child !== panelElement && child.isConnected) { @@ -457,6 +454,9 @@ const focusPanel = (panel: PanelRegion, child: HTMLElement | null, gristDoc: Gri child.removeAttribute(ATTRS.focusedElement); }, {once: true}); child.focus?.(); + } else { + // No child to focus found: just focus the panel + focusPanelElement(panelElement); } if (gristDoc) { @@ -465,6 +465,16 @@ const focusPanel = (panel: PanelRegion, child: HTMLElement | null, gristDoc: Gri } }; +const focusPanelElement = (panelElement: HTMLElement) => { + // tabindex is set here and removed later with removeTabIndexes(), instead of + // directly set on the element on creation, for a reason: + // if we happen to just click on a non-focusable element inside a 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(); +}; + const focusViewLayout = (gristDoc: GristDoc) => { triggerFocusGrab(); gristDoc.viewModel.focusedRegionState('in'); @@ -551,7 +561,7 @@ const getSibling = ( */ const blurPanelChild = (panel: PanelRegion) => { const panelElement = getPanelElement(panel.id); - if (panelElement?.contains(document.activeElement) && document.activeElement !== panelElement) { + if (containsActiveElement(panelElement)) { (document.activeElement as HTMLElement).blur(); } }; @@ -577,6 +587,13 @@ const isFocusableElement = (el: EventTarget | null): boolean => { return false; }; +/** + * Check if the document.activeElement is a child of the given element. + */ +const containsActiveElement = (el: HTMLElement | null): boolean => { + return el?.contains(document.activeElement) && document.activeElement !== el || false; +}; + /** * Remove the visual highlight on elements that are styled as focused elements of panels. */ diff --git a/app/client/models/entities/ViewRec.ts b/app/client/models/entities/ViewRec.ts index 3acdfefb86..d3690639f8 100644 --- a/app/client/models/entities/ViewRec.ts +++ b/app/client/models/entities/ViewRec.ts @@ -22,7 +22,7 @@ export interface ViewRec extends IRowModel<"_grist_Views"> { // 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 + // - 'related' means the currently focused region is not the view but something related to it (e.g. the creator panel) focusedRegionState: ko.Observable<'out' | 'in' | 'related'>; // Saved collapsed sections. From 2435e2735af068958877d46fe7cc7d3caa17f955 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 27 May 2025 16:10:03 +0200 Subject: [PATCH 27/44] (kb) simplify alwaysOn commands code (dmitry's feedback) --- app/client/components/commands.ts | 6 ++++-- app/client/lib/Mousetrap.js | 28 +++++++--------------------- 2 files changed, 11 insertions(+), 23 deletions(-) diff --git a/app/client/components/commands.ts b/app/client/components/commands.ts index 6a18ab3623..aafdc2dfb5 100644 --- a/app/client/components/commands.ts +++ b/app/client/components/commands.ts @@ -256,8 +256,10 @@ export class Command implements CommandDef { // 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]; - const bind = this.alwaysOn ? Mousetrap.bindAlwaysOn : Mousetrap.bind; - bind(key, wrapKeyCallback(commandGroup.commands[commandName], commandName)); + if (this.alwaysOn) { + Mousetrap.markAlwaysOnShortcut(key); + } + Mousetrap.bind(key, wrapKeyCallback(commandGroup.commands[commandName], commandName)); } else { Mousetrap.unbind(key); } diff --git a/app/client/lib/Mousetrap.js b/app/client/lib/Mousetrap.js index 8590fadcfd..5cb655f77c 100644 --- a/app/client/lib/Mousetrap.js +++ b/app/client/lib/Mousetrap.js @@ -36,14 +36,15 @@ if (typeof window === 'undefined') { * elements. See also 'attach' method of commands.CommandGroup. */ MousetrapProtype.stopCallback = function(e, element, combo, sequence) { + if (mousetrapBindingsPaused) { + return true; + } + // 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; - } // If we have a custom stopCallback, use it now. const custom = customStopCallbacks.get(element); if (custom) { @@ -80,24 +81,9 @@ 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(); + Mousetrap.markAlwaysOnShortcut = function(combo) { + alwaysOnCallbacks[combo] = true; + } module.exports = Mousetrap; } From 0a1b7714610131c4166de73977f4532b2eaa5296 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 27 May 2025 16:11:29 +0200 Subject: [PATCH 28/44] (kb) no need for getActiveElement helper (thanks dmitry) --- test/nbrowser/RegionFocusSwitcher.ts | 2 +- test/nbrowser/gristUtils.ts | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/test/nbrowser/RegionFocusSwitcher.ts b/test/nbrowser/RegionFocusSwitcher.ts index ff0f941482..280e5b5625 100644 --- a/test/nbrowser/RegionFocusSwitcher.ts +++ b/test/nbrowser/RegionFocusSwitcher.ts @@ -8,7 +8,7 @@ const isClipboardFocused = () => { }; const isNormalElementFocused = async (containerSelector?: string) => { - const activeElement = await gu.getActiveElement(); + const activeElement = await driver.switchTo().activeElement(); const isException = await activeElement.matches( '.test-left-panel, .test-top-header, .test-right-panel, .test-main-content, body, textarea.copypaste.mousetrap' ); diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index f0c48f9424..e1ede2b35b 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -237,10 +237,6 @@ export async function selectAll() { await driver.executeScript('document.activeElement.select()'); } -export async function getActiveElement() { - return await driver.executeScript('return document.activeElement'); -} - /** * Detaches section from the layout. Used for manipulating sections in the layout. */ From 1cb6447b82dc2fe0d503a80b0ac91ca74493d223 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 27 May 2025 19:43:10 +0200 Subject: [PATCH 29/44] (kb) trying to clean up (yet again) the regionFocusSwitcher init thanks dmitry for the numerous suggestions (still not sure this is all good, wip wip) --- app/client/components/GristDoc.ts | 2 +- app/client/components/RegionFocusSwitcher.ts | 74 ++++---------------- app/client/ui/AccountPage.ts | 3 +- app/client/ui/AdminPanel.ts | 3 +- app/client/ui/App.ts | 2 +- app/client/ui/AppUI.ts | 7 +- app/client/ui/AuditLogsPage.ts | 3 +- app/client/ui/PagePanels.ts | 20 +++--- app/client/ui/WelcomePage.ts | 3 +- 9 files changed, 31 insertions(+), 86 deletions(-) diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index 94fe84f23e..074e1cda7b 100644 --- a/app/client/components/GristDoc.ts +++ b/app/client/components/GristDoc.ts @@ -223,7 +223,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; + public get regionFocusSwitcher() { return this.app.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 b2f3e6dc79..cfea0691f2 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -1,4 +1,4 @@ -import {Disposable, dom, Listener, Observable, styled} from 'grainjs'; +import {Disposable, dom, Observable, styled} from 'grainjs'; import {mod} from 'app/common/gutil'; import {SpecialDocPage} from 'app/common/gristUrls'; import isEqual from 'lodash/isEqual'; @@ -36,8 +36,7 @@ interface State { export class RegionFocusSwitcher extends Disposable { // State with currently focused region private readonly _state: Observable; - - private _gristDocObs?: Observable; + private get _gristDocObs() { return this._app?.pageModel?.gristDoc; } // Previously focused elements for each panel (not used for view section ids) private _prevFocusedElements: Record = { left: null, @@ -46,40 +45,12 @@ export class RegionFocusSwitcher extends Disposable { main: null, }; - private _initiated: Observable; - private _initListener?: Listener; - - constructor(private _app: App) { + constructor(private _app?: App) { super(); this._state = Observable.create(this, { region: undefined, initiator: undefined, }); - this._initiated = Observable.create(this, false); - } - - public init(pageContainer: HTMLElement) { - 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._initListener = this._gristDocObs.addListener((doc, prevDoc) => { - if (doc && prevDoc === null) { - doc.regionFocusSwitcher = this; - this.init(pageContainer); - } - }); - return; - } this.autoDispose(commands.createGroup({ nextRegion: () => this._cycle('next'), @@ -91,16 +62,19 @@ export class RegionFocusSwitcher extends Disposable { this.autoDispose(this._state.addListener(this._onStateChange.bind(this))); const focusActiveSection = () => this.focusActiveSection(); - this._app.on('clipboard_focus', focusActiveSection); - this.onDispose(() => this._app.off('clipboard_focus', focusActiveSection)); + this._app?.on('clipboard_focus', focusActiveSection); + this.onDispose(() => { + this._app?.off('clipboard_focus', focusActiveSection); + this.reset(); + }); + } + public onPageDomLoaded(el: HTMLElement) { if (this._gristDocObs) { const onClick = this._onClick.bind(this); - pageContainer.addEventListener('mouseup', onClick); - this.onDispose(() => pageContainer.removeEventListener('mouseup', onClick)); + el.addEventListener('mouseup', onClick); + this.onDispose(() => el.removeEventListener('mouseup', onClick)); } - - this._initiated.set(true); } public focusRegion( @@ -139,11 +113,6 @@ export class RegionFocusSwitcher extends Disposable { dom.attr('aria-label', ariaLabel), dom.attr(ATTRS.regionId, id), dom.cls('kb-focus-highlighter-group', use => { - const initiated = use(this._initiated); - if (!initiated) { - return false; - } - // highlight focused elements everywhere except in the grist doc views if (id !== 'main') { return true; @@ -158,11 +127,6 @@ export class RegionFocusSwitcher extends Disposable { return isSpecialPage(gristDoc); }), dom.cls(use => { - const initiated = use(this._initiated); - if (!initiated) { - return ''; - } - const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; // if we are not on a grist doc, whole page is always focusable if (!gristDoc) { @@ -186,26 +150,12 @@ export class RegionFocusSwitcher extends Disposable { return ''; }), cssFocusedPanel.cls('-focused', use => { - 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); diff --git a/app/client/ui/AccountPage.ts b/app/client/ui/AccountPage.ts index be5dcd4092..7e81a20bc2 100644 --- a/app/client/ui/AccountPage.ts +++ b/app/client/ui/AccountPage.ts @@ -59,8 +59,7 @@ export class AccountPage extends Disposable { headerMain: this._buildHeaderMain(), contentMain: this._buildContentMain(), testId, - }, { - regionFocusSwitcher: this._appObj.regionFocusSwitcher, + app: this._appObj, }); } diff --git a/app/client/ui/AdminPanel.ts b/app/client/ui/AdminPanel.ts index ff86d4cdc7..fcd02d9753 100644 --- a/app/client/ui/AdminPanel.ts +++ b/app/client/ui/AdminPanel.ts @@ -48,8 +48,7 @@ export class AdminPanel extends Disposable { headerMain: this._buildMainHeader(pageObs), contentTop: buildHomeBanners(this._appModel), contentMain: this._buildMainContent(), - }, { - regionFocusSwitcher: this._appObj.regionFocusSwitcher, + app: this._appObj, }); } diff --git a/app/client/ui/App.ts b/app/client/ui/App.ts index 3eb0f8b4b7..3ee24c3b93 100644 --- a/app/client/ui/App.ts +++ b/app/client/ui/App.ts @@ -58,6 +58,7 @@ export class AppImpl extends DisposableWithEvents implements App { // Track the most recently created DocPageModel, for some error handling. public pageModel?: DocPageModel; + // Track the RegionFocusSwitcher created by pagePanels, so that the codebase can access it. public regionFocusSwitcher?: RegionFocusSwitcher; private _settings: ko.Observable<{features?: ISupportedFeatures}>; @@ -83,7 +84,6 @@ export class AppImpl extends DisposableWithEvents implements App { 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 293cd4a142..8abe01089e 100644 --- a/app/client/ui/AppUI.ts +++ b/app/client/ui/AppUI.ts @@ -128,8 +128,7 @@ function pagePanelsHome(owner: IDisposableOwner, appModel: AppModel, app: App) { contentMain: createDocMenu(pageModel), contentTop: buildHomeBanners(appModel), testId, - }, { - regionFocusSwitcher: app.regionFocusSwitcher, + app, }); } @@ -139,7 +138,6 @@ 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); @@ -188,7 +186,6 @@ function pagePanelsDoc(owner: IDisposableOwner, appModel: AppModel, appObj: App) contentTop: buildDocumentBanners(pageModel), contentBottom: dom.create(createBottomBarDoc, pageModel, leftPanelOpen, rightPanelOpen), banner: dom.create(ViewAsBanner, pageModel), - }, { - regionFocusSwitcher: appObj.regionFocusSwitcher, + app: appObj, }); } diff --git a/app/client/ui/AuditLogsPage.ts b/app/client/ui/AuditLogsPage.ts index 1116ca539c..d9bf9cd0cb 100644 --- a/app/client/ui/AuditLogsPage.ts +++ b/app/client/ui/AuditLogsPage.ts @@ -74,8 +74,7 @@ export class AuditLogsPage extends Disposable { headerMain: this._buildHeader(), contentTop: buildHomeBanners(this._appModel), contentMain: this._buildContent(), - }, { - regionFocusSwitcher: this._appObj.regionFocusSwitcher, + app: this._appObj, }); } diff --git a/app/client/ui/PagePanels.ts b/app/client/ui/PagePanels.ts index 53084c3078..4c1fdcdbac 100644 --- a/app/client/ui/PagePanels.ts +++ b/app/client/ui/PagePanels.ts @@ -16,6 +16,7 @@ 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 {App} from 'app/client/ui/App'; const t = makeT('PagePanels'); @@ -46,16 +47,11 @@ export interface PageContents { testId?: TestId; contentTop?: DomElementArg; contentBottom?: DomElementArg; -} -interface PagePanelsOptions { - regionFocusSwitcher?: RegionFocusSwitcher; + app?: App; } -export function pagePanels( - page: PageContents, - options: PagePanelsOptions = { } -) { +export function pagePanels(page: PageContents) { const testId = page.testId || noTestId; const left = page.leftPanel; const right = page.rightPanel; @@ -65,7 +61,7 @@ export function pagePanels( const bannerHeight = Observable.create(null, 0); const isScreenResizingObs = isScreenResizing(); - const regionFocusSwitcher = options.regionFocusSwitcher; + const appObj = page.app; let lastLeftOpen = left.panelOpen.get(); let lastRightOpen = right?.panelOpen.get() || false; @@ -75,6 +71,11 @@ export function pagePanels( let contentTopDom: HTMLElement; let onLeftTransitionFinish = noop; + const regionFocusSwitcher = RegionFocusSwitcher.create(null, appObj); + if (appObj) { + appObj.regionFocusSwitcher = regionFocusSwitcher; + } + // When switching to mobile mode, close panels; when switching to desktop, restore the // last desktop state. const sub1 = subscribe(isNarrowScreenObs(), (use, narrow) => { @@ -128,6 +129,7 @@ export function pagePanels( dom.autoDispose(sub2), dom.autoDispose(commandsGroup), dom.autoDispose(leftOverlap), + dom.autoDispose(regionFocusSwitcher), dom('div', testId('top-panel'), page.contentTop, elem => { contentTopDom = elem; }), dom.maybe(page.banner, () => { let elem: HTMLElement; @@ -145,7 +147,7 @@ export function pagePanels( }), cssContentMain( (el) => { - regionFocusSwitcher?.init(el); + regionFocusSwitcher?.onPageDomLoaded(el); }, leftPaneDom = cssLeftPane( testId('left-panel'), diff --git a/app/client/ui/WelcomePage.ts b/app/client/ui/WelcomePage.ts index 3a017fba7d..84b43354ec 100644 --- a/app/client/ui/WelcomePage.ts +++ b/app/client/ui/WelcomePage.ts @@ -59,8 +59,7 @@ export class WelcomePage extends Disposable { page === 'teams' ? dom.create(buildWelcomeSitePicker, this._appModel) : this._buildPageContent(page) ), - }, { - regionFocusSwitcher: this._appObj.regionFocusSwitcher, + app: this._appObj, }); } From f5f26e0a2179227342882d6675a8bd2c84bbb4c4 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 27 May 2025 18:52:11 +0200 Subject: [PATCH 30/44] (kb) enabling some mouse-specific regionfocusswitcher tests --- test/nbrowser/RegionFocusSwitcher.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/nbrowser/RegionFocusSwitcher.ts b/test/nbrowser/RegionFocusSwitcher.ts index 280e5b5625..3af8f0cc6d 100644 --- a/test/nbrowser/RegionFocusSwitcher.ts +++ b/test/nbrowser/RegionFocusSwitcher.ts @@ -253,7 +253,7 @@ describe("RegionFocusSwitcher", function () { assert.isTrue(await isClipboardFocused()); }); - it.skip("should focus a panel-region when clicking an input child element", async function() { + 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); @@ -268,7 +268,7 @@ describe("RegionFocusSwitcher", function () { assert.isTrue(await isClipboardFocused()); }); - it.skip("should focus a section-region when clicking on it", async function() { + it("should focus a section-region when clicking on it", async function() { const session = await gu.session().teamSite.login(); await session.tempNewDoc(cleanup); From 1a902154254a61600c1ff6c6f3cc6db71d6608c8 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 27 May 2025 18:56:02 +0200 Subject: [PATCH 31/44] (kb) remove the clipboard_forbid_focus class it _seems_ unnecessary. needs more testing --- app/client/components/Clipboard.js | 6 ------ app/client/components/RegionFocusSwitcher.ts | 12 +++++------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/app/client/components/Clipboard.js b/app/client/components/Clipboard.js index e4a76c4db1..c87b82c4ae 100644 --- a/app/client/components/Clipboard.js +++ b/app/client/components/Clipboard.js @@ -304,14 +304,8 @@ async function getTextFromClipboardItem(clipboardItem, type) { * 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_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) { - if (elem && elem.classList.contains('clipboard_forbid_focus')) { - return false; - } if (elem && elem.closest('.clipboard_group_focus')) { return true; } diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index cfea0691f2..ab169d7b6f 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -126,28 +126,26 @@ export class RegionFocusSwitcher extends Disposable { } return isSpecialPage(gristDoc); }), - dom.cls(use => { + 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 if (!gristDoc) { - return 'clipboard_group_focus'; + return true; } // on a grist doc, panel content is focusable only if it's the current region const current = use(this._state).region; if (current?.type === 'panel' && current.id === id) { - return 'clipboard_group_focus'; + return true; } if (gristDoc) { use(gristDoc.activeViewId); } // on a grist doc, main panel is focusable only if we are not the actual document view if (id === "main") { - return isSpecialPage(gristDoc) - ? 'clipboard_group_focus' - : 'clipboard_forbid_focus'; + return isSpecialPage(gristDoc); } - return ''; + return false; }), cssFocusedPanel.cls('-focused', use => { const current = use(this._state); From 6640902dac9bb9859332fd20e736a974d93ab7e9 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 28 May 2025 11:23:41 +0200 Subject: [PATCH 32/44] (kb) refactor classes code on region focus switcher --- app/client/components/RegionFocusSwitcher.ts | 33 ++++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index ab169d7b6f..89ccb04e1d 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -1,4 +1,4 @@ -import {Disposable, dom, Observable, styled} from 'grainjs'; +import {Disposable, dom, Observable, styled, UseCBOwner} from 'grainjs'; import {mod} from 'app/common/gutil'; import {SpecialDocPage} from 'app/common/gristUrls'; import isEqual from 'lodash/isEqual'; @@ -114,17 +114,9 @@ export class RegionFocusSwitcher extends Disposable { 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); + return id !== 'main' + ? true + : this._canTabThroughMainRegion(use); }), dom.cls('clipboard_group_focus', use => { const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; @@ -132,18 +124,14 @@ export class RegionFocusSwitcher extends Disposable { if (!gristDoc) { return true; } - // on a grist doc, panel content is focusable only if it's the current region const current = use(this._state).region; if (current?.type === 'panel' && current.id === id) { return true; } - if (gristDoc) { - use(gristDoc.activeViewId); - } // on a grist doc, main panel is focusable only if we are not the actual document view if (id === "main") { - return isSpecialPage(gristDoc); + return this._canTabThroughMainRegion(use); } return false; }), @@ -346,6 +334,17 @@ export class RegionFocusSwitcher extends Disposable { return this.focusRegion({type: 'panel', id: 'right'}, {initiator: {type: 'cycle'}}); } + private _canTabThroughMainRegion(use: UseCBOwner) { + const gristDoc = this._gristDocObs ? use(this._gristDocObs) : null; + if (!gristDoc) { + return true; + } + if (gristDoc) { + use(gristDoc.activeViewId); + } + return isSpecialPage(gristDoc); + } + /** * Returns the grist doc only if its has a view layout, meaning it has view sections. * From 31b5fafdf282412233fc509fa0dfa1767bc5cdc4 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 28 May 2025 11:26:06 +0200 Subject: [PATCH 33/44] (kb) better highlight some components when focused on some elements, the highlight is not that distinguishable because it is really close to the element that itself has pronounced styling ; now we better see --- app/client/ui/AppHeader.ts | 2 ++ app/client/ui/DocMenuCss.ts | 4 ++-- app/client/ui/LeftPanelCommon.ts | 1 + app/client/ui2018/buttons.ts | 3 ++- app/client/ui2018/checkbox.ts | 1 + 5 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/client/ui/AppHeader.ts b/app/client/ui/AppHeader.ts index f011a7a277..99fb490c43 100644 --- a/app/client/ui/AppHeader.ts +++ b/app/client/ui/AppHeader.ts @@ -330,6 +330,8 @@ const cssAppLogo = styled('a._cssAppLogo', ` overflow: hidden; border-right-color: var(--middle-border-color, ${theme.appHeaderBorder}); + outline-offset: -1px; + &-grist-logo { background-image: var(--icon-GristLogo); background-color: ${vars.logoBg}; diff --git a/app/client/ui/DocMenuCss.ts b/app/client/ui/DocMenuCss.ts index 18e8302ac2..4d555397c0 100644 --- a/app/client/ui/DocMenuCss.ts +++ b/app/client/ui/DocMenuCss.ts @@ -110,11 +110,11 @@ export const STICKY_HEADER_HEIGHT_PX = 76; export const stickyHeader = styled(headerWrap, ` height: ${STICKY_HEADER_HEIGHT_PX}px; position: sticky; - top: 0px; + top: 3px; /* leave room for the kb focus highlight when using nextRegion command */ background-color: ${theme.mainPanelBg}; z-index: ${vars.stickyHeaderZIndex}; margin-bottom: 0px; - padding: 16px 0px 24px 0px; + padding: 13px 0px 24px 0px; `); export const allDocsTemplates = styled('div', ` diff --git a/app/client/ui/LeftPanelCommon.ts b/app/client/ui/LeftPanelCommon.ts index df2a9deffa..677bcbb91f 100644 --- a/app/client/ui/LeftPanelCommon.ts +++ b/app/client/ui/LeftPanelCommon.ts @@ -152,6 +152,7 @@ export const cssPageLink = styled('a', ` padding-left: 24px; outline: none; cursor: pointer; + outline-offset: -3px; &, &:hover, &:focus { text-decoration: none; outline: none; diff --git a/app/client/ui2018/buttons.ts b/app/client/ui2018/buttons.ts index 04dfbdf8ff..40d2f074d3 100644 --- a/app/client/ui2018/buttons.ts +++ b/app/client/ui2018/buttons.ts @@ -38,6 +38,8 @@ export const cssButton = styled('button', ` cursor: pointer; + outline-offset: 2px; + &-large { font-weight: 500; padding: 10px 24px; @@ -69,7 +71,6 @@ export const cssButton = styled('button', ` background-color: ${theme.controlDisabledBg}; border-color: ${theme.controlDisabledBg}; } - `); interface IButtonProps { diff --git a/app/client/ui2018/checkbox.ts b/app/client/ui2018/checkbox.ts index 239989bfbf..46a31922b2 100644 --- a/app/client/ui2018/checkbox.ts +++ b/app/client/ui2018/checkbox.ts @@ -49,6 +49,7 @@ export const cssCheckboxSquare = styled('input', ` width: 16px; height: 16px; outline: none !important; + outline-offset: 2px; --radius: 3px; From 2d72cb7eb672c15cd5248019001739cf5ef10ae3 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 28 May 2025 11:29:51 +0200 Subject: [PATCH 34/44] (kb) change kb focus ring color (now in the theme) - dont use default browser color - add the color in the theme to allow customization --- app/client/components/KeyboardFocusHighlighter.ts | 5 ++--- app/common/ThemePrefs.ts | 2 ++ app/common/themes/Base.ts | 2 ++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/client/components/KeyboardFocusHighlighter.ts b/app/client/components/KeyboardFocusHighlighter.ts index ff0f72240e..3eb7b3a1a6 100644 --- a/app/client/components/KeyboardFocusHighlighter.ts +++ b/app/client/components/KeyboardFocusHighlighter.ts @@ -7,6 +7,7 @@ * 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 { components } from "app/common/ThemePrefs"; import { Disposable, dom, styled } from "grainjs"; export class KeyboardFocusHighlighter extends Disposable { @@ -34,8 +35,6 @@ export class KeyboardFocusHighlighter extends Disposable { const cssKeyboardUser = styled('div', ` & .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; + outline: 2px solid ${components.kbFocusHighlight} !important; } `); diff --git a/app/common/ThemePrefs.ts b/app/common/ThemePrefs.ts index e378acde9f..10d1e53201 100644 --- a/app/common/ThemePrefs.ts +++ b/app/common/ThemePrefs.ts @@ -578,6 +578,7 @@ export const componentsCssMapping = { cardButtonBorderSelected: 'card-button-border-selected', cardButtonShadow: 'card-button-shadow', formulaIcon: 'formula-icon', + kbFocusHighlight: 'kb-focus-highlight', } as const; export const tokens = Object.fromEntries( @@ -1300,6 +1301,7 @@ export interface BaseThemeTokens { cardButtonBorderSelected: Token; cardButtonShadow: Token; formulaIcon: Token; + kbFocusHighlight: Token; }; } diff --git a/app/common/themes/Base.ts b/app/common/themes/Base.ts index 6d25514997..e433cf7985 100644 --- a/app/common/themes/Base.ts +++ b/app/common/themes/Base.ts @@ -490,5 +490,7 @@ export const Base: BaseThemeTokens = { cardButtonShadow: 'rgba(0,0,0,0.1)', formulaIcon: '#D0D0D0', + + kbFocusHighlight: tokens.primary, } }; From b43631f1be6de2479c09d3d29b8a1e120a33b274 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 28 May 2025 11:31:31 +0200 Subject: [PATCH 35/44] (kb) change visual behavior of focus highlight - make focus ring more pronounced (width) - stop highlighting current section when focused in it; just highlight the currently focused thing. it was misleading --- app/client/components/KeyboardFocusHighlighter.ts | 2 +- app/client/components/RegionFocusSwitcher.ts | 15 ++++----------- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/app/client/components/KeyboardFocusHighlighter.ts b/app/client/components/KeyboardFocusHighlighter.ts index 3eb7b3a1a6..4b42db8768 100644 --- a/app/client/components/KeyboardFocusHighlighter.ts +++ b/app/client/components/KeyboardFocusHighlighter.ts @@ -35,6 +35,6 @@ export class KeyboardFocusHighlighter extends Disposable { const cssKeyboardUser = styled('div', ` & .kb-focus-highlighter-group :is(a, input, textarea, select, button, [tabindex="0"]):focus-visible { - outline: 2px solid ${components.kbFocusHighlight} !important; + outline: 3px solid ${components.kbFocusHighlight} !important; } `); diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 89ccb04e1d..85a603c5a4 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -7,8 +7,8 @@ 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'; +import {components} from 'app/common/ThemePrefs'; const t = makeT('RegionFocusSwitcher'); @@ -619,19 +619,12 @@ const maybeNotifyAboutCreatorPanel = (gristDoc: GristDoc, cycleRegions: Region[] }; 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: 3px solid ${components.kbFocusHighlight} !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; + &-focused [${ATTRS.focusedElement}]:focus { + outline: 3px solid ${components.kbFocusHighlight} !important; } `); From 7dd4267227735bf9e15d97458bd111ee6fee068e Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 28 May 2025 12:18:03 +0200 Subject: [PATCH 36/44] (kb) better visually mark when the active section is not focused this makes for a way easier to understand keyboard navigation between creator panel and widgets --- app/client/components/ViewLayout.css | 5 +++++ app/client/components/buildViewSectionDom.ts | 1 + app/common/ThemePrefs.ts | 8 ++++++++ app/common/themes/Base.ts | 1 + app/common/themes/GristDark.ts | 1 + app/common/themes/GristLight.ts | 1 + app/common/themes/HighContrastLight.ts | 1 + 7 files changed, 18 insertions(+) diff --git a/app/client/components/ViewLayout.css b/app/client/components/ViewLayout.css index 72feef0fa0..16059af9c5 100644 --- a/app/client/components/ViewLayout.css +++ b/app/client/components/ViewLayout.css @@ -95,6 +95,11 @@ border-left: 1px solid var(--grist-theme-widget-active-border, var(--grist-color-light-green)); } +.active_section--no-focus > .view_data_pane_container { + box-shadow: -2px 0 0 0px var(--grist-theme-widget-active-non-focused-border); + border-left: 1px solid var(--grist-theme-widget-active-non-focused-border); +} + .active_section > .view_data_pane_container.viewsection_type_detail { /* this color is a translucent version of grist-color-light-green */ box-shadow: -2px 0 0 0px var(--grist-theme-cursor-inactive, var(--grist-color-inactive-cursor)); diff --git a/app/client/components/buildViewSectionDom.ts b/app/client/components/buildViewSectionDom.ts index 28e51f1a89..749b56bf81 100644 --- a/app/client/components/buildViewSectionDom.ts +++ b/app/client/components/buildViewSectionDom.ts @@ -95,6 +95,7 @@ export function buildViewSectionDom(options: { cssViewLeaf.cls(''), cssViewLeafInactive.cls('', (use) => !vs.isDisposed() && !use(vs.hasVisibleFocus)), dom.cls('active_section', vs.hasFocus), + dom.cls('active_section--no-focus', (use) => !vs.isDisposed() && use(vs.hasFocus) && !use(vs.hasRegionFocus)), 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', diff --git a/app/common/ThemePrefs.ts b/app/common/ThemePrefs.ts index 10d1e53201..77405935da 100644 --- a/app/common/ThemePrefs.ts +++ b/app/common/ThemePrefs.ts @@ -81,6 +81,7 @@ export const tokensCssMapping = { primaryMuted: 'primary-muted', primaryDim: 'primary-dim', primaryEmphasis: 'primary-emphasis', + primaryTransparent: 'primary-transparent', white: 'white', black: 'black', @@ -260,6 +261,7 @@ export const componentsCssMapping = { widgetBg: 'widget-bg', widgetBorder: 'widget-border', widgetActiveBorder: 'widget-active-border', + widgetActiveNonFocusedBorder: 'widget-active-non-focused-border', widgetInactiveStripesLight: 'widget-inactive-stripes-light', widgetInactiveStripesDark: 'widget-inactive-stripes-dark', pinnedDocFooterBg: 'pinned-doc-footer-bg', @@ -799,6 +801,11 @@ export interface SpecificThemeTokens { */ primaryEmphasis: Token; + /** + * primary color with around 50% opacity + */ + primaryTransparent: Token; + controlBorderRadius: Token; /** @@ -1101,6 +1108,7 @@ export interface BaseThemeTokens { widgetBg: Token; widgetBorder: Token; widgetActiveBorder: Token; + widgetActiveNonFocusedBorder: Token; widgetInactiveStripesLight: Token; pinnedDocFooterBg: Token; pinnedDocBorder: Token; diff --git a/app/common/themes/Base.ts b/app/common/themes/Base.ts index e433cf7985..944a97489a 100644 --- a/app/common/themes/Base.ts +++ b/app/common/themes/Base.ts @@ -193,6 +193,7 @@ export const Base: BaseThemeTokens = { widgetBg: tokens.bg, widgetBorder: tokens.decoration, widgetActiveBorder: tokens.primary, + widgetActiveNonFocusedBorder: tokens.primaryTransparent, widgetInactiveStripesLight: tokens.bgSecondary, /* Pinned Docs */ diff --git a/app/common/themes/GristDark.ts b/app/common/themes/GristDark.ts index c43cb4809d..ff63dcc809 100644 --- a/app/common/themes/GristDark.ts +++ b/app/common/themes/GristDark.ts @@ -25,6 +25,7 @@ export const GristDark: ThemeTokens = { primaryMuted: '#1da270', primaryDim: '#157a54', primaryEmphasis: '#13d78d', + primaryTransparent: 'rgba(23, 179, 120, 0.5)', controlBorderRadius: '4px', diff --git a/app/common/themes/GristLight.ts b/app/common/themes/GristLight.ts index cc537f2129..ba9c26e48a 100644 --- a/app/common/themes/GristLight.ts +++ b/app/common/themes/GristLight.ts @@ -25,6 +25,7 @@ export const GristLight: ThemeTokens = { primaryMuted: '#009058', primaryDim: '#007548', primaryEmphasis: '#b1ffe2', + primaryTransparent: 'rgba(22, 179, 120, 0.5)', controlBorderRadius: '4px', diff --git a/app/common/themes/HighContrastLight.ts b/app/common/themes/HighContrastLight.ts index 406b0d2462..70eda75feb 100644 --- a/app/common/themes/HighContrastLight.ts +++ b/app/common/themes/HighContrastLight.ts @@ -16,6 +16,7 @@ export const HighContrastLight: ThemeTokens = { primary: '#0f7b51', primaryMuted: '#196C47', primaryDim: '#196C47', + primaryTransparent: 'rgba(15, 123, 81, 0.5)', components: { ...GristLight.components, From fc980720b9dc539029d095b6a14df0323484bf6e Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 28 May 2025 12:39:23 +0200 Subject: [PATCH 37/44] (kb) remove unnecessary helper function (thanks dmitry) --- app/client/components/Clipboard.js | 12 ------------ app/client/components/RegionFocusSwitcher.ts | 4 ++-- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/app/client/components/Clipboard.js b/app/client/components/Clipboard.js index c87b82c4ae..19035c3720 100644 --- a/app/client/components/Clipboard.js +++ b/app/client/components/Clipboard.js @@ -319,18 +319,6 @@ function allowFocus(elem) { 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/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 85a603c5a4..66af9dd852 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -3,8 +3,8 @@ 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 {FocusLayer} from 'app/client/lib/FocusLayer'; 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 BaseView from 'app/client/components/BaseView'; @@ -423,7 +423,7 @@ const focusPanelElement = (panelElement: HTMLElement) => { }; const focusViewLayout = (gristDoc: GristDoc) => { - triggerFocusGrab(); + FocusLayer.grabFocus(); gristDoc.viewModel.focusedRegionState('in'); }; From f796a2744321c2674de08ecd59d73378b18c27eb Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 28 May 2025 13:21:23 +0200 Subject: [PATCH 38/44] (kb) change the creator panel shortcut warning logic this is following dmitry's feedback --- app/client/components/RegionFocusSwitcher.ts | 121 +++++++++++-------- app/client/components/commands.ts | 37 +----- 2 files changed, 75 insertions(+), 83 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 66af9dd852..3703c84d9f 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -44,6 +44,12 @@ export class RegionFocusSwitcher extends Disposable { right: null, main: null, }; + // Command history exclusively here to warn the user about the creator panel shortcut if needed + private _commandsHistory: { + name: 'nextRegion' | 'prevRegion' | 'creatorPanel', + timestamp: number + }[] = []; + private _warnedAboutCreatorPanel = false; constructor(private _app?: App) { super(); @@ -53,9 +59,20 @@ export class RegionFocusSwitcher extends Disposable { }); this.autoDispose(commands.createGroup({ - nextRegion: () => this._cycle('next'), - prevRegion: () => this._cycle('prev'), - creatorPanel: () => this._toggleCreatorPanel(), + nextRegion: () => { + this._logCommand('nextRegion'); + this._maybeNotifyAboutCreatorPanel(); + return this._cycle('next'); + }, + prevRegion: () => { + this._logCommand('prevRegion'); + this._maybeNotifyAboutCreatorPanel(); + return this._cycle('prev'); + }, + creatorPanel: () => { + this._logCommand('creatorPanel'); + return this._toggleCreatorPanel(); + }, cancel: this._onEscapeKeypress.bind(this), }, this, true)); @@ -151,9 +168,6 @@ export class RegionFocusSwitcher extends Disposable { direction, gristDoc ), {initiator: {type: 'cycle'}}); - if (gristDoc) { - maybeNotifyAboutCreatorPanel(gristDoc, cycleRegions); - } } /** @@ -166,6 +180,7 @@ export class RegionFocusSwitcher extends Disposable { if (!gristDoc) { return; } + this._commandsHistory = []; const closestRegion = (event.target as HTMLElement)?.closest(`[${ATTRS.regionId}]`); if (!closestRegion) { return; @@ -360,6 +375,56 @@ export class RegionFocusSwitcher extends Disposable { } return null; } + + private _logCommand(name: 'nextRegion' | 'prevRegion' | 'creatorPanel') { + if (this._commandsHistory.length > 20) { + this._commandsHistory.shift(); + } + this._commandsHistory.push({name, timestamp: Date.now()}); + } + + /** + * As a user, it's not obvious that the creator panel needs a different shortcut than the other regions. + * + * So the user might try to use the next/prevRegion shortcut to access the creator panel. + * We show a warning letting him now about the specific creator panel shortcut when we think he is "searching" for it. + */ + private _maybeNotifyAboutCreatorPanel() { + if (this._warnedAboutCreatorPanel) { + return; + } + const usedCreatorPanelCommand = this._commandsHistory.some(cmd => cmd.name === 'creatorPanel'); + if (usedCreatorPanelCommand) { + return; + } + const gristDoc = this._getGristDoc(); + if (!gristDoc) { + return; + } + const now = Date.now(); + const commandsInLast20Secs = this._commandsHistory.filter(cmd => cmd.timestamp > now - (1000 * 20)); + const cycleRegions = getCycleRegions(gristDoc); + // the logic is: if in the last 20 seconds, the user pressed the same cycle shortcut enough times + // to do 2 full cycles through the regions, we assume he is trying to access the creator panel. + const warn = commandsInLast20Secs.length > ((cycleRegions.length * 2) - 1) + && ( + commandsInLast20Secs.every(cmd => cmd.name === 'nextRegion') + || commandsInLast20Secs.every(cmd => cmd.name === 'prevRegion') + ); + if (warn) { + this._app?.topAppModel.notifier.createUserMessage( + t( + 'Trying to access the creator panel? Use {{key}}.', + {key: commands.allCommands.creatorPanel.humanKeys} + ), + { + level: 'info', + key: 'rfs-cp-warn', + } + ); + this._warnedAboutCreatorPanel = true; + } + } } /** @@ -574,50 +639,6 @@ const isSpecialPage = (doc: GristDoc | null) => { return false; }; - -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 { outline: 3px solid ${components.kbFocusHighlight} !important; diff --git a/app/client/components/commands.ts b/app/client/components/commands.ts index aafdc2dfb5..04a06c3aff 100644 --- a/app/client/components/commands.ts +++ b/app/client/components/commands.ts @@ -53,32 +53,6 @@ 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 @@ -259,7 +233,7 @@ export class Command implements CommandDef { if (this.alwaysOn) { Mousetrap.markAlwaysOnShortcut(key); } - Mousetrap.bind(key, wrapKeyCallback(commandGroup.commands[commandName], commandName)); + Mousetrap.bind(key, wrapKeyCallback(commandGroup.commands[commandName])); } else { Mousetrap.unbind(key); } @@ -273,14 +247,11 @@ export class Command implements CommandDef { } /** - * 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. + * 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). */ -function wrapKeyCallback(callback: Func, commandName: string) { +function wrapKeyCallback(callback: Func) { return function() { - addToHistory(commandName); return callback(...arguments) || false; }; } From fc9eeac837535de301267943f29f69ee15726d1c Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 28 May 2025 17:16:05 +0200 Subject: [PATCH 39/44] (kb) trap focus inside panels this makes sure that when we tab out of bounds of a panel, we wrap to the first (or last) element in it instead of going out of it. this is to prevent focusing back the active section when tabbing out of the panel (since tabbing out of the panel makes document.activeElement an element not allowed by the clipboard, it focuses back the active section automatically). --- app/client/components/RegionFocusSwitcher.ts | 27 ++ app/client/lib/trapTabKey.ts | 244 +++++++++++++++++++ 2 files changed, 271 insertions(+) create mode 100644 app/client/lib/trapTabKey.ts diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 3703c84d9f..48e9505737 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -4,6 +4,7 @@ import {SpecialDocPage} from 'app/common/gristUrls'; import isEqual from 'lodash/isEqual'; import {makeT} from 'app/client/lib/localization'; import {FocusLayer} from 'app/client/lib/FocusLayer'; +import {trapTabKey} from 'app/client/lib/trapTabKey'; import * as commands from 'app/client/components/commands'; import {App} from 'app/client/ui/App'; import {GristDoc} from 'app/client/components/GristDoc'; @@ -292,6 +293,7 @@ export class RegionFocusSwitcher extends Disposable { ? current.initiator.event : undefined; + disableFocusLock(); removeFocusRings(); removeTabIndexes(); if (!mouseEvent) { @@ -456,6 +458,7 @@ const focusPanel = (panel: PanelRegion, child: HTMLElement | null, gristDoc: Gri if (!panelElement) { return; } + enableFocusLock(panelElement); // Child element found: focus it if (child && child !== panelElement && child.isConnected) { @@ -578,6 +581,30 @@ const blurPanelChild = (panel: PanelRegion) => { } }; +let _focusLocked: {el: HTMLElement | null, cb: ((event: KeyboardEvent) => void) | null} = {el: null, cb: null}; + +const disableFocusLock = () => { + const {el, cb} = _focusLocked; + if (el && cb) { + el.removeEventListener('keydown', cb); + _focusLocked = {el: null, cb: null}; + } +}; + +const enableFocusLock = (panelElement: HTMLElement) => { + disableFocusLock(); + const focusTrap = (event: KeyboardEvent) => { + if (event.key === 'Tab') { + trapTabKey(panelElement, event); + } + }; + panelElement.addEventListener('keydown', focusTrap); + _focusLocked = { + el: panelElement, + cb: focusTrap + }; +}; + const getPanelElement = (id: Panel): HTMLElement | null => { return document.querySelector(getPanelElementId(id)); }; diff --git a/app/client/lib/trapTabKey.ts b/app/client/lib/trapTabKey.ts new file mode 100644 index 0000000000..0c7a697b63 --- /dev/null +++ b/app/client/lib/trapTabKey.ts @@ -0,0 +1,244 @@ +/** + * Code is authored by Kitty Giraudel for a11y-dialog https://github.com/KittyGiraudel/a11y-dialog, thanks to her! + * + * As keyboard-handling is very specific in Grist, we'd rather copy/paste some base code that can be easily modified, + * rather than relying on a library. + * + * The MIT License (MIT) + * + * Copyright (c) 2025 Kitty Giraudel + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated + * documentation files (the "Software"), to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, + * and to permit persons to whom the Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED + * TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF + * CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +/** + * Trap the tab key within the given element. + * + * This only wraps focus when we detect the user tabs out of bounds (tab on the last focusable element, + * or shift+tab on the first focusable element). It doesn't force focus any other way, so it doesn't + * interfere with potential popups appearing or anything else. + */ +export function trapTabKey(container: HTMLElement, tabEvent: KeyboardEvent) { + const [firstFocusableEl, lastFocusableEl] = getFocusableEdges(container); + + const activeEl = getActiveEl(); + + // If the SHIFT key is pressed while tabbing (moving backwards) and the + // currently focused item is the first one, move the focus to the last + // focusable item from the element + if (tabEvent.shiftKey && activeEl === firstFocusableEl) { + lastFocusableEl?.focus(); + tabEvent.preventDefault(); + } + + // If the SHIFT key is not pressed (moving forwards) and the currently focused + // item is the last one, move the focus to the first focusable item from the element + else if (!tabEvent.shiftKey && activeEl === lastFocusableEl) { + firstFocusableEl?.focus(); + tabEvent.preventDefault(); + } +} + +function getFocusableEdges(el: HTMLElement) { + // Check for a focusable element within the subtree of the given element. + const firstEl = findFocusableEl(el, true); + + // Only if we find the first element do we need to look for the last one. If + // there’s no last element, we set `lastEl` as a reference to `firstEl` so + // that the returned array is still always of length 2. + const lastEl = firstEl ? findFocusableEl(el, false) || firstEl : null; + + return [firstEl, lastEl] as const; +} + +function findFocusableEl( + el: HTMLElement, + forward: boolean +): HTMLElement | null { + // If we’re walking forward, check if this element is focusable, and return it + // immediately if it is. + if (forward && isFocusable(el)) { return el; } + + // We should only search the subtree of this element if it can have focusable + // children. + if (canHaveFocusableChildren(el)) { + // Start walking the DOM tree, looking for focusable elements. + // Case 1: If this element has a shadow root, search it recursively. + if (el.shadowRoot) { + // Descend into this subtree. + let next = getNextChildEl(el.shadowRoot, forward); + + // Traverse the siblings, searching the subtree of each one for focusable + // elements. + while (next) { + const focusableEl = findFocusableEl(next as HTMLElement, forward); + if (focusableEl) { return focusableEl; } + next = getNextSiblingEl(next as HTMLElement, forward); + } + } + + // Case 2: If this element is a slot for a Custom Element, search its + // assigned elements recursively. + else if (el.localName === 'slot') { + const assignedElements = (el as HTMLSlotElement).assignedElements({ + flatten: true, + }) as HTMLElement[]; + if (!forward) { assignedElements.reverse(); } + + for (const assignedElement of assignedElements) { + const focusableEl = findFocusableEl(assignedElement, forward); + if (focusableEl) { return focusableEl; } + } + } + // Case 3: this is a regular Light DOM element. Search its subtree. + else { + // Descend into this subtree. + let next = getNextChildEl(el, forward); + + // Traverse siblings, searching the subtree of each one + // for focusable elements. + while (next) { + const focusableEl = findFocusableEl(next as HTMLElement, forward); + if (focusableEl) { return focusableEl; } + next = getNextSiblingEl(next as HTMLElement, forward); + } + } + } + + // If we’re walking backward, we want to check the element’s entire subtree + // before checking the element itself. If this element is focusable, return + // it. + if (!forward && isFocusable(el)) { return el; } + + return null; +} + +/** + * Get the active element, accounting for Shadow DOM subtrees. + * @author Cory LaViska + * @see: https://www.abeautifulsite.net/posts/finding-the-active-element-in-a-shadow-root/ + */ +export function getActiveEl( + root: Document | ShadowRoot = document +): Element | null { + const activeEl = root.activeElement; + + if (!activeEl) { return null; } + + // If there’s a shadow root, recursively find the active element within it. + // If the recursive call returns null, return the active element + // of the top-level Document. + if (activeEl.shadowRoot) + { return getActiveEl(activeEl.shadowRoot) || document.activeElement; } + + // If not, we can just return the active element + return activeEl; +} + +/** + * Determine if an element can have focusable children. Useful for bailing out + * early when walking the DOM tree. + * @example + * This div is inert, so none of its children can be focused, even though they + * meet our criteria for what is focusable. Once we check the div, we can skip + * the rest of the subtree. + * ```html + *
+ * + * Link + *
+ * ``` + */ +function canHaveFocusableChildren(el: HTMLElement) { + // The browser will never send focus into a Shadow DOM if the host element + // has a negative tabindex. This applies to both slotted Light DOM Shadow DOM + // children + if (el.shadowRoot && el.getAttribute('tabindex') === '-1') { return false; } + + // Elemments matching this selector are either hidden entirely from the user, + // or are visible but unavailable for interaction. Their descentants can never + // receive focus. + return !el.matches(':disabled,[hidden],[inert]'); +} + + +function getNextChildEl(el: ParentNode, forward: boolean) { + return forward ? el.firstElementChild : el.lastElementChild; +} + +function getNextSiblingEl(el: HTMLElement, forward: boolean) { + return forward ? el.nextElementSibling : el.previousElementSibling; +} + +/** + * Determine if an element is hidden from the user. + */ +const isHidden = (el: HTMLElement) => { + // Browsers hide all non- descendants of closed
elements + // from user interaction, but those non- elements may still match our + // focusable-selectors and may still have dimensions, so we need a special + // case to ignore them. + if ( + el.matches('details:not([open]) *') && + !el.matches('details>summary:first-of-type') + ) + { return true; } + + // If this element has no painted dimensions, it's hidden. + return !(el.offsetWidth || el.offsetHeight || el.getClientRects().length); +}; + +/** + * Determine if an element is focusable and has user-visible painted dimensions. + */ +const isFocusable = (el: HTMLElement) => { + // A shadow host that delegates focus will never directly receive focus, + // even with `tabindex=0`. Consider our custom element, which + // delegates focus to its shadow button: + // + // + // #shadow-root + // + // + // + // The browser acts as as if there is only one focusable element – the shadow + // button. Our library should behave the same way. + if (el.shadowRoot?.delegatesFocus) { return false; } + + return el.matches(focusableSelectors.join(',')) && !isHidden(el); +}; + +const notInert = ':not([inert]):not([inert] *)'; +const notNegTabIndex = ':not([tabindex^="-"])'; +const notDisabled = ':not(:disabled)'; + +const focusableSelectors = [ + `a[href]${notInert}${notNegTabIndex}`, + `area[href]${notInert}${notNegTabIndex}`, + `input:not([type="hidden"]):not([type="radio"])${notInert}${notNegTabIndex}${notDisabled}`, + `input[type="radio"]${notInert}${notNegTabIndex}${notDisabled}`, + `select${notInert}${notNegTabIndex}${notDisabled}`, + `textarea${notInert}${notNegTabIndex}${notDisabled}`, + `button${notInert}${notNegTabIndex}${notDisabled}`, + `details${notInert} > summary:first-of-type${notNegTabIndex}`, + // Discard until Firefox supports `:has()` + // See: https://github.com/KittyGiraudel/focusable-selectors/issues/12 + // `details:not(:has(> summary))${notInert}${notNegTabIndex}`, + `iframe${notInert}${notNegTabIndex}`, + `audio[controls]${notInert}${notNegTabIndex}`, + `video[controls]${notInert}${notNegTabIndex}`, + `[contenteditable]${notInert}${notNegTabIndex}`, + `[tabindex]${notInert}${notNegTabIndex}`, +]; From 5024369fb7ac31be948cd43cde51aa584ec78616 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 11 Jun 2025 17:18:26 +0200 Subject: [PATCH 40/44] (kb) silently fail instead of crashing this is following dmitry's feedback. this is actually more of a "impossible use case" where the idea is more to notify the dev while working and should not happen at all in production but i guess doing this is better still, just in case --- app/client/components/RegionFocusSwitcher.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index 48e9505737..e8c0e39c18 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -105,10 +105,12 @@ export class RegionFocusSwitcher extends Disposable { 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'); + console.error('main panel is not supported when a view layout is rendered'); + return; } if (!gristDoc && region?.type === 'section') { - throw new Error('view section id is not supported when no view layout is rendered'); + console.error('view section id is not supported when no view layout is rendered'); + return; } this._state.set({region, initiator: options.initiator}); From 6a2a7914f09c4a024e8745d2c466754d53cd448c Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Wed, 11 Jun 2025 17:44:07 +0200 Subject: [PATCH 41/44] (tests) update DocUsageTracking tests to handle new keyboard behavior --- test/nbrowser/DocUsageTracking.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/nbrowser/DocUsageTracking.ts b/test/nbrowser/DocUsageTracking.ts index 330ef87b62..c2a4cf2ea9 100644 --- a/test/nbrowser/DocUsageTracking.ts +++ b/test/nbrowser/DocUsageTracking.ts @@ -52,6 +52,9 @@ describe('DocUsageTracking', function() { await gu.getPageItem('AttachmentsTable').click(); await gu.addColumn("Attachments", "Attachment"); + // Focus out of the creator panel + await driver.sendKeys(Key.ESCAPE, Key.ESCAPE); + // Upload some files into the first row. (We're putting Grist docs in a Grist doc!) await driver.sendKeys(Key.ENTER); await gu.fileDialogUpload( From 63977a717b682db364c5c31f63479ef8ecb8e63f Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Fri, 13 Jun 2025 15:16:42 +0200 Subject: [PATCH 42/44] (regions) use more precise region roles for some app regions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the document top panel is a header. the main region is … the main region. others are just custom regions, themselves containing some more specific things like nav or headers that we can change later --- app/client/components/RegionFocusSwitcher.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/client/components/RegionFocusSwitcher.ts b/app/client/components/RegionFocusSwitcher.ts index e8c0e39c18..1578b3acb1 100644 --- a/app/client/components/RegionFocusSwitcher.ts +++ b/app/client/components/RegionFocusSwitcher.ts @@ -129,7 +129,11 @@ export class RegionFocusSwitcher extends Disposable { public panelAttrs(id: Panel, ariaLabel: string) { return [ - dom.attr('role', 'region'), + dom.attr('role', id === 'main' + ? 'main' + : id === 'top' + ? 'banner' + : 'region'), dom.attr('aria-label', ariaLabel), dom.attr(ATTRS.regionId, id), dom.cls('kb-focus-highlighter-group', use => { From c8330cd8c929c9a5bb389864e29f2bf7b6c5d6a4 Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Mon, 16 Jun 2025 13:03:59 +0200 Subject: [PATCH 43/44] (kb) undo the change on doclistmenu padding to prevent scrolling glitches the sticky doc list in the homepage takes precedence over the region cycle highlight focus ; but if we scroll we now had a 3px gap from the top. Not wanted. Just revert this change, we can tinker with something better later. --- app/client/ui/DocMenuCss.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/client/ui/DocMenuCss.ts b/app/client/ui/DocMenuCss.ts index 4d555397c0..18e8302ac2 100644 --- a/app/client/ui/DocMenuCss.ts +++ b/app/client/ui/DocMenuCss.ts @@ -110,11 +110,11 @@ export const STICKY_HEADER_HEIGHT_PX = 76; export const stickyHeader = styled(headerWrap, ` height: ${STICKY_HEADER_HEIGHT_PX}px; position: sticky; - top: 3px; /* leave room for the kb focus highlight when using nextRegion command */ + top: 0px; background-color: ${theme.mainPanelBg}; z-index: ${vars.stickyHeaderZIndex}; margin-bottom: 0px; - padding: 13px 0px 24px 0px; + padding: 16px 0px 24px 0px; `); export const allDocsTemplates = styled('div', ` From e3cc8f3e1aa04c1edd1a45b4a5f6932a69cae3dc Mon Sep 17 00:00:00 2001 From: Emmanuel Pelletier Date: Tue, 17 Jun 2025 14:31:38 +0200 Subject: [PATCH 44/44] (modals) when using keyboard, make sure focused things are visible our keyboard focus-ring trick needs a specific class to be enabled and it wasn't applied in modals. Now we correctly see the focused things --- app/client/ui2018/modals.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/app/client/ui2018/modals.ts b/app/client/ui2018/modals.ts index 454dbeb1c2..b63ee53a87 100644 --- a/app/client/ui2018/modals.ts +++ b/app/client/ui2018/modals.ts @@ -244,6 +244,7 @@ export function modal( dialogDom = cssModalDialog( createFn(ctl, owner), + dom.cls('kb-focus-highlighter-group'), cssModalDialog.cls('-collapsing', variant === 'collapsing'), dom.on('click', (ev) => ev.stopPropagation()), noEscapeKey ? null : dom.onKeyDown({ Escape: close }),