Repository navigation
feat(clustering): propagate contentDescription to marker options for accessibility - #1035
Conversation
…accessibility Support setting accessibility contentDescription on clustered map markers, resolving accessibility issues where TalkBack and UIAutomator announce generic "Map Marker" instead of meaningful pin information. - Add contentDescription to ClusteringMarkerProperties and its helper composable. - Propagate custom contentDescription or ClusterItem.title to MarkerOptions in ComposeUiClusterRenderer. - Add regression tests in GoogleMapViewClusteringTests verifying both default item title fallback and explicit custom contentDescription. Fixes #683, #706
kikoso
left a comment
There was a problem hiding this comment.
Thanks for picking this up, it is a long-standing accessibility issue! Two comments, the binary compatibility one is the important one.
| anchor: Offset? = null, | ||
| zIndex: Float? = null, | ||
| rotation: Float? = null, | ||
| contentDescription: String? = null, |
There was a problem hiding this comment.
Adding a parameter to a public composable changes its JVM signature, so anything compiled against 9.0.x that calls ClusteringMarkerProperties(...) would fail with NoSuchMethodError until it is recompiled. Could we keep the old signature as a @Deprecated(level = DeprecationLevel.HIDDEN) overload that delegates to the new one? That keeps it binary compatible, as AGENTS.md asks for public API changes.
There was a problem hiding this comment.
Good catch! Added the 3-parameter ClusteringMarkerProperties overload marked @Deprecated(level = DeprecationLevel.HIDDEN) delegating to the 4-parameter version, preserving binary compatibility for pre-compiled binaries.
There was a problem hiding this comment.
Added the 3-parameter
ClusteringMarkerPropertiesoverload marked@Deprecated(level = DeprecationLevel.HIDDEN)delegating to the 4-parameter version
Thanks, this looks good. Keeping the defaults on the hidden overload also keeps the synthetic $default signature the same as in 9.0.x, so we should be fine.
| } | ||
|
|
||
| private fun MarkerOptions.getContentDescription(): String? { | ||
| val field = MarkerOptions::class.java.declaredFields.firstOrNull { |
There was a problem hiding this comment.
This reads obfuscated fields from MarkerOptions (zzb, zzc, zzr), so it can silently start reading a different field after a Play services update. In the first test the expected value is also the item title, so if it ever picks up the title field the test would pass even without the fix. What about moving the fallback logic into a small internal function and testing it directly, or at least giving the item a title that differs from the expected description?
There was a problem hiding this comment.
Great suggestions.
- Extracted the resolution logic into
resolveItemContentDescriptionandresolveClusterContentDescriptioninternal functions, and added dedicated unit tests inClusterRendererContentDescriptionTest.ktcovering fallback, override, and null cases without touching Android or Play services. - In the instrumentation test, removed all hardcoded obfuscated field names (
zzb,zzc,zzr) and added a dynamic field counter and distinct titles vs descriptions to eliminate any chance of false positives.
There was a problem hiding this comment.
Extracted the resolution logic into
resolveItemContentDescriptionandresolveClusterContentDescriptioninternal functions, and added dedicated unit tests
I like this much better! The unit tests in ClusterRendererContentDescriptionTest now cover the fallback without depending on Play services, and with a distinct title the instrumentation test can't pass by accident anymore.
Coverage (unit tests)Compared against
Line and branch coverage from unit test reports. History is recorded on the |
Code Coverage
Files
|
…nd test robustness - Retain 3-parameter ClusteringMarkerProperties overload marked @deprecated(level = DeprecationLevel.HIDDEN) for binary compatibility. - Extract description resolution logic to internal resolveItemContentDescription and resolveClusterContentDescription functions with dedicated unit tests. - Remove obfuscated Proguard field references in GoogleMapViewClusteringTests and ensure distinct title vs description values.
Coverage (instrumentation tests)
Files
|
| internal fun <T : ClusterItem> resolveClusterContentDescription( | ||
| cluster: Cluster<T>, | ||
| customContentDescription: String?, | ||
| ): String? = customContentDescription |
There was a problem hiding this comment.
nit: cluster is not used here, so this just returns customContentDescription. Is it meant as a hook for a default like "N items" later, or could we drop the parameter (or the helper) for now?
There was a problem hiding this comment.
Good point! Dropped resolveClusterContentDescription and inlined props?.contentDescription directly at the call site until we introduce a default cluster description format.
Summary
Support setting accessibility
contentDescriptionon clustered map markers, resolving accessibility issues where TalkBack and UIAutomator announce generic "Map Marker" instead of meaningful pin information.Fixes #683, #706
Changes
Clustering.kt: AddedcontentDescriptionproperty toClusteringMarkerPropertiesand its@Composable fun ClusteringMarkerPropertieshelper function.ClusterRenderer.kt:onBeforeClusterItemRendered: Propagate customcontentDescription(or fallback toClusterItem.title) intoMarkerOptions.contentDescription.onBeforeClusterRendered: Propagate custom clustercontentDescriptionintoMarkerOptions.contentDescription.InvalidatingComposeView: Includeproperties.contentDescriptionin theLaunchedEffectinvalidation key set so dynamic updates re-render the marker.GoogleMapViewClusteringTests.kt:testClusteringPropagatesItemTitleToMarkerContentDescription: Asserts that uncustomized cluster items automatically propagateitem.titletoMarkerOptions.contentDescription.testClusteringMarkerPropertiesCustomContentDescription: Asserts that explicitClusteringMarkerProperties(contentDescription = ...)is propagated toMarkerOptions.contentDescription.Verification
./gradlew :maps-compose-utils:testDebugUnitTest lint(clean pass).GoogleMapViewClusteringTestson Android emulator (medium_phone, API 36) with 100% pass rate (5/5 tests).Reviewers
@kikoso @LoyalAbbas