-
Notifications
You must be signed in to change notification settings - Fork 80
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
AOSP commonization: removing seemingly redundant drag cancellation for iOS magnifier #1713
Conversation
…FieldSelectionManager (for commonization with AOSP)
on draft until tested |
Retested |
@@ -461,10 +461,7 @@ internal class TextFieldSelectionManager(val undoManager: UndoManager? = null) { | |||
updateFloatingToolbar(show = true) | |||
} | |||
|
|||
override fun onCancel() { | |||
draggingHandle = 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.
The code looks reasonable (The same happened in onStop()
).
Could you please elaborate, what fixes this removal?
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 does look reasonable indeed.
My point here is that we should upstream this to AOSP, and the AOSP doesn't have these lines and doesn't have the stuck magnifier bug. It was assumed in the magnifier PR that this issue couldn't be reproduced on Android because the reproduction steps couldn't be replicated there, which turned out to be incorrect.
So I have tested that on physical iOS and Android devices, and I couldn't reproduce the issue without these lines.
Although I remember that bug too, I think we can remove these lines if we can't reproduce it now.
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.
Can we upstream these lines to be on the safe 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.
Or instead we can write some test in AOSP to make sure that we won't have regression here
I could retest and record reproduction steps if the bug is still present, but i'm unable to build iOS part on macos 15 / xcode 16 - xcodebuild crashes on uikit utils module. Still can do it with IPA if you want or if there is a build workaround |
@alexzhirkevich , we don't have proper solution as well. A workaround is to use XCode 15.4 to build and test the app on a device. Build on simulator should work fine, just clean all cached (compose-mulatiplatform-core/out) and restart the gradle deamon. |
@alexzhirkevich, in addition to @ASalavei answer, building on a device may require resetting the checkbox "Automatically manage signing" and changing bundle identifier for each build upd. If you manage to reproduce it, could you please attach a video and the reproduction steps? Thanks in advance, you will help us greatly |
I confirm the bug is no longer reproducible |
@alexzhirkevich, many thanks! |
Potentially fixes CMP-5763 and CMP-5764
I wasn't able to reproduce the bug, which should be fixed by these lines (it was introduced here), so I suggest retesting magnifier behavior without these lines and removing them afterwards if my suspicion is correct.