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

Convert to using list of accessibility elements for iOS tab indexes #11077

Merged
merged 4 commits into from
Jul 16, 2020

Conversation

PureWeen
Copy link
Contributor

@PureWeen PureWeen commented Jun 16, 2020

Description of Change

Convert to using list of accessibility elements over methods. My theory here is that the performance of the individual methods when going across the bridge was causing hiccups with Appium.

I noticed on a ListView that it would call the "getMethods" for every single elements as you moved the mouse around which caused it to move incredibly slow.

Once I swapped over to just using the getter it all seemed to work much smoother

#9702 (comment)

Issues Resolved

Platforms Affected

  • iOS

Testing Procedure

PR Checklist

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

@PureWeen PureWeen added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Jun 16, 2020
@PureWeen PureWeen requested a review from samhouts June 16, 2020 18:17
@samhouts samhouts added a/webview i/regression t/bug 🐛 4.0.0 regression on 4.0.0 i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often m/high impact ⬛ partner/cat 😻 labels Jun 16, 2020
// lazy-loading this list so that the expensive call to GetAccessibilityElements only happens when VoiceOver is on.
if (_accessibilityElements == null || _accessibilityElements.Count == 0)
{
_accessibilityElements = _parent.GetAccessibilityElements();
var elements =_parent.GetAccessibilityElements();
if(elements != null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The documentation for this property says the default is null
https://developer.apple.com/documentation/objectivec/nsobject/1615147-accessibilityelements

Containers can implement this property instead of the dynamic methods to support the retrieval of the contained elements. The default value of this property is nil.

When I do that it seems like the accessibility just returns to the default behavior. I added all the repro's from the PR listed here #9702 and I tested the Accessibility demo on xamarin-samples.

We might need to add the setAccessibilityElements side of this as well for certain edge cases.

For now we'll keep it simple and see if that works

@samhouts samhouts added the Core label Jun 17, 2020
@PureWeen
Copy link
Contributor Author

Failures unrelated

@samhouts samhouts added this to In Progress in 4.7.0 Jun 20, 2020
@samhouts samhouts added this to In progress in Sprint 172 Jun 20, 2020
@PureWeen
Copy link
Contributor Author

@arevellfraedom summarized these properties really well here #9702 (comment)

@arevellfraedom I've tested through all the issues associated with issue #9702 and added UI Tests for them as well

If you have any additional cases or things you feel this PR might break please let me know

AFAICT this statement you made

I think sending null might even remove an override, but haven't checked.

Is true

Here's the nuget if you want to test
https://dev.azure.com/xamarin/public/_build/results?buildId=21211&view=artifacts&type=publishedArtifacts

@PureWeen
Copy link
Contributor Author

All green

@samhouts samhouts changed the base branch from 4.7.0 to master June 25, 2020 22:38
@samhouts samhouts removed this from In Progress in 4.7.0 Jun 25, 2020
@samhouts samhouts changed the base branch from master to 4.8.0 June 26, 2020 00:00
@samhouts samhouts added this to Ready for Review (PRs) in Sprint 173 Jul 2, 2020
@samhouts samhouts moved this from Ready for Review (PRs) to In progress in Sprint 173 Jul 13, 2020
@samhouts samhouts added this to In Review in Accessibility Jul 13, 2020
@PureWeen PureWeen removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Jul 15, 2020
Copy link
Member

@samhouts samhouts left a comment

Choose a reason for hiding this comment

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

Wunderbar!

Sprint 173 automation moved this from In progress to Ready for Review (PRs) Jul 16, 2020
@samhouts samhouts added the blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. label Jul 16, 2020
@samhouts samhouts merged commit 35e5dc9 into 4.8.0 Jul 16, 2020
Accessibility automation moved this from In Review to Done Jul 16, 2020
Sprint 173 automation moved this from Ready for Review (PRs) to Done Jul 16, 2020
@samhouts samhouts deleted the ios_appium_fixes branch July 16, 2020 23:20
@samhouts samhouts modified the milestone: 4.8.0 Jul 23, 2020
@samhouts samhouts added this to Done in vCurrent (4.8.0) Jul 30, 2020
@samhouts samhouts removed this from Done in Accessibility Aug 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4.0.0 regression on 4.0.0 a/a11y 🔍 a/webview blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. Core i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often i/regression m/high impact ⬛ p/iOS 🍎 partner/cat 😻 t/bug 🐛
Projects
No open projects
Sprint 172
  
Continued in next sprint
Sprint 173
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants