-
Notifications
You must be signed in to change notification settings - Fork 252
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
[SuperEditor][SuperTextField][mobile] Do not crop magnifier at screen edges (Resolves #2167) #2482
Conversation
@matthew-carroll I had to add some tolerance to the goldens added in this PR, because they are always failing with some pixel diff in the CI. |
// of the magnifier. To counteract this, shift the content back before | ||
// applying the scaling, so it remains centered after scaling. | ||
..translate( | ||
-size.width * (magnificationScale - 1) / 2, |
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.
Only comment is that it's not clear to me why we're adding (1 * width) and (1 * heigh) back to the value. We should find some way to explain this in the comment above. Also, to make that fact very clear, we should probably change the translation to:
-(size.width * magnificationScale / 2) + (size.width / 2)
I believe that's the same equation.
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.
In fact, the calculation can be:
-(size.width - (size.width * magnificationScale / 2))
or
(size.width / 2) - (size.width * magnificationScale / 2)
or
-(size.width * magnificationScale - size.width) / 2
I think the last one is the easiest to understand.
f30ad2e
to
6249291
Compare
@matthew-carroll It seems the test "keeps current selection when tapping on caret" from the previous PR started to fail. In a place where we call It seems that, somehow, we are trying to tap while the magnifier is running the exit animation, and the tap is being reported to the magnifier instead of the editor. I modified the test to call |
Why not place an |
@matthew-carroll No particular reason. Updated to do that. |
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.
LGTM
[SuperEditor][SuperTextField][mobile] Do not crop magnifier at screen edges. Resolves #2167
Currently, our magnifiers aren't confined within the screen edges:
SuperEditor Android
Super Editor iOS
Constraining the magnifier to screen edges should be as simple as applying a
ScreenFollowerBoundary
to theFollower
. However, we use aFractionalTranslation
to align theFollower
and this doesn't seem to play nicely withScreenFollowerBoundary
. We can see in the following video that the aligner uses the position before the transformation to constrain it:android_magnifier.webm
To make it work correctly, I had to get rid of the
FractionalTranslation
. It took a bit of trial and error to make adjust the magnifier transformations to make it correctly center the magnified content. Some of our offsets were not accounting for the device pixel ratio, so I also adjusted them.There are some places that we are still not using follow_the_leader, so I also modified those places.
Results:
android_magnifier_edges.webm
Simulator.Screen.Recording.-.iPhone.15.Plus.-.2024-12-31.at.09.12.10.mp4