Skip to content

Preserve accessibility semantics on actionable list rows #516

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.

Sign up for GitHub

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

Closed
caspersimon wants to merge 1 commit into from
Closed

Preserve accessibility semantics on actionable list rows #516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 4 additions & 27 deletions Sources/SkipUI/SkipUI/Commands/Menu.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,10 @@ public final class Menu : View, Renderable {
// `ModifiedContent`, not on `stripped`. Harvest them here so the
// resulting Compose `DropdownMenuItem` carries the same test tag
// and content description a regular Button would.
let itemModifier = accessibilityModifier(for: renderable, context: context)
let itemModifier = composeAccessibilityModifiers(
for: renderable,
context: context
).modifier
if let button = stripped as? Button {
let isSelected: Bool?
if let tagModifier = TagModifier.on(content: renderable, role: .tag) {
Expand Down Expand Up @@ -203,32 +206,6 @@ public final class Menu : View, Renderable {
}
}

/// Walks `renderable`'s modifier chain and composes the `Modifier`
/// transform from every `.accessibility`-role `RenderModifier` (i.e.
/// `.accessibilityIdentifier(_:)`, `.accessibilityLabel(_:)`, etc.) so
/// it can be applied to a raw Compose element. `forEachModifier` yields
/// outermost-first; we collect into a list and apply in reverse so the
/// outermost ends up wrapping the inner ones — matching how a normal
/// `Renderable.Render` would build the chain.
@Composable private static func accessibilityModifier(for renderable: Renderable, context: ComposeContext) -> Modifier {
var collected: [RenderModifier] = []
let _: Bool? = renderable.forEachModifier { (mod: ModifierProtocol) -> Bool? in
if let renderMod = mod as? RenderModifier, renderMod.role == .accessibility {
collected.append(renderMod)
}
return nil
}
var modifier: Modifier = Modifier
for renderMod in collected.reversed() {
if let modAction = renderMod.modifierAction {
var ctx = context
ctx.modifier = modifier
modifier = modAction(ctx)
}
}
return modifier
}

@Composable private static func RenderDropdownMenuItem(for view: ComposeBuilder, context: ComposeContext, modifier: Modifier = Modifier, tintColor: Color? = nil, isSelected: Bool? = nil, action: () -> Void) {
let renderables = view.Evaluate(context: context, options: 0)
let label = renderables.firstOrNull()?.strip() as? Label
Expand Down
83 changes: 66 additions & 17 deletions Sources/SkipUI/SkipUI/Containers/List.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ import androidx.compose.ui.geometry.RoundRect
import androidx.compose.ui.graphics.Path.Companion.combine
import androidx.compose.ui.graphics.PathOperation
import androidx.compose.ui.platform.LocalDensity
import androidx.compose.ui.semantics.hideFromAccessibility
import androidx.compose.ui.semantics.semantics
import androidx.compose.ui.zIndex
import androidx.compose.ui.unit.Density
import androidx.compose.ui.unit.Dp
Expand Down Expand Up @@ -422,23 +424,43 @@ public final class List : View, Renderable {
let badge = badgeModifier.badge
let (isListItem, listItemAction) = item.shouldRenderListItem(context: context)
if isListItem {
let actionModifier: Modifier
let rowModifier: Modifier
let rowContent: Renderable
let rowContentModifiers: kotlin.collections.List<ModifierProtocol>
if let listItemAction {
let isDisabled = !EnvironmentValues.shared.isEnabled || item.forEachModifier { ($0 as? DisabledModifier)?.disabled } == true
actionModifier = Modifier.clickable(onClick: listItemAction, enabled: !isDisabled)
let baseModifier = Modifier.clickable(
onClick: listItemAction,
enabled: !isDisabled
).then(modifier)
let accessibilityComposition = EnvironmentValues.shared.setValuesWithReturn({
$0.setisEnabled(!isDisabled)
return ComposeResult.ok
}, in: {
return composeAccessibilityModifiers(
for: item,
context: context,
modifier: baseModifier
)
})
rowModifier = accessibilityComposition.modifier
rowContent = item.strip()
rowContentModifiers = accessibilityComposition.remainingModifiers
} else {
actionModifier = Modifier
rowModifier = modifier
rowContent = item
rowContentModifiers = listOf()
}
if let badge {
Row(modifier: actionModifier.then(modifier), horizontalArrangement: Arrangement.SpaceBetween, verticalAlignment: androidx.compose.ui.Alignment.CenterVertically) {
Row(modifier: rowModifier, horizontalArrangement: Arrangement.SpaceBetween, verticalAlignment: androidx.compose.ui.Alignment.CenterVertically) {
Box(modifier: Modifier.weight(Float(1.0)), contentAlignment: androidx.compose.ui.Alignment.CenterStart) {
item.RenderListItem(context: context, modifiers: listOf())
rowContent.RenderListItem(context: context, modifiers: rowContentModifiers)
}
RenderBadge(badge: badge, prominence: badgeModifier.prominence ?? .standard, context: context)
}
} else {
Box(modifier: actionModifier.then(modifier), contentAlignment: androidx.compose.ui.Alignment.CenterStart) {
item.RenderListItem(context: context, modifiers: listOf())
Box(modifier: rowModifier, contentAlignment: androidx.compose.ui.Alignment.CenterStart) {
rowContent.RenderListItem(context: context, modifiers: rowContentModifiers)
}
}
} else {
Expand Down Expand Up @@ -528,6 +550,8 @@ public final class List : View, Renderable {
let leadingSwipe = swipeConfigs.leading
let trailingSwipe = swipeConfigs.trailing
let hasUserSwipe = leadingSwipe != nil || trailingSwipe != nil
let isEnabled = EnvironmentValues.shared.isEnabled &&
content.forEachModifier { ($0 as? DisabledModifier)?.disabled } != true
let isDeleteEnabled = (editActions.contains(.delete) || onDelete != nil) && editActionsModifier.isDeleteDisabled != true
let isMoveEnabled = (editActions.contains(.move) || onMove != nil) && editActionsModifier.isMoveDisabled != true
guard isDeleteEnabled || isMoveEnabled || hasUserSwipe else {
Expand Down Expand Up @@ -556,7 +580,7 @@ public final class List : View, Renderable {
}
} : nil
itemContent = { rowModifier in
RenderSwipeableItem(content: content, level: level, context: context, modifier: rowModifier, styling: styling, leadingConfig: leadingSwipe, trailingConfig: trailingSwipe, rowKey: key, activeSwipeKey: activeSwipeKey, onDestructiveDelete: onDestructiveDelete)
RenderSwipeableItem(content: content, level: level, context: context, modifier: rowModifier, styling: styling, leadingConfig: leadingSwipe, trailingConfig: trailingSwipe, rowKey: key, activeSwipeKey: activeSwipeKey, isEnabled: isEnabled, onDestructiveDelete: onDestructiveDelete)
}
} else if isDeleteEnabled {
let rememberedOnDelete = rememberUpdatedState({
Expand Down Expand Up @@ -608,7 +632,7 @@ public final class List : View, Renderable {
/// The foreground row determines the cell's height; reveal buttons match it
/// via `Modifier.matchParentSize()` so we never propagate unbounded height
/// constraints up into the surrounding LazyColumn.
@Composable private func RenderSwipeableItem(content: Renderable, level: Int, context: ComposeContext, modifier: Modifier, styling: ListStyling, leadingConfig: SwipeActionsConfig?, trailingConfig: SwipeActionsConfig?, rowKey: String, activeSwipeKey: MutableState<String?>, onDestructiveDelete: (() -> Void)? = nil) {
@Composable private func RenderSwipeableItem(content: Renderable, level: Int, context: ComposeContext, modifier: Modifier, styling: ListStyling, leadingConfig: SwipeActionsConfig?, trailingConfig: SwipeActionsConfig?, rowKey: String, activeSwipeKey: MutableState<String?>, isEnabled: Bool, onDestructiveDelete: (() -> Void)? = nil) {
let coroutineScope = rememberCoroutineScope()

/* Extract Buttons and per-button .tint(_:) values from each edge's
Expand Down Expand Up @@ -701,6 +725,19 @@ public final class List : View, Renderable {
let trailingNaturalState = remember { mutableFloatStateOf(Float(0)) }
let leadingNaturalState = remember { mutableFloatStateOf(Float(0)) }

/* A disabled SwiftUI row owns no swipe interaction. Close an already
revealed row and clear its active key as soon as inherited enabled
state changes, while input and semantics are gated synchronously
below. */
LaunchedEffect(isEnabled) {
if isEnabled == false && anchoredState.currentValue != SwipeAnchor.closed {
anchoredState.snapTo(SwipeAnchor.closed)
if activeSwipeKey.value == rowKey {
activeSwipeKey.value = nil
}
}
}

/* When a *different* row's swipe opens, animate this row closed.
Matches iOS list behavior of one open swipe at a time. */
let currentlyOpen = activeSwipeKey.value
Expand Down Expand Up @@ -729,7 +766,14 @@ public final class List : View, Renderable {
snap back to closed. */
LaunchedEffect(anchoredState.settledValue) {
let settled = anchoredState.settledValue
if settled == SwipeAnchor.trailingFull {
if isEnabled == false {
if settled != SwipeAnchor.closed {
anchoredState.snapTo(SwipeAnchor.closed)
}
if activeSwipeKey.value == rowKey {
activeSwipeKey.value = nil
}
} else if settled == SwipeAnchor.trailingFull {
if let action = trailingFullSwipeTarget?.action {
action()
}
Expand Down Expand Up @@ -811,7 +855,7 @@ public final class List : View, Renderable {
continue
}
Box(modifier: Modifier.weight(weight).fillMaxHeight()) {
RenderSwipeRevealButton(button: button, sizeModifier: Modifier.fillMaxSize(), context: context, tintOverride: leadingTintMap[button], onTap: {
RenderSwipeRevealButton(button: button, sizeModifier: Modifier.fillMaxSize(), context: context, tintOverride: leadingTintMap[button], isEnabled: isEnabled, isAccessibilityVisible: isEnabled && revealedLeadingPx > Float(1), onTap: {
button.action()
if button.role == ButtonRole.destructive, let onDestructiveDelete {
onDestructiveDelete()
Expand All @@ -826,7 +870,7 @@ public final class List : View, Renderable {
} else {
Row(modifier: Modifier.fillMaxHeight().wrapContentWidth().onSizeChanged { leadingNaturalState.value = Float($0.width) }) {
for button in leadingButtons {
RenderSwipeRevealButton(button: button, sizeModifier: Modifier.fillMaxHeight().widthIn(min: minButtonWidthDp), context: context, tintOverride: leadingTintMap[button], onTap: {
RenderSwipeRevealButton(button: button, sizeModifier: Modifier.fillMaxHeight().widthIn(min: minButtonWidthDp), context: context, tintOverride: leadingTintMap[button], isEnabled: isEnabled, isAccessibilityVisible: isEnabled && revealedLeadingPx > Float(1), onTap: {
button.action()
if button.role == ButtonRole.destructive, let onDestructiveDelete {
onDestructiveDelete()
Expand Down Expand Up @@ -854,7 +898,7 @@ public final class List : View, Renderable {
continue
}
Box(modifier: Modifier.weight(weight).fillMaxHeight()) {
RenderSwipeRevealButton(button: button, sizeModifier: Modifier.fillMaxSize(), context: context, tintOverride: trailingTintMap[button], onTap: {
RenderSwipeRevealButton(button: button, sizeModifier: Modifier.fillMaxSize(), context: context, tintOverride: trailingTintMap[button], isEnabled: isEnabled, isAccessibilityVisible: isEnabled && revealedTrailingPx > Float(1), onTap: {
button.action()
if button.role == ButtonRole.destructive, let onDestructiveDelete {
onDestructiveDelete()
Expand All @@ -869,7 +913,7 @@ public final class List : View, Renderable {
} else {
Row(modifier: Modifier.fillMaxHeight().wrapContentWidth().onSizeChanged { trailingNaturalState.value = Float($0.width) }) {
for button in trailingButtons {
RenderSwipeRevealButton(button: button, sizeModifier: Modifier.fillMaxHeight().widthIn(min: minButtonWidthDp), context: context, tintOverride: trailingTintMap[button], onTap: {
RenderSwipeRevealButton(button: button, sizeModifier: Modifier.fillMaxHeight().widthIn(min: minButtonWidthDp), context: context, tintOverride: trailingTintMap[button], isEnabled: isEnabled, isAccessibilityVisible: isEnabled && revealedTrailingPx > Float(1), onTap: {
button.action()
if button.role == ButtonRole.destructive, let onDestructiveDelete {
onDestructiveDelete()
Expand Down Expand Up @@ -926,7 +970,7 @@ public final class List : View, Renderable {
let o = anchoredState.offset
IntOffset(o.isNaN() ? 0 : o.toInt(), 0)
}
.anchoredDraggable(state: anchoredState, orientation: Orientation.Horizontal)
.anchoredDraggable(state: anchoredState, orientation: Orientation.Horizontal, enabled: isEnabled)
) {
RenderItem(content: content, level: level, context: context, styling: styling)
/* Subtle scrim that intensifies with swipe progress so the row
Expand Down Expand Up @@ -982,7 +1026,7 @@ public final class List : View, Renderable {
/// stretch mode it's fillMaxSize() inside a weighted Box so the button
/// expands with the row. Padding around the label keeps icon/text from
/// touching the cell edges.
@Composable private func RenderSwipeRevealButton(button: Button, sizeModifier: Modifier, context: ComposeContext, tintOverride: Color? = nil, onTap: () -> Void) {
@Composable private func RenderSwipeRevealButton(button: Button, sizeModifier: Modifier, context: ComposeContext, tintOverride: Color? = nil, isEnabled: Bool, isAccessibilityVisible: Bool, onTap: () -> Void) {
let backgroundColor: androidx.compose.ui.graphics.Color
let contentColor: androidx.compose.ui.graphics.Color
if button.role == ButtonRole.destructive {
Expand Down Expand Up @@ -1011,7 +1055,12 @@ public final class List : View, Renderable {
Row(modifier: sizeModifier
.clipToBounds()
.background(backgroundColor)
.clickable(onClick: onTap)
.semantics {
if isAccessibilityVisible == false {
hideFromAccessibility()
}
}
.clickable(onClick: onTap, enabled: isEnabled)
.padding(horizontal: 12.dp, vertical: 8.dp),
horizontalArrangement: Arrangement.Center,
verticalAlignment: androidx.compose.ui.Alignment.CenterVertically) {
Expand Down
Loading
Loading