-
Notifications
You must be signed in to change notification settings - Fork 81
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
Conversation
* 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() |
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.
It should return rect from parameters, not zero rect in the case
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.
Same as:
#1165 (comment)
rect.toDpRect(windowContainer.density()).asCGRect() | ||
} else { | ||
// windowContainer is not attached to any window | ||
CGRectZero.readValue() |
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.
Again, return rect
in this case
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.
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
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.
Yes we do. for this class without context from its usages, it means that _windowContainer
== compose view
|
||
val windowContainerWindow = windowContainer.window | ||
|
||
fun UIWindow.density(): Density { |
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.
no need to be local function, move to the end of the file
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 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.
// windowContainer is not attached to any window | ||
CGRectZero.readValue() | ||
} | ||
} else { |
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.
check if windowContainerWindow != windowContainer
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.
windowContainer.window will be null in case it's true, it's a redundant check.
Proposed Changes
Fix the logical error where
boundsInWindow
is treated as view local coordinates.Testing
Test: Demo NativeModalWithNavigation now resolves correct
accessibilityFrame
. So is the case for the views whose frames don't match the app root UIWindow bounds.