-
Notifications
You must be signed in to change notification settings - Fork 988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: [Accessibility] Fixing TabPage keyboard tooltips #2719
Changes from all commits
8e06755
da8eca3
556d5df
1502ec6
3090dfa
d5ae952
893a559
8d76402
2628a12
438be4a
7ab23e6
d996028
f4a8c28
6033690
a75c4a4
5c37142
23cb2fc
1294182
d687164
f813971
790e322
15f4d8c
da75950
3d6baa8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
override System.Windows.Forms.TabControl.OnGotFocus(System.EventArgs e) -> void | ||
override System.Windows.Forms.TabControl.OnLostFocus(System.EventArgs e) -> void | ||
override System.Windows.Forms.TabPage.OnGotFocus(System.EventArgs e) -> void | ||
override System.Windows.Forms.TabPage.OnLostFocus(System.EventArgs e) -> void | ||
virtual System.Windows.Forms.Control.SetToolTip(System.Windows.Forms.ToolTip toolTip, string toolTipText) -> void | ||
override System.Windows.Forms.UpDownBase.SetToolTip(System.Windows.Forms.ToolTip toolTip, string toolTipText) -> void | ||
override System.Windows.Forms.TabControl.SetToolTip(System.Windows.Forms.ToolTip toolTip, string toolTipText) -> void | ||
override System.Windows.Forms.Label.SetToolTip(System.Windows.Forms.ToolTip toolTip, string toolTipText) -> void | ||
override System.Windows.Forms.ListView.SetToolTip(System.Windows.Forms.ToolTip toolTip, string toolTipText) -> void | ||
override System.Windows.Forms.TabPage.SetToolTip(System.Windows.Forms.ToolTip toolTip, string toolTipText) -> void | ||
override System.Windows.Forms.TreeView.SetToolTip(System.Windows.Forms.ToolTip toolTip, string toolTipText) -> void |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10918,6 +10918,19 @@ internal static IntPtr SetUpPalette(IntPtr dc, bool force, bool realizePalette) | |
return result; | ||
} | ||
|
||
protected virtual void SetToolTip(ToolTip toolTip, string toolTipText) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to have xml-docs, so they will flow into the docs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll do, thanks! |
||
{ } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please place the closing brace on a new line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason can't respond to the other comment above
protected methods are usually not to be called by user code, but you are right, naming should be improved
How about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If user builds a custom control then all
Whilst we may have few odd ducks, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If you want to look for alternatives there is also There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There are two ways of doing this now: public class MyForm : Form
{
private ToolTip myToolTip = new ToolTip();
public void Method()
{
ToolTip myToolTip = new ToolTip();
// we can do this...
this.SetToolTip(myToolTip , toolTipText);
// ...or this
myToolTip.SetToolTip(this, toolTipText);
}
}
// - or -
public class MyCustom : UserControl
{
private ToolTip myToolTip = new ToolTip();
public void Initialise(ToolTip myToolTip)
{
// we can do this...
this.SetToolTip(myToolTip, toolTipText)
// ...or this
myToolTip.SetToolTip(this, toolTipText);
}
} I see the point, it is easy to get confused. 🤔 Though in the most cases calling Maybe public class MyCustom : UserControl
{
private ToolTip myToolTip = new ToolTip();
public void Initialise(ToolTip myToolTip)
{
myToolTip.SetToolTip(this, toolTipText); // <-- this will call into MyCustom.AssignControlToolTip()
}
protected override void AssignControlToolTip(ToolTip toolTip, string toolTipText)
{
// custom logic goes here
}
} |
||
|
||
internal void SetToolTipInternal(ToolTip toolTip, string toolTipText) | ||
{ | ||
if (!IsHandleCreated || toolTip == null) | ||
{ | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
SetToolTip(toolTip, toolTipText); | ||
} | ||
|
||
protected void SetTopLevel(bool value) | ||
{ | ||
if (value && IsActiveX) | ||
|
@@ -14149,9 +14162,11 @@ IList<Rectangle> IKeyboardToolTip.GetNeighboringToolsRectangles() | |
|
||
bool IKeyboardToolTip.IsHoveredWithMouse() | ||
{ | ||
return ClientRectangle.Contains(PointToClient(MousePosition)); | ||
return IsHoveredWithMouse; | ||
} | ||
|
||
private protected virtual bool IsHoveredWithMouse => ClientRectangle.Contains(PointToClient(MousePosition)); | ||
|
||
bool IKeyboardToolTip.HasRtlModeEnabled() | ||
{ | ||
Control topLevelControl = TopLevelControlInternal; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1601,7 +1601,7 @@ private bool ShouldSerializeImage() | |
/// <summary> | ||
/// Called by ToolTip to poke in that Tooltip into this ComCtl so that the Native ChildToolTip is not exposed. | ||
/// </summary> | ||
internal void SetToolTip(ToolTip toolTip) | ||
protected override void SetToolTip(ToolTip toolTip, string toolTipText) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗️ Just realised, this was previously not accessible outside the assembly, and now we're exposing it to the outside. |
||
{ | ||
if (toolTip != null && !controlToolTip) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5072,9 +5072,14 @@ internal void UpdateSavedCheckedItems(ListViewItem item, bool addItem) | |
/// <summary> | ||
/// Called by ToolTip to poke in that Tooltip into this ComCtl so that the Native ChildToolTip is not exposed. | ||
/// </summary> | ||
internal void SetToolTip(ToolTip toolTip, string toolTipCaption) | ||
protected override void SetToolTip(ToolTip toolTip, string toolTipText) | ||
{ | ||
this.toolTipCaption = toolTipCaption; | ||
if (toolTip == null) | ||
{ | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good call. |
||
} | ||
|
||
this.toolTipCaption = toolTipText; | ||
|
||
// native ListView expects tooltip HWND as a wParam and ignores lParam | ||
IntPtr oldHandle = User32.SendMessageW(this, (User32.WM)LVM.SETTOOLTIPS, toolTip.Handle, IntPtr.Zero); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1106,6 +1106,19 @@ internal TabPage GetTabPage(int index) | |
return _tabPages[index]; | ||
} | ||
|
||
internal Rectangle GetItemRectangle(int index) | ||
{ | ||
if (index < 0 || index >= TabCount) | ||
{ | ||
return Rectangle.Empty; | ||
} | ||
|
||
RECT rectangle = new RECT(); | ||
User32.SendMessageW(this, (User32.WM)ComCtl32.TCM.GETITEMRECT, (IntPtr)index, ref rectangle); | ||
|
||
return RectangleToScreen(rectangle); | ||
} | ||
|
||
/// <summary> | ||
/// This has package scope so that TabStrip and TabControl can call it. | ||
/// </summary> | ||
|
@@ -1360,6 +1373,20 @@ protected virtual void OnDrawItem(DrawItemEventArgs e) | |
_onDrawItem?.Invoke(this, e); | ||
} | ||
|
||
protected override void OnGotFocus(EventArgs e) | ||
{ | ||
if (TabCount > 0 && SelectedTab != null) | ||
{ | ||
KeyboardToolTipStateMachine.Instance.NotifyAboutGotFocus(SelectedTab); | ||
} | ||
else | ||
{ | ||
KeyboardToolTipStateMachine.Instance.NotifyAboutGotFocus(this); | ||
} | ||
|
||
base.OnGotFocus(e); | ||
} | ||
|
||
/// <summary> | ||
/// Actually goes and fires the OnLeave event. Inheriting controls | ||
/// should use this to know when the event is fired [this is preferable to | ||
|
@@ -1381,6 +1408,11 @@ protected override void OnEnter(EventArgs e) | |
{ | ||
SelectedTab.FireEnter(e); | ||
} | ||
|
||
if (TabCount == 0 && Enabled) | ||
{ | ||
KeyboardToolTipStateMachine.Instance.NotifyAboutGotFocus(this); | ||
} | ||
} | ||
|
||
/// <summary> | ||
|
@@ -1403,9 +1435,24 @@ protected override void OnLeave(EventArgs e) | |
{ | ||
SelectedTab.FireLeave(e); | ||
} | ||
|
||
base.OnLeave(e); | ||
} | ||
|
||
protected override void OnLostFocus(EventArgs e) | ||
{ | ||
if (TabCount > 0 && SelectedTab != null) | ||
{ | ||
KeyboardToolTipStateMachine.Instance.NotifyAboutLostFocus(SelectedTab); | ||
} | ||
else | ||
{ | ||
KeyboardToolTipStateMachine.Instance.NotifyAboutLostFocus(this); | ||
} | ||
|
||
base.OnLostFocus(e); | ||
} | ||
|
||
/// <summary> | ||
/// We override this to get tabbing functionality. | ||
/// If overriding this, remember to call base.onKeyDown. | ||
|
@@ -1484,6 +1531,11 @@ protected virtual void OnSelectedIndexChanged(EventArgs e) | |
UpdateTabSelection(GetState(State.UISelection)); | ||
SetState(State.UISelection, false); | ||
_onSelectedIndexChanged?.Invoke(this, e); | ||
|
||
if (TabCount > 0 && SelectedTab != null) | ||
{ | ||
KeyboardToolTipStateMachine.Instance.NotifyAboutGotFocus(SelectedTab); | ||
} | ||
} | ||
|
||
/// <summary> | ||
|
@@ -1662,11 +1714,16 @@ private void ResizePages() | |
/// <summary> | ||
/// Called by ToolTip to poke in that Tooltip into this ComCtl so that the Native ChildToolTip is not exposed. | ||
/// </summary> | ||
internal void SetToolTip(ToolTip toolTip, string controlToolTipText) | ||
protected override void SetToolTip(ToolTip toolTip, string toolTipText) | ||
{ | ||
if (toolTip == null || !ShowToolTips) | ||
{ | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same as previous: arguments check. |
||
} | ||
|
||
User32.SendMessageW(this, (User32.WM)ComCtl32.TCM.SETTOOLTIPS, toolTip.Handle); | ||
GC.KeepAlive(toolTip); | ||
_controlTipText = controlToolTipText; | ||
_controlTipText = toolTipText; | ||
} | ||
|
||
private void SetTabPage(int index, TabPage value) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,8 +8,10 @@ | |
using System.Diagnostics; | ||
using System.Drawing; | ||
using System.Drawing.Design; | ||
using System.Linq; | ||
using System.Runtime.InteropServices; | ||
using System.Windows.Forms.Layout; | ||
using static Interop; | ||
|
||
namespace System.Windows.Forms | ||
{ | ||
|
@@ -26,6 +28,11 @@ namespace System.Windows.Forms | |
[DefaultProperty("Text")] | ||
public class TabPage : Panel | ||
{ | ||
/// <summary> | ||
/// Internal property that has the default <see cref='ToolTip'/> instance if <see cref='ToolTipText'/> is set. | ||
/// This instance is replaced if an external ToolTip instance will be set. | ||
/// </summary> | ||
private ToolTip _toolTip; | ||
private ImageList.Indexer _imageIndexer; | ||
private string _toolTipText = string.Empty; | ||
private bool _enterFired = false; | ||
|
@@ -360,7 +367,7 @@ public string ToolTipText | |
get => _toolTipText; | ||
set | ||
{ | ||
if (value == null) | ||
if (string.IsNullOrWhiteSpace(value)) | ||
{ | ||
value = string.Empty; | ||
} | ||
|
@@ -372,6 +379,28 @@ public string ToolTipText | |
|
||
_toolTipText = value; | ||
UpdateParent(); | ||
|
||
ToolTip.SetToolTip(this, value); | ||
KeyboardToolTipStateMachine.Instance.Hook(this, ToolTip); | ||
} | ||
} | ||
|
||
private protected override bool IsHoveredWithMouse | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider a simplified name - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
get | ||
{ | ||
if (ParentInternal is TabControl tabControl) | ||
{ | ||
for (int i = 0; i < tabControl.TabCount; i++) | ||
{ | ||
if (tabControl.GetItemRectangle(i).Contains(MousePosition)) | ||
{ | ||
return true; | ||
} | ||
} | ||
} | ||
|
||
return ParentInternal.RectangleToScreen(ClientRectangle).Contains(MousePosition); | ||
} | ||
} | ||
|
||
|
@@ -442,6 +471,13 @@ internal void FireEnter(EventArgs e) | |
OnEnter(e); | ||
} | ||
|
||
protected override void OnGotFocus(EventArgs e) | ||
{ | ||
KeyboardToolTipStateMachine.Instance.NotifyAboutGotFocus(this); | ||
|
||
base.OnGotFocus(e); | ||
} | ||
|
||
/// <summary> | ||
/// Actually goes and fires the OnEnter event. Inheriting controls should use this to know | ||
/// when the event is fired [this is preferable to adding an event handler on yourself for | ||
|
@@ -464,6 +500,23 @@ protected override void OnEnter(EventArgs e) | |
} | ||
} | ||
|
||
internal override Rectangle GetToolNativeScreenRectangle() | ||
vladimir-krestov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
// A tooltip will be shown near a selected tab | ||
if (ParentInternal is TabControl tabControl) | ||
{ | ||
return tabControl.GetItemRectangle(tabControl.SelectedIndex); | ||
} | ||
|
||
return Rectangle.Empty; | ||
} | ||
|
||
internal ToolTip ToolTip | ||
{ | ||
get => _toolTip ??= new ToolTip(); | ||
set => _toolTip = value; | ||
} | ||
|
||
/// <summary> | ||
/// Actually goes and fires the OnLeave event. Inheriting controls should use this to know | ||
/// when the event is fired [this is preferable to adding an event handler on yourself for | ||
|
@@ -483,10 +536,18 @@ protected override void OnLeave(EventArgs e) | |
base.OnLeave(e); | ||
} | ||
|
||
KeyboardToolTipStateMachine.Instance.NotifyAboutLostFocus(this); | ||
_leaveFired = false; | ||
} | ||
} | ||
|
||
protected override void OnLostFocus(EventArgs e) | ||
{ | ||
KeyboardToolTipStateMachine.Instance.NotifyAboutLostFocus(this); | ||
|
||
base.OnLostFocus(e); | ||
} | ||
|
||
protected override void OnPaintBackground(PaintEventArgs e) | ||
{ | ||
// Utilize the TabRenderer new to Whidbey to draw the tab pages so that the panels are | ||
|
@@ -542,6 +603,28 @@ protected override void SetBoundsCore(int x, int y, int width, int height, Bound | |
} | ||
} | ||
|
||
protected override void SetToolTip(ToolTip toolTip, string toolTipText) | ||
{ | ||
if (toolTip == null) | ||
{ | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same as previous: argument check. |
||
} | ||
|
||
// Check if there is an existing ToolTip object that is showing tooltips, | ||
// if so - reset it to avoid showing several tooltips (old and new) at the same time. | ||
if (ToolTip != toolTip) | ||
{ | ||
ToolTip.SetToolTip(this, null); | ||
ToolTip = toolTip; | ||
} | ||
|
||
// Show the same tooltip for the page's tab. | ||
if (toolTipText != null && ToolTipText != toolTipText) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test |
||
{ | ||
ToolTipText = toolTipText; | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Determines if the Location property needs to be persisted. | ||
/// </summary> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used
protected
forSetToolTip
method thereby adding API for developers and they can change this implementation.This is correct or I need to hide this
protected
API using theprivate protected
modifier?/cc: @RussKie, @Tanya-Solyanik, @merriemcgaw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change to the public surface will not require designer changes, I don't see any problems with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you explain, when to use Tooltip.SetToolTip and when to use Control.SetToolTip?
Perhaps this method can be named differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the thing, I can't see any real benefit of using one over the other.
myToolTip.SetToolTip(myControl, toolTipText)
calls intomyControl.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 ofToolTip
and exposing properties likeToolTipText
. So now we have an inconsistent devex because for some controls a developer must use an instance of ownToolTip
, 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 - eachControl
allows setting its tooltips in a common manner. There be 🐉 🐉 though (e.g. theLabel
'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.