Closing Popup sets wrong focus.

[grokys]

Describe the bug

Closing a popup can set focus to the wrong control. Reported by @jankrib .

To Reproduce

  <Window.Styles>
    <Style Selector="Button:focus">
      <Setter Property="Background" Value="Green" />
    </Style>
  </Window.Styles>
  <StackPanel Margin="10">
    <ToggleButton Name="ShowToggle" Focusable="False">
      <TextBlock Text="Show" />
    </ToggleButton>
    <Button Name="Target" Content="?" HorizontalAlignment="Center"/>
    <Popup IsOpen="{Binding #ShowToggle.IsChecked}" PlacementTarget="{Binding #Target}" PlacementMode="Left">
      <Border Background="Red" Width="200" Height="100">
      </Border>
    </Popup>
    <StackPanel>
      <Button Content="Click me to delete to set focus to null" Click="Clicked"/>
      <Button Content="Click me to delete to set focus to null" Click="Clicked"/>
      <Button Content="Click me to delete to set focus to null" Click="Clicked"/>
    </StackPanel>
  </StackPanel>

public void Clicked(object sender, RoutedEventArgs args)
{
	if (sender is Button button)
	{
		button.FindLogicalAncestorOfType<StackPanel>()?.Children.Remove(button);
	}
}

1. Click "Show". You will see the red popup open
2. Click a button with the text "Click me to delete to set focus to null"
3. Click "Show" to hide the popup
4. The "?" button is focused, which was never user-focused

Expected behaviour

Either:

• The "Show" button should be focused as that's what was focused when the popup was shown; or
• There should be no focus as after the popup was shown, a button was focused which was then removed (setting focus to null)

Environment

• OS: Windows/all
• Avalonia-Version: 11.0.5

Additional context

This behaviour was added in #4809 which said:

Alternatively, we could store the current focus on open and restore it on close (this should be the PlacementTarget anyway)

This may be a better solution. An even better solution might be to let our focus scope code handle this.

[jankrib]

For expected behavior, I would argue it should be:
The "Show" button should be focused because closing a popup that does not have keyboard focus inside should not affect focused element.

Additional context:
In my view the main problem here is the subtle difference between KeyboardFocus and Focus that exists because of different focus scopes. This was a problem in WPF too just because of naming. IsFocused property on a control doesn't really tell you. You just have to know that if it is not called something with Keyboard it means scoped.

Take this code in InputElement:

        protected override void OnDetachedFromVisualTreeCore(VisualTreeAttachmentEventArgs e)
        {
            base.OnDetachedFromVisualTreeCore(e);

            if (IsFocused)
            {
                FocusManager.GetFocusManager(this)?.ClearFocus();
            }
        }

What it does here is to say if this control is focused in it's scope clear focus for the app.

The introduction of FocusManager.GetFocusManager(this) leads to additional confusion because it applies that there is an additional level in the focus tree where you can have multiple managers per app. As far as I know there is not.

[grokys]

The "Show" button should be focused because closing a popup that does not have keyboard focus inside should not affect focused element.

But by clicking the "Click me to delete to set focus to null" button, the "Show" button is no longer focused. So if this were to be refocused on close, then we might have unwanted behavior again.

In my view the main problem here is the subtle difference between KeyboardFocus and Focus that exists because of different focus scopes.

There are no focus scopes in play here. The only focus scope is the Window in this example.

[jankrib]

PopupRoot is a focus scope. It doesn't matter in this version of the example since we use the delete buttons to make focus null. The focus scope issue is probably best defined as a separate issue.

The reason why I argued the "Show" should have focus is not because it had focus before you click the delete button. I should have probably made a separate "Hide" button for the example. When you click the "Show" button to hide the popup you expect the button to get focus because you click it. In the example above the "Show" button has Focusable set to false, though. So I guess expected focus should be that it remains null.

[grokys]

PopupRoot is a focus scope

Are you sure? It doesn't implement IFocusScope, and I can't see any other focus scopes being registered in the focus manager

When you click the "Show" button to hide the popup you expect the button to get focus because you click it.

But the "Show" button is explicitly marked as non-focusable?

[jankrib]

PopupRoot implements IPopupHost which implements IFocusScope.

Yes. So in this example it shouldn't.

[grokys]

PopupRoot implements IPopupHost which implements IFocusScope.

Ah ok sorry, I missed that transitive reference. Ok, there's definitely something wrong with focus scopes in this case then. Investigating further.

[grokys]

Tested this with WPF, and WPF sets the keyboard focus via the following mechanism:

• Control is removed from tree, which sets IsVisible = false
• KeyboardDevice.OnIsVisibleChanged is called; which calls
• KeyboardDevice.ReevaluateFocusAsync; which Dispatcher.Invokes
• KeyboardDevice.ReevaluateFocusCallback which has the comment "The default action is to reset focus to the root element of the active presentation source."
• Focus is reset to the root element which causes FrameworkElement.OnPreviewGotKeyboardFocus to be called; which calls
• FocusManager.GetFocusedElement
• Which reads the value of the FocusedElementProperty which contains the focused element for the root scope
• And sets the focus to that

The end result being that the focused element in the Window scope is restored.

[grokys]

My suggestion is to remove the hackfix in #4809 and modify our focus scopes to restore focus to any outermost focus scope when FocusManager.ClearFocus is called (e.g. on removing, hiding, disabling a control). The behavior of ClearFocus would then be:

• If the current focus scope is the outermost focus scope, then focus will be cleared as currently. Note: Key events are still sent directly to the Window even though no focus is present
  • This mirrors WPF to some extent which sets a kind of implicit focus to the window, but doesn't raise any focus events
  • The main difference between Avalonia and WPF here is that in WPF the Window is reported as focused, even though it didn't receive any GotFocus events to tell it that it got focus, whereas null is reported as focused in Avalonia, even though the Window still gets key events.
  • Yes it's a mess in both frameworks
• If the focus scope is a nested focus scope, then the root focus scope will be activated and the focused control in that scope will be keyboard-focused

Note that the above isn't special-casing the root focus scope, it's just that we're removing the control from the root focus scope and so it by definition won't have a focused element.

Pinging @maxkatz6 @amwx for feedback as you've both worked on focus more recently than me.