superlistapp / super_editor

A Flutter toolkit for building document editors and readers
https://superlist.com/SuperEditor/
MIT License
1.53k stars 225 forks source link

magnifier magnifies the wrong area #1957

Closed BazinC closed 1 day ago

BazinC commented 3 weeks ago

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 commented 2 weeks ago

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

BazinC commented 2 weeks ago

@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 commented 1 week ago

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

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 commented 1 week ago

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 commented 1 week ago

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

BazinC commented 4 days ago

@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 https://github.com/superlistapp/super_editor/pull/1973 cc @angelosilvestre

angelosilvestre commented 1 day ago

@matthew-carroll Should this be closed now that https://github.com/superlistapp/super_editor/pull/1973 is merged?

matthew-carroll commented 1 day ago

I think so.