Skip to content

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
erikdarlingdata merged 2 commits into
devfrom
feature/4684-plus-tab-uia-and-row-label
Sep 29, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
feature/4684-plus-tab-uia-and-row-label

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

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 two Send-priority dispatcher operations about 0.3 ms apart.

  • Op 1: 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.
  • Op 2: 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 peer SetFocus() adds 1; SetFocus() then Select() adds 2; a real UIA client Select() adds 2. So a test that sends one peer Select() 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.sqlplan ran 117 times for 1 row in total: 1/117 = 0.0085 rows per execution against an estimate of 0.0096. The label's N0 printed 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 to Background priority 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 outrank Background, 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 by Reset(), and leaves the #2825 first-realization path and its _addTabInsertDeferred latch untouched.

Item 2: one shared formatter, PlanRowAccuracy.FormatActualOfEstimate, beside the ratio it explains; PlanViewerControl.Rendering.cs now 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

  • RED on the unfixed controller (new class StandalonePlanViewerPlusTabTests, Lite.Tests): UiaSelectOnPlus_FocusThenSelect expected 2 sub-tabs, got 3 (two added); UiaSelectOnPlus_SelectThenFocus same; 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 peer Select() (1 add), mouse-style IsSelected and SelectedItem (1 add each), two separate mouse selections (2 adds), and the host source pins.
  • GREEN after the fix: all 10 tests in that class, plus PlanViewerCapabilityPinTests (the [BUG] DarlingViewer crash when trying to View Stored Plan for High Impact Queries #2825 latch pin) and PlanViewer4632FallbackTests.
  • Item 2 RED: the formatter did not exist, so there was no red run. KeyLookupFixture_OldInlineLabel_PrintedZeroOfZeroAtEightyNinePercent pins that the old inline expression gives 0 of 0 (89%) on the fixture. New tests in Viewer4684RowLabelTests (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.
  • Full Lite.Tests: 5548 total, 0 failed (0 build warnings).
  • Darling.Tests classes named Viewer* (-class "*.Viewer*"): 2019 total, 0 failed, 127 skipped (I did not look into the skips; none are in the new classes).
  • Merged origin/dev (Custom tab headers draw the on-accent ink; the status-bar collector text follows a live theme switch (#4678, #4679) #4682 landed, it also edits StandalonePlanViewerController.cs); clean merge, both kept.
  • A committed real-UIA-client test. I ran one as a throwaway probe (not committed) for the numbers above. A committed one needs a shown, activated window and cross-thread UIA calls, which this suite avoids (see MainWindowAccessKeyTests), so the committed tests reproduce the pair with the exact call TabItem.OnPreviewGotKeyboardFocus makes plus the peer's Select().

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 TOTAL ActualRows against expectedRows = RowEstimateHelper.GetExpectedRows(node), which is EstimateRows * ActualExecutions on 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

  • The Dashboard has its own copy of the add-tab path (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.
  • Design call to double-check: the absorb is dispatcher-turn based, not time based. In the measured UIA pair the second operation arrived 0.3 ms after the first, ahead of any Background work. A heavily delayed second call could still add a second tab.
  • A value of 1 or over keeps N0 even next to a fraction: 3 of 0.4 (750%), and 0.6 of 1.2 prints 0.6 of 1 (50%).
  • The source pin that both hosts build sub-tabs through the controller is PlanViewerHost_BuildsItsSubTabsThroughTheSharedController in Lite.Tests/StandalonePlanViewerPlusTabTests.cs.

CHANGELOG

SECTION: Fixed
ENTRY:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant