Repository navigation
Plan viewer: a screen-reader select on the + tab adds one sub-tab, and a row label under 1 agrees with its percentage (#4684) - #4688
Merged
Conversation
…d a sub-1 row label agrees with its percentage (#4684)
…uia-and-row-label
erikdarlingdata
marked this pull request as ready for review
September 29, 2026 00:12
This was referenced Sep 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4684.
Two fixes in the shared plan viewer (
PerformanceMonitor.Ui, used by Lite and the Darling Viewer). Item 2 has no issue of its own.Why
1. A UI Automation select on "+" added two sub-tabs (#4684). I reproduced it with a real UIA client round trip against a shown window: 3 sub-tabs plus "+" became 5 plus "+" after one
SelectionItemPattern.Select().Root cause, measured with dispatcher hooks and stack traces (not guessed): the UIA core does not call
Select()alone. It first sets focus on the item, then selects it, as twoSend-priority dispatcher operations about 0.3 ms apart.AutomationPeer.SetFocus->UIElement.Focus->TabItem.OnPreviewGotKeyboardFocus. A TabItem selects itself when it gets focus, so "+" is selected and the controller adds a tab and selects it.SelectorItemAutomationPeer.Select->SelectionChanger.SelectJustThisItem. The first add already moved the selection off "+", so this is a real selection change of "+" again, and the controller adds a second tab.Direct peer measurements at dev 003980e with the same rig: a lone
ISelectionItemProvider.Select()adds 1; a lone peerSetFocus()adds 1;SetFocus()thenSelect()adds 2; a real UIA clientSelect()adds 2. So a test that sends one peerSelect()and expects 2 tabs does not fail on today's code, because it skips the focus step the UIA core adds. The regression tests below reproduce the pair instead.2. "0 of 0 (89%)". The Key Lookup in
key_lookup_plan.sqlplanran 117 times for 1 row in total: 1/117 = 0.0085 rows per execution against an estimate of 0.0096. The label'sN0printed both as "0" and still printed the 89% they make.What changes
Fix used for item 1: the fallback, a gesture-scoped absorb, in
StandalonePlanViewerController. The duplicate cannot be stopped at its source. The pair comes from the UIA core, and "focus selects the tab" is WPF's. Stopping focus from selecting "+" would also stop keyboard arrow navigation and mouse clicks from adding a tab, which is a design change I did not make. So the first add now opens a gesture (_gestureAddedSubTab); a "+" selection that lands before the dispatcher has drained toBackgroundpriority re-selects the tab just added instead of adding another. The add's own queued layout and render work, and the UIA client's second operation, all outrankBackground, so the pair lands inside the gesture and a later click does not. The latch only re-selects a tab that still exists, is cleared byReset(), and leaves the #2825 first-realization path and its_addTabInsertDeferredlatch untouched.Item 2: one shared formatter,
PlanRowAccuracy.FormatActualOfEstimate, beside the ratio it explains;PlanViewerControl.Rendering.csnow calls it. Values of 1 and over print exactly as before (N0). A value strictly between 0 and 1 prints in significant digits: two to begin with, and more only when the two printed numbers, divided, would not round to the printed percentage. Zero prints "0". The percentage is never hidden (it is left off only when the estimate is 0, as before). Culture follows the current culture, like the interpolated label it replaces.Before / after on the fixture:
0 of 0 (89%)->0.0085 of 0.0096 (89%).Test plan
StandalonePlanViewerPlusTabTests, Lite.Tests):UiaSelectOnPlus_FocusThenSelectexpected 2 sub-tabs, got 3 (two added);UiaSelectOnPlus_SelectThenFocussame;TwoSeparateSelectionsOfPlus(uia)expected 3, got 5 (four added for two gestures); the closed-tab test in its first form expected 2, got 3. Green before and after: lone peerSelect()(1 add), mouse-styleIsSelectedandSelectedItem(1 add each), two separate mouse selections (2 adds), and the host source pins.PlanViewerCapabilityPinTests(the [BUG] DarlingViewer crash when trying to View Stored Plan for High Impact Queries #2825 latch pin) andPlanViewer4632FallbackTests.KeyLookupFixture_OldInlineLabel_PrintedZeroOfZeroAtEightyNinePercentpins that the old inline expression gives0 of 0 (89%)on the fixture. New tests inViewer4684RowLabelTests(Darling.Tests): the exact key-lookup string, a set of 1-and-over cases that must not change, fraction cases with exact strings, a 2000-pair fixed-seed sweep proving the printed numbers reproduce the printed percentage, a de-DE culture case, and a source pin that the node label goes through the formatter.Lite.Tests: 5548 total, 0 failed (0 build warnings).Viewer*(-class "*.Viewer*"): 2019 total, 0 failed, 127 skipped (I did not look into the skips; none are in the new classes).StandalonePlanViewerController.cs); clean merge, both kept.MainWindowAccessKeyTests), so the committed tests reproduce the pair with the exact callTabItem.OnPreviewGotKeyboardFocusmakes plus the peer'sSelect().PerformanceStudio (read-only)
Does PerformanceStudio's desktop viewer print "0 of 0 (NN%)" for that Key Lookup? No. Its node label is
src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs:311:Text = $"{node.ActualRows:N0} of {expectedRows:N0}{accuracy}",. It compares the TOTALActualRowsagainstexpectedRows = RowEstimateHelper.GetExpectedRows(node), which isEstimateRows * ActualExecutionson the inner side of a Nested Loops (src/PlanViewer.Core/Services/RowEstimateHelper.cs:52-57). For the Key Lookup that is 0.00964372 x 117 = 1.13, so I expect "1 of 1 (89%)" (from reading the code, not by running it). It scales the estimate up rather than the actual down, so it does not hit this problem. Nothing in PerformanceStudio was edited.Notes
deprecated/Dashboard/MainWindow.PlanViewer.cs, with its own__PLAN_ADD_TAB__handling); left alone as instructed. I found no Dashboard copy of the row label.Backgroundwork. A heavily delayed second call could still add a second tab.N0even next to a fraction:3 of 0.4 (750%), and0.6 of 1.2prints0.6 of 1 (50%).PlanViewerHost_BuildsItsSubTabsThroughTheSharedControllerinLite.Tests/StandalonePlanViewerPlusTabTests.cs.CHANGELOG
SECTION: Fixed
ENTRY:
REF:
[Plan viewer: a screen-reader select on the + tab adds one sub-tab, and a row label under 1 agrees with its percentage (#4684) #4688]: Plan viewer: a screen-reader select on the + tab adds one sub-tab, and a row label under 1 agrees with its percentage (#4684) #4688
Addendum (moved from a PR comment)
Addendum: the gesture latch closes at Background priority, and Input priority outranks Background. So a second physical click on the plus tab that reaches the dispatcher before the Background clear runs (an unusually fast double click on a busy UI thread) is coalesced into one add. On an idle thread a human double click is far slower than the drain, and the tests only cover selections that the dispatcher separates (TwoSeparateSelectionsOfPlus pumps to SystemIdle between them). That matches the intended behavior: a second selection that lands before the dispatcher has handled the first add is absorbed.