Add support for AutomationPeer by miguelrochefort · Pull Request #141 · unoplatform/uno

miguelrochefort · 2018-08-20T16:47:48Z

No description provided.

davidjohnoliver · 2018-08-21T15:44:52Z

+- Avoid stacking `TextBlock`s inside  a `Panel` when you can use `Inlines` inside a `TextBlock` (using `LineBreak` if necessary). This allows the screen reader to read all the text at once, instead of having the user select every part manually.
+- Use a converter to trim long text. While a `TextBlock` might ellipsize long text, the screen reader will read the entire text provided.
+- Avoid creating custom controls when you can use built-in ones. If you must, make sure to implement and provide an appropriate `AutomationPeer`.
+- You can disable accessibility focus of native elements using `android:ImportantForAccessibility="No"` and `ios:IsAccessibilityElement="False"`.


Is there a UWP/Uno equivalent of these properties?

You can disable accessibility focus of UWP/Uno elements using AutomationProperties.AccessibilityView="Raw".

It won't work on native elements, hence the ImportantFocAccessibility/IsAccessibilityElement workaround.

davidjohnoliver · 2018-08-21T15:50:13Z

+					}
+					else
+					{
+						if ("".Log().IsEnabled(LogLevel.Error))


Shouldn't this be typeof(CommandBarRenderer).Log()?

davidjohnoliver · 2018-08-21T16:13:41Z

 	{
+		#region Public
+
+		public void InvalidatePeer()


[NotImplemented]?

davidjohnoliver · 2018-08-21T16:15:46Z

+{
+	public  partial class ComboBoxAutomationPeer : SelectorAutomationPeer, Provider.IExpandCollapseProvider, Provider.IValueProvider, Provider.IWindowProvider
+	{
+		public ExpandCollapseState ExpandCollapseState


[NotImplemented]

davidjohnoliver · 2018-08-21T16:19:33Z

+
+		private int GetMaxTextLength()
+		{
+			return Owner is TextBox textBox ? textBox.MaxLength : 0;


Why is this not virtual? Should there be a TODO to use ITextProvider?

I'm just feeding Android's AccessibilityNodeInfo what it wants. I don't know if there's an equivalent in AutomationPeer's API.

There's an ITextProvider interface but it sounds like a mess: https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.automation.provider.itextprovider

ITextProvider, ITextProvider2 and ITextRangeProvider aren't implemented by any existing Windows Runtime automation peers using this definition of the interface. The text models supported by Windows Runtime text controls such as TextBox and RichTextBlock do implement some of these patterns, but do so at a native level that doesn't appear in the Windows Runtime definitions of the API surface. For more info, see the peer classes for the various Windows Runtime text controls.

I can't find anything about maximum text length at first glance.

davidjohnoliver · 2018-08-21T16:22:23Z

+			{
+				if (Owner is View view && AutomationProperties.GetAccessibilityView(Owner) != AccessibilityView.Raw)
+				{
+					// TODO: How expensive is this? Is this called recursively for all children?


I refactored that part, confirmed this wasn't a problem, and explained why.

davidjohnoliver · 2018-08-21T16:28:42Z

 	public partial class ImplicitTextBlock : TextBlock
 	{
-		public ImplicitTextBlock() { }
+		public ImplicitTextBlock(DependencyObject parent)


Is the reference to the ContentControl being held? Ie is there any risk of a memory leak?

No. It only needs its parent to retrieve its AutomationProperties.AccessibilityView value once.

davidjohnoliver · 2018-08-21T16:34:57Z

 			IsChecked = false;
 		}
+
+		protected virtual void OnToggle()


OnAutomationToggle()? Is it used/useful in any other context?

OnToggle is part of ToggleButton's contract.

AutomationPeerToggle exists to allow OnToggle to be called from automation peers.

davidjohnoliver · 2018-08-21T16:39:38Z

 			{
-				(e as View).ContentDescription = e.GetType().ToString();
+				// TODO: This causes Android to cycle through every visible IFrameworkElement every time the accessibility focus is changed
+				// and calls the overriden InitializeAccessibilityNodeInfo method. This could eventually cause performance issues when accessibility is enabled.


When is 'eventually'? Is this a serious performance concern?

'eventually' is if we have complex UI with lots of elements. I've only noticed lags when cycling through accessible elements in debug.

The performance problem seem to be due to interop. It should only affect focus change on devices with TalkBack turned on. A possible optimization could be to set this value to false if an element's CreateAutomationPeer returns null.

Add the second paragraph of your reply to the comment.

carldebilly

supports

carldebilly · 2018-08-23T15:11:07Z

 					new XElement("string",
 						new XAttribute("formatted", "false"), // allows special characters (%, $, etc.)
-						new XAttribute("name", resource.Key),
+						new XAttribute("name", AndroidResourceNameEncoder.Encode(resource.Key)),


This change is related to AutomationPeers?

I created a different PR: #156

carldebilly · 2018-08-23T15:12:27Z

 		public static IEnumerable<UIView> EnumerateAllChildren(this UIView view, int maxDepth = 20)
 		{
-			return FindSubviews(view, _ => true, maxDepth);
+			var children = view.GetChildren().OfType<UIElement>();


children variable is not used...

carldebilly · 2018-08-23T15:14:27Z

+			return key
+				.Replace('[', '_')
+				.Replace(']', '_')
+				.Replace(':', '_');


Is it something called often? Could have perf issues.

carldebilly · 2018-08-23T15:15:25Z

@@ -0,0 +1,47 @@
+namespace Windows.UI.Xaml.Automation.Peers
+{
+	public enum AutomationControlType


Alpha ordering?

... or is this order important? If yes needs a comment about it.

The order is important. I added the values.

…-fix fix(snackbar): Fixed Error binding nullref

miguelrochefort requested review from MatFillion, carldebilly, davidjohnoliver, dr1rrb, jeremiethibeault and jeromelaban August 20, 2018 18:48

miguelrochefort force-pushed the dev/miro/accessibility branch 2 times, most recently from 2add77e to 405ca80 Compare August 21, 2018 12:05

davidjohnoliver approved these changes Aug 21, 2018

View reviewed changes

miguelrochefort force-pushed the dev/miro/accessibility branch 3 times, most recently from 2b7ac0e to afd33ac Compare August 22, 2018 15:54

carldebilly reviewed Aug 23, 2018

View reviewed changes

carldebilly approved these changes Aug 23, 2018

View reviewed changes

miguelrochefort force-pushed the dev/miro/accessibility branch 3 times, most recently from c0e02d3 to 1fe6e7e Compare August 29, 2018 19:11

Add support for AutomationPeer

8aa2203

miguelrochefort force-pushed the dev/miro/accessibility branch from 1fe6e7e to 8aa2203 Compare August 29, 2018 19:50

miguelrochefort merged commit 7e70a42 into master Aug 29, 2018

carldebilly deleted the dev/miro/accessibility branch August 31, 2018 00:15

kazo0 pushed a commit to kazo0/uno that referenced this pull request Mar 6, 2026

Merge pull request unoplatform#141 from unoplatform/dev/nich/snackbar…

1501bc4

…-fix fix(snackbar): Fixed Error binding nullref

Conversation

miguelrochefort commented Aug 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miguelrochefort Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carldebilly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

miguelrochefort Aug 21, 2018 •

edited

Loading