-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Improved handling of Text Area focus state #2323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved handling of Text Area focus state #2323
Conversation
… in testing that this would help with.
This is no longer necessary, having fixed the flaws in logic about when focus is grabbed.
| * this hook will track the "focussed" state, and reinstate it once the element | ||
| * is no longer hidden. | ||
| */ | ||
| export function useFocusPreservation(element: HTMLElement | null, isHidden: Boolean) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the primitive boolean type instead of Boolean for the isHidden parameter.
| export function useFocusPreservation(element: HTMLElement | null, isHidden: Boolean) { | |
| export function useFocusPreservation(element: HTMLElement | null, isHidden: boolean) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
|
Added the below to a closed PR ( #2263 (comment) ); I see this one's open. Haven't reviewed differences between the techniques; mine's a state machine approach. Hope this helps! I mentioned this in #2189 (comment) , just wanted to note it here: I've worked on a fairly comprehensive state machine for maintaining focus. It may be lacking a few edge cases? I put it here a few days ago, then got busy with life :) |
The two fixes attempt to implement different behaviours, so it depends a bit what the desired behaviour is (which I don't think is clear, and is not specified anywhere that I know, so we're both working from our intuition about what we would expect, where we might have different expectations/intuition). The scenarios I focused on in my fix were:
The function I implemented was to always restore the focus on TextArea to what it was before - i.e. if it was focused before ChatView/extension was hidden, make it focused when ChatView becomes visible/not hidden again; and if it was not focused before, then don't focus it again. This can only be achieved by actively monitoring for focus/blur events on TextArea (which I added; the existing code and your new code don't do this). Some scenarios that I didn't consider/cover, that look to be handled in your new code: Other points to highlight from my PR, that I think are definitely beneficial.
If we can identify additional behaviours that are desirable, but not yet implemented in my PR (e.g. items 1-3 above, I'd be happy to extend my PR and the UTs to cover them). |
|
@diarmidmackenzie If this assessment isn't correct or there's more you'd like to discuss, please let us know and we can revisit this. For future contributions, please follow our issue-first approach. Thanks again for your effort! |
|
Seems reasonable. Thanks for checking in on this. |
Context
After making a pragmatic fix under PR#2263, I came across #2189, and as per comments there, I realized that there were various further issues with how focus on the text box is managed.
As described there, when the Roo Chat tab becomes visible again (either by switching between Roo tabs) or by switching between VSCode extensions, there is no way to determine from existing state whether or not focus should be re-instated on the text panel.
This PR explicitly tracks whether there is focus on this panel, and then re-instates it when the panel comes back into view.
Implementation
Basic approach is:
A bit of complexity arose because of the various ways that the text area can "blur" that we don't want to count as blurring, in particular
To accommodate the last of these, I had to extend the ExtensionMessage API to add a new "didBecomeInvisible" message that fires when the VSCode extension is hidden.
I added a decent set of Unit Tests to cover these various flows as well.
Screenshots
N/A
How to Test
Key manual tests are:
Get in Touch
diarmid/diarmidm on Discord
Important
Improves text area focus handling in
ChatViewby tracking focus state and reinstating it when the view becomes visible, using a newuseFocusPreservationhook.ChatView.tsxto manage text area focus state.useFocusPreservationhook inuseFocusPreservation.tsxto handle focus state management.didBecomeInvisibleaction toExtensionMessageinExtensionMessage.ts.ChatView.test.tsxto verify focus behavior when view visibility changes.This description was created by
for 86d9722. It will automatically update as commits are pushed.