From f887267362acc67c992ad179ddcd1fcd8dd9e5d2 Mon Sep 17 00:00:00 2001 From: Kodjo Sossouvi Date: Sat, 21 Mar 2026 18:08:13 +0100 Subject: [PATCH] Optimizing keyboard navigation and selection handling --- src/myfasthtml/assets/core/keyboard.js | 28 +++++++++- src/myfasthtml/assets/datagrid/datagrid.css | 9 +++ src/myfasthtml/assets/datagrid/datagrid.js | 4 +- src/myfasthtml/controls/DataGrid.py | 62 ++++++++++++++------- tests/controls/test_datagrid.py | 4 +- 5 files changed, 82 insertions(+), 25 deletions(-) diff --git a/src/myfasthtml/assets/core/keyboard.js b/src/myfasthtml/assets/core/keyboard.js index f60e107..2509eb9 100644 --- a/src/myfasthtml/assets/core/keyboard.js +++ b/src/myfasthtml/assets/core/keyboard.js @@ -1,7 +1,15 @@ /** * Create keyboard bindings */ + +// Set window.KEYBOARD_DEBUG = true in the browser console to enable traces +window.KEYBOARD_DEBUG = false; + (function () { + function kbLog(...args) { + if (window.KEYBOARD_DEBUG) console.debug('[Keyboard]', ...args); + } + /** * Global registry to store keyboard shortcuts for multiple elements */ @@ -172,8 +180,11 @@ // Add snapshot to history KeyboardRegistry.snapshotHistory.push(snapshot); + kbLog(`key="${key}" | history length=${KeyboardRegistry.snapshotHistory.length} | registeredElements=${KeyboardRegistry.elements.size}`); + // Cancel any pending timeout if (KeyboardRegistry.pendingTimeout) { + kbLog(` cancelled pending timeout`); clearTimeout(KeyboardRegistry.pendingTimeout); KeyboardRegistry.pendingTimeout = null; KeyboardRegistry.pendingMatches = []; @@ -198,8 +209,7 @@ const currentNode = traverseTree(treeRoot, KeyboardRegistry.snapshotHistory); if (!currentNode) { - // No match in this tree, continue to next element - // console.debug("No match in tree for event", key); + kbLog(` element="${elementId}" → no match in tree`); continue; } @@ -212,6 +222,8 @@ // Check if there are longer sequences possible (node has children) const hasLongerSequences = currentNode.children.size > 0; + kbLog(` element="${elementId}" | isInside=${isInside} | hasMatch=${hasMatch} | hasLongerSequences=${hasLongerSequences}`); + // Track if ANY element has longer sequences possible if (hasLongerSequences) { anyHasLongerSequence = true; @@ -221,6 +233,7 @@ if (hasMatch) { const requireInside = currentNode.config["require_inside"] === true; const enabled = isCombinationEnabled(data.controlDivId, currentNode.combinationStr); + kbLog(` combination="${currentNode.combinationStr}" | requireInside=${requireInside} | enabled=${enabled}`); if (enabled && (!requireInside || isInside)) { currentMatches.push({ elementId: elementId, @@ -228,10 +241,14 @@ combinationStr: currentNode.combinationStr, isInside: isInside }); + } else { + kbLog(` → skipped (requireInside=${requireInside} but isInside=${isInside}, or disabled)`); } } } + kbLog(` result: matches=${currentMatches.length} | anyHasLongerSequence=${anyHasLongerSequence}`); + // Prevent default if we found any match and not in input context if (currentMatches.length > 0 && !isInInputContext()) { event.preventDefault(); @@ -239,6 +256,7 @@ // Decision logic based on matches and longer sequences if (currentMatches.length > 0 && !anyHasLongerSequence) { + kbLog(` → TRIGGER immediately`); // We have matches and NO element has longer sequences possible // Trigger ALL matches immediately for (const match of currentMatches) { @@ -249,6 +267,7 @@ KeyboardRegistry.snapshotHistory = []; } else if (currentMatches.length > 0 && anyHasLongerSequence) { + kbLog(` → WAITING ${KeyboardRegistry.sequenceTimeout}ms (longer sequence possible)`); // We have matches but AT LEAST ONE element has longer sequences possible // Wait for timeout - ALL current matches will be triggered if timeout expires @@ -256,6 +275,7 @@ const savedEvent = event; // Save event for timeout callback KeyboardRegistry.pendingTimeout = setTimeout(() => { + kbLog(` → TRIGGER after timeout`); // Timeout expired, trigger ALL pending matches for (const match of KeyboardRegistry.pendingMatches) { triggerHtmxAction(match.elementId, match.config, match.combinationStr, match.isInside, savedEvent); @@ -268,10 +288,12 @@ }, KeyboardRegistry.sequenceTimeout); } else if (currentMatches.length === 0 && anyHasLongerSequence) { + kbLog(` → WAITING (partial match, no full match yet)`); // No matches yet but longer sequences are possible // Just wait, don't trigger anything } else { + kbLog(` → NO MATCH, clearing history`); // No matches and no longer sequences possible // This is an invalid sequence - clear history KeyboardRegistry.snapshotHistory = []; @@ -280,11 +302,13 @@ // If we found no match at all, clear the history // This handles invalid sequences like "A C" when only "A B" exists if (!foundAnyMatch) { + kbLog(` → foundAnyMatch=false, clearing history`); KeyboardRegistry.snapshotHistory = []; } // Also clear history if it gets too long (prevent memory issues) if (KeyboardRegistry.snapshotHistory.length > 10) { + kbLog(` → history too long, clearing`); KeyboardRegistry.snapshotHistory = []; } } diff --git a/src/myfasthtml/assets/datagrid/datagrid.css b/src/myfasthtml/assets/datagrid/datagrid.css index 619a740..957060c 100644 --- a/src/myfasthtml/assets/datagrid/datagrid.css +++ b/src/myfasthtml/assets/datagrid/datagrid.css @@ -146,6 +146,15 @@ outline: none; } +.dt2-cell-input { + width: 100%; +} + +.dt2-cell-input:focus { + outline: none; + box-shadow: none; +} + .dt2-cell:hover, .dt2-selected-cell { background-color: var(--color-selection); diff --git a/src/myfasthtml/assets/datagrid/datagrid.js b/src/myfasthtml/assets/datagrid/datagrid.js index 70e0905..3339645 100644 --- a/src/myfasthtml/assets/datagrid/datagrid.js +++ b/src/myfasthtml/assets/datagrid/datagrid.js @@ -698,7 +698,7 @@ function updateDatagridSelection(datagridId) { if (cellElement) { cellElement.classList.add('dt2-selected-focus'); cellElement.style.userSelect = 'text'; - cellElement.focus({ preventScroll: true }); + requestAnimationFrame(() => cellElement.focus({ preventScroll: false })); hasFocusedCell = true; } } else if (selectionType === 'cell') { @@ -750,7 +750,7 @@ function updateDatagridSelection(datagridId) { if (!hasFocusedCell) { const grid = document.getElementById(datagridId); - if (grid) grid.focus({ preventScroll: true }); + if (grid) requestAnimationFrame(() => grid.focus({ preventScroll: true })); } } diff --git a/src/myfasthtml/controls/DataGrid.py b/src/myfasthtml/controls/DataGrid.py index f532001..b924280 100644 --- a/src/myfasthtml/controls/DataGrid.py +++ b/src/myfasthtml/controls/DataGrid.py @@ -1,3 +1,4 @@ +import bisect import html import logging import re @@ -80,6 +81,7 @@ class DatagridState(DbObject): self.selection: DatagridSelectionState = DatagridSelectionState() self.cell_formats: dict = {} self.table_format: list = [] + self.ns_visible_indices: list[int] | None = None class DatagridSettings(DbObject): @@ -403,8 +405,12 @@ class DataGrid(MultipleInstance): if col.visible and col.type != ColumnType.RowSelection_] def _get_visible_row_indices(self) -> list[int]: - df = self._get_filtered_df() - return list(df.index) if df is not None else [] + if self._state.ns_visible_indices is None: + if self._df is None: + self._state.ns_visible_indices = [] + else: + self._state.ns_visible_indices = list(self._apply_filter(self._df).index) + return self._state.ns_visible_indices def _navigate(self, pos: tuple, direction: str) -> tuple: col_pos, row_index = pos @@ -421,11 +427,11 @@ class DataGrid(MultipleInstance): prev_cols = [c for c in navigable_cols if c < col_pos] return (prev_cols[-1], row_index) if prev_cols else pos elif direction == "down": - next_rows = [r for r in visible_rows if r > row_index] - return (col_pos, next_rows[0]) if next_rows else pos + next_pos = bisect.bisect_right(visible_rows, row_index) + return (col_pos, visible_rows[next_pos]) if next_pos < len(visible_rows) else pos elif direction == "up": - prev_rows = [r for r in visible_rows if r < row_index] - return (col_pos, prev_rows[-1]) if prev_rows else pos + prev_pos = bisect.bisect_left(visible_rows, row_index) - 1 + return (col_pos, visible_rows[prev_pos]) if prev_pos >= 0 else pos return pos @@ -439,9 +445,13 @@ class DataGrid(MultipleInstance): return None - def _update_current_position(self, pos): + def _update_current_position(self, pos, reset_selection: bool = False): self._state.selection.last_selected = self._state.selection.selected self._state.selection.selected = pos + + if reset_selection: + self._state.selection.extra_selected.clear() + self._state.edition.under_edition = None self._state.save() def _get_format_rules(self, col_pos, row_index, col_def): @@ -508,14 +518,23 @@ class DataGrid(MultipleInstance): self._columns.insert(0, DataGridRowSelectionColumnState()) def _enter_edition(self, pos): + logger.debug(f"enter_edition: {pos=}") col_pos, row_index = pos col_def = self._columns[col_pos] if col_def.type in (ColumnType.RowSelection_, ColumnType.RowIndex, ColumnType.Formula): return self.render_partial() + if col_def.type == ColumnType.Bool: + return self._toggle_bool_cell(col_pos, row_index, col_def) self._state.edition.under_edition = pos self._state.save() return self.render_partial("cell", pos=pos) + def _toggle_bool_cell(self, col_pos, row_index, col_def): + col_array = self._fast_access.get(col_def.col_id) + current_value = col_array[row_index] if col_array is not None and row_index < len(col_array) else False + self._data_service.set_data(col_def.col_id, row_index, not bool(current_value)) + return self.render_partial("cell", pos=(col_pos, row_index)) + def _convert_edition_value(self, value_str, col_type): if col_type == ColumnType.Number: try: @@ -645,11 +664,13 @@ class DataGrid(MultipleInstance): def filter(self): logger.debug("filter") self._state.filtered[FILTER_INPUT_CID] = self._datagrid_filter.get_query() + self._state.ns_visible_indices = None return self.render_partial("body") def handle_on_click(self, combination, is_inside, cell_id): logger.debug(f"on_click table={self.get_table_name()} {combination=} {is_inside=} {cell_id=}") if is_inside and cell_id: + logger.debug(f" is_inside=True") self._state.selection.extra_selected.clear() pos = self._get_pos_from_element_id(cell_id) @@ -659,9 +680,15 @@ class DataGrid(MultipleInstance): pos == self._state.selection.selected and self._state.edition.under_edition is None): return self._enter_edition(pos) + else: + logger.debug( + f" {pos=}, selected={self._state.selection.selected}, under_edition={self._state.edition.under_edition}") self._update_current_position(pos) + else: + logger.debug(f" is_inside=False") + return self.render_partial() def on_mouse_selection(self, combination, is_inside, cell_id_mousedown, cell_id_mouseup): @@ -685,13 +712,15 @@ class DataGrid(MultipleInstance): def on_key_pressed(self, combination, has_focus, is_inside): logger.debug(f"on_key_pressed table={self.get_table_name()} {combination=} {has_focus=} {is_inside=}") if combination == "esc": - self._update_current_position(None) - self._state.selection.extra_selected.clear() + self._update_current_position(None, reset_selection=True) + return self.render_partial("cell", pos=self._state.selection.last_selected) + elif (combination == "enter" and self._settings.enable_edition and self._state.selection.selected and self._state.edition.under_edition is None): return self._enter_edition(self._state.selection.selected) + elif combination in self._ARROW_KEY_DIRECTIONS: current_pos = (self._state.selection.selected or self._state.selection.last_selected @@ -1139,25 +1168,19 @@ class DataGrid(MultipleInstance): ) def mk_selection_manager(self): - - extra_attr = { - "hx-on::after-settle": f"updateDatagridSelection('{self._id}');", - } - selected = [] - + if self._state.selection.selected: # selected.append(("cell", self._get_element_id_from_pos("cell", self._state.selection.selected))) selected.append(("focus", self._get_element_id_from_pos("cell", self._state.selection.selected))) - + for extra_sel in self._state.selection.extra_selected: selected.append(extra_sel) - + return Div( *[Div(selection_type=s_type, element_id=f"{elt_id}") for s_type, elt_id in selected], id=f"tsm_{self._id}", selection_mode=f"{self._state.selection.selection_mode}", - **extra_attr, ) def mk_aggregation_cell(self, col_def, row_index: int, footer_conf, oob=False): @@ -1244,7 +1267,8 @@ class DataGrid(MultipleInstance): id=self._id, cls="grid", style="height: 100%; grid-template-rows: auto 1fr;", - tabindex="-1" + tabindex="-1", + **{"hx-on:htmx:after-swap": f"if(event.detail.target.id==='tsm_{self._id}') updateDatagridSelection('{self._id}');"} ) def render_partial(self, fragment="cell", **kwargs): diff --git a/tests/controls/test_datagrid.py b/tests/controls/test_datagrid.py index 45b039c..689a306 100644 --- a/tests/controls/test_datagrid.py +++ b/tests/controls/test_datagrid.py @@ -281,14 +281,14 @@ class TestDataGridBehaviour: # Selection and Interaction # ------------------------------------------------------------------ - def test_i_can_on_key_pressed_esc_clears_selection(self, datagrid): + def test_i_can_on_key_pressed_esc_clears_selection(self, datagrid_with_data): """Test that pressing ESC resets both the focused cell and extra selections. ESC is the standard 'deselect all' shortcut. Both selected and extra_selected must be cleared so the grid visually deselects everything and subsequent navigation starts from a clean state. """ - dg = datagrid + dg = datagrid_with_data dg._state.selection.selected = (1, 2) dg._state.selection.extra_selected.append(("range", (0, 0, 2, 2)))