dialog: Focus first control on open#2377
Conversation
| 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| { |
There was a problem hiding this comment.
We can't use defer for this, we was tired for this.
If user operations fast to close, this method will have unexpected behaviors
There was a problem hiding this comment.
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:
-
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 theirFocusHandlewhile 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. -
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?
There was a problem hiding this comment.
They are not a good solution.
Actually, we have already considered for this before, but still not get a good choice.
There was a problem hiding this comment.
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_focusAPI.
Given this, I will temporarily close this PR (Pull Request) until GPUI provides more robust low-level focus handling primitives.
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
How to Test
Run the focused dialog regression tests:
cargo test -p gpui-component open_dialog_Run the full
gpui-componenttest suite:cargo test -p gpui-componentVerify the web/wasm build path:
cd crates/story-web cargo build --target wasm32-unknown-unknownManually verify the story gallery:
Checklist
cargo runfor story tests related to the changes.