[SuperEditor][Mobile] Fix caret position after rotation (Resolves #2056) #2057
Conversation
|
@matthew-carroll I had to set the golden tests to be ignored for Android, it seems a failure caused by some anti-aliasing. I already filed #1996 to see what we can do. |
|
@knopp Could you please try this PR? |
|
I tested it and it fixes the issue. |
| @@ -353,6 +361,12 @@ class AndroidControlsDocumentLayerState | |||
| } | |||
| } | |||
|
|
|||
| @override | |||
| DocumentSelectionLayout? computeLayoutDataWithDocumentLayout(BuildContext context, DocumentLayout documentLayout) { | |||
There was a problem hiding this comment.
Is there a reason that this method now needs to be implemented when it wasn't before?
There was a problem hiding this comment.
Previously, all the implementation was already in the computeLayoutDataWithDocumentLayout method. But now that we need to access the content's RenderBox I switched to use computeLayoutData directly.
Now we don't need computeLayoutDataWithDocumentLayout, but since it doesn't have a default implementation in the superclass, we need to implement it...
| @@ -321,13 +327,15 @@ class AndroidControlsDocumentLayerState | |||
| // Default caret width used by the Android caret. | |||
| const caretWidth = 2; | |||
|
|
|||
| final layerBox = context.findRenderObject() as RenderBox?; | |||
| if (layerBox != null && layerBox.hasSize && caretRect.left + caretWidth >= layerBox.size.width) { | |||
| // Use the content's RenderBox instead of the layer's RenderBox, because at this point, the layer | |||
There was a problem hiding this comment.
Can you explain this? I'm not sure I understand what this is saying, or why it's the case...
There was a problem hiding this comment.
Updated the comment.
| @@ -192,3 +292,22 @@ Future<void> _pumpCaretTestApp(WidgetTester tester) async { | |||
| ], | |||
| ).pump(); | |||
| } | |||
|
|
|||
| Future<TestDocumentContext> _pumpPhoneRotationTestApp(WidgetTester tester) async { | |||
There was a problem hiding this comment.
Can you add a comment for when/how this is meant to be used? Typically pumping is a thing that happens initially, so the idea of pumping for rotation isn't immediately obvious without further instructions.
There was a problem hiding this comment.
I changed the name to make it simpler. This method has just the initial pump, I added the phoneRotation just to clarify in which kind of test this is used.
super_editor/lib/src/infrastructure/platforms/android/android_document_controls.dart
Outdated
Show resolved
Hide resolved
| import 'package:golden_toolkit/golden_toolkit.dart'; | ||
| import 'package:super_editor/super_editor.dart'; | ||
| import 'package:super_editor/super_editor_test.dart'; | ||
|
|
||
| import '../../test/super_editor/supereditor_test_tools.dart'; | ||
| import '../test_tools_goldens.dart'; | ||
|
|
||
| void main() { | ||
| Future<void> main() async { | ||
| await loadAppFonts(); |
There was a problem hiding this comment.
Is this loadAppFonts() going to impact tests that are run in other files after this one?
There was a problem hiding this comment.
It shouldn't. If we don't specify a style rule with the font family we will still use Ahem.
There was a problem hiding this comment.
Can you verify that? I was just doing some golden work in swift_ui and if I call loadAppFonts() from the test config, I get regular fonts in the goldens. The only difference between calling loadAppFonts() from the test config vs calling it from a specific test file is that some tests might run before the test file and won't be impacted. But I'm worried about any tests that run after the given test file.
There was a problem hiding this comment.
It can impact if other files specify the font family. Once we load the fonts, it seems we cannot unload them.
There was a problem hiding this comment.
Then should we move this into the global test config to avoid burying it in a single file?
There was a problem hiding this comment.
It seems our global test config already has that, so we don't need to add it in this file. Removed.
[SuperEditor][Mobile] Fix caret position after rotation. Resolves #2056
On mobile, rotating the phone is causing the caret to be displayed at the wrong position:
cursor_position.mov
This was caused by #1967
We are accessing the layer's
RenderBoxto get the size, to be able to adjust the caret for situations where the caret is pushed off-screen.The issue is that, at the point in the pipeline, the layer hasn't been laid out yet for the current frame.
To fix the issue, I changed it to get the width from the content's
RenderBoxinstead of the layer'sRenderBox. Since both should always be the same size, this shouldn't be an issue.We have tests for caret position after phone rotation, but I'm not sure yet why these tests didn't catch this issue. Maybe it's related to the text used in those tests...
Also, it seems that when rotating from landscape to portrait, this issue doesn't happen. It only seems to happen when rotating from portrait to landscape.