Skip to content

fix(keyboard): preserve focus across dialogs, tabs, and drags - #1900

Merged
l0gicgate merged 8 commits into
mainfrom
fix/1859-keyboard-focus-edge-cases
Oct 27, 2026
Merged

l0gicgate merged 8 commits into
mainfrom
fix/1859-keyboard-focus-edge-cases

Conversation

@JoeJoeflyn

@JoeJoeflyn JoeJoeflyn commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Keep keyboard focus in the visible file pane after dialogs close and views change, and preserve an open modal's focus when a background operation finishes. Settings now refuses to open underneath Properties.

Reuse the shared modal focus restoration and heading-drag finalizer, including the chooser. Escape cancels heading reordering without clearing file selection or saving a new order; later drags still work. Preserve startup and renamed-row focus while discarding obsolete keyboard refocus work.

Includes the original outside-click fix from #1879 rather than a second implementation. Merges current main and reuses #1890's rename-cursor and playback-test fixes without duplicating them.

Visual evidence

Before:

before-short.mp4

After:

after-short.mp4

How to test

Use disposable files. Do not confirm permanent deletion.

  1. Select a file, press Shift+Delete, cancel by clicking the backdrop, then use an arrow key. Also dismiss the Copy-or-move prompt with Copy, Escape, Cancel, X, and its backdrop using a test destination on another filesystem with the drop strategy set to Ask.
  2. Open two tabs on different folders. In the second tab select a file, switch views with Ctrl+1/2/3, and navigate with arrows.
  3. Open Properties with Alt+Enter and press Ctrl+,. Close Properties, open Settings, and click outside its panel.
  4. In List, drag Mode past Size, keep the mouse held, press Escape, and release. Repeat in the portal chooser; then try another drag normally.
  5. Start a long compression, open a delete confirmation while it runs, and let compression finish. Cancel the confirmation. Also rename or create an entry that moves in the sorted listing and immediately navigate with arrows.

Expected result: arrows navigate the visible pane; hidden tabs and background completions do not steal focus; Settings stays closed behind Properties and closes on an outside click; Escape preserves heading order and file selection without closing the chooser; an open modal retains its focused control.

Related issue

Closes #1859

Reuses #1879 and the fixes already merged in #1890.

pdwaldrop and others added 3 commits October 24, 2026 23:04
The backdrop click handler tested the click against the responsive bin
around the dialog, which fills the window, so every click counted as inside
the dialog and none closed Settings. Test against the dialog panel, whose
margin is the area the backdrop shows.
Reuse modal focus restoration for deletion and cross-volume prompts, keep hidden tabs and background completions from taking focus, and guard Settings against window-level dialogs. Share heading-drag cancellation between browser and chooser. Preserve initial and renamed-row focus while cancelling obsolete keyboard refocus work.

Extend existing behavioral coverage, retain chooser selection on Escape, and make the near-end route playback check observe actual progress instead of a transient playing frame.

Refs #1859
@github-actions github-actions Bot added the fix label Oct 26, 2026
@JoeJoeflyn

Copy link
Copy Markdown
Collaborator Author

Implementation and validation

Merged main at be3343a5 in f051ca55 and resolved both conflicts. Keep #1890's cursor-identity check for bound-row callbacks and its route-playback regression unchanged; the separate view-restoration callback still invalidates pending work on keyboard navigation/view changes. The outside-click implementation retains #1879's original commit and authorship.

The remaining changes share one focus eligibility policy and one heading-drag finalizer across the browser/chooser. No new polling or per-row input handlers; no measured performance improvement claimed. Regressions exercise input routing, hidden tabs, startup, filtering, modal ownership, dismissal routes, file safety, and selection after drag cancellation—not layout assertions.

Final affected validation after merging main

All GUI checks used the pinned container with isolated Xvfb/D-Bus; no desktop session was used. Scoped Rust commands used the ignored helper target/issue-1859-rust.sh, which verifies the pinned base, reuses target/quality-container, and runs ./scripts/test-headless.py --locked --config profile.dev.debug=0 --config build.incremental=false inside it.

  • bash target/issue-1859-rust.sh ui::window::tests::keyboard_dispatch::pane_focus -- --test-threads=1 — 7 passed.
  • bash target/issue-1859-rust.sh ui::chooser::tests::keyboard:: -- --test-threads=1 — 16 passed.
  • bash target/issue-1859-rust.sh ui::window::tests::keyboard_dispatch::filter_focus:: -- --test-threads=1 — 12 passed.
  • bash target/issue-1859-rust.sh docked_completion_preserves_modal_focus_when_created_entries_are_selected -- --test-threads=1 — 1 passed, exercises all three views with real compression completion semantics.
  • bash target/issue-1859-rust.sh closing_settings_cancels_the_theme_preview_and_collapses_the_editor -- --test-threads=1 — 1 passed.
  • bash target/issue-1859-rust.sh settings_launcher_refuses_to_open_over_another_modal -- --test-threads=1 — 1 passed.
  • ./scripts/qa.sh focus-usable-after-view-change list-column-drag-applies-on-release cross-volume-enter-choice dismissed-overlays-return-focus modal-dialog-blocks-window-shortcuts settings-closes-on-an-outside-click chooser-heading-escape-keeps-selection committed-rename-stays-visible route-preview-shows-offline-track-and-elevation/list — 21 passed, including thirteen cross-volume prompt/view/dismissal runs.
  • ./scripts/quality.sh fmt — passed.
  • ./scripts/quality.sh clippy — passed.
  • git diff --check — passed.

Scope rationale: shared focus/view switching also affects initial mapping, chooser startup, filters, renamed rows, and background completions. The selected tests cover those callers plus every reported dismissal/reorder case. The merge did not change the test image, dependencies, or harness; Cargo's incoming change is the application's version bump.

Earlier full sweeps and intentional omissions

Full ./scripts/quality.sh test and ./scripts/qa.sh runs were attempted on intermediate code because the focus infrastructure is shared. They were not clean. The full QA run initially passed 601/603; the rename regression and route-playback timing check were subsequently fixed, and the relevant cases now pass. The failing Rust cases also passed isolated reruns after fixes/readiness corrections; unrelated timing failures were not addressed by rerunning the whole suite.

Per the scoped-rerun policy, full suites were not repeated after those fixes or the main merge. No clean final full-suite run is claimed. The final evidence is 38 selected Rust tests and 21 selected QA runs, plus formatting and Clippy. Required full CI remains authoritative before merge; this PR stays draft, so the workflow intentionally skips its build/test jobs until the owner marks it ready.

Local videos

The owner's requested captures remain ignored and are not uploaded or committed. before-short.mp4 is 30.4 seconds on original main d6c74746; after-short.mp4 is 32 seconds on committed fix 329aa2bd, before the main integration. Both show the same eight triggers/results, with setup/waits removed. The live after checks passed; raw footage, source ranges and protocol transcript remain under target/issue-1859-videos/after/. The recording session is stopped.

@JoeJoeflyn
JoeJoeflyn marked this pull request as ready for review October 26, 2026 16:26
Raise the direct minimum and regenerate the lockfile for zstd 0.14.3, zstd-safe 8.2.0, and zstd-sys 2.1.2. Resolve RUSTSEC-2026-0391, RUSTSEC-2026-0398, RUSTSEC-2026-0403, and RUSTSEC-2026-0410 without weakening the dependency policy.

Refs #1859
The metadata regression must not assume a scrollable context-menu item fits inside the screen. Invoke the same menu action directly; retain its dialog and preview assertions. Pointer input remains covered by the context-menu scenarios.

Refs #1859
@JoeJoeflyn

Copy link
Copy Markdown
Collaborator Author

CI fixes

  • 19965d50 fix(deps): update zstd to patched releases — raise the manifest minimum to zstd 0.14.3; Cargo updates only zstd, zstd-safe 8.2.0, and zstd-sys 2.1.2. Their upstream minimum requirements enforce the patched transitive versions too. Resolves RUSTSEC-2026-0391, -0398, -0403, and -0410 without new audit exceptions or policy changes.
  • d8ff32f8 test(preview): activate Properties through its menu action — the metadata scenario reproducibly clicked below the monitor edge after the native-menu changes on main. Invoke the same accessible menu action instead of assuming it fits on screen. Retain all dialog/preview assertions; pointer routing remains covered by rename-from-context-menu and the existing context-menu scenarios.

Main was already current at be3343a5; no further merge or conflict resolution was needed. No review comments were outstanding. No tests were removed, no new version/constant assertions were added, and the application architecture, toolkit/runtime patches, and image inputs are unchanged. The original focus fixes and reused #1879/#1890 implementations remain intact.

Validation

Dependency changes justified one full pinned Rust sweep and one full pinned QA sweep. Subsequent failures were isolated rather than repeating full suites.

  • CARGO_HOME="$PWD/target/quality-container/cargo" cargo deny --locked check — reproduced all four advisories before the update; advisories, bans, licenses, and sources pass afterward.
  • ./scripts/quality.sh test — local unsharded run: 3556 passed, 9 failed, 11 existing ignores. All nine failures passed exact isolated reruns (below), without weakening assertions or changing application code. The subsequent full CI Rust shards are green.
  • ./scripts/qa.sh — 618/620 passed. media-details-properties-only/icons reproduced and is fixed above. markdown-equations-keep-prose/columns passed its isolated rerun.
  • ./scripts/qa.sh media-details-properties-only/icons markdown-equations-keep-prose/columns — 2/2 passed three consecutive times after the setup correction.
  • ./scripts/qa.sh media-details-properties-only markdown-equations-keep-prose/columns rename-from-context-menu context-menu-keeps-column-target raw-details-preview-and-properties context-menu-baseline — 10/10 passed, retaining menu pointer/action ownership and RAW/preview caller coverage.
  • ./scripts/quality.sh fmt, ./scripts/quality.sh clippy, git diff origin/main...HEAD --check — passed on the final changes.
Exact isolated Rust commands — each selected one test and passed

The ignored helper uses the verified pinned image and existing quality cache and invokes scripts/test-headless.py; every GUI check used private Xvfb/D-Bus, never the desktop.

bash target/issue-1859-rust.sh ui::media::tests::reopening_resumes_where_the_preview_closed -- --exact --test-threads=1
bash target/issue-1859-rust.sh ui::missing_tools::tests::missing_tools_dialog_shows_caller_content_and_closes -- --exact --test-threads=1
bash target/issue-1859-rust.sh ui::progress_dock::tests::already_hovered_notifications_start_paused_and_pinning_is_independent -- --exact --test-threads=1
bash target/issue-1859-rust.sh ui::window::composition::tabs::tests::restored_tabs_open_once_a_worker_checks_their_folders -- --exact --test-threads=1
bash target/issue-1859-rust.sh ui::window::drive_dialogs::tests::format_dialog_reads_drive_facts_off_the_ui_thread_and_continue_waits_for_them -- --exact --test-threads=1
bash target/issue-1859-rust.sh ui::window::tests::keyboard_dispatch::filter_focus::space_acts_on_a_selected_hit_only_from_the_filter -- --exact --test-threads=1
bash target/issue-1859-rust.sh ui::window::tests::keyboard_dispatch::overlay_focus::closing_settings_returns_focus_to_a_focused_filter_field -- --exact --test-threads=1
bash target/issue-1859-rust.sh ui::window::tests::keyboard_dispatch::pane_focus::empty_directory_focus_is_a_named_mapped_surface_and_tabs_out -- --exact --test-threads=1
bash target/issue-1859-rust.sh ui::window::tests::keyboard_dispatch::video_clips::enter_on_a_video_preview_opens_it_where_it_stopped -- --exact --test-threads=1

CI: all required checks green at d8ff32f8 — run 40380321570, including the dependency audit, build/lint, all four Rust shards, aggregate coverage gate, release workflow scripts, and PR metadata/type checks.

The CI QA job remains intentionally skipped because it runs only on manual dispatch. No clean final full local QA or unsharded Rust run is claimed; the initial full results and passing scoped reruns are recorded above. No merge or readiness change was made; the owner's PR description and video attachments were preserved.

@l0gicgate

Copy link
Copy Markdown
Contributor

Review and cleanup

Merged main at ac5ab4f22 in f7ad381c5. There were no conflicts. The PR's own zstd bump (19965d50) now matches main's #1904 exactly, so Cargo.toml and Cargo.lock no longer appear in the diff against main.

Review against the description and #1859. I checked each open item in the issue against the code, and each one has a test that fails without its fix:

  • Focus after dialogs close. The delete confirmation and Copy-or-move prompt now hand focus back through remember_modal_focus_for_listing, replacing the extra focus_active() calls. Docked completions under a modal are blocked by focus_navigation::browser_can_take_focus in apply_collection_scroll, focus_visible_pane, focus_rebuilt_active_column and focus_surface_or. Coverage: dismissed-overlays-return-focus, all 13 cross-volume-enter-choice runs, and docked_completion_preserves_modal_focus_when_created_entries_are_selected, which covers all three views.
  • Ctrl+1/2/3 with hidden tabs. An unmapped widget inside a non-visible tab-context stack child no longer takes focus. The new cursor_focus_generation drops queued idle refocus work once a newer navigation happens. Coverage: focus-usable-after-view-change.
  • Ctrl+, behind Properties. SettingsLauncher::show now uses the window-wide visible_modal_layer. Before, it only scanned its own tab overlay and missed window-level dialogs. Coverage: modal-dialog-blocks-window-shortcuts and settings_launcher_refuses_to_open_over_another_modal; the test fixture was updated to the real tab-window structure.
  • Outside click on Settings (from fix(settings): close Settings on a click outside the panel #1879). The hit test now uses panel instead of the full-window responsive bin. Coverage: settings-closes-on-an-outside-click and the backdrop route in ui::settings::tests::dismissal.
  • Escape during a List heading drag. Window and chooser Escape both go through ModeViews::cancel_column_drag, which shares ListColumnLayout::finish_drag with the release path. It clears the pending commit before GestureDrag::reset, so the drag-end that reset emits saves nothing. Coverage: list-column-drag-applies-on-release and chooser-heading-escape-keeps-selection.

There were no review threads or inline comments to reconcile. Only the author's two status comments were on the PR.

Tests and comments audit. The new and changed Rust tests and QA scenarios each test input routing, focus lifecycle or file safety, and none is tautological, so nothing was removed. I kept every added comment: each explains a non-obvious reason (drag-end ordering on reset, why startup focus is allowed before mapping, why the panel is the hit-test target, why the recycled rename-row cursor is restored).

Fixes pushed: none. I found no merge blockers.

Validation (on the merged tree, in an isolated worktree):

  • ./scripts/test-headless.py -- ui::chooser::tests::keyboard:: ui::window::tests::keyboard_dispatch::pane_focus ui::window::tests::keyboard_dispatch::overlay_focus docked_completion_preserves_modal_focus ui::window::composition::settings::tests ui::settings::tests::dismissal: 34 passed
  • ./scripts/qa.sh --test-threads 6 chooser-heading-escape-keeps-selection cross-volume-enter-choice dismissed-overlays-return-focus modal-dialog-blocks-window-shortcuts settings-closes-on-an-outside-click focus-usable-after-view-change media-details-properties-only list-column-drag-applies-on-release: 20/20 passed
  • ./scripts/quality.sh fmt, ./scripts/quality.sh clippy, git diff origin/main...HEAD --check: clean
  • Not run: full local Rust and QA suites. The merge was conflict-free and touched no shared infrastructure, so CI is the full suite.

Follow-ups outside this PR

  • cancel_column_drag resets the first GestureDrag on the headings. Today there is only one (column-drag); the resize handles keep theirs on child widgets. Matching on the gesture's name would keep this working if another gesture is added later.

CI: all required checks green at f7ad381c5 (the QA scenarios job is skipped because it runs only on manual dispatch).

# Conflicts:
#	tests/qa/scenarios/properties_media/media-details-properties-only.jsonl
@l0gicgate

Copy link
Copy Markdown
Contributor

Merge conflict resolved

Merged main at 923b39ddf in c479098dd. #1901 and this PR both changed how media-details-properties-only reaches Properties in the context menu, because the item can sit below the bottom of the menu. #1901 scrolls the menu and then really clicks the item. This PR used accessibility activation instead. I kept main's version, so it still exercises the real pointer path. The PR no longer changes that file. No other file was changed by both sides.

Validation (on the merged tree, in an isolated worktree):

  • ./scripts/test-headless.py -- docked_completion_preserves_modal_focus ui::window::composition::settings::tests ui::window::tests::keyboard_dispatch::video_clips: 11 passed
  • ./scripts/qa.sh --test-threads 6 media-details-properties-only modal-dialog-blocks-window-shortcuts: 2/2 passed
  • git diff origin/main...HEAD --check: clean. I didn't rerun fmt and clippy because the resolution changed no Rust code.

CI: all required checks green at c479098dd (the QA scenarios job is skipped because it runs only on manual dispatch).

@l0gicgate
l0gicgate merged commit 86df474 into main Oct 27, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keyboard and focus issues from QA sweep

3 participants