FIX: Re-focus Action after RedrawUI #2043
Conversation
|
|
| public override void RedrawUI(ViewState viewState) | ||
| { | ||
| // Store the current focus before Clear/Rebuild | ||
| Focusable focusedElement = m_ActionsTreeView.focusController.focusedElement; |
There was a problem hiding this comment.
this is not a good workaround because the focused element that is retrieved here might not be valid after the m_ActionsTreeView.Rebuild().
The Rebuild can create and/or recycle visualelement. https://docs.unity3d.com/6000.0/Documentation/ScriptReference/UIElements.BaseVerticalCollectionView.Rebuild.html
The focused element might also not represent the right sub visualelement since it's a stack of focused element.
RefreshItems() could mitigate the refres problem but the behavior is not predictable across version
https://docs.unity3d.com/6000.0/Documentation/ScriptReference/UIElements.BaseVerticalCollectionView.RefreshItems.html
There was a problem hiding this comment.
I can see now that it is not reliable since the test I added fails at random.
RefreshItems() doesn't work because the issue occurs in virtualizationController.Refresh() which is called by both. RefreshItems causes the issue intermittently instead, but also causes some other selection issues
| } | ||
|
|
||
| [UnityTest] | ||
| public IEnumerator CanRenameAction() |
There was a problem hiding this comment.
great to see a test for this! There were a lot of cases on renaming and focus issues already.
tspiller
force-pushed
the
isxb-1024-action-navigation-after-rename
branch
from
de1aa47 to
aeedaec
Compare
tspiller
changed the title
There was a problem hiding this comment.
I don't have any objections, it looks good to me. @bmalrat has more insights on the UITK focus particularities though. I also add @Pauliusd01 for review.
There was a problem hiding this comment.
If it works, it works. Since you're extending TreeViewItem, which already does a lot of things, I don't think it would be easy to rewrite the Rename logic to make the item not lose focus in the first place. If you want more insight on TreeViewItem itself though, you can ask Guillaume Riendeau.
| //Everything else has already been taken restored in OnEditTextFinished() but this has to happen after | ||
| //listView/treeView reclaims the focus in RedrawUI | ||
| label.Q<Label>().Focus(); | ||
| IsTextFieldFocused = false; |
There was a problem hiding this comment.
IsTextFieldFocused should not be changed here. It should be driven by ui toolkit when the item lose focus. Is it necessary?
| //get moved again using a similar workaround when finished editing text | ||
| //Everything else has already been taken restored in OnEditTextFinished() but this has to happen after | ||
| //listView/treeView reclaims the focus in RedrawUI | ||
| label.Q<Label>().Focus(); |
There was a problem hiding this comment.
I have some concern on this line.
If the rename end by a clic in an other area, like action maps list, does it steal the focus?
| return; | ||
| m_ActionsTreeView.ScrollToItemById(id); | ||
| var treeViewItem = m_ActionsTreeView.GetRootElementForId(id)?.Q<InputActionsTreeViewItem>(); | ||
| treeViewItem?.FocusOnRenameFinish(); |
There was a problem hiding this comment.
Does it really needs to use this new function ?
the Id here might not be the expected one. If during the rename you click on another element "id" will be the selected id and not the one that was renamed.
Locally it seems to work well with treeViewItem?.FocusOnRenameFinish(); replaced by treeViewItem?.Focus();
6 participants
Description
FocusOnRenameTextField() changes the focus to the renameTextfield explicitly, this moves the focus back again using a similar workaround when finished editing text.
Testing status & QA
Tested locally on 6000. Added new CanRenameAction test.
Overall Product Risks
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: