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

Magnifier for iOS 17+ #969

Closed

Conversation

alexzhirkevich
Copy link

Proposed Changes

  • Magnifier for iOS 17+
  • Modifier.magnifier in common

Testing

Test: try magnifier in all types of text fields

Issues Fixed

Fixes: JetBrains/compose-multiplatform#3692

@alexzhirkevich alexzhirkevich marked this pull request as ready for review December 27, 2023 18:22
@mazunin-v-jb
Copy link

mazunin-v-jb commented Jan 5, 2024

Hello! Thank you for the request.
I'll look thoroughly into it on weekend.
And we need one more person to review, I'll ask someone from our team.

@mazunin-v-jb mazunin-v-jb self-requested a review January 5, 2024 21:47
@alexzhirkevich
Copy link
Author

I'd wait for #980. It contains stable magnifier definition that is slightly different from current

@mazunin-v-jb
Copy link

Hello, @alexzhirkevich!
We discussed this change with Google, the general idea is good, but it needs to be iterated a bit on their side.
The changes in commonMain should be done in AOSP first. We avoid changing commonMain API here without changing it in AOSP first without a good reason.
So, could you please create a CL (Change List, PR) in AOSP project first?

Just in case, here is some useful information:

  1. How to clone AOSP:
mkdir ~/bin
PATH=~/bin:$PATH
curl https://storage.googleapis.com/git-repo-downloads/repo > ~/bin/repo
chmod a+x ~/bin/repo
mkdir androidx-main && cd androidx-main
repo init -u https://android.googlesource.com/platform/manifest -b androidx-main --partial-clone --clone-filter=blob:limit=10M
repo sync
  1. Guides:
    https://source.android.com/docs/setup/contribute/submit-patches
    https://source.android.com/docs/setup/contribute/life-of-a-patch

If that is cumbersome, we also gladly accept changes only in platform sourceSets.

@alexzhirkevich
Copy link
Author

alexzhirkevich commented Jan 16, 2024

Let's have it in uikitMain only for now then? Anyway the amount of people using this modifier for purposes other than text selection is quite low I guess. Having default magnifier for text fields and selection container will close 99% of this modifier usage. Modifier itself can be internal for now, just to implement the default text selection magnifier that I lack a lot.

@mazunin-v-jb
Copy link

Yes, I think making uikit only implementation of magnifier is a good solution here 👍

@alexzhirkevich
Copy link
Author

alexzhirkevich commented Jan 17, 2024

@mazunin-v-jb
I found some bugs in text selection while implementing magnifier. Reproduces on current jb-main df6b5e63c69c35c4c18022ef99e017072af4601d:

RPReplay_Final1705527572.MP4

Double tap with hidden keyboard causes menu flashing:

RPReplay_Final1705527489.MP4

Double tap menu with open keyboard is incorrect. If you then drag this text a little bit, correct menu shows:

RPReplay_Final1705527500.MP4

@alexzhirkevich alexzhirkevich marked this pull request as draft January 18, 2024 11:57
@alexzhirkevich
Copy link
Author

Merged with 1.6 beta and moved to uikit-only here: #1000

@mazunin-v-jb
Copy link

mazunin-v-jb commented Jan 22, 2024

@alexzhirkevich, thank you! I'll take a look

Upd. I reproduced the first issue, but couldn't reproduce the second one. Could you please create an issue here with the flashing context menu and tag me in it?

@alexzhirkevich
Copy link
Author

alexzhirkevich commented Jan 22, 2024

There is no extra info i can provide actually except that i run mpp app on 12 mini /17.2.1. May be related to the #964. Or because of ClearFocusBox is used there

@alexzhirkevich
Copy link
Author

@mazunin-v-jb Yep, it does not happen on Example1 screen. Seems like ClearFocusBox issue

@alexzhirkevich alexzhirkevich deleted the magnifier-ios-17 branch March 23, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants