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.

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

Fix a11y wrong bounds calculation #1165

Merged
merged 2 commits into from
Mar 7, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,10 @@ private class AccessibilityElement(

override fun accessibilityFrame(): CValue<CGRect> =
getOrElse(CachedAccessibilityPropertyKeys.accessibilityFrame) {
mediator.convertRectToWindowSpaceCGRect(semanticsNode.boundsInWindow)
// AX services expect the frame to be in the coordinate space of the root UIWindow
// [semanticsNode.boundsInWindow] provide it in so called `window container` space,
// which can be different from the root UIWindow space.
mediator.convertContainerWindowRectToRootWindowCGRect(semanticsNode.boundsInWindow)
}


Expand Down Expand Up @@ -877,6 +880,7 @@ internal class AccessibilityMediator(
private val owner: SemanticsOwner,
coroutineContext: CoroutineContext,
private val getAccessibilitySyncOptions: () -> AccessibilitySyncOptions,
val convertContainerWindowRectToRootWindowCGRect: (Rect) -> CValue<CGRect>,
val performEscape: () -> Boolean
) {
/**
Expand Down Expand Up @@ -975,13 +979,6 @@ internal class AccessibilityMediator(

}

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())
val localSpaceCGRect = rect.toDpRect(density).asCGRect()
return window.convertRect(localSpaceCGRect, fromView = view)
}

fun dispose() {
check(isAlive) { "AccessibilityMediator is already disposed" }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,23 @@
package androidx.compose.ui.platform

import androidx.compose.ui.geometry.Offset
import androidx.compose.ui.geometry.Rect
import androidx.compose.ui.uikit.systemDensity
import androidx.compose.ui.unit.Density
import androidx.compose.ui.unit.IntSize
import androidx.compose.ui.unit.asCGPoint
import androidx.compose.ui.unit.asCGRect
import androidx.compose.ui.unit.asDpOffset
import androidx.compose.ui.unit.toDpOffset
import androidx.compose.ui.unit.toDpRect
import androidx.compose.ui.unit.toOffset
import kotlinx.cinterop.CValue
import kotlinx.cinterop.readValue
import kotlinx.cinterop.useContents
import platform.CoreGraphics.CGRect
import platform.CoreGraphics.CGRectZero
import platform.UIKit.UIView
import platform.UIKit.UIWindow

/**
* Tracking a state of window.
Expand All @@ -33,6 +42,9 @@ internal class PlatformWindowContext {
private val _windowInfo = WindowInfoImpl()
val windowInfo: WindowInfo get() = _windowInfo

/**
* A container used for additional layers and as reference for window coordinate space.
*/
private var _windowContainer: UIView? = null

fun setWindowFocused(focused: Boolean) {
Expand Down Expand Up @@ -65,6 +77,36 @@ internal class PlatformWindowContext {
)
}

/**
* Converts the given [Rect] from the coordinate space of the container window to the coordinate
* space of the root UIWindow in which hierarchy with the container window resides.
*/
fun convertContainerWindowRectToRootWindowCGRect(rect: Rect): CValue<CGRect> {
val windowContainer = _windowContainer ?: return CGRectZero.readValue()
Copy link
Member

Choose a reason for hiding this comment

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

It should return rect from parameters, not zero rect in the case

Copy link
Author

Choose a reason for hiding this comment

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

Same as:
#1165 (comment)


val windowContainerWindow = windowContainer.window

fun UIWindow.density(): Density {
Copy link
Member

Choose a reason for hiding this comment

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

no need to be local function, move to the end of the file

Copy link
Author

Choose a reason for hiding this comment

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

I don't want to move it in that scope, because that's not a real density with correct fontScale. It's only there because following calculations need Density instead of just Float, and that can be derived from UIWindow directly.

return Density(screen.scale.toFloat())
}

return if (windowContainerWindow == null) {
if (windowContainer is UIWindow) {
// windowContainer is the root window itself
rect.toDpRect(windowContainer.density()).asCGRect()
MatkovIvan marked this conversation as resolved.
Show resolved Hide resolved
} else {
// windowContainer is not attached to any window
CGRectZero.readValue()
Copy link
Member

Choose a reason for hiding this comment

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

Again, return rect in this case

Copy link
Author

Choose a reason for hiding this comment

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

Do we support the case where _windowContainer is null and we still perform this calculation? Why should we return any value at this point? I'd better throw an exception instead, tbh

Copy link
Member

Choose a reason for hiding this comment

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

Yes we do. for this class without context from its usages, it means that _windowContainer == compose view

}
} else {
Copy link
Member

Choose a reason for hiding this comment

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

check if windowContainerWindow != windowContainer

Copy link
Author

Choose a reason for hiding this comment

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

windowContainer.window will be null in case it's true, it's a redundant check.

val cgRect = rect
.toDpRect(windowContainerWindow.density())
.asCGRect()

windowContainer.convertRect(cgRect, toView = windowContainerWindow)
MatkovIvan marked this conversation as resolved.
Show resolved Hide resolved
}
}

private fun convertPoint(point: Offset, fromView: UIView, toView: UIView): Offset {
return if (fromView != toView) {
val density = fromView.systemDensity
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import androidx.compose.runtime.InternalComposeApi
import androidx.compose.runtime.MutableState
import androidx.compose.runtime.mutableStateOf
import androidx.compose.ui.geometry.Offset
import androidx.compose.ui.geometry.Rect
import androidx.compose.ui.graphics.asComposeCanvas
import androidx.compose.ui.input.InputMode
import androidx.compose.ui.input.key.KeyEvent
Expand Down Expand Up @@ -83,6 +84,7 @@ import org.jetbrains.skiko.SkikoKeyboardEventKind
import platform.CoreGraphics.CGAffineTransformIdentity
import platform.CoreGraphics.CGAffineTransformInvert
import platform.CoreGraphics.CGPoint
import platform.CoreGraphics.CGRect
import platform.CoreGraphics.CGRectMake
import platform.CoreGraphics.CGRectZero
import platform.CoreGraphics.CGSize
Expand Down Expand Up @@ -124,6 +126,7 @@ private class SemanticsOwnerListenerImpl(
private val container: UIView,
private val coroutineContext: CoroutineContext,
private val getAccessibilitySyncOptions: () -> AccessibilitySyncOptions,
private val convertContainerWindowRectToRootWindowCGRect: (Rect) -> CValue<CGRect>,
private val performEscape: () -> Boolean
) : PlatformContext.SemanticsOwnerListener {
var current: Pair<SemanticsOwner, AccessibilityMediator>? = null
Expand All @@ -135,6 +138,7 @@ private class SemanticsOwnerListenerImpl(
semanticsOwner,
coroutineContext,
getAccessibilitySyncOptions,
convertContainerWindowRectToRootWindowCGRect,
performEscape
)
}
Expand Down Expand Up @@ -294,6 +298,9 @@ internal class ComposeSceneMediator(
getAccessibilitySyncOptions = {
configuration.accessibilitySyncOptions
},
convertContainerWindowRectToRootWindowCGRect = {
windowContext.convertContainerWindowRectToRootWindowCGRect(it)
},
performEscape = {
val down = onKeyboardEvent(
KeyEvent(
Expand Down
Loading