WIP: [Accessibility] Fixing TabPage keyboard tooltips #2719

vladimir-krestov · 2020-01-15T12:18:31Z

Fixes #2717
Original bug: 604799: [Accessibility] TabPage has no keyboard tooltip

Proposed changes

Add a virtual IsHoveredWithMouse property to change its value in inherited classes
Change switch-case implementation in KeyboardToolTipStateMachine.cs due to extremely inconvenient use when debugging
Implement keyboard tooltips for TabPages. Now a Tab and a Page have one tooltip
Add a TabControl test app

Customer Impact

A user can get a tooltip using a keyboard

Regression?

No

Risk

Minimal

Screenshots

Before

TabPages have no keyboard tooltips
A TabPage tab has an incorrect tooltip text (TabControl text instead TabPage.ToolTipText property value)
A user gets only a TabControl tooltip using a Tab-navigation

After

TabPages have keyboard tooltips
TabPage tab has the same tooltip as a page. If set a new tooltip or change ToolTipText property value it will affect both tooltips. It is a simulation of that a tab tooltip and a page tooltip are one united tooltip
A user can get TabControl tooltip if this TabControl has no pages

Test methodology

CTI
Manual UI testing

Test environment(s)

SDK Version: 5.0.100-alpha1-016143
Microsoft Windows [Version 10.0.18363.592]

Microsoft Reviewers: Open in CodeFlow

vladimir-krestov · 2020-01-15T12:36:50Z

Have not tested yet

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs

src/System.Windows.Forms/src/System/Windows/Forms/KeyboardToolTipStateMachine.cs

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs

src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/TabControlTest.cs

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs

codecov · 2020-02-12T15:40:16Z

Codecov Report

Merging #2719 into master will decrease coverage by 2.29143%.
The diff coverage is 38.2716%.

@@                 Coverage Diff                 @@
##              master       #2719         +/-   ##
===================================================
- Coverage   62.16042%   59.86899%   -2.29143%     
===================================================
  Files           1259        1241         -18     
  Lines         449450      431873      -17577     
  Branches       39227       38826        -401     
===================================================
- Hits          279380      258558      -20822     
- Misses        164590      167936       +3346     
+ Partials        5480        5379        -101

Flag	Coverage Δ
#Debug	`59.86899% <38.2716%> (-2.29144%)`	⬇️
#production	`32.42114% <27.0073%> (-0.94061%)`	⬇️
#test	`98.98365% <100%> (+0.00914%)`	⬆️

vladimir-krestov · 2020-02-13T10:10:31Z

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs

@@ -1225,6 +1246,7 @@ public void SetToolTip(Control control, string caption)
        {
            TipInfo info = new TipInfo(caption, TipInfo.Type.Auto);
            SetToolTipInternal(control, info);
+            control.LostFocus += (object sender, EventArgs e) => Hide(control);


I added this handler to fix a tooltip showing for a control when it loses focus.

Without this handler:

Incorrect behavior when some different ToolTip instances are set:

Incorrect behavior when one ToolTip instance is set:

With this handler:

Expected work:

@RussKie, @M-Lipin, @Tanya-Solyanik, @JuditRose, @hughbe, @weltkante, could you please look and say what negative effect this might give for other controls and tooltips? I did not find any bugs.

Well, I recognize the behavior you describe and try to fix, I have seen it before in our applications but I never digged down to find its root cause. I don't know if the behavior was intended, so with your change you could either be breaking intended behavior or fixing a bug.

Either way your implementation is incorrect and can't be left this way, you are subscribing within a public API without ever unsubscribing, so multiple calls will accumulate event handlers. The typical pattern is unsubscribing and resubscribing - the first unsubscribe cleans up previous subscriptions (no-op if none exist). However since you are using a lambda capturing the control as a closure variable I don't know if this pattern works here. (I don't know how equality works when closures are in play, I'd expect each closure to be distinct so to unsubscribe you'd have to save the closure away, but I may be mistaken. If you want to test it you'd have to check that its not called multiple times after multiple SetToolTip calls by putting breakpoints or debug output into the LostFocus handler. You'd also have to check that calling SetToolTip on multiple controls doesn't unsubscribe each other.)

Good points, I will look at ways to fix these issues. Thank you!

@weltkante, I changed the implementation. Subscription and Unsubscription happen once now when setting a tooltip many times, I checked. Please review.

Also, I'll ask testers to test these changes using automated and manual testing.

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs

weltkante

The approach looks right but might need some more fine-tuning, I think you are still double-subscribing.

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs

src/System.Windows.Forms/src/System/Windows/Forms/TabControl.cs

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs

vladimir-krestov · 2020-02-20T11:45:45Z

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs

@@ -10971,6 +10971,19 @@ internal static IntPtr SetUpPalette(IntPtr dc, bool force, bool realizePalette)
            return result;
        }

+        protected virtual void SetToolTip(ToolTip toolTip, string toolTipText)


I used protected for SetToolTip method thereby adding API for developers and they can change this implementation.
This is correct or I need to hide this protected API using the private protected modifier?
/cc: @RussKie, @Tanya-Solyanik, @merriemcgaw

Any custom control must be able to hook into the tooltip display chain to override the display logic, if necessary. With that, I believe, should be made protected.

This change to the public surface will not require designer changes, I don't see any problems with it.

How would you explain, when to use Tooltip.SetToolTip and when to use Control.SetToolTip?
Perhaps this method can be named differently?

That's the thing, I can't see any real benefit of using one over the other.
myToolTip.SetToolTip(myControl, toolTipText) calls into myControl.SetToolTip(myToolTip, toolTipText) which recurses.

I can guess the original idea of ToolTip was to extend arbitrary controls. However then a number of controls decided to displaying tooltips in their own way (e.g. Label when its text is ellipsised) by keeping own instances of ToolTip and exposing properties like ToolTipText. So now we have an inconsistent devex because for some controls a developer must use an instance of own ToolTip, and for some other control - not.

With this API cleanup we further blurring the line and removing reasons to use ToolTip control. Instead making it a little more consistent experience - each Control allows setting its tooltips in a common manner. There be 🐉 🐉 though (e.g. the Label's case) that we'll likely need to resolve. We touched on these with @vladimir-krestov and he was meant to raise issues for tracking purposes.
Also now it may be possible to reduce number of instances of ToolTip, have one global singleton per app and pass it to all controls that require tooltips.

Now we can probably take this further and question whether we can have a single instance of a ToolTip provider/renderer. But that's a whole separate discussion.

Tanya-Solyanik

Looks good, please see my comments though

Tanya-Solyanik · 2020-02-25T21:21:51Z

src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/TabPageTests.cs

+        {
+            using TabPage tabPage = new TabPage();
+            tabPage.CreateControl();
+            string expected = "Some text";


consider factoring this string out into a const field and using it everywhere in this file. Not necessarily in this same PR

Tanya-Solyanik · 2020-02-25T21:23:02Z

src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/TabControlTest.cs

+    {
+        public MyLabel()
+        {
+            this.SetToolTip(new ToolTip(), "balas");


please remove this qualifier

Tanya-Solyanik · 2020-02-25T21:24:07Z

src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/TabControlTest.cs

+        public TabControlTest()
+        {
+            InitializeComponent();
+            //toolTip.SetToolTip(tabControl1, "TabControl");


please remove the commented out line

Tanya-Solyanik · 2020-02-25T21:26:29Z

src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/TabControlTest.cs

+
+        private void ButtonClick(object sender, EventArgs e)
+        {
+            // It will be removed after testing


If this was useful for feature development, it will be also useful when debugging this feature. Are there any problems with keeping this test?

The project "MultipleControls" test app already has TabControl inside for testing so I planned to remove the current app after the fix. Do you think we need a separate test app for TabControl to test TabPage tooltips work? If so, I'll refactor it later.

Tanya-Solyanik · 2020-02-25T21:27:07Z

src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/TabControlTest.cs

+                    toolTip.SetToolTip(tabPage1, "This is page 1");
+                    break;
+                case 2:
+                    tabPage2.ToolTipText = "2) Some tt text";


tt-> tooltip

Tanya-Solyanik · 2020-02-25T22:09:44Z

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs

-            CheckNativeToolTip(control);
-            CheckCompositeControls(control);
-
+            control?.SetToolTipInternal(this, GetToolTip(control));


on this line we know that control is not null be cause cast on line 644 succeed

Tanya-Solyanik · 2020-02-25T22:19:54Z

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs

+            }
+        }
+
+        private protected override bool IsHoveredWithMouse


Consider a simplified name - IsHovered

This property overrides Control.IsHoveredWithMouse property, which related to IKeyboardToolTip.IsHoveredWithMouse() method.

I think it will be incorrect to make different names.

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs

src/System.Windows.Forms/src/System/Windows/Forms/UpDownBase.cs

RussKie · 2020-02-24T09:34:51Z

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs

@@ -10931,6 +10931,19 @@ internal static IntPtr SetUpPalette(IntPtr dc, bool force, bool realizePalette)
            return result;
        }

+        protected virtual void SetToolTip(ToolTip toolTip, string toolTipText)


We need to have xml-docs, so they will flow into the docs.

M-Lipin

Looks good to me. There are several minor points to refactor and the point about tight coupling between Controls and ToolTips: Control references ToolTips in Control.SetToolTip() method and ToolTip references Control in ToolTip.SetToolTip() method. It would be better to refactor it to reduce coupling.

M-Lipin · 2020-02-27T07:42:40Z

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs

+        {
+            if (!IsHandleCreated || toolTip == null)
+            {
+                return;


Why return;? It seems to me that method should call CreateHandle(); in case IsHandleCreated == false and should throw an ArgumentNullException in case toolTip == null.

Definitely not, you must not call CreateHandle when setting a tooltip, this would cause handle instantiation in the constructor during InitializeComponents before all designer properties have been set. The rest of the tooltip infrastructure has logic to not create handles and store the tooltip somewhere else when the handle is not created.

If anything you could throw an exception if this code path is taken when no handle is available, because it would be a bug in the internal logic.

M-Lipin · 2020-02-27T07:52:50Z

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs

@@ -10931,6 +10931,19 @@ internal static IntPtr SetUpPalette(IntPtr dc, bool force, bool realizePalette)
            return result;
        }

+        protected virtual void SetToolTip(ToolTip toolTip, string toolTipText)


I see that toolTipText is not used in some implementation. Please use default value for this.

I'll do, thanks!

M-Lipin · 2020-02-27T07:56:13Z

src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs

-            this.toolTipCaption = toolTipCaption;
+            if (toolTip == null)
+            {
+                return;


It seems that call SetToolTip() with toolTip = null is not correct and we should throw ArgumentNullException. Maybe argument checks can be added to the base implementation and overridden methods should call base.SetToolTip() before the remaining implementation.

That's a good call.
We'll need to record this in dotnet/docs#17085

M-Lipin · 2020-02-27T07:59:20Z

src/System.Windows.Forms/src/System/Windows/Forms/TabControl.cs

        {
+            if (toolTip == null || !ShowToolTips)
+            {
+                return;


The same as previous: arguments check.

M-Lipin · 2020-02-27T08:03:28Z

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs

+        {
+            if (toolTip == null)
+            {
+                return;


The same as previous: argument check.

M-Lipin · 2020-02-27T08:05:40Z

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs

+            }
+
+            // Show the same tooltip for the page's tab.
+            if (toolTipText != null && ToolTipText != toolTipText)


The test ToolTipText != toolTipText is already in the property setter implementation.

M-Lipin · 2020-02-27T08:11:22Z

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs

        {
-            if (toolTip != null)
+            if (toolTip == null || !ShowNodeToolTips)


Check `toolTip' for null in base method.

vladimir-krestov · 2020-09-09T07:16:04Z

Will be reopened later

vladimir-krestov requested a review from a team as a code owner January 15, 2020 12:18

vladimir-krestov self-assigned this Jan 15, 2020

hughbe reviewed Jan 15, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs Outdated Show resolved Hide resolved

hughbe reviewed Jan 15, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs Outdated Show resolved Hide resolved

RussKie reviewed Jan 15, 2020

View reviewed changes

RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Jan 15, 2020

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jan 22, 2020

Tanya-Solyanik reviewed Jan 22, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs Outdated Show resolved Hide resolved

Tanya-Solyanik reviewed Jan 22, 2020

View reviewed changes

src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/TabControlTest.cs Show resolved Hide resolved

vladimir-krestov changed the title ~~[Accessibility] Fixing TabPage keyboard tooltips~~ WIP: [Accessibility] Fixing TabPage keyboard tooltips Jan 28, 2020

vladimir-krestov force-pushed the dev/v-vlkres/TabControlToolTips branch from 06ce4fe to b482c78 Compare January 29, 2020 10:48

RussKie reviewed Jan 31, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs Outdated Show resolved Hide resolved

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs Show resolved Hide resolved

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.cs Outdated Show resolved Hide resolved

vladimir-krestov added the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Feb 13, 2020

vladimir-krestov commented Feb 13, 2020

View reviewed changes

vladimir-krestov commented Feb 17, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs Outdated Show resolved Hide resolved

weltkante suggested changes Feb 17, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs Outdated Show resolved Hide resolved

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs Outdated Show resolved Hide resolved

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs Outdated Show resolved Hide resolved

ghost added 📭 waiting-author-feedback The team requires more information from the author and removed 📭 waiting-author-feedback The team requires more information from the author labels Feb 17, 2020

vladimir-krestov added the waiting-review This item is waiting on review by one or more members of team label Feb 17, 2020

RussKie requested changes Feb 18, 2020

View reviewed changes

ghost added 📭 waiting-author-feedback The team requires more information from the author and removed 📭 waiting-author-feedback The team requires more information from the author labels Feb 18, 2020

weltkante reviewed Feb 20, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs Show resolved Hide resolved

vladimir-krestov commented Feb 20, 2020

View reviewed changes

vladimir-krestov force-pushed the dev/v-vlkres/TabControlToolTips branch from e6081ad to 378d6ea Compare February 21, 2020 11:58

Tanya-Solyanik reviewed Feb 25, 2020

View reviewed changes

RussKie reviewed Feb 26, 2020

View reviewed changes

M-Lipin reviewed Feb 27, 2020

View reviewed changes

vladimir-krestov added 23 commits March 31, 2020 17:39

Fixed a typo

da8eca3

Fixed build error

556d5df

Fixed review points

1502ec6

Fixed setting a tooltip text in the first time

3090dfa

Fixed getting tooltips

d5ae952

Added unit tests

893a559

Made code refactoring

8d76402

Fixed getting a tab tooltip position (magic)

2628a12

Fixed CI build errors

438be4a

Fixed a tooltips synchronization

7ab23e6

A bit of code refactoring

d996028

Fixed some test issues

f4a8c28

Fixed a declaration

6033690

Fixed tooltip showing for internal items

a75c4a4

Fixed handling LostFocus event

5c37142

Fixed event handling implementation

23cb2fc

Code refactoring

1294182

Fixed review issues

d687164

Inverted dependencies for all controls inside ToolTIp.SetToolTip method

f813971

Fixed the build

790e322

Code refactoring

15f4d8c

Fixed the conflict bugs

da75950

Fixed build

3d6baa8

vladimir-krestov force-pushed the dev/v-vlkres/TabControlToolTips branch from 378d6ea to 3d6baa8 Compare March 31, 2020 15:06

vladimir-krestov marked this pull request as draft April 24, 2020 07:17

vladimir-krestov closed this Sep 9, 2020

vladimir-krestov mentioned this pull request Jan 1, 2021

Adding the ability to get keyboard toolTips for TabPages of a TabControl #4421

Merged

ghost added the draft draft PR label Sep 11, 2021

ghost locked as resolved and limited conversation to collaborators Feb 2, 2022

WIP: [Accessibility] Fixing TabPage keyboard tooltips #2719

WIP: [Accessibility] Fixing TabPage keyboard tooltips #2719

Conversation

vladimir-krestov commented Jan 15, 2020 • edited by ghost Loading

Proposed changes

Customer Impact

Regression?

Risk

Screenshots

Before

After

Test methodology

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

vladimir-krestov commented Jan 15, 2020

codecov bot commented Feb 12, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Without this handler:

With this handler:

weltkante Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltkante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Lipin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltkante Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimir-krestov commented Sep 9, 2020

vladimir-krestov commented Jan 15, 2020 •

edited by ghost

Loading

codecov bot commented Feb 12, 2020 •

edited

Loading

weltkante Feb 13, 2020 •

edited

Loading

weltkante Feb 27, 2020 •

edited

Loading