Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[iOS|Android] Fix screen reader order #9702

Merged
merged 16 commits into from
Mar 13, 2020
Merged

[iOS|Android] Fix screen reader order #9702

merged 16 commits into from
Mar 13, 2020

Conversation

samhouts
Copy link
Member

@samhouts samhouts commented Feb 25, 2020

Description of Change

  • iOS MasterDetailPage will no longer read MasterPage contents when the master page is hidden and will no longer read DetailPage contents when the master page is visible.
  • iOS can now tab stop on a Frame and can read its contents properly.
  • iOS/Android screen readers should no longer skip over layouts and should read all children in declaration order and/or TabIndex order.
  • TabIndex should no longer return elements that are Enabled = false or IsAccessibilityElement = false.

Issues Resolved

API Changes

These are breaking API changes, but they drive me crazy. I will revert if someone has an issue with this.

TabIndexExtensions:

-public static SortedDictionary<int, List<ITabStopElement>> GetSortedTabIndexesOnParentPage(this VisualElement element, out int countChildrensWithTabStopWithoutThis)
+public static SortedDictionary<int, List<ITabStopElement>> GetSortedTabIndexesOnParentPage(this VisualElement element)


-public static IDictionary<int, List<ITabStopElement>> GetTabIndexesOnParentPage(this ITabStopElement element, out int countChildrensWithTabStopWithoutThis, bool checkContainsElement = true)
+public static IDictionary<int, List<ITabStopElement>> GetTabIndexesOnParentPage(this ITabStopElement element, out int countChildrenWithTabStopWithoutThis)

Platforms Affected

  • Core/XAML (all platforms)
  • iOS
  • Android
  • UWP

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

  1. Use TalkBack/VoiceOver to test the Control Gallery and make sure everything is read in an expected order.
  2. Use keyboard to test that TabIndex is still functioning as expected.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@samhouts

This comment has been minimized.

@arevellfraedom
Copy link

Thanks @samhouts, I started testing that package in https://github.com/xamarin/xamarin-forms-samples/tree/master/UserInterface/Accessibility (Pulled as a zip from microsoft docs) and while the first page behaves as expected (with no tab indexes set). The tab-indexes page will allow the user to tab to the first button (Descending TabIndex), after which it gets stuck there.

For us, this is not an issue (as we don't intend to use TabIndex) but thought you'd prefer to know that.

// use OS default--there's no need for us to keep going if there's one or fewer tab indexes!
if (tabIndexes.Count <= 1)
return base.FocusSearch(focused, direction);

int tabIndex = element.TabIndex;
AView control = null;
int attempt = 0;

Choose a reason for hiding this comment

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

@samhouts looking at the below code, it looks like the discovery of the next tab stop in the do while loop will only happen once, as maxAttempts = 0, and attempt starts at 0 but will be 1 at this stage, am I missing something?

Choose a reason for hiding this comment

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

Ah sorry missed the out in the extension

@arevellfraedom
Copy link

arevellfraedom commented Mar 8, 2020

@samhouts hate to bring up the old "when will this be out", but.... when do you expect this to be out? We need to rely on this change for our upcoming release, which realistically means it being part of a 4.5 patch with 1-2 months. If this is not realistic, can you give an estimate of when it may be part of an official release so we may plan?

Assuming it will indeed be merged to 4.5 and not 4.4

@samhouts
Copy link
Member Author

samhouts commented Mar 9, 2020

@arevellfraedom So, first, whenever we merge something to a release branch like 4.4.0, it gets merged up through all the active releases and then to master. Thus it will be in 4.4.0, 4.5.0, and 4.6.0, and in the nightlies from master.

Second, I can't make any promises about release dates, but yes, this has a high probability of being released within two weeks. 🤞

@arevellfraedom
Copy link

That would be amazing and I highly appreciate the effort. This change is very positive and will improve the accessibility of probably 90% of Xamarin forms apps, which I’m sure those users affected will very much appreciate.

I will put some effort in shortly to look beyond the cases we don’t specifically need, but others might, to see if I can provide any more feedback to ensure this PR is as good as possible.

@arevellfraedom
Copy link

arevellfraedom commented Mar 10, 2020

Hi @samhouts - I have done additional validation of the keyboard tabbing on Android and have a couple of observations.

These are all using https://github.com/xamarin/xamarin-forms-samples/tree/master/UserInterface/Accessibility:

Bold = fail
Italic = pass

4.4.0.993041

  1. Ascending tabindex
  • Tabbing from nav bar: gets stuck on "Ascending tabindex" button
  • If manually tap in "Enter data", then can tab through fields until "Select a Monkey" after which point it is impossible to proceed
  • If skip to "Enter search term" then can tab through the last three elements, but then which can't be tabbed through the same as "Select a Monkey"
  1. Descending tabindex
  • Same pass/fail as ascending (can't exit nav bar etc), just in reverse
  1. All tabindex = 0
  • Can tab from nav bar and through buttons
  • Can't tab through Date or "Select a Monkey"
  • On wrap around, skips buttons
  1. Toggle IsTabStop
  • No fields can be tabbed through
  • Ignored if all tabindex = 0
  1. Alternating IsTabStop
  • Tabbing from nav bar: gets stuck on "Ascending tabindex" button
  • Ignored if all tabindex = 0
  • If "Enter data" tapped, then "Enter name" correctly skipped on tab

4.4.0.991757

  • Tabbing from nav bar: gets stuck on some outer element and does not get into buttons
  • If manually tap in "Enter data", then can tab through fields until "Select a Monkey" after which point it is impossible to proceed
  • If skip to "Enter search term" then can tab through the last three elements, but then which can't be tabbed through the same as "Select a Monkey"
  1. Descending tabindex
  • Same pass/fail as ascending (can't exit nav bar etc), just in reverse
  1. All tabindex = 0
  • Tabbing from nav bar: gets stuck on some outer element and does not get into buttons
  • Can't tab through Date or "Select a Monkey"
  • On wrap around, skips buttons
  1. Toggle IsTabStop
  • No fields can be tabbed through
  • Still works if tabindex = 0
  1. Alternating IsTabStop
  • Tabbing from nav bar: gets stuck on some outer element and does not get into buttons
  • Still works if tabindex = 0
  • If "Enter data" tapped, then "Enter name" correctly skipped on tab

3.6.0

  1. Ascending tabindex
  • Can tab from nav bar and through buttons
  • Tabbing through "Select a Monkey" tabs to next field after popup value selected
  • On wrap, buttons are tabbed through then fields
  1. Descending tabindex
  • Same passes as Ascending
  1. All tabindex = 0
  • Same passes as Ascending
  1. Toggle IsTabStop
  • Fields cannot be tabbed through
  1. Alternating IsTabStop
  • Same passes as Ascending

Summary
3.6.0 was basically perfect
4.4.0.991757 had particular problems getting from the navbar and when tabindex = 0
4.4.0.993041 corrects behaviour when tabindex = 0, but still struggles otherwise, in particular gets stuck on the first button, doesn't allow fields such as "Select a Monkey" to be tabbed through and also has a regression when tabindex = 0 but tabstop = false

4.4.0.993041 is still good for us, but does restore full 3.6.0 functionality

@samhouts
Copy link
Member Author

@arevellfraedom Thank you so much for the detailed test and review!! Given this does improve behavior, we'll merge it, and then we'll continue working on the remaining issues you mention.

The basic problem is that once we override the accessibility tree in iOS, we're stuck with that, even if we don't need to alter it. I'm having trouble finding a way to accurately reproduce it, since iOS doesn't seem to expose it. If ANYONE has has success with that, I'd love to hear what you did!

Thanks to EVERYONE who has been pushing this area forward, either by your reports, testing, review, etc!!

@samhouts samhouts merged commit 77e411d into 4.4.0 Mar 13, 2020
@samhouts samhouts deleted the fix-9653 branch March 13, 2020 16:38
@arevellfraedom
Copy link

arevellfraedom commented Mar 13, 2020

@arevellfraedom Thank you so much for the detailed test and review!! Given this does improve behavior, we'll merge it, and then we'll continue working on the remaining issues you mention.

The basic problem is that once we override the accessibility tree in iOS, we're stuck with that, even if we don't need to alter it. I'm having trouble finding a way to accurately reproduce it, since iOS doesn't seem to expose it. If ANYONE has has success with that, I'd love to hear what you did!

Thanks to EVERYONE who has been pushing this area forward, either by your reports, testing, review, etc!!

@samhouts , if I understand the issue you are facing with iOS correctly, I may be able to help.
You are correct that once you implement the UIAccessibilityContainer protocol (through exporting accessibilityElementCount etc) you are stuck with overriding the children of that view for accessibility. Once you have implemented the protocol, you can't get back to asking the OS to do it instead, at least not without obj-c method swizzling.

I see two options:

  1. Have the PageContainer (the one implementing the protocol) be the View of the VC conditional on whether you want to override the accessibility tree. Not sure how practical this is but it would be the most flexible option (I think)

  2. Use the less known fact you can call the setAccessibilityElements method of UIAccessibilityContainer.
    So the strange thing about this protocol is that you can either act by responding as a getter (whenever the OS feels it needs to know the accessibility children) or simply set them whenever you choose. In many way, acting as a getter is easier and more reliable (as it is called at the right times and can respond with elements based on the situation), but it has the major downside, as you have seen, that it is impossible to reply with "do the default". That is what setAccessibilityElements can be useful for.

So to make this useful, the first thing to determine is when to call it - the good news is that the answer is "any time before voiceover scans the page". So ViewDidLoad for example is just fine. So simply go through the logic of TabStop and TabIndex, and when you are ready, call setAccessibilityElements with the children or simply don't call it if you don't want to override. I think sending null might even remove an override, but haven't checked.

The second thing is to know how to call it - this is not so easy as this method is so strange it does not appear on the IUIAccessibilityContainer interface. What you need to do instead is:

  1. Create a selector for it
    SetAccessibilityElementsSelector = new Selector("setAccessibilityElements:");
  2. Prepare a NSMutableArray<object> of the child elements you wish to set
  3. Prepare a way of sending a message to the container you want to set the children on
    [DllImport("/usr/lib/libobjc.dylib", EntryPoint = "objc_msgSend")]
    private static extern void void_objc_msgSend_IntPtr(IntPtr receiver, IntPtr selector, IntPtr arg1);
  4. Send the message
    void_objc_msgSend_IntPtr(containingView.Handle, SetAccessibilityElementsSelector.Handle, array?.Handle ?? IntPtr.Zero);
  5. This is probably most usefully exposed as an extension method with signature
    public static void SetAccessibilityElements<TElement>([NotNull] this UIView containingView, [CanBeNull, ItemNotNull] IEnumerable<TElement> childElements) where TElement : NSObject

Hopefully I have understood the question and that gives you some new options.

@samhouts samhouts added this to the 4.4.0 milestone Mar 20, 2020
@bjarteskogoy
Copy link

Will this show up in Xamarin.Forms 4.5 soon?

@PureWeen
Copy link
Contributor

This should be available in the nuget that went out yesterday.

@samhouts correct me if I'm wrong

@samhouts
Copy link
Member Author

Yep, that's correct. It's now available in the latest 4.4.0, 4.5.0, and 4.6.0 releases. Thanks!

@rnic99
Copy link

rnic99 commented Mar 31, 2020

@samhouts Im not seeing any major difference in accessibility behavior on the latest stable of 4.5, however I am seeing a significant improvement on the 4.6 pre-release

@samhouts
Copy link
Member Author

@rnic99 You tried 4.5.0.495 and there was no difference?

@rnic99
Copy link

rnic99 commented Mar 31, 2020

@samhouts Yep pulled down 4.5.0.495 this morning and the behavior was identical. I should also point out I was testing on an iPhone 6 with iOS version 12 and a OnePlus 3t with android 9.

@arevellfraedom
Copy link

It would appear 4.5 SR2 was taken shortly before this PR was merged, although I cannot be sure

@samhouts
Copy link
Member Author

4.5.0.495 was release-4.5.0-sr2.2, and this commit is in it. That's strange.

@rnic99
Copy link

rnic99 commented Mar 31, 2020

@samhouts I just re-downloaded the stable release and it seems to be working fine on Android, and iOS. Or atleast its showing the same behavior as it was in 4.6 pre. My apologies for the confusion. I have a feeling there was a problem where even though it was showing 4.5 under my XF packages it was still deploying as 4.4.

@samhouts
Copy link
Member Author

@rnic99 Phew! I was hoping that was the case. Thanks for checking again!!

@rnic99
Copy link

rnic99 commented Mar 31, 2020

@samhouts of course, sorry for causing distress

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