Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios] Refactors accessibilityElementAtIndex #15303

Closed
wants to merge 18 commits into from

Conversation

julianrex
Copy link
Contributor

@julianrex julianrex commented Aug 2, 2019

Fixes #14324 (and should address some reported associated out-of-bounds crashes)

This PR requires

The main issue behind #14324 was that the same work was being repeatedly performed; if you look through the call stacks you can see that -accessibilityElementCount is called first, then -accessibilityElementAtIndex: that many times.

Instead, this PR generates the accessibility elements once (if it has been requested). This can still be a lengthly process, though we should be able to handle some of this on a background queue eventually (in a follow-on PR). This PR is about getting rid of the hang, crashes and generally improving performance.

The "places" listed in the accessibility value are now the closest towns to the center/user location - as opposed to what has been returned from queryRenderedFeatures.

Considering performance: looking at accessibilityElementForRoadFeature: (which takes up the bulk of the time of the new -generateAccessibilityElementsIfNeeded) we see

Image 2019-07-30 at 12 06 57 PM

that UIAccessibilityConvertPathFunction takes the bulk of the time, and this is out of our control. We may be able to call this a background thread, but given it is UI related I am skeptical.

Once queryRenderedFeatures is asynchronous, then we should revisit this to try to background as much as we can (and post the accessibility notifications when done).

However, I think we need to revisit accessibility in general, as it doesn't give a good user-experience - and we're currently doing too much work for little benefit.

@friedbunny would you mind walking me through #14832 - as I have not tested that yet.

@julianrex julianrex added iOS Mapbox Maps SDK for iOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold accessibility Integration with screen readers and other assistive technology needs changelog Indicates PR needs a changelog entry prior to merging. labels Aug 2, 2019
@julianrex julianrex added this to the release-queso milestone Aug 2, 2019
@julianrex julianrex requested a review from 1ec5 as a code owner August 2, 2019 15:10
@julianrex julianrex requested a review from a team August 2, 2019 15:10
@julianrex
Copy link
Contributor Author

I tested #14832 with iosapp on an iPod 7 running 13b5 - and it seems ok for me. Occasionally it wouldn't recognize my command - but I think that was more to do with voice recognition.

@julianrex julianrex self-assigned this Sep 16, 2019
@julianrex julianrex removed this from the release-ristretto milestone Sep 30, 2019
@julianrex
Copy link
Contributor Author

Closing, since the relevant parts need to be ported to mapbox-gl-native-ios. I've transferred the original issue #14324 to mapbox/mapbox-gl-native-ios#220.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility Integration with screen readers and other assistive technology ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold iOS Mapbox Maps SDK for iOS needs changelog Indicates PR needs a changelog entry prior to merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants