-
-
Notifications
You must be signed in to change notification settings - Fork 152
Initial accessibility support in Compose #1069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: compose-accessibility
Are you sure you want to change the base?
Initial accessibility support in Compose #1069
Conversation
…its implementations
...n/java/com/patrykandpatrick/vico/compose/cartesian/accessibility/AccessibilityHighlighter.kt
Show resolved
Hide resolved
...n/java/com/patrykandpatrick/vico/compose/cartesian/accessibility/AccessibilityHighlighter.kt
Show resolved
Hide resolved
vico/compose/src/main/java/com/patrykandpatrick/vico/compose/cartesian/CartesianChartHost.kt
Show resolved
Hide resolved
@@ -207,6 +210,7 @@ internal fun CartesianChartHostImpl( | |||
|
|||
layerDimensions.clear() | |||
chart.prepare(measuringContext, layerDimensions) | |||
targets = chart.allTargets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update targets
after calling chart.prepare()
, since it looks like they're updated during that call - before it, targets
were empty.
Let me know if I got that right, or if there's a better place to do this and it works differently under the hood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
targets
are updated in CartesianChart.draw
call. Also, it seems that we don't need targets
MutableState
property. The targets can be passed directly to AccessibilityHighlighter
from CartesianChart
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without targets
MutableState
the AccessibilityHighlighter
is never recomposed with the updated targets
. I initially used the approach that you mention - passing targets directly from CartesianChart
, but it only passes initial targets which are empty list. This can be tested if you replace targets = targets
with targets = chart.allTargets
. I'll focus on other improvements first and we can improve this once we have the Provider approach in place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that it's still the case? I've run a quick check and passing chart.allTargets
directly to AccessibilityHighlighter
worked fine in the sample app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right this is no longer needed - I switched to passing chart.allTargets
directly in f11217a. Thanks for checking it.
vico/compose/src/main/java/com/patrykandpatrick/vico/compose/cartesian/CartesianChartHost.kt
Show resolved
Hide resolved
...ore/src/main/java/com/patrykandpatrick/vico/core/cartesian/data/ColumnCartesianLayerModel.kt
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/patrykandpatrick/vico/core/cartesian/data/ColumnCartesianLayerModel.kt
Outdated
Show resolved
Hide resolved
...n/java/com/patrykandpatrick/vico/compose/cartesian/accessibility/AccessibilityHighlighter.kt
Outdated
Show resolved
Hide resolved
Hello, @Leedwon. Thanks for your contribution. Below are some general comments. We'll address your comments individually shortly.
|
Hi @Gowsky, thanks for your reply and the suggested ideas. If feature parity is required, I think it would be better to retarget this to the feature branch, to keep this PR focused on the compose part (could you please create one?). Once the compose part is merged, we’ll also have a clearer idea of what basic accessibility for Vico should look like, so we can port it to Views and Compose Multiplatform. I’ll take a look at the other points soon and try to apply your suggestions. |
Hello, and thanks for the PR! I’ve created a feature branch and made it the target. |
Hi @Gowsky, before I start improving the code in this PR I wanted to clarify couple of things:
ios_demo.mov
For example, in the
It’s not as smart (since “Year 2019” is repeated), but it’s much simpler from a usage standpoint, and it avoids having two separate providers for content descriptions. WDYT?
|
The provider's function wouldn't receive a single Regarding point 2, you can use the index of the We also plan on adding an Regarding point 3, yes, there should be a single provider, but it's not just that x and y would be handled together. Instead, since the function would receive a
This can be ignored for now. It would be optimal to move things around a bit more here, converting
Yes. You can use |
vico/compose/src/main/java/com/patrykandpatrick/vico/compose/cartesian/CartesianChartHost.kt
Outdated
Show resolved
Hide resolved
@@ -165,6 +165,7 @@ internal fun CartesianChartHostImpl( | |||
remember(chart.layerPadding, model.extraStore) { chart.layerPadding(model.extraStore) }, | |||
pointerPosition = pointerPosition.value, | |||
) | |||
var drawingContext by remember { mutableStateOf<CartesianDrawingContext?>(null) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we have other way to pass drawingContext
to AccessibilityHighlighter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can improve this and avoid using MutableState
which causes excessive recompositions.
The CartesianDrawingContext
instance creation needs to made right before the Box
composable. The canvas
instance won't be available at this point. Thus, the canvas
parameter of CartesianDrawingContext
function can be changed into a parameter with a default value. For the default value, we can use a new private val emptyCanvas = Canvas()
in the CartesianDrawingContext.kt
file. For the chart to be drawn to the correct Canvas
, the following lines need to be updated like so:
- chart.draw(context)
- measuringContext.reset()
+ context.withCanvas(canvas) {
+ chart.draw(context)
+ measuringContext.reset()
+ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done b411a30 - I'm not sure if we need rememberCartesianDrawingContext
given that android.graphics.Canvas
is probably not considered stable by compose, but I introduced it to follow the existing convention. Stability could also be checked and if needed compose_compiler_config
could be added but this is out of scope of this PR
...n/java/com/patrykandpatrick/vico/compose/cartesian/accessibility/AccessibilityHighlighter.kt
Outdated
Show resolved
Hide resolved
@@ -58,6 +59,8 @@ public fun rememberCartesianChart( | |||
decorations: List<Decoration> = emptyList(), | |||
persistentMarkers: (CartesianChart.PersistentMarkerScope.(ExtraStore) -> Unit)? = null, | |||
getXStep: ((CartesianChartModel) -> Double) = { it.getXDeltaGcd() }, | |||
contentDescriptionProvider: DefaultCartesianMarker.ContentDescriptionProvider = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if it should be added to CartesianChart
or to CartesianChartHost
. I picked the former, because it is not only compose-specific, but a core class, so the provider can be used in views and multiplatform as well
@Composable | ||
fun JetpackComposeGoldPrices(modifier: Modifier = Modifier) { | ||
val modelProducer = remember { CartesianChartModelProducer() } | ||
LaunchedEffect(Unit) { | ||
modelProducer.runTransaction { | ||
// Learn more: https://patrykandpatrick.com/y3c4gz. | ||
candlestickSeries(x, opening, closing, low, high, contentDescriptions) | ||
candlestickSeries(x, opening, closing, low, high) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added several fixup! commits to revert changes related to adding contentDescription to chart entries, which are no longer relevant.
I'm keeping these commits for now to maintain visibility during review, but once the PR is approved, I'll rebase and squash the fixups to produce a clean final history.
import com.patrykandpatrick.vico.core.common.LegendItem | ||
import com.patrykandpatrick.vico.core.common.Position | ||
import com.patrykandpatrick.vico.core.common.data.ExtraStore | ||
import com.patrykandpatrick.vico.core.common.shape.CorneredShape | ||
import kotlinx.coroutines.runBlocking | ||
|
||
private val LegendLabelKey = ExtraStore.Key<Set<String>>() | ||
private val ContentDescriptionProvider = | ||
DefaultCartesianMarker.ContentDescriptionProvider { _, targets -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to use val legendLabels = context.extraStore[LegendLabelKey]
instead of legendLabels = data.keys
to showcase the extraStore
usage, as you suggested. However, for some reason, the extraStore
available here doesn't contain that key, which leads to app crashes.
I briefly tried to debug it, and it looks like the key is present in the extraStore
from measuringContext
when creating the legend, but it's missing from the drawingContext
in this part of the code. The same thing happens with the marker ValueFormatter
- the key is missing during the format call.
Could you help me understand why these extraStores differ, and how we could ensure that the key is available in this context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help me understand why these extraStores differ
ExtraStore
defined in MeasuringContext
is used for internal drawing data. The ExtraStore
, which has consumer-provided extras, is accessible via CartesianMeasuringContext.model.extraStore
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context, I changed it to use model.extraStore
Hi @Gowsky, I added changes that use the Provider approach that you suggested. Couple of things to note:
Demo:vico_demo.mp4 |
Hi @Gowsky I've addressed latest comments, could you please take a look once you find some time |
Hi @Leedwon. Yes, I will when I have the time. |
Hi @Gowsky, just following up - any chance you'll have time to take a look in near future? Sorry to nudge, but this is a blocker for some accessibility work on our side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one more important thing to add. CartesianLayerModel.Entry
has no seriesIndex
property which is required for ContentDescriptionProvider
to label values properly. The content description for AI test scores chart uses "Image recognition" legend label for every top-most point at a given x value.
@@ -165,6 +165,7 @@ internal fun CartesianChartHostImpl( | |||
remember(chart.layerPadding, model.extraStore) { chart.layerPadding(model.extraStore) }, | |||
pointerPosition = pointerPosition.value, | |||
) | |||
var drawingContext by remember { mutableStateOf<CartesianDrawingContext?>(null) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can improve this and avoid using MutableState
which causes excessive recompositions.
The CartesianDrawingContext
instance creation needs to made right before the Box
composable. The canvas
instance won't be available at this point. Thus, the canvas
parameter of CartesianDrawingContext
function can be changed into a parameter with a default value. For the default value, we can use a new private val emptyCanvas = Canvas()
in the CartesianDrawingContext.kt
file. For the chart to be drawn to the correct Canvas
, the following lines need to be updated like so:
- chart.draw(context)
- measuringContext.reset()
+ context.withCanvas(canvas) {
+ chart.draw(context)
+ measuringContext.reset()
+ }
@@ -207,6 +210,7 @@ internal fun CartesianChartHostImpl( | |||
|
|||
layerDimensions.clear() | |||
chart.prepare(measuringContext, layerDimensions) | |||
targets = chart.allTargets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that it's still the case? I've run a quick check and passing chart.allTargets
directly to AccessibilityHighlighter
worked fine in the sample app.
val accessibilityManager = | ||
context.getSystemService(Context.ACCESSIBILITY_SERVICE) as? AccessibilityManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be wrapped with a remember
block.
val accessibilityManager = | |
context.getSystemService(Context.ACCESSIBILITY_SERVICE) as? AccessibilityManager | |
val accessibilityManager = | |
remember { context.getSystemService(Context.ACCESSIBILITY_SERVICE) as? AccessibilityManager } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 0559fcf
|
||
Box( | ||
modifier = | ||
Modifier.offset(x = canvasX.pxToDp() - width / 2, y = canvasTopY.pxToDp()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
graphicsLayer
has a better performance than offset
and works well here.
Modifier.offset(x = canvasX.pxToDp() - width / 2, y = canvasTopY.pxToDp()) | |
Modifier.graphicsLayer { | |
translationX = canvasX - width.toPx() / 2 | |
translationY = canvasTopY | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 80711b6
public fun getContentDescription( | ||
context: CartesianDrawingContext, | ||
targets: List<CartesianMarker.Target>, | ||
): CharSequence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The accessibility services expect String
, thus returning a CharSequence
here most likely isn't what we're looking for. Any extra information stored in CharSequence
subclasses will be lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 06608ff
@@ -309,6 +309,22 @@ public open class DefaultCartesianMarker( | |||
} | |||
} | |||
|
|||
/** Provides contentDescription for [CartesianMarker] * */ | |||
public fun interface ContentDescriptionProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to a separate file. It isn't used by DefaultCartesianMarker
and isn't really related to CartesianMarker
in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done fa5bb1a
…ng over others, which caused lower columns to not be announced by TalkBack
Ensure all Highlighters have the same height to maintain consistent read order. Previously, TalkBack could jump between entries as it prioritizes top-to-bottom, start-to-end reading.
…targets by canvasX value
…t description is now provided via ContentDescriptionProvider
…ely on internal canvas
…CartesianLayerModel.Entry to enforce seriesIndex usage
Hi @Gowsky, I’ve rebased and squashed all fixup commits to keep the history clean. From my side, the PR is ready - unless you see anything that needs changing, I think we can go ahead and merge it. |
Thanks, @Leedwon. We need to wait for @patrickmichalik's review before merging it. |
Hi @patrickmichalik 👋, is there a chance you could take a look sometime soon? Just asking to get an idea of when it might be merged 🙏 |
Hi @patrickmichalik 👋 Just wanted to check in on this - totally understand if things are busy, but I’d appreciate any thoughts or an idea of when this might get a look. Thanks so much! 🙏 |
Description
This PR adds minimal accessibility support to Vico charts. It introduces a
contentDescription
parameter, allowing meaningful descriptions to be passed for data points. These descriptions will be read by TalkBack, improving the chart experience for screen reader users.Note: this doesn’t make the entire chart accessible, full support should be introduced in separate PRs. If preferred, I can retarget this PR to a feature branch instead of master, but it should already provide some accessibility improvements on its own.
How to Review This PR:
The suggested way to review this PR is by going commit-by-commit. I’ve left comments in places where I saw potential for improvement or ran into limitations. I hope that your deep knowledge of the library can help improve or fix some of these areas.
PR Scope
Out of scope
This should be handled in a future PR, as it requires implementing
BringIntoViewRequester
together withBringIntoViewResponder
and connecting it toVicoScrollState
. The main challenge here is the lack of a reliable callback that tells us when an element is focused by accessibility services unfortunately,.onFocusChanged { }
doesn’t get triggered during accessibility navigation. To keep this PR smaller, I’m leaving this out for now.Demos
In each demo, TalkBack is enabled and I’m using left/right swipe gestures (default accessibility navigation method), to move between elements with semantics. You’ll notice that chart data points with
contentDescription
are now announced.Previously, the entire chart was skipped, and accessibility focus jumped directly to previous/next arrows.
Please watch the demos with sound on to hear the TalkBack announcements in action.
column_chart_demo.mp4
line_chart_demo.mp4
candlestick_demo.mp4
Known issues
This section describes known issues currently affecting chart accessibility. These can be addressed in this PR if you consider them blockers, or in future PRs. In most cases, I ran into some issues while trying to fix them, which is why they’re currently left as-is.
Combo charts
In combo charts, some data points may prevent others from being announced. For example, in the demo video, you can see that for x = 8 and x = 9, the column data points are not announced. This happens because the line points are positioned higher than the columns, and the accessibility system assumes the columns are not visible (and thus not focusable).
combo_chart_issues.mp4
Negative values
The highlighter for negative values starts from the bottom instead of the top. It might look a bit weird visually, but it’s not a problem for TalkBack.
negative_values_issues.mp4
Clickable charts with markers
When charts contain markers (making them clickable), the accessibility focus might jump to the touched position. This only happens when the user swipes back and forth through accessibility elements while swiping on the chart itself. If the user swipes just under or over the chart, focus moves correctly between data points.. This can be seen i.e on the Electric car sales chart.
Resources
https://eevis.codes/blog/2023-07-24/more-accessible-graphs-with-jetpack-compose-part-1-adding-content/