[SuperEditor][web] Fix text input after a deletion (Resolves #1224) #1269
Conversation
|
@brian-superlist Could you please try this PR? |
super_editor/lib/src/default_editor/document_hardware_keyboard/document_keyboard_actions.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/default_editor/document_hardware_keyboard/document_keyboard_actions.dart
Outdated
Show resolved
Hide resolved
| @@ -903,7 +903,7 @@ final defaultImeKeyboardActions = <DocumentKeyboardAction>[ | |||
| deleteToStartOfLineWithCmdBackspaceOnMac, | |||
| deleteWordUpstreamWithAltBackspaceOnMac, | |||
| deleteWordUpstreamWithControlBackspaceOnWindowsAndLinux, | |||
| deleteUpstreamContentWithBackspace, | |||
| deleteUpstreamContentWithBackspaceOnNonWeb, | |||
There was a problem hiding this comment.
Can you please add appropriate tests that replicate the web behavior of sending both to ensure the problem is solved?
|
@angelosilvestre Thanks for the follow-up. This one is working better, typing generally doesn't get stuck, but it looks like there are two issues: Issue 1: Cannot remove a node by pressing delete key Reproduction Steps:
Expected: Node is deleted and selection returns to end of previous node Screen.Recording.2023-08-02.at.5.10.16.PM.movLogs Issue 2: Selected Text + Deletion does not work correctly Reproduction Steps:
Expected: Selected Content is Deleted This also does not work if you type normal characters instead of pressing the backspace / delete key. Screen.Recording.2023-08-02.at.5.12.21.PM.movLogs No error logs while pressing backspace/delete. The following logs are printed when you start typing other characters. If this is a separate problem with selection on web, I can file a new issue. However, I did not observe this behavior on |
angelosilvestre
force-pushed
the
1224_fix_input_web
branch
from
c265ccf to
ca1ceab
Compare
I was able to fix this issue. It's the same root cause as the first issue. We are changing the selection when we are handling the deltas and also on key events. I applied the same solution. However, I've found a flutter web issue: even when we are expanding the selection upstream (shift + left arrow), the IME reports a downstream selection. This causes the caret to be display at the wrong position. I filed flutter/flutter#131906. |
|
Thanks, @angelosilvestre. Overall, working better than the previous version! I found one problem with the current PR. Is it related to the Flutter issue you mentioned? Reproduction Steps
Screen.Recording.2023-08-07.at.10.48.27.AM.movLogs |
|
@brian-superlist Thanks. I will investigate. |
|
@brian-superlist This issue seems to be related to flutter/flutter#131023 I pushed a commit in this PR related to the new line duplication. |
|
Great, text editing is feeling much better overall! The next problem I've found relates to multiple nodes. Problem 1
Expected: Selected text is removed and replaced with the character I've typed Screen.Recording.2023-08-08.at.10.26.10.AM.movProblem 2 Selection across nodes does not work.
Expected: Text is selected and can be modified Screen.Recording.2023-08-08.at.10.30.06.AM.mov |
This seems to be the same as flutter/flutter#131023
We do need a fix on our side, but we need to receive the selection affinity correctly in order to fix this (flutter/flutter#131906) |
|
@angelosilvestre @brian-superlist - Please let me know when you think this is ready for my final review to merge in. |
|
I don't know the internals of Super Editor well, but I'm a little curious if problem 2 is really related only to the Flutter affinity bug? For example, I can also create the problem starting from a node, moving the caret from left to right. If it definitely is, I'd say this one is good to go until we fix the underlying Flutter issues Reproduction Steps:
Expected: Text in the next node is selected Screen.Recording.2023-08-10.at.11.33.23.AM.mov |
|
@brian-superlist I have a fix for moving the caret left to right. I should push it today. |
|
Thanks @angelosilvestre! As some general feedback, I found many of these issues reported in this thread by doing very quick functional testing (< 3 minutes). To speed up PRs like this in the future and reduce these back-and-forth rounds of time-delayed feedback, I'd ask you to do more thorough functional testing on your side before asking for functional testing on our side. Before we merge this one in, I'd recommend taking 5-10 minutes to run the example app on web, use the editor to write more content "like a normal user," and see if you run into any issues that I might have missed. |
|
@brian-superlist Will do! |
|
@angelosilvestre Thanks 😄 |
This is really something that should be handled by tests. I don't really want any humans on any side spending a lot of time being test robots. Are we simply lacking appropriate tests? Or is this fundamentally a web issue, and therefore dependent on web integration tests? |
Thanks Matt. Yes, I complete agree -- we should certainly have test coverage for these issues and not rely on repeated manual QA to check regressions.
Again, I agree. As we are on such different timezones, feedback cycles can be a bit slow. For example, you or Angelo might fix something in your morning or early afternoon, but often we won't be able to provide feedback until your next day (because it's evening or night time in Europe). Therefore, on more exploratory fixes like this one, it'd be great to do a bit more functional testing, find the more obvious bugs, write test cases for them, fix those bugs + make the test cases pass, and then pass it over to our side for more functional verification. Overall, I think that could lead to faster feedback loops. Does that seem reasonable? Happy to discuss this over slack or google meet as well to ensure we understand each other. |
|
@angelosilvestre can you check whether we're missing widget tests for this, or whether we need integration tests? If the problem is that we need a web integration test, can you try writing a single integration test that covers one of these interactions and see if it works? I've run into major problems with web integration tests in the past, but perhaps they're usable now. |
|
With the changes from this PR we will need to add some tests for web. However, there are some places where we check for We can introduce an overridable value instead of checking |
Aren't we primarily dealing with selection changes? Wouldn't an integration test use the real IME to do that? This PR accumulated a number of different issues. It's possible that some of those can't reasonably be tested, but the issues that can be tested, should be tested, because the alternative is to have people manually do these things over and over every time we release. |
angelosilvestre
force-pushed
the
1224_fix_input_web
branch
from
c92c1e8 to
af64077
Compare
|
I think it's good now. There are three remaining issues that are flutter issues:
This is caused by flutter/flutter#131023 and seems to be solved by flutter/engine#44595.
I added this issue to the flutter ticket and it seems the fix is in progress: flutter/flutter#131023 (comment)
This is caused by flutter/flutter#131906. |
|
@angelosilvestre you enumerated the issues that this PR doesn't fix. Can you please enumerate what this PR does fix? |
|
angelosilvestre
force-pushed
the
1224_fix_input_web
branch
from
be43087 to
0078dcc
Compare
|
Solved conflicts. |
| @@ -432,7 +432,6 @@ class _ExampleEditorState extends State<ExampleEditor> { | |||
| ], | |||
| gestureMode: _gestureMode, | |||
| inputSource: _inputSource, | |||
| keyboardActions: _inputSource == TextInputSource.ime ? defaultImeKeyboardActions : defaultKeyboardActions, | |||
There was a problem hiding this comment.
Why was this removed?
There was a problem hiding this comment.
We don't need it because the constructor already does the same.
| @@ -232,6 +233,18 @@ ExecutionInstruction anyCharacterOrDestructiveKeyToDeleteSelection({ | |||
| return ExecutionInstruction.haltExecution; | |||
| } | |||
|
|
|||
| ExecutionInstruction deleteUpstreamContentWithBackspaceWithIme({ | |||
There was a problem hiding this comment.
This name isn't accurate. This handler isn't about IME - it's about web. But also, why do we need a new handler that just adds an if-statement? Can't we add the isWeb conditional to the existing handler?
There was a problem hiding this comment.
We must ignore the key event only if we use IME as the input source. When using keyboard as the input source we still need to handle backspace.
Inside the key handler we don't know which input source is being used. We could add it to SuperEditorContext, then we would be able to just modify the existing handler.
There was a problem hiding this comment.
In that case, I think the way we've handled things like this in the past is to define keyboard handlers that block execution, rather than composing conditional handlers around other handlers. Those blocking handlers are probably easier to name accurately. I bet that approach will also prove easier for people to understand.
There was a problem hiding this comment.
Understood. I'll use that approach.
| return moveUpDownLeftAndRightWithArrowKeys(editContext: editContext, keyEvent: keyEvent); | ||
| } | ||
|
|
||
| ExecutionInstruction moveUpAndDownWithArrowKeysOnWeb({ |
There was a problem hiding this comment.
What's special about these web versions of arrow handlers vs the ones we've already implemented? I don't see anything obvious that looks unique here.
There was a problem hiding this comment.
When using web with IME as the input source, left and right arrow keys generate non-text deltas which change the position. We need to handle only up and down arrow keys to move between lines and nodes.
There was a problem hiding this comment.
Does this mean that we've duplicated all the vertical arrow key behavior just because our original handler included both horizontal and vertical implementations?
If so, we should separate the original handler into two handlers. One for horizontal and one for vertical. Then we can leave the vertical one alone, and we can introduce a blocking handler for the horizontal case on web.
| // we forward the newline action to performAction. | ||
| if (defaultTargetPlatform == TargetPlatform.android || kIsWeb) { | ||
| if (defaultTargetPlatform == TargetPlatform.android) { |
There was a problem hiding this comment.
Did we get this condition wrong from the beginning, or did Flutter change something and now web doesn't do this?
There was a problem hiding this comment.
I think something has changed, but I would need to test with previous Flutter versions to confirm it.
| @@ -750,6 +751,19 @@ ExecutionInstruction backspaceToClearParagraphBlockType({ | |||
| return didClearBlockType ? ExecutionInstruction.haltExecution : ExecutionInstruction.continueExecution; | |||
| } | |||
|
|
|||
| ExecutionInstruction enterToInsertBlockNewlineWithIme({ | |||
There was a problem hiding this comment.
Same question here as elsewhere - why create a new handler? Can we just add the web conditional to the existing handler?
There was a problem hiding this comment.
When we use IME as the input source we still need to handle this key events, even on web.
| /// Overrides the value of [isWeb]. | ||
| /// | ||
| /// This is intended to be used in tests. | ||
| bool? debugIsWebOverride; |
There was a problem hiding this comment.
Please add the appropriate annotation for test related APIs. And then you don't need to state that in the comments.
Also, I'm not sure a bool is the right type for this. Shouldn't the developer be able to override this value in both directions? Either force it to "yes we're on web" or force it to "no we're not on web"? That would require a 3 value type.
There was a problem hiding this comment.
Please add the appropriate annotation for test related APIs. And then you don't need to state that in the comments.
Do you mean visibleForTesting?
Also, I'm not sure a bool is the right type for this. Shouldn't the developer be able to override this value in both directions? Either force it to "yes we're on web" or force it to "no we're not on web"? That would require a 3 value type.
As this is a nullable bool, this is already possible:
null: use isWeb value
true: we are on web
false: we are not on web
There was a problem hiding this comment.
I added a dart doc, but if you think we should use an enum we can change it.
There was a problem hiding this comment.
Yeah. visibleForTesting.
In general, it's probably not a good idea to give significance to a null bool. That condition is likely to be overlooked. You can do a nullable enum, but a nullable bool is just begging for confusion.
| @@ -416,7 +416,7 @@ class TestSuperEditorConfigurator { | |||
| imeOverrides: _config.imeOverrides, | |||
| keyboardActions: [ | |||
| ..._config.prependedKeyboardActions, | |||
| ...defaultKeyboardActions, | |||
| ...(_config.inputSource == TextInputSource.ime ? defaultImeKeyboardActions : defaultKeyboardActions), | |||
There was a problem hiding this comment.
Test tools should not be making SuperEditor policy decisions. We pass _config.inputSource into SuperEditor. If any such decision needs to be made, it should be made by SuperEditor. This configuration system only exists to let testers provide non-default values for SuperEditor properties.
There was a problem hiding this comment.
SuperEditor already has this policy, but as we are always setting a value for keyboardActions we are overriding it and always using defaultKeyboardActions, even when using IME.
There was a problem hiding this comment.
Are we doing this with any other configuration property? Or is this the first one that we've pulled out from SuperEditor into the configuration system?
There was a problem hiding this comment.
It seems it's just for this property. We do a similar thing for the component builders, but as we have only one list of defaultComponentBuilders this isn't a problem.
super_editor/test/test_tools.dart
Outdated
| @@ -85,6 +84,46 @@ void testWidgetsOnDesktop( | |||
| testWidgetsOnLinux("$description (on Linux)", test, skip: skip, variant: variant); | |||
| } | |||
|
|
|||
| /// A widget test that runs a variant for every desktop platform, e.g., | |||
| /// Mac, Windows, Linux and for macOS Web. | |||
There was a problem hiding this comment.
Is running on Mac web going to be sufficient? This would seem to suggest that when it comes to web behaviors, the underlying platform doesn't matter. But that's not accurate, is it? Aren't web shortcuts and selection movements typically the same as the underlying platform?
There was a problem hiding this comment.
Modified to run on all platforms on web.
angelosilvestre
force-pushed
the
1224_fix_input_web
branch
from
160ac3a to
55b4cdb
Compare
|
@matthew-carroll Rebased onto main to fix a compilation error due to the change in the |
[SuperEditor][web] Fix text input after a deletion. Resolves #1224
On web, if we quickly press backspace and type a character at the end of a paragraph we get the following exception and the text input crashes:
The issue is that we are handling the deletion twice on web. In IME mode, Flutter reports both the deletion delta and the backspace keypress.
This PR changes
defaultImeKeyboardActionsto include a different version ofdeleteUpstreamContentWithBackspace, which does nothing when running on web.