Skip to content

dialog: Focus first control on open#2377

Closed
lurenjia534 wants to merge 1 commit into
longbridge:mainfrom
lurenjia534:main
Closed

dialog: Focus first control on open#2377
lurenjia534 wants to merge 1 commit into
longbridge:mainfrom
lurenjia534:main

Conversation

@lurenjia534

Copy link
Copy Markdown
Contributor

Closes #2068

Description

This PR fixes the initial focus behavior for Dialog.

Previously, opening a dialog focused the dialog container itself. As a result, when the first interactive control inside the dialog was an input, users could not type immediately after the dialog opened unless they clicked the input or pressed Tab first.

The dialog now performs a one-time initial focus pass after it is rendered. If the dialog container still owns focus, it advances focus to the first keyboard-focusable descendant. If there is no focusable descendant, focus is restored to the dialog container. Explicit focus set by a dialog builder is preserved and will not be overridden.

Screenshot

Before After
focus stayed on the dialog container itself 20260516_054649

How to Test

Run the focused dialog regression tests:

cargo test -p gpui-component open_dialog_

Run the full gpui-component test suite:

cargo test -p gpui-component

Verify the web/wasm build path:

cd crates/story-web
cargo build --target wasm32-unknown-unknown

Manually verify the story gallery:

cargo run

Checklist

  • I have read the CONTRIBUTING document and followed the guidelines.
  • Reviewed the changes in this PR and confirmed AI generated code (If any) is accurate.
  • Passed cargo run for story tests related to the changes.
  • Tested macOS, Windows and Linux platforms performance (if the change is platform-specific). N/A: this change is not platform-specific.

let on_cancel = self.button_props.on_cancel.clone();
if self.focus_first_descendant {
let focus_handle = self.focus_handle.clone();
window.defer(cx, move |window, cx| {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use defer for this, we was tired for this.

If user operations fast to close, this method will have unexpected behaviors

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use defer for this, we was tired for this.

If user operations fast to close, this method will have unexpected behaviors

Thank you for the review. I agree that using defer here is risky, especially when the dialog is opened and closed quickly.

I revisited the focus timing issue. The goal of this PR / issue is still to focus the first focusable control automatically when a dialog opens. The difficulty is that focus_next(cx) relies on tab stops from the already rendered frame, while focusable children in a newly opened dialog are only registered during the current render/paint phase. So calling focus_next(cx) synchronously from open_dialog or Dialog::render does not reliably discover the new dialog children; delaying it with defer makes discovery work, but introduces the lifecycle issue you pointed out.

I see two possible directions:

  1. Keep the original automatic behavior, but implement it without defer. This likely requires a synchronous initial-focus scope/registry during dialog rendering, where focusable descendants register their FocusHandle while rendering and the first eligible descendant is focused once. This preserves the original behavior, but the change is larger and may need to touch more focusable components.

  2. Use a smaller explicit API, for example:

dialog.initial_focus(&input.read(cx).focus_handle(cx))

Then Root::render_dialog_layer can perform a one-shot focus handoff only if the dialog container still owns focus. This avoids defer and avoids focus_next, but it changes the behavior from automatic discovery to caller-provided initial focus.

For this issue, would you prefer that we keep the original automatic “first focus handle” behavior even if the implementation is larger, or would the explicit initial_focus API be acceptable as a smaller and safer change?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not a good solution.

Actually, we have already considered for this before, but still not get a good choice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not a good solution.

Actually, we have already considered for this before, but still not get a good choice.

Thank you for your confirmation.

After trying the two solutions mentioned above, I explored a new direction: using a "one-shot" on_focus_in listener on the dialog container, hoping to transfer focus during the lifecycle of the focus in GPUI—specifically after the newly rendered frame has been officially replaced and displayed.

However, under the current GPUI public API, this approach still does not work. Context::on_focus_in is activated via the defer mechanism; this means that new subscribers are inactive before the activation actually occurs. Since the open_dialog method completes both the listener registration and the focus operation on the dialog container within the same update cycle, the subscription is not active when the first "focus-in" event is triggered, causing the listener to miss the initial focus lifecycle.

In summary, based on the current API, I have not been able to find an ideal component-level solution that satisfies all of the following constraints:

  • Retain the original automatic initial focus behavior;
  • Avoid introducing delayed focus handling tasks;
  • Avoid maintaining an additional focus registry at the component level;
  • Avoid turning this issue into a demand for an explicit initial_focus API.

Given this, I will temporarily close this PR (Pull Request) until GPUI provides more robust low-level focus handling primitives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dialog: Focus to first focus_handle when Dialog open.

2 participants