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][SuperReader][SuperTextField] Update iOS magnifier (Resolves #1958) #1973
Conversation
@angelosilvestre can you show a zoomed out video of iOS with slow animations that clearly shows the appearance and disappearance of the native iOS magnifier? It's difficult to compare the zoomed out Super Editor version against that initial iOS video. I'm curious if the Super Editor magnifier border should really be that wide. Also, given that we can't render a true squircle shape, I'm wondering if our intro and exit animation should go from a small circle to a rounded rectangle. Or, similarly, keep the same corner radius the whole time, which will naturally start as a circle and then become a rounded rectangle. |
CC @BazinC |
My.Movie.mp4 |
@matthew-carroll I updated the animation. At the beginning of the animation the shape looks like more with a pill Simulator.Screen.Recording.-.iPhone.15.-.2024-05-04.at.13.59.30.mp4With slow animations: Simulator.Screen.Recording.-.iPhone.15.-.2024-05-04.at.13.59.50.mp4 |
return Center( | ||
child: IOSFollowingMagnifier.roundedRectangle( | ||
magnifierKey: magnifierKey, | ||
leaderLink: focalPoint, | ||
offsetFromFocalPoint: const Offset(0, -72), | ||
offsetFromFocalPoint: const Offset(0, -230), |
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.
Why did the magic number change? There probably shouldn't have been a magic number in the first place, so let's at least comment where that number came from and how someone can get a new one, if needed.
On a related note, did you check what happens on desktop when using the magnifier? I'm wondering if the reason you had to change this number was due to pixel density. If so, it shouldn't be a constant.
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.
Why did the magic number change?
Since the magnifier size changed, this value needs to be adjusted to prevent the magnifier from covering the caret. This value was chosen empirically...
On a related note, did you check what happens on desktop when using the magnifier? I'm wondering if the reason you had to change this number was due to pixel density. If so, it shouldn't be a constant.
Yeah, it doesn't look right when running on desktop:
Screen.Recording.2024-05-06.at.20.56.28.mov
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.
Should this be upated with pixel density? Also, we're still missing a comment explanation about where the number came from.
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.
This value is already adjusted to the pixel ratio in the magnifier widget.
return Center( | ||
child: IOSFollowingMagnifier.roundedRectangle( | ||
magnifierKey: magnifierKey, | ||
leaderLink: focalPoint, | ||
offsetFromFocalPoint: const Offset(0, -72), | ||
offsetFromFocalPoint: const Offset(0, -230), | ||
animationController: animationController, |
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.
Is there a reason we pass the AnimationController
from the outside? Isn't the animation always the same? If so, it seems excessive to make the client responsible for configuring an animation and running it. Is there any reason that this widget shouldn't operate like an implicitly animated widget?
If you want optional outside control for verification purposes, then the controller should be optional and documented as such.
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.
Should we pass the animation value as ValueListenable
here? Just the percentage?
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 question is "why?". What kind of control is needed? Would it suffice to simply pass a bool
to a property called show
? Could the magnifier then show/hide itself?
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.
Would it suffice to simply pass a bool to a property called show? Could the magnifier then show/hide itself?
Yeah, it can. Updated to do that.
@@ -283,7 +283,7 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor> | |||
Offset? _startDragPositionOffset; | |||
double? _dragStartScrollOffset; | |||
Offset? _globalDragOffset; | |||
final _magnifierOffset = ValueNotifier<Offset?>(null); | |||
final _magnifierFocalPoint = ValueNotifier<Offset?>(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 new name for this property doesn't make it clear whether this is an absolute position, or whether this is a delta. Please double check the name, and also add a comment for clarity.
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.
Updated
super_editor/lib/src/default_editor/document_gestures_touch_ios.dart
Outdated
Show resolved
Hide resolved
/// Updates the magnifier focal point in relation to the current drag position. | ||
void _updateMagnifierFocalPoint() { | ||
late DocumentPosition? docPositionToMagnify; | ||
|
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.
What ended up being the issue that caused this change in code? What were we doing wrong before?
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.
This code is here to fix the magnification area issue described in #1957
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.
Ok, but I asked what ended up being the issue? What was the root problem that you found and fixed with that code?
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 root cause is that we were using the finger position as the focal point.
@@ -1824,3 +1892,6 @@ class SuperEditorIosHandlesDocumentLayerBuilder implements SuperEditorLayerBuild | |||
); | |||
} | |||
} | |||
|
|||
const defaultIosMagnifierAnimationDuration = Duration(milliseconds: 180); |
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.
What about the default curve? I would guess that iOS isn't using a linear curve.
BTW, didn't we determine that iOS disappears faster than it appears?
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.
Yeah, We have another constant for the exit animation.
super_editor/lib/src/infrastructure/platforms/ios/magnifier.dart
Outdated
Show resolved
Hide resolved
}); | ||
|
||
final Offset offsetFromFocalPoint; | ||
final double animationPercentage; |
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 doesn't make a lot of sense to have a concept of "animation percentage". It begs the question "what animation"? Moreover, as mentioned above, we might actually need different intro vs exit animations.
Instead, you should capture the individual properties that you care about and then let the animation code handle the responsibility of translating animation progress into magnifier properties.
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.
Would it make more sense to have the animation progress and the animation direction (forward/reverse) ?
super_editor/lib/src/infrastructure/platforms/ios/magnifier.dart
Outdated
Show resolved
Hide resolved
@@ -132,6 +132,10 @@ typedef DocumentFloatingToolbarBuilder = Widget Function( | |||
/// ``` | |||
typedef DocumentMagnifierBuilder = Widget Function(BuildContext, Key magnifierKey, LeaderLink focalPoint); | |||
|
|||
/// A [DocumentMagnifierBuilder] that animates the magnifier's appearance and disappearance. |
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.
Does Android have intro and exit animations, too? If so, we should probably migrate both magnifiers to the new signature, and keep the existing builder name, instead of introducing a second one.
Moreover, this builder should probably take two optional values, perhaps introPercent
and exitPercent
. Only one is provided at a given time.
typedef DocumentMagnifierBuilder = Widget Function(buildContext, Key magnifierKey, LeaderLink focalPoint, {double? introPercent, double? exitPercent});
Would that work?
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.
Moreover, this builder should probably take two optional values, perhaps introPercent and exitPercent. Only one is provided at a given time.
Wouldn't it be better if we provide only one percent and the direction of the animation?
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.
That should be fine
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.
Does Android have intro and exit animations, too?
Android doesn't seem to have intro and exit animations.
@matthew-carroll I tried to simplify the usage of magic numbers and adapted the code to take the pixel density into consideration: Simulator.Screen.Recording.-.iPhone.15.-.2024-05-09.at.21.39.37.mp4With slow animations: Simulator.Screen.Recording.-.iPhone.15.-.2024-05-09.at.21.38.58.mp4 |
return Center( | ||
child: IOSFollowingMagnifier.roundedRectangle( | ||
magnifierKey: magnifierKey, | ||
leaderLink: focalPoint, | ||
offsetFromFocalPoint: const Offset(0, -72), | ||
offsetFromFocalPoint: const Offset(0, -230), |
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.
Should this be upated with pixel density? Also, we're still missing a comment explanation about where the number came from.
super_editor/example/lib/demos/editor_configs/demo_mobile_editing_ios.dart
Outdated
Show resolved
Hide resolved
@@ -1006,7 +1008,6 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor> | |||
} | |||
|
|||
void _onPanEnd(DragEndDetails details) { | |||
_magnifierOffset.value = 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.
Does this not need to be replaced with some other kind of update?
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.
I don't think it's necessary. We already call _controlsController.hideMagnifier()
.
final _magnifierOffset = ValueNotifier<Offset?>(null); | ||
|
||
/// The [Offset] of the magnifier's focal point in the [DocumentLayout] coordinate space. | ||
final _magnifierFocalPointInContentSpace = ValueNotifier<Offset?>(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.
What is "content space"? Do you mean "document space"?
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.
Yes, updated.
show: visible, | ||
// The bottom of the magnifier sits above the focal point. | ||
// Leave a few pixels between the bottom of the magnifier and the focal point. | ||
offsetFromFocalPoint: const Offset(0, -20), |
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.
Why did this change from -72 to -20? Should this number account for pixel density?
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.
This changed because the follower alignment was changed, so this number needed to be adjusted. It was chosen empirically.
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.
Ok, but what about pixel density?
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 the _IOSFollowingMagnifierState
build method, we multiply the offsetFromFocalPoint
by the device pixel ratio.
// the actual offset, or it should be `widget.offsetFromFocalPoint.dy / magnificationLevel`. Neither | ||
// of those align the focal point correctly. The following offset was found empirically to give the | ||
// desired results. These values seem to work even with different pixel densities. | ||
offsetFromFocalPoint: Offset( |
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.
Should the device pixel ratio be used here, too?
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.
While I was testing this, the value didn't seem to be affected by the pixel ratio.
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.
Do you know why it wasn't effected? That seems concerning.
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.
I'm not sure why this is the case...
|
||
final Offset offsetFromFocalPoint; | ||
final double animationValue; | ||
final AnimationStatus animationDirection; |
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.
Using AnimationStatus
for this property isn't appropriate. There's no meaning behind dismissed
or completed
, and yet those would be valid values.
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.
Updated
super_editor/lib/src/super_reader/read_only_document_ios_touch_interactor.dart
Outdated
Show resolved
Hide resolved
leaderLink: magnifierFocalPoint, | ||
offsetFromFocalPoint: const Offset(0, -72), | ||
offsetFromFocalPoint: const Offset(0, -230), |
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.
Should this be based on device pixel ratio?
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.
This value is already adjusted to the pixel ratio in IOSFollowingMagnifier
.
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.
What do you mean by adjusted? This is a hard-coded offset...
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 the _IOSFollowingMagnifierState
build method, we multiply the offsetFromFocalPoint
by the device pixel ratio.
return IOSFollowingMagnifier.roundedRectangle( | ||
leaderLink: widget.editingController.magnifierFocalPoint, | ||
show: showMagnifier, | ||
offsetFromFocalPoint: const Offset(0, -230), |
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.
Should this use device pixel ratio?
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.
This value is already adjusted to the pixel ratio in IOSFollowingMagnifier
.
@angelosilvestre It looks like there are multiple |
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 - I'll go ahead and merge this for forward progress, but please come back and document the DPI vs pixel values when you get a chance.
[SuperEditor][SuperReader][SuperTextField] Update iOS magnifier. Resolves #1958
The current appearance and behavior of our iOS magnifier doesn't match the native appearance and behavior.
Native iOS:
native.ios.magnifier.webm
SuperEditor:
supereditor.ios.magnifier.webm
This PR adds entrance/exit animations and updates the size and shape of the magnifier:
Simulator.Screen.Recording.-.iPhone.15.-.2024-04-28.at.17.18.40.mp4
Here's the appearance with slow animations:
Simulator.Screen.Recording.-.iPhone.15.-.2024-04-28.at.17.20.58.mp4
This PR also includes the fix for the issue iOS described in #1957