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 magnifies the wrong area #1957

Closed
BazinC opened this issue Apr 22, 2024 · 8 comments · Fixed by #1990
Closed

magnifier magnifies the wrong area #1957

BazinC opened this issue Apr 22, 2024 · 8 comments · Fixed by #1990
Assignees
Labels
area_super_reader Related to SuperReader area_supereditor Pertains to SuperEditor area_supertextfield Pertains to SuperTextField bounty_junior f:clickup time:2 type_bug Something isn't working

Comments

@BazinC
Copy link

BazinC commented Apr 22, 2024

Package Version
stable or main branch

To Reproduce
On ios, long press on text to display magnifier.

Actual behavior
See video below. simulator (iPhone 15 iOS 17.0) running super editor example. magnified area is off. It looks like it magnifies the content below the cursor.

supereditor.ios.magnifier.webm

Expected behavior

native.ios.magnifier.webm

Same with Android simulator (Pixel 7 Pro API 32):
Screenshot_1713780723

Platform
iOS and Android

Flutter version
3.19.1

Additional context
I suggest the super_editor default magnifier to use or take inspiration from Flutter CupertinoTextMagnifier/TextMagnifier

@angelosilvestre
Copy link
Collaborator

@BazinC Are you using some specific settings to trigger this issue on Android? I was able to reproduce it on iOS only.

@angelosilvestre angelosilvestre added the awaiting-customer-feedback Waiting for the customer to respond to us label Apr 30, 2024
@BazinC
Copy link
Author

BazinC commented May 2, 2024

@angelosilvestre For Android I'm just running the demo app, from stable branch, on an Android Simulator (Pixel 7 Pro API 32).
I don't have a physical Android device with me right now. I can try later when I have one.

image

@matthew-carroll
Copy link
Contributor

matthew-carroll commented May 7, 2024

@angelosilvestre @BazinC - It looks like we're seeing unpredictable API level problems for Android:

  • Pixel 7 Pro emulator, API 32: The magnification region is too low
  • Pixel 7 Pro emulator, API 33: The magnification region is correct
  • Pixel 7 Pro emulator, API 34: The magnification region is too low

EDIT:

I take that back. Somehow I got the devices mixed up. It looks like all Pixel 7 Pro emulators have the problem. The Pixel 6 emulator doesn't. I noticed that there's a difference in pixel density between Pixel 7 Pro and Pixel 6.

@angelosilvestre please see if you can work a MediaQuery pixel density into the number. For example, right now the vertical offset is -58 and when that number works as desired, the pixel density is 2.625. This implies that the density independent offset is -58 / 2.625 ~= 22. Therefore, if we change to Offset(0, -22 * MediaQuery.of(context).devicePixelRatio) I think it should be OK everywhere. We'll need a similar change for iOS, but with a different value that's chosen for the iOS magnifier size.

@angelosilvestre
Copy link
Collaborator

For the Pixel 7 Pro emulator, the devicePixelRatio is 3.5, and Offset(0, -22 * MediaQuery.of(context).devicePixelRatio) doesn't produce correct results:

image

The expression produces -77.0 while the value to magnify the correct area is -45, where 45 / 3.5 ~= 13.

@angelosilvestre
Copy link
Collaborator

@matthew-carroll If we just do -150 / pixelRatio it seems to produce the correct results. -150 is the follower offset.

@BazinC
Copy link
Author

BazinC commented May 15, 2024

@matthew-carroll I suggest you reopen the ticket since it is not fixed on ios.
The ios fix seems to be implemented in this PR #1973
cc @angelosilvestre

@angelosilvestre angelosilvestre removed the awaiting-customer-feedback Waiting for the customer to respond to us label May 15, 2024
@angelosilvestre
Copy link
Collaborator

@matthew-carroll Should this be closed now that #1973 is merged?

@matthew-carroll
Copy link
Contributor

I think so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area_super_reader Related to SuperReader area_supereditor Pertains to SuperEditor area_supertextfield Pertains to SuperTextField bounty_junior f:clickup time:2 type_bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants