Fixes #5635. Hide active popover on DeRegister to clear stale cells - #5657
Conversation
DeRegister cleared the active popover without Hide, so SetNeedsDraw never ran. Keyboard Enter with a handled Accepting handler that moved focus took that path and left the dropped list's cells behind. Co-authored-by: Tig <tig@users.noreply.github.com>
The buffer-only check still passed without the fix because a later draw could hide the stale cells. Assert NeedsDraw on the dialog and the covered label immediately after Enter. Co-authored-by: Tig <tig@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Reviewed the production diff: Hide-before-unregister in Waiting on remaining Windows/macOS checks. Do not merge until those are green. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7465b0bd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Hide raises VisibleChanged. A handler can call Show and restore _activePopover before DeRegister returns. Clear that pointer after unregistering so the popover cannot stay active and unregistered. Co-authored-by: Tig <tig@users.noreply.github.com>
A VisibleChanged handler could call Show during Hide while the popover was still registered, leaving it visible. The new test failed on that order. DeRegister now removes from the registry first, then Hide, so Show cannot run. SetNeedsDraw still happens for #5635. Co-authored-by: Tig <tig@users.noreply.github.com>
WaitIteration used ExternalCancellationTokenSource! during Stop/Dispose. CleanupApplication can already have nulled that source, which produced the macOS IntegrationTests NRE on WithListView_NavigatesItems. Co-authored-by: Tig <tig@users.noreply.github.com>
2 participants
Fixes
Summary
A read-only DropDownList left the dropped list's text on screen after Enter when Accepting was handled and focus moved. Selection and focus were correct. The popover was no longer active. The cells it covered were not repainted. Mouse accept already repainted.
The keyboard path lost focus and called DeRegister while the popover was still the active one. DeRegister cleared
_activePopoverwithout Hide. Hide is the method that callsTopRunnableView.SetNeedsDraw(). A later Visible=false Hide then no-oped because the popover was no longer active.I hide the active popover so the covered region is invalidated. I unregister it first so a VisibleChanged handler cannot call Show during Hide and leave the popover visible, active, or registered.
Changes
ApplicationPopover.DeRegisterremoves the popover from the registry, then hides it if it was active.DropDownList.OnHasFocusChanginghides the list popover before DeRegister on focus loss.Testing
I wrote tests that fail on
developand pass with this change.DeRegister_ActivePopover_HidesAndInvalidatesTopRunnableasserts Hide and NeedsDraw.DeRegister_VisibleChangedShow_LeavesUnregisteredInactiveAndHiddenfailed onfa930207because Hide still saw the popover as registered. After unregister-before-Hide it passes: not registered, not active, not visible.Enter_HandledAccepting_DoesNotLeaveStalePopoverCellsis the issue repro.MouseAccept_HandledAccepting_DoesNotLeaveStalePopoverCellskeeps the mouse path honest.CI follow-up
macOS Integration Tests failed on
WithListView_NavigatesItems(dotnet driver) at SHA766b96f00. That test is ListView cursor navigation. It is not a DropDownList or popover test.The NRE was in
AppTestHelper.WaitIterationduring Stop/Dispose.CleanupApplicationcan already have nulledExternalCancellationTokenSourcewhile Stop still calls WaitIteration. WaitIteration used!on that source.This is a helper shutdown race. It is not caused by the DeRegister change. I did not Ignore the integration test.
I capture the CTS, treat a null or cancelled source as already stopped, and set Finished before clearing the source.
WaitIteration_WhenExternalCancellationTokenSourceIsNull_DoesNotThrowfailed on the old!dereference and passes after the harden.CI is green on
16d45a112, including Integration Tests on macOS, Ubuntu, and Windows.Pull Request checklist:
dotnet testbefore commit///style comments)To pull down this PR locally: