Skip to content

notification: Keep notifications above modal layers - #3577

Draft
dux-web wants to merge 5 commits into
longbridge:mainfrom
dux-web:sailry/notification-main-review
Draft

dux-web wants to merge 5 commits into
longbridge:mainfrom
dux-web:sailry/notification-main-review

Conversation

@dux-web

@dux-web dux-web commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Notifications raised while a dialog is open must stay visible and usable. Defer the notification stack above the dialog layers, including stacked dialogs, while preserving safe-area insets, sheet offsets, and dialog focus.

The regression opens one and then two real Kit dialogs, raises a notification in each, hovers and clicks its dismiss button, checks the close callback, and verifies that the dialogs and their focus remain intact. The command-host fixture explicitly requests each repaint whose factory counts it asserts; its exact header/footer/item counts, query, and native confirmation assertions remain unchanged.

Public API

None.

Validation

  • Formatting with Rust 1.98.0 and git diff --check passed for the changed files.
  • Current head ece5608b0a4c019e7eb96afdd0f96816efbe3b02 merges upstream 861070bb50573741e17b6cdbb99c00ff1658f687 without rewriting the existing PR history. This includes the upstream scroll-claim test fix from scrollable_mask: Dispatch the overscroll steps of the scroll claim test within one frame #3648, which addresses the previous head's three default-backend CI failures. No additional Base, dependency, or lockfile changes are introduced by this PR.
  • CI for this head is queued or running; its results are pending. It covers the default and GPUI Fast overlay tests, Shell command-host tests, and the existing CI checks.

The interaction tests use headless Kit windows; no native OS-window or pixel verification is claimed. Prepared with AI assistance.

Copy link
Copy Markdown
Member

CI currently fails in GPUI Shell Components: retained_command_typed_entries_query_and_confirm_callbacks_are_native expected one header build after the second draw and got zero. The same test passed on this PR’s base. The log alone does not establish that notification deferral caused it; please investigate or reproduce that failure before landing.

The change also needs a window-level regression for a notification raised while a dialog is open, including stacked dialogs: verify visibility, dismiss-button interaction and unchanged dialog focus. The current source-only validation does not exercise that behavior.

@dux-web
dux-web force-pushed the sailry/notification-main-review branch from 7cca3a8 to bfefc2b Compare October 22, 2026 05:09
@dux-web

dux-web commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased onto da8b2ba4 and added the requested window regression in bfefc2b9.

I reproduced the Shell failure rather than assuming it was unrelated: latest main with the original fixture passes; adding only notification deferral reproduces the exact second-draw header count of 0 instead of 1. GPUI Fast can reuse a clean retained view on draw(). The fixture now calls Window::refresh() before each repaint it intends to count, including the two refreshed-snapshot repaints. It keeps all exact factory-count, query and native confirm assertions. The full command-host module passes (7 tests).

The new notification test exercises one dialog and then stacked dialogs through the real Kit window, including hover, the actual dismiss button, its close callback and unchanged dialog focus. Without the root-layer fix it fails because modal occlusion prevents the dismiss button becoming visible on hover; with the fix all 8 overlay tests pass on both default GPUI and GPUI Fast 0.1.10. Targeted three-crate Clippy and workspace formatting also pass. These are headless interaction checks; I have not claimed native OS-window or pixel verification.

The PR body contains the exact local validation scope. These changes were prepared with AI assistance.

Final CI update for bfefc2b9: 15 jobs pass, including GPUI Fast on macOS/Linux/Windows, all Shell jobs, lint and the WebView/backend checks. The three default-backend test jobs fail at the same unchanged gpui-base assertion, scrollable_mask::tests::horizontal_scroll_area_claims_only_steps_that_move (0px versus 30px). I checked each failed log: it is the sole failure on each platform. This assertion already fails on the rebased upstream commit da8b2ba4: upstream macOS job, PR macOS job, PR Linux job, PR Windows job. This PR has no changes under crates/base; I have kept the notification fix scoped and left the unrelated Base assertion unchanged.

@cavivie

cavivie commented Oct 28, 2026

Copy link
Copy Markdown

bump

Preserve the notification deferral and dialog interaction regression while retaining upstream safe-inset geometry. Absorb the upstream scroll-claim test correction without changing the PR's scope or dependency selection.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants