Skip to content
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

Jump to bottom

[a11y] Pass the id of the node whose layout changed to accessibility controllers #1162

Merged
merged 1 commit into from
Mar 5, 2024
Merged

[a11y] Pass the id of the node whose layout changed to accessibility controllers #1162

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions compose/ui/ui/api/desktop/ui.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3152,6 +3152,7 @@ public abstract interface class androidx/compose/ui/platform/PlatformContext$Roo
}

public abstract interface class androidx/compose/ui/platform/PlatformContext$SemanticsOwnerListener {
public abstract fun onLayoutChange (Landroidx/compose/ui/semantics/SemanticsOwner;I)V
public abstract fun onSemanticsChange (Landroidx/compose/ui/semantics/SemanticsOwner;)V
public abstract fun onSemanticsOwnerAppended (Landroidx/compose/ui/semantics/SemanticsOwner;)V
public abstract fun onSemanticsOwnerRemoved (Landroidx/compose/ui/semantics/SemanticsOwner;)V
Expand Down
10 changes: 10 additions & 0 deletions ...ui/ui/src/desktopMain/kotlin/androidx/compose/ui/platform/a11y/AccessibilityController.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,16 @@ internal class AccessibilityController(
syncNodesChannel.trySend(Unit)
}

/**
* Invoked when the position and/or size of the [SemanticsNode] with the given semantics id
* changed.
*/
fun onLayoutChanged(@Suppress("UNUSED_PARAMETER") nodeId: Int) {
// TODO: Only recompute the layout-related properties of the node
nodeMappingIsValid = false
syncNodesChannel.trySend(Unit)
}

/**
* The [SemanticsNode] that is the root of the semantics node tree.
*/
Expand Down
5 changes: 4 additions & 1 deletion ...se/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.desktop.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ import javax.swing.SwingUtilities
import kotlin.coroutines.CoroutineContext
import kotlin.math.roundToInt
import org.jetbrains.skia.Canvas
import org.jetbrains.skiko.ClipComponent
import org.jetbrains.skiko.ClipRectangle
import org.jetbrains.skiko.ExperimentalSkikoApi
import org.jetbrains.skiko.GraphicsApi
Expand Down Expand Up @@ -634,6 +633,10 @@ internal class ComposeSceneMediator(
override fun onSemanticsChange(semanticsOwner: SemanticsOwner) {
_accessibilityControllers[semanticsOwner]?.onSemanticsChange()
}

override fun onLayoutChange(semanticsOwner: SemanticsOwner, semanticsNodeId: Int) {
_accessibilityControllers[semanticsOwner]?.onLayoutChanged(nodeId = semanticsNodeId)
}
}

private inner class DesktopPlatformContext : PlatformContext by PlatformContext.Empty {
Expand Down
5 changes: 4 additions & 1 deletion compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,10 @@ internal class RootNodeOwner(
}

override fun onLayoutChange(layoutNode: LayoutNode) {
platformContext.semanticsOwnerListener?.onSemanticsChange(semanticsOwner)
platformContext.semanticsOwnerListener?.onLayoutChange(
semanticsOwner = semanticsOwner,
semanticsNodeId = layoutNode.semanticsId
)
}

override fun getFocusDirection(keyEvent: KeyEvent): FocusDirection? {
Expand Down
10 changes: 10 additions & 0 deletions compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformContext.skiko.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import androidx.compose.ui.geometry.Rect
import androidx.compose.ui.input.InputMode
import androidx.compose.ui.input.InputModeManager
import androidx.compose.ui.input.pointer.PointerIcon
import androidx.compose.ui.node.LayoutNode
import androidx.compose.ui.node.Owner
import androidx.compose.ui.node.RootForTest
import androidx.compose.ui.scene.ComposeScene
Expand Down Expand Up @@ -125,6 +126,15 @@ interface PlatformContext {
* @see Owner.onSemanticsChange
*/
fun onSemanticsChange(semanticsOwner: SemanticsOwner)

/**
* Callback method that is called when the position and/or size of the [LayoutNode] with
* the given semantics id changed.
*
* Note that the id, rather than the [LayoutNode] itself, is passed here because
* [LayoutNode] is an internal type, so it can't be exposed in a public method.
*/
fun onLayoutChange(semanticsOwner: SemanticsOwner, semanticsNodeId: Int)
Copy link
Member

Choose a reason for hiding this comment

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

We already discussed this with @elijah-semyonov and as far as I remember we decided to introduce optional second parameter to onSemanticsChange instead of adding a new callback

Copy link
Author

Choose a reason for hiding this comment

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

Having it in a separate function adds more information; namely that only the bounds of the node have changed, and not any semantic properties.

Copy link
Author

@m-sasha m-sasha Mar 5, 2024 •
edited

Choose a reason for hiding this comment

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

The API changed here isn't public, by the way, so no need for the "changes in API" tag.

Copy link
Member

Choose a reason for hiding this comment

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

it's public but with "internal" annotation. The rule for tag is basically - "changes in desktop/ui.api file".
I'd apply the rule regarding two reviewers here

Copy link
Author

Choose a reason for hiding this comment

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

I disagree. @internalapi implies that it's internal, and only technically public because of the limitations of the language. We don't make any compatibility guarantees about such code.

Copy link
Member

Choose a reason for hiding this comment

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

ok, it's not a big deal in this case

}

companion object {
Expand Down
8 changes: 8 additions & 0 deletions compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/platform/Accessibility.uikit.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -967,6 +967,14 @@ internal class AccessibilityMediator(
isCurrentComposeAccessibleTreeDirty = true
}

fun onLayoutChange(nodeId: Int) {
debugLogger?.log("onLayoutChange (nodeId=$nodeId)")

// TODO: Only recompute the layout-related properties of the node
isCurrentComposeAccessibleTreeDirty = true

}

fun convertRectToWindowSpaceCGRect(rect: Rect): CValue<CGRect> {
val window = view.window ?: return CGRectMake(0.0, 0.0, 0.0, 0.0)
val density = Density(window.screen.scale.toFloat())
Expand Down
8 changes: 8 additions & 0 deletions compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,14 @@ private class SemanticsOwnerListenerImpl(
current.second.onSemanticsChange()
}
}

override fun onLayoutChange(semanticsOwner: SemanticsOwner, semanticsNodeId: Int) {
val current = current ?: return

if (current.first == semanticsOwner) {
current.second.onLayoutChange(nodeId = semanticsNodeId)
}
MatkovIvan marked this conversation as resolved.
Show resolved Hide resolved
}
}

private class RenderingUIViewDelegateImpl(
Expand Down