Don't enter a KeyboardNavigationMode.None group via its active element - #22154
Don't enter a KeyboardNavigationMode.None group via its active element#22154ethanstoner wants to merge 3 commits into
Conversation
A container with KeyboardNavigationMode.None is still entered when it has a TabOnceActiveElement pointing at one of its own children. The existing Next_None_Skips_Container sets the active element to a button outside the container, so it never covered this.
GetNextTab and GetPrevTab fall back to TabOnceActiveElement when entering a container, without checking the navigation mode. ItemsControl.OnGotFocus records the last focused child as that active element, so once a ListBox item has been focused the ListBox is entered on Tab even with KeyboardNavigation.TabNavigation="None". A TreeView with no focused item has no active element, which is why it looked like it behaved correctly. A None group now never hands out an element inside itself.
|
You can test this PR using the following package version. |
|
Please read the following Contributor License Agreement (CLA). If you agree with the CLA, please reply with the following: Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by AvaloniaUI OÜ and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant AvaloniaUI OÜ, and those who receive the Submission directly b. Patent License. You grant AvaloniaUI OÜ, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to AvaloniaUI OÜ. You agree to notify AvaloniaUI OÜ in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the Republic of Estonia, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and AvaloniaUI OÜ dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. |
🤦♂️ |
| return container; | ||
|
|
||
| // Using ActiveElement if set | ||
| var activeElement = GetActiveElement(container); |
There was a problem hiding this comment.
This code comes directly from WPF:
Does this issue also reproduce on WPF?
There was a problem hiding this comment.
No, it doesn't reproduce on WPF, and the reason is that WPF gates the write rather than the read.
KeyboardNavigation.UpdateActiveElement only records an active element for Once groups:
// Update ActiveElement only if container has TabNavigation = Once
if (GetKeyNavigationMode(container) == KeyboardNavigationMode.Once)
{
SetActiveElement(container, activeElement);
}That's the only write path: SetActiveElement is private with a single caller, and SetTabOnceActiveElement is only called from it. So in WPF a None container never has an active element, and the code you linked never gets the chance to follow one.
Avalonia records it unconditionally, in three places: ItemsControl.OnGotFocus (ItemsControl.cs:543), and the selection anchor in SelectingItemsControl.cs:539 and :971.
The selection path is the one in the report. The ListBox has an active element as soon as an item is selected, without ever being focused. That is also why the reporter's TreeView looked correct: it had no selected item, so nothing recorded one.
| if (e == null) | ||
| { | ||
| // Using ActiveElement if set | ||
| var activeElement = GetActiveElement(container); |
There was a problem hiding this comment.
Again, this code is directly from WPF:
I'd like to understand why we'd want to diverge from WPF.
There was a problem hiding this comment.
Agreed that matching WPF should be the default, so I tried WPF's approach first: gate the write the way UpdateActiveElement does and leave both of these hunks untouched.
It doesn't survive here. Avalonia uses TabOnceActiveElement for a second job that WPF doesn't give it, as the VirtualizingStackPanel focus anchor (VirtualizingStackPanel.cs:1140 and :1386).
Gating the write on Once broke that. Avalonia.Controls.UnitTests went from 3915 passed / 0 failed to 11 failures, all VirtualizingStackPanel focus tests (Focusing_Another_Element_Recycles_Original_Focus_Element, Focused_Element_Outside_Realized_Range_Is_Not_Arranged_In_Viewport and similar).
So the divergence is forced by that property carrying two responsibilities in Avalonia. The read side was where I could fix it without disturbing virtualization.
The cleaner long term option is to split the two concerns, a navigation active element and a separate virtualization anchor. Then Avalonia could take WPF's write gate verbatim and both of these hunks would go back to matching WPF exactly. That's a bigger change and your call, and I'm happy to do it that way if you'd prefer.
The existing tests set the active element by hand. This one goes through the path the report describes: a ListBox with TabNavigation="None" and a selected item, which records the active element via selection rather than focus. Fails without the navigation fix, passes with it.
|
Fair point on the sample app. I've added a ListBox level test that goes through the path in the report rather than setting the active element by hand: a ListBox with It fails on |
|
You can test this PR using the following package version. |
|
@ethanstoner is your human there? |
Fixes #22048.
Split into the two commits the contributing guide asks for: the failing tests first, then the fix.
Root cause
GetNextTabandGetPrevTabfall back toTabOnceActiveElementwhen entering a container, and neither checked the navigation mode first:ItemsControl.OnGotFocusrecords the last focused child as that active element:So once a
ListBoxitem has been focused, theListBoxhas an active element pointing inside itself, and Tab enters it even withTabNavigation="None".GetNextTabInGroupalready returnsnullforNone, so the group walk was correct; only this fallback leaked.This also explains the asymmetry in the report.
TreeViewis not inherently better behaved here, it just had no focused item in the repro and therefore no active element. ATreeViewwhose item has been focused should show the same bug.The fix
A
Nonegroup never hands out an element inside itself, not even a remembered one.GetNextTabskips the active-element lookup forNoneand falls through to the existing group walk, which already returns nothing forNone;GetPrevTabdoes the same.I deliberately left
IsTabStop(container)above it alone.Nonegoverns navigation within the group, and a container that is itself a tab stop should still be reachable, which matches WPF. Changing that would be a wider behavioural change than this issue calls for.Tests
Two new tests in
KeyboardNavigationTests_Tab, one per direction. Both fail on the first commit and pass on the second:after the fix:
Worth noting why this was not already covered: the existing
Next_None_Skips_Containerdoes callSetTabOnceActiveElement, but points it at a button in a sibling panel, which is also the expected result, so the assertion passes whether or not the fallback fires.Avalonia.Controls.UnitTestsas well, since this is a shared navigation path: 3915 total, 0 failed, 3914 succeeded, 1 skipped.Windows, .NET 10, Release.
Not verified
I have not run the attached sample app, only the unit tests, so I have not confirmed the end-to-end fix in a real window. I also have not checked whether
TreeViewreproduces the same bug once one of its items has been focused, though the analysis above says it should.AI usage
Per the contributing guide, which accepts AI-assisted contributions but wants a human in the loop: Claude Code (Claude Opus 5) helped locate the cause and draft this. I reviewed the diff and ran all the test runs above myself, and will handle review comments personally.