Skip to content
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

Tooltip Text support for Views #8864

Merged
merged 19 commits into from
Aug 20, 2022
Merged

Tooltip Text support for Views #8864

merged 19 commits into from
Aug 20, 2022

Conversation

Redth
Copy link
Member

@Redth Redth commented Jul 20, 2022

Description of Change

Open questions

  1. Everyone ok with an attached property? Yes they are

  2. Is ToolTipProperty an ok name? WinUI uses ToolTipService. I mainly went with ToolTipProperty because of SemanticProperties and AutomationProperties. We don't really have anything suffixed with Service

  3. is IToolTipElement a valid name for something that has a ToolTip? Similar question here Context menu support for Windows and MacCatalyst #9174 (comment)

  4. Should ToolTip be a concrete class on ToolTipProperty and inherit from Element so you can set it in one go? We have cases for both. Right now we are following more of the Font approach here. https://docs.microsoft.com/en-us/windows/windows-app-sdk/api/winrt/microsoft.ui.xaml.controls.tooltipservice?view=windows-app-sdk-1.1 . The main reason to do this is if we want to allow people to instantiate an entire ToolTip and if we think specifying via a tooltip class is easier for the user.

API CHANGES

MAUI.Controls

// Other XAML Frameworks use ToolTipService
public class ToolTipProperties 
{
	public static readonly BindableProperty TextProperty = BindableProperty.CreateAttached("Text", typeof(string), typeof(ToolTipProperties), defaultValue: null, propertyChanged: OnToolTipPropertyChanged);
}

MAUI.Core

namespace Microsoft.Maui
{
	public partial class ToolTip
	{
		/// <summary>
		/// ToolTip content.
		/// </summary>
		public object? Content { get; set; }
	}
}
namespace Microsoft.Maui
{
	/// <summary>
	/// Indicates that this element has a ToolTip to show
	/// </summary>
	public interface IToolTipElement
	{
		ToolTip? ToolTip { get; }
	}
}

Samples

Windows:
image

MacCatalyst:
image

Android:
image

Issues Fixed

Partially implements: #1889 There's no placement property here yet, the UIKit API's seem to allow this customization, nor do the Android API's.

@Redth Redth added this to the .NET 7 milestone Jul 20, 2022
@Redth Redth added legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor t/enhancement ☀️ New feature or request labels Jul 20, 2022
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice and simple tbh. Love it.

Copy link
Member

@rachelkang rachelkang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far!

I think we can benefit from adding some semantics here as well. From an accessibility perspective, the tooltip behavior is inconsistent between the platforms (tested Android, Catalyst) and in my opinion, not ideal on any at the moment.

On Android:

  1. When the screen reader focus is on the button, the tooltip gets announced (as a hint) in addition to the text (this is good)
  2. When long pressing to activate the hover with screen reader on, the toolitip appears visually but the screen reader does not note anything (this can be improved by using the Announce API imho)

MacCatalyst:

  1. The tooltip text never gets announced (should at least get announced as a hint)
  2. Same as Android

^ Those are my two cents on how it should behave from a basic accessibility standpoint. There are more complexities involved when we consider things like how to dismiss the tooltip message (the "mouse off" equivalent of screen reader - would probably need to consider the various types of announcement APIs that interrupt you vs. let you finish)

cc @PureWeen

@Redth Redth mentioned this pull request Aug 2, 2022
22 tasks
@Redth
Copy link
Member Author

Redth commented Aug 2, 2022

LGTM so far!

I think we can benefit from adding some semantics here as well. From an accessibility perspective, the tooltip behavior is inconsistent between the platforms (tested Android, Catalyst) and in my opinion, not ideal on any at the moment.

On Android:

  1. When the screen reader focus is on the button, the tooltip gets announced (as a hint) in addition to the text (this is good)
  2. When long pressing to activate the hover with screen reader on, the toolitip appears visually but the screen reader does not note anything (this can be improved by using the Announce API imho)

MacCatalyst:

  1. The tooltip text never gets announced (should at least get announced as a hint)
  2. Same as Android

^ Those are my two cents on how it should behave from a basic accessibility standpoint. There are more complexities involved when we consider things like how to dismiss the tooltip message (the "mouse off" equivalent of screen reader - would probably need to consider the various types of announcement APIs that interrupt you vs. let you finish)

cc @PureWeen

On android, does it make sense that since the screenreader reads the text on focus, it's redundant to read it again on long press? I'm hesitant to change the default behavior here given the logical path to activating the tooltip through a screenreader would already read out the text before that action.

For catalyst, what event would we ideally hook into for announcing the tooltip? focused?

I think we could maybe open a new issue to track improvements to the screenreader experience around tooltips and keep this PR focused on the API wrapping each of the platform implementations.

@Redth Redth self-assigned this Aug 2, 2022
@mattleibow
Copy link
Member

Now I am gonna be that guy... ToolTip or Tooltip? Should the T in Tip be uppercase or lowercase?

@campersau
Copy link
Contributor

Framework Spelling
WinUI3 ToolTip
WPF ToolTip
WinForms ToolTip
iOS toolTip
Android Tooltip
Tizen tooltip

@rmarinho
Copy link
Member

rmarinho commented Aug 5, 2022

How do i select the placement of the ToolTip? Colors etc?

Redth added 8 commits August 11, 2022 16:05
Tooltips are only supported in the Android API as of 26+.  Prior to this, the TooltipCompat.SetToolTipText emulates the behavior however there is no TooltipCompat.GetToolTipText equivalent, so there's no way to actually 'get' the text that's currently set For now there's no valid way to test these cases, so we'll skip them for now.
@PureWeen
Copy link
Member

LGTM so far!

I think we can benefit from adding some semantics here as well. From an accessibility perspective, the tooltip behavior is inconsistent between the platforms (tested Android, Catalyst) and in my opinion, not ideal on any at the moment.

On Android:

  1. When the screen reader focus is on the button, the tooltip gets announced (as a hint) in addition to the text (this is good)
  2. When long pressing to activate the hover with screen reader on, the toolitip appears visually but the screen reader does not note anything (this can be improved by using the Announce API imho)

MacCatalyst:

  1. The tooltip text never gets announced (should at least get announced as a hint)
  2. Same as Android

^ Those are my two cents on how it should behave from a basic accessibility standpoint. There are more complexities involved when we consider things like how to dismiss the tooltip message (the "mouse off" equivalent of screen reader - would probably need to consider the various types of announcement APIs that interrupt you vs. let you finish)

cc @PureWeen

I think for now we should open a follow up issue to assess how ToolTips are typically surfaced from an a11y standpoint. All of the code in this PR uses platform concepts of ToolTips so we're not inventing anything across any platforms.

@jsuarezruiz jsuarezruiz self-requested a review August 17, 2022 14:40
# Conflicts:
#	src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt
#	src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt
Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would miss something like an IsOpen property, but as a basic implementation it works fine on Windows and macOS.

@PureWeen PureWeen requested a review from jsuarezruiz August 19, 2022 14:45
@PureWeen
Copy link
Member

PureWeen commented Aug 19, 2022

I would miss something like an IsOpen property, but as a basic implementation it works fine on Windows and macOS.

I agree but I don't think this is super easy to achieve with the platform APIs. I haven't found a great way to detect this on Android. If users want richer content/experience then they should probably use PopOver on the community toolkit

@mattleibow mattleibow dismissed rachelkang’s stale review August 20, 2022 11:33

Ready for now, any a11y issues are platform-specific. See #8864 (comment)

@PureWeen PureWeen merged commit b34a4f1 into main Aug 20, 2022
@PureWeen PureWeen deleted the dev/tooltips branch August 20, 2022 17:57
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fixed-in-7.0.0-rc.1.6683 legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor t/enhancement ☀️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants