Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix some issues with focus scopes #13409

Merged
merged 7 commits into from
Nov 12, 2023
Merged

Fix some issues with focus scopes #13409

merged 7 commits into from
Nov 12, 2023

Conversation

grokys
Copy link
Member

@grokys grokys commented Oct 27, 2023 •
edited
Loading

What does the pull request do?

As part of the fix for #13325 I needed to do some basic refactoring to FocusManager to fix our broken 2015-era focus scope handling.

  • Correctly focus outermost focus scope when a control was removed (previously didn't have enough context to do so, and so this feature only worked when hiding/disabling a control)
  • Correctly clear the focused element in a scope when it is removed
  • Store the focused element in an attached property instead of a dictionary like WPF: means we need to do a little less bookkeeping

FocusManager really needs a rewrite like the one started in #8392 - this PR however doesn't even attempt that, it just aims to fix the existing focus scope handling.

Breaking Changes

Previously the reference assembly for FocusManager was exporting a compiler-generated enumerable class:

[PrivateApi]
public class FocusManager : IFocusManager
{
    [CompilerGenerated]
    public sealed class <GetFocusScopeAncestors>d__18 : IEnumerable<IFocusScope>, IEnumerable, IEnumerator<IFocusScope>, IEnumerator, IDisposable
    {
        public IInputElement <>3__control;

        [DebuggerHidden]
        public <GetFocusScopeAncestors>d__18(int <>1__state);
    }
}

This class wasn't exposed by the API so couldn't be used, but is no longer being generated after this PR. Added a suppression for it,

Fixed issues

Fixes #13325

@grokys
Remove focus hack from Popup.
6fcf5bc
@grokys
Added failing focus scope tests.
2a3ab7c
@grokys
Refactor focus scopes in FocusManager.
0665fb3 
- Store focused element within a scope using an attached property (like WPF)
- Store current focus root so that focus can be restored to that root when a focused control or active focus scope is removed

Fixes #13325
grokys
grokys commented Oct 27, 2023
View reviewed changes
src/Avalonia.Controls/Primitives/PopupRoot.cs Outdated
@@ -77,7 +77,7 @@ public PopupRoot(TopLevel parent, IPopupImpl impl, IAvaloniaDependencyResolver?
/// <summary>
/// Gets the control that is hosting the popup root.
/// </summary>
Visual? IHostedVisualTreeRoot.Host => VisualParent;
Visual? IHostedVisualTreeRoot.Host => Parent as Visual ?? ParentTopLevel;
Copy link
Member Author

Choose a reason for hiding this comment

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

@Gillibald this fixes what I think was an incorrect change in one of your commits. I think the change in that commit was wrong because VisualParent should always be null for PopupRoot considering that it's a visual root.

jankrib
jankrib reviewed Oct 27, 2023
View reviewed changes
Copy link
Contributor

@jankrib jankrib left a comment

Choose a reason for hiding this comment

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

Nice😃 This is going t help a lot!

src/Avalonia.Base/Input/FocusManager.cs
}
else if (Current != null)
else if (_focusRoot?.GetValue(FocusedElementProperty) is { } restore &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the _focusRoot concept a bit confusing. Wouldn't it be better to just stick to focus scopes like in the old implementation?

At least in theory there could be another focus scope between the cleared scope and the VisualRoot.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just following WPF's behavior of focusing the outermost focus scope when a focused control is removed. The reason we need to store the focus root is that at the time the focused control is removed we may not have all the information we need to find that outermost scope.

@grokys
Suppress API compat error.
65a4503 
This was being produced for a compiler-generated enumerable class that was erroneously being included in the reference assembly for `FocusManager`.
@grokys
Remove focus hack from ContextMenu.
eb97e6b 
And add failing test now that the hack is removed.
@grokys
Try to return a rooted host visual.
96a978b 
Fixes failing test from previous comment where focus wasn't restored when closing a context menu.
@grokys
Merge branch 'master' into fixes/13325-focus-root-scope
5bf4684
@grokys grokys marked this pull request as ready for review November 9, 2023 08:57
@grokys
Copy link
Member Author

grokys commented Nov 9, 2023

I think this PR should be ready for review now.

@grokys grokys requested a review from maxkatz6 November 9, 2023 08:57
@maxkatz6 maxkatz6 added this pull request to the merge queue Nov 12, 2023
Merged via the queue into master with commit 79acab1 Nov 12, 2023
7 checks passed
@maxkatz6 maxkatz6 deleted the fixes/13325-focus-root-scope branch November 12, 2023 02:26
@maxkatz6 maxkatz6 added the backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch label Nov 28, 2023
maxkatz6 pushed a commit that referenced this pull request Dec 5, 2023
@grokys @maxkatz6
Fix some issues with focus scopes (#13409)
fa69cf2 
* Remove focus hack from Popup.

* Added failing focus scope tests.

* Refactor focus scopes in FocusManager.

- Store focused element within a scope using an attached property (like WPF)
- Store current focus root so that focus can be restored to that root when a focused control or active focus scope is removed

Fixes #13325

* Suppress API compat error.

This was being produced for a compiler-generated enumerable class that was erroneously being included in the reference assembly for `FocusManager`.

* Remove focus hack from ContextMenu.

And add failing test now that the hack is removed.

* Try to return a rooted host visual.

Fixes failing test from previous comment where focus wasn't restored when closing a context menu.
#Conflicts:
#	src/Avalonia.Controls/Primitives/Popup.cs
@maxkatz6 maxkatz6 added backported-11.0.x and removed backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch labels Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jankrib jankrib jankrib left review comments

@maxkatz6 maxkatz6 maxkatz6 approved these changes

Assignees
No one assigned
Labels
backported-11.0.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Closing Popup sets wrong focus.
3 participants
@grokys @jankrib @maxkatz6