[SuperEditor][SuperReader][SuperTextField] Update iOS magnifier (Resolves #1958) #1973

angelosilvestre · 2024-04-28T20:22:41Z

[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

…lves #1958)

matthew-carroll · 2024-05-01T04:29:37Z

@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.

matthew-carroll · 2024-05-01T04:30:26Z

CC @BazinC

angelosilvestre · 2024-05-02T22:39:18Z

@matthew-carroll

My.Movie.mp4

angelosilvestre · 2024-05-04T17:03:36Z

@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.mp4

With slow animations:

Simulator.Screen.Recording.-.iPhone.15.-.2024-05-04.at.13.59.50.mp4

matthew-carroll · 2024-05-06T22:00:21Z

super_editor/example/lib/demos/editor_configs/demo_mobile_editing_ios.dart

    return Center(
      child: IOSFollowingMagnifier.roundedRectangle(
        magnifierKey: magnifierKey,
        leaderLink: focalPoint,
-        offsetFromFocalPoint: const Offset(0, -72),
+        offsetFromFocalPoint: const Offset(0, -230),


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.

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

Should this be upated with pixel density? Also, we're still missing a comment explanation about where the number came from.

This value is already adjusted to the pixel ratio in the magnifier widget.

matthew-carroll · 2024-05-06T22:01:49Z

super_editor/example/lib/demos/editor_configs/demo_mobile_editing_ios.dart

    return Center(
      child: IOSFollowingMagnifier.roundedRectangle(
        magnifierKey: magnifierKey,
        leaderLink: focalPoint,
-        offsetFromFocalPoint: const Offset(0, -72),
+        offsetFromFocalPoint: const Offset(0, -230),
+        animationController: animationController,


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.

Should we pass the animation value as ValueListenable here? Just the percentage?

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?

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.

matthew-carroll · 2024-05-06T22:03:26Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

@@ -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);


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.

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

matthew-carroll · 2024-05-06T22:06:18Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

+  /// Updates the magnifier focal point in relation to the current drag position.
+  void _updateMagnifierFocalPoint() {
+    late DocumentPosition? docPositionToMagnify;
+


What ended up being the issue that caused this change in code? What were we doing wrong before?

This code is here to fix the magnification area issue described in #1957

Ok, but I asked what ended up being the issue? What was the root problem that you found and fixed with that code?

The root cause is that we were using the finger position as the focal point.

matthew-carroll · 2024-05-06T22:10:37Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

@@ -1824,3 +1892,6 @@ class SuperEditorIosHandlesDocumentLayerBuilder implements SuperEditorLayerBuild
    );
  }
 }
+
+const defaultIosMagnifierAnimationDuration = Duration(milliseconds: 180);


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?

Yeah, We have another constant for the exit animation.

super_editor/lib/src/infrastructure/platforms/ios/magnifier.dart

matthew-carroll · 2024-05-06T22:13:48Z

super_editor/lib/src/infrastructure/platforms/ios/magnifier.dart

+  });
+
+  final Offset offsetFromFocalPoint;
+  final double animationPercentage;


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.

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

matthew-carroll · 2024-05-06T22:16:36Z

super_editor/lib/src/infrastructure/platforms/mobile_documents.dart

@@ -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.


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?

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?

That should be fine

Does Android have intro and exit animations, too?

Android doesn't seem to have intro and exit animations.

angelosilvestre · 2024-05-10T00:44:03Z

@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.mp4

With slow animations:

Simulator.Screen.Recording.-.iPhone.15.-.2024-05-09.at.21.38.58.mp4

matthew-carroll · 2024-05-13T19:44:59Z

super_editor/example/lib/demos/editor_configs/demo_mobile_editing_ios.dart

    return Center(
      child: IOSFollowingMagnifier.roundedRectangle(
        magnifierKey: magnifierKey,
        leaderLink: focalPoint,
-        offsetFromFocalPoint: const Offset(0, -72),
+        offsetFromFocalPoint: const Offset(0, -230),


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

matthew-carroll · 2024-05-13T19:47:12Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

@@ -1006,7 +1008,6 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor>
  }

  void _onPanEnd(DragEndDetails details) {
-    _magnifierOffset.value = null;


Does this not need to be replaced with some other kind of update?

I don't think it's necessary. We already call _controlsController.hideMagnifier().

matthew-carroll · 2024-05-13T19:48:23Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

-  final _magnifierOffset = ValueNotifier<Offset?>(null);
+
+  /// The [Offset] of the magnifier's focal point in the [DocumentLayout] coordinate space.
+  final _magnifierFocalPointInContentSpace = ValueNotifier<Offset?>(null);


What is "content space"? Do you mean "document space"?

Yes, updated.

matthew-carroll · 2024-05-13T19:49:35Z

super_editor/lib/src/default_editor/document_gestures_touch_ios.dart

+      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),


Why did this change from -72 to -20? Should this number account for pixel density?

This changed because the follower alignment was changed, so this number needed to be adjusted. It was chosen empirically.

Ok, but what about pixel density?

In the _IOSFollowingMagnifierState build method, we multiply the offsetFromFocalPoint by the device pixel ratio.

matthew-carroll · 2024-05-13T19:57:07Z

super_editor/lib/src/infrastructure/platforms/ios/magnifier.dart

+                  // 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(


Should the device pixel ratio be used here, too?

While I was testing this, the value didn't seem to be affected by the pixel ratio.

Do you know why it wasn't effected? That seems concerning.

I'm not sure why this is the case...

matthew-carroll · 2024-05-13T19:58:41Z

super_editor/lib/src/infrastructure/platforms/ios/magnifier.dart

+
+  final Offset offsetFromFocalPoint;
+  final double animationValue;
+  final AnimationStatus animationDirection;


Using AnimationStatus for this property isn't appropriate. There's no meaning behind dismissed or completed, and yet those would be valid values.

super_editor/lib/src/super_reader/read_only_document_ios_touch_interactor.dart

matthew-carroll · 2024-05-13T20:00:10Z

super_editor/lib/src/super_reader/read_only_document_ios_touch_interactor.dart

      leaderLink: magnifierFocalPoint,
-      offsetFromFocalPoint: const Offset(0, -72),
+      offsetFromFocalPoint: const Offset(0, -230),


Should this be based on device pixel ratio?

This value is already adjusted to the pixel ratio in IOSFollowingMagnifier.

What do you mean by adjusted? This is a hard-coded offset...

In the _IOSFollowingMagnifierState build method, we multiply the offsetFromFocalPoint by the device pixel ratio.

matthew-carroll · 2024-05-13T20:00:40Z

super_editor/lib/src/super_textfield/ios/_editing_controls.dart

+        return IOSFollowingMagnifier.roundedRectangle(
+          leaderLink: widget.editingController.magnifierFocalPoint,
+          show: showMagnifier,
+          offsetFromFocalPoint: const Offset(0, -230),


Should this use device pixel ratio?

This value is already adjusted to the pixel ratio in IOSFollowingMagnifier.

matthew-carroll · 2024-05-18T21:20:50Z

@angelosilvestre It looks like there are multiple Offsets that I assumed were DIP values, which were actually pixel values. Can you please carefully document each such Offset so that the reader knows whether he's reading a DIP value or a pixel value. Otherwise, users won't know whether they have the responsibility to divide by the pixel density, or whether it's done for them.

matthew-carroll

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.

…lves #1958) (#1973)

…lves superlistapp#1958) (superlistapp#1973)

[SuperEditor][SuperReader][SuperTextField] Update iOS magnifier (Reso…

8abf4f9

…lves #1958)

angelosilvestre requested a review from matthew-carroll April 28, 2024 20:26

Update animation

8c18804

matthew-carroll reviewed May 6, 2024

View reviewed changes

angelosilvestre added 3 commits May 6, 2024 21:19

PR updates

74187e7

Move AnimationController into Magnifier

f3be395

Add animation status in viewmodel

70316cd

angelosilvestre requested a review from matthew-carroll May 8, 2024 00:59

angelosilvestre mentioned this pull request May 9, 2024

[SuperEditor][Android] Fix magnified area (Resolves #1957) #1990

Merged

Adjust magic numbers

cb9ccf4

matthew-carroll reviewed May 13, 2024

View reviewed changes

PR updates

eb3510d

angelosilvestre requested a review from matthew-carroll May 13, 2024 22:41

BazinC mentioned this pull request May 15, 2024

magnifier magnifies the wrong area #1957

Closed

matthew-carroll approved these changes May 18, 2024

View reviewed changes

matthew-carroll merged commit d2aeb94 into main May 18, 2024
11 checks passed

matthew-carroll deleted the 1958_update-ios-magnifier branch May 18, 2024 21:30

github-actions bot pushed a commit that referenced this pull request May 18, 2024

[SuperEditor][SuperReader][SuperTextField] Update iOS magnifier (Reso…

f777654

…lves #1958) (#1973)

github-actions bot mentioned this pull request May 18, 2024

Cherry Pick: [SuperEditor][SuperReader][SuperTextField] Update iOS magnifier (Resolves #1958) #2016

Merged

angelosilvestre mentioned this pull request May 19, 2024

Document usage of density independent pixels #2018

Merged

matthew-carroll pushed a commit that referenced this pull request May 20, 2024

[SuperEditor][SuperReader][SuperTextField] Update iOS magnifier (Reso…

c21e04f

…lves #1958) (#1973)

quaaantumdev pushed a commit to quaaantumdev/super_editor that referenced this pull request May 23, 2024

[SuperEditor][SuperReader][SuperTextField] Update iOS magnifier (Reso…

4a6aa31

…lves superlistapp#1958) (superlistapp#1973)

[SuperEditor][SuperReader][SuperTextField] Update iOS magnifier (Resolves #1958) #1973

[SuperEditor][SuperReader][SuperTextField] Update iOS magnifier (Resolves #1958) #1973

Conversation

angelosilvestre commented Apr 28, 2024

matthew-carroll commented May 1, 2024

matthew-carroll commented May 1, 2024

angelosilvestre commented May 2, 2024

angelosilvestre commented May 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelosilvestre commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-carroll commented May 18, 2024

matthew-carroll left a comment

Choose a reason for hiding this comment