-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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
Text editing inside of Transformed.scale #146019
Text editing inside of Transformed.scale #146019
Conversation
Works by properly using local and global coords. The drag event details is relative to the selection handles, which are in an Overlay, so the localPosition isn't relative to the RenderEditable. Must manually transform the globalPosition instead. Caveat: There is a delay in when the drag is detected, and when scaled down really tiny, it's very pronounced.
d55e6d6
to
9a23b70
Compare
067a491
to
a78d201
Compare
@@ -646,7 +646,13 @@ class TextSelectionOverlay { | |||
required Offset globalGesturePosition, | |||
required TextPosition currentTextPosition, | |||
}) { | |||
final Offset globalRenderEditableTopLeft = renderEditable.localToGlobal(Offset.zero); | |||
final RenderBox? overlay = Overlay.of(context, rootOverlay: true).context.findRenderObject() as RenderBox?; |
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 class probably indirectly stores a reference to the target Overlay somewhere, so you don't have to find it again?
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 just looked and couldn't find one, only references to the OverlayEntries.
final Offset globalRenderEditableTopLeft = renderEditable.localToGlobal(Offset.zero); | ||
final RenderBox? overlay = Overlay.of(context, rootOverlay: true).context.findRenderObject() as RenderBox?; | ||
final Offset overlayGesturePosition = overlay?.globalToLocal(globalGesturePosition) ?? globalGesturePosition; | ||
final Offset globalRenderEditableTopLeft = renderEditable.localToGlobal(Offset.zero, ancestor: overlay); |
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.
nit: get the transform and apply it to the Rect instead?
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.
also why is it called global if the coordinate system is just the overlay?
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.
-
TIL MatrixUtils.transformRect.
-
You're right, I'll change the naming to "overlay" instead of "global" and make sure all the calculations are relative to that. This will actually matter if there is a transform above the MaterialApp, so we should get it right.
renderEditable.getLocalRectForCaret(positionAtBeginningOfLine).topCenter, | ||
renderEditable.getLocalRectForCaret(positionAtEndOfLine).bottomCenter, | ||
); | ||
final Rect globalLineBoundaries = Rect.fromPoints( |
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.
Consider storing the transform and applying it instead of using localToGlobal
.
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.
See my response to #146019 (comment).
final double centerOfLineGlobal = renderObject.localToGlobal( | ||
Offset(0.0, centerOfLineLocal), | ||
).dy; | ||
_endHandleDragPositionToCenterOfLine = centerOfLineGlobal - details.globalPosition.dy; |
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.
_endHandleDragPositionToCenterOfLine = centerOfLineGlobal - details.globalPosition.dy; | |
_endHandleDragPositionToCenterOfLine = centerOfLineGlobal - details.globalPosition.dy; |
Offset(0.0, centerOfLineLocal), | ||
).dy; | ||
_endHandleDragPositionToCenterOfLine = centerOfLineGlobal - details.globalPosition.dy; | ||
// This adjusts for the fact that the selection handles may not perfectly |
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
refers to using seslecionEndpoints
instead of the pointer event? Then maybe move the comment up?
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 rephrased the comment to be more clear about what it's referring to.
// selection handle, whereas this is relative to the RenderEditable. | ||
final Offset localPosition = renderObject.globalToLocal(details.globalPosition); | ||
|
||
final double nextEndHandleDragPositionLocal = _getHandleDy( |
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 do we have to deal with drag position deltas and change coordinate systems back and forth? Can we use the current location to calculate where the handle/magnifier should be?
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 trickiness here is that the handle needs to snap to the scaled lines. The snapping calculations are done in local coordinates and then it's converted back to global coordinates. After reading your comment I played around with some refactoring but ended up just renaming some things to try to make it more clear.
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.
Doesn't getPositionForPoint
do the snapping? Why is additional snapping needed?
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.
Sorry for my bad explanation, it's more subtle than my comment made it seem. Simply maintaining the offset between the contact point and the place in the text it points to is not enough. You can see why by doing this:
- Get out your phone and find a multiline text field (native or Flutter app).
- Tap to place the cursor.
- Drag the cursor slowly down until it jumps to the next line.
- Immediately drag back up until it jumps to the original line.
Notice that it doesn't jump back immediately. There is no balance point at which the cursor can jump up or down with a slight drag, like there would be if it was simply using an offset. You must drag a full line height for it to change lines in either direction.
I'm adding some details about this in the code comments.
@@ -16287,6 +16287,94 @@ void main() { | |||
isNull, | |||
); | |||
}); | |||
|
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.
Can you add tests for the RenderEditable changes?
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
// selection handle, whereas this is relative to the RenderEditable. | ||
final Offset localPosition = renderObject.globalToLocal(details.globalPosition); | ||
|
||
final double nextEndHandleDragPositionLocal = _getHandleDy( |
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.
Doesn't getPositionForPoint
do the snapping? Why is additional snapping needed?
flutter/flutter@533d04d...4967a94 2024-04-09 tessertaha@gmail.com Fix Flutter `README.md` image sources (flutter/flutter#146430) 2024-04-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0226114d5c54 to 5a824e22deb2 (1 revision) (flutter/flutter#146477) 2024-04-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 932c55025b87 to 0226114d5c54 (2 revisions) (flutter/flutter#146475) 2024-04-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8701a9a7fa41 to 932c55025b87 (1 revision) (flutter/flutter#146468) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 269aa69f47df to 8701a9a7fa41 (4 revisions) (flutter/flutter#146463) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from ceb5fa2c8651 to 269aa69f47df (1 revision) (flutter/flutter#146449) 2024-04-08 jmccandless@google.com Text editing inside of Transformed.scale (flutter/flutter#146019) 2024-04-08 leroux_bruno@yahoo.fr Fix DropdownButtonFormField throws when onChange is null (flutter/flutter#146342) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1e88b2dbc7f7 to ceb5fa2c8651 (3 revisions) (flutter/flutter#146447) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from cc741b5ee89d to 1e88b2dbc7f7 (1 revision) (flutter/flutter#146445) 2024-04-08 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#146444) 2024-04-08 tessertaha@gmail.com Add a custom shape example for `AppBar.shape` (flutter/flutter#146421) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Fixes bugs in the text selection positioning calculations so that they work even when the field is scaled. In many cases, we were simply translating things around without applying the proper localToGlobal (or vice versa) transform. | Before | After | | --- | --- | | <img src="https://github.com/flutter/flutter/assets/389558/a5a45472-98c5-4cdf-b382-218971fd9404" /> | <img src="https://github.com/flutter/flutter/assets/389558/f396a1af-2546-4e38-a9d9-6c6edfa38d94" /> | Partial fix for: flutter#144685 It looks like there are other problems where transforms aren't applied properly. Putting a transform at the root of the application, above MaterialApp, will expose more problems. <details> <summary>Sample code</summary> ```dart import 'package:flutter/material.dart'; import 'package:webview_flutter/webview_flutter.dart'; void main() => runApp(const _App()); class _App extends StatelessWidget { const _App(); @OverRide Widget build(BuildContext context) { return const MaterialApp(home: _Home()); } } class _Home extends StatefulWidget { const _Home(); @OverRide State<_Home> createState() => _HomeState(); } class _HomeState extends State<_Home> { final _controller = WebViewController(); final TextEditingController textEditingController = TextEditingController( text: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.', ); final OverlayPortalController overlayPortalController = OverlayPortalController(); @OverRide void initState() { super.initState(); _controller ..setJavaScriptMode(JavaScriptMode.unrestricted) ..loadRequest(Uri.https('api.flutter.dev')); } @OverRide Widget build(BuildContext context) { overlayPortalController.show(); return Scaffold( appBar: AppBar( title: const Text('Scaled WebView Tester'), ), body: Stack( children: <Widget>[ Transform.scale( alignment: Alignment.topLeft, scale: 0.5, child: TextField( controller: textEditingController, maxLines: null, ), ), OverlayPortal( controller: overlayPortalController, overlayChildBuilder: (BuildContext context) { return Positioned( top: 0.0, left: 0.0, child: SizedBox( height: 1000, width: 1000, child: Stack( children: <Widget>[ Positioned( top: 90.0, left: 0.0, child: Container( height: 1.0, width: 1000, color: Colors.blue, ), ), Positioned( top: 102.0, left: 0.0, child: Container( height: 1.0, width: 1000, color: Colors.blue, ), ), ], ), ), ); }, ), ], ), ); } } ``` </details>
flutter/flutter@533d04d...4967a94 2024-04-09 tessertaha@gmail.com Fix Flutter `README.md` image sources (flutter/flutter#146430) 2024-04-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0226114d5c54 to 5a824e22deb2 (1 revision) (flutter/flutter#146477) 2024-04-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 932c55025b87 to 0226114d5c54 (2 revisions) (flutter/flutter#146475) 2024-04-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8701a9a7fa41 to 932c55025b87 (1 revision) (flutter/flutter#146468) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 269aa69f47df to 8701a9a7fa41 (4 revisions) (flutter/flutter#146463) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from ceb5fa2c8651 to 269aa69f47df (1 revision) (flutter/flutter#146449) 2024-04-08 jmccandless@google.com Text editing inside of Transformed.scale (flutter/flutter#146019) 2024-04-08 leroux_bruno@yahoo.fr Fix DropdownButtonFormField throws when onChange is null (flutter/flutter#146342) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1e88b2dbc7f7 to ceb5fa2c8651 (3 revisions) (flutter/flutter#146447) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from cc741b5ee89d to 1e88b2dbc7f7 (1 revision) (flutter/flutter#146445) 2024-04-08 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#146444) 2024-04-08 tessertaha@gmail.com Add a custom shape example for `AppBar.shape` (flutter/flutter#146421) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
flutter/flutter@533d04d...4967a94 2024-04-09 tessertaha@gmail.com Fix Flutter `README.md` image sources (flutter/flutter#146430) 2024-04-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0226114d5c54 to 5a824e22deb2 (1 revision) (flutter/flutter#146477) 2024-04-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 932c55025b87 to 0226114d5c54 (2 revisions) (flutter/flutter#146475) 2024-04-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8701a9a7fa41 to 932c55025b87 (1 revision) (flutter/flutter#146468) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 269aa69f47df to 8701a9a7fa41 (4 revisions) (flutter/flutter#146463) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from ceb5fa2c8651 to 269aa69f47df (1 revision) (flutter/flutter#146449) 2024-04-08 jmccandless@google.com Text editing inside of Transformed.scale (flutter/flutter#146019) 2024-04-08 leroux_bruno@yahoo.fr Fix DropdownButtonFormField throws when onChange is null (flutter/flutter#146342) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1e88b2dbc7f7 to ceb5fa2c8651 (3 revisions) (flutter/flutter#146447) 2024-04-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from cc741b5ee89d to 1e88b2dbc7f7 (1 revision) (flutter/flutter#146445) 2024-04-08 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#146444) 2024-04-08 tessertaha@gmail.com Add a custom shape example for `AppBar.shape` (flutter/flutter#146421) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Fixes bugs in the text selection positioning calculations so that they work even when the field is scaled. In many cases, we were simply translating things around without applying the proper localToGlobal (or vice versa) transform.
Partial fix for: #144685
It looks like there are other problems where transforms aren't applied properly. Putting a transform at the root of the application, above MaterialApp, will expose more problems.
Sample code