[Web] Fix insertions/deletions at inverted selection for TextEditingDeltas #44693

Renzo-Olivares · 2023-08-14T19:23:42Z

Fixes issue where the delta range would be calculated wrong when the selection is inverted.

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

Renzo-Olivares · 2023-08-14T19:27:24Z

lib/web_ui/test/engine/text_editing_test.dart

+      final List<dynamic> eventArgs = <dynamic>[
+        'beforeinput',
+        js_util.jsify(<String, dynamic>{
+          'inputType': 'insertLineBreak',


@mdebbar This test currently fails because there is no DomInputEvent defined for InputEvent which contains inputType. In handleBeforeInput we check the inputType in the logic. Is there a way to test this?

PR to fix the tests: Renzo-Olivares#87

Thanks for the fix! I was trying to avoid adding to the DOM API but if that's the only way to test this I think its okay. InputEvent is a basic building block like KeyboardEvent and other related events so it would be good to add.

Yeah. It seems to me that extending DOM bindings where needed is better than ad-hoc wrapper just for the test.

knopp · 2023-08-21T20:41:27Z

lib/web_ui/test/engine/text_editing_test.dart

@@ -3,6 +3,7 @@
 // found in the LICENSE file.

 import 'dart:async';
+import 'dart:js_interop';


Seems causing the CI check to fail.

Renzo-Olivares · 2023-08-21T21:41:59Z

cc @htoor3 @mdebbar for feedback/thoughts.

htoor3 · 2023-08-23T22:35:22Z

Thank you for this change @Renzo-Olivares!

There appear to be a few linting errors

warning - lib/src/engine/text_editing/text_editing.dart:564:96 - The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion

warning - lib/src/engine/text_editing/text_editing.dart:567:91 - The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion

warning - lib/src/engine/text_editing/text_editing.dart:567:125 - The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion

Also it doesn't look like we're sending selectionAffinity information back to the framework (ref). Is this something that might cause issues in situations where baseOffset > extentOffset? Framework seems to just default to downstream (ref)

knopp · 2023-08-24T07:08:44Z

Thank you for this change @Renzo-Olivares!

There appear to be a few linting errors
warning - lib/src/engine/text_editing/text_editing.dart:564:96 - The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion

warning - lib/src/engine/text_editing/text_editing.dart:567:91 - The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion

warning - lib/src/engine/text_editing/text_editing.dart:567:125 - The '!' will have no effect because the receiver can't be null. Try removing the '!' operator. - unnecessary_non_null_assertion
Also it doesn't look like we're sending selectionAffinity information back to the framework (ref). Is this something that might cause issues in situations where baseOffset > extentOffset? Framework seems to just default to downstream (ref)

TextAffinity should be reported, but it's somewhat orthogonal to inverted selection (baseOffset > extentOffset). I have a PR to properly report inverted selection here: #44806, it just waits for this one to land.

Renzo-Olivares · 2023-08-24T17:26:17Z

@htoor3 Sorry about that must've missed these. They should be fixed now. selectionAffinity is important but piping it through has no affect on the fix in this PR.

In general the framework does use selectionAffinity to calculate some text boundaries (ref1) and (ref2) so it would be good to pipe it through eventually. From looking into the other platforms TextInputPlugin it seems only iOS/macOS have non-default values for selectionAffinity, for reference:
(Android)
(macOS)
(iOS)
(Windows)
(Linux)

htoor3 · 2023-08-24T18:47:55Z

@htoor3 Sorry about that must've missed these. They should be fixed now. selectionAffinity is important but piping it through has no affect on the fix in this PR.

In general the framework does use selectionAffinity to calculate some text boundaries (ref1) and (ref2) so it would be good to pipe it through eventually. From looking into the other platforms TextInputPlugin it seems only iOS/macOS have non-default values for selectionAffinity, for reference: (Android) (macOS) (iOS) (Windows) (Linux)

Yep it didn't seem like it was a pressing TODO, but I was curious if it was related to this issue. I think there's a fallback in editable_text.dart (_checkNeedsAdjustAffinity()) that just overwrites the engine value with whatever the framework already has.

This change lgtm! There are just a few more breaking tests in what looks like Desktop Safari only - HybridTextEditing Supports new line at inverted selection and HybridTextEditing Supports deletion at inverted selection

Desktop Safari can be a little weird to test. Take a look at this comment here - it's possible adding these to ensure the placement of the input element could resolve those tests.

knopp · 2023-08-31T06:41:14Z

I can't reproduce the safari error locally (though I do get another test failing - "text editing styles invisible element"). Could this be a problem with different Safari version?

knopp · 2023-08-31T06:58:59Z

Right. It seems like the test that is failing for me (element.style.background is not transparent, but none) would pass in Safari 15, but fails in Safar 16.

Renzo-Olivares · 2023-08-31T18:08:24Z

I don't think this PR is a result of those failures. I tried running tests locally on main without any changes and got two failures: HybridTextEditing focus and connection with blur and text editing styles invisible element. It looks like the linux web engine is failing so i'll try to build the engine on my linux machine and see if I can work this out.

knopp · 2023-08-31T20:32:06Z

I don't think this PR is a result of those failures. I tried running tests locally on main without any changes and got two failures: HybridTextEditing focus and connection with blur and text editing styles invisible element. It looks like the linux web engine is failing so i'll try to build the engine on my linux machine and see if I can work this out.

I think you're getting the failure because after setting style.background="transparent" on element and then getting the style.background returns none on Safari 16, but transparent on Safari 15. I think the CI might be running the tests on Safari 15, hence the discrepancy.

EDIT: messed up the version numbers.

knopp · 2023-09-03T13:35:33Z

I played with this a little bit. It turns out that Safari 15 doesn't support setting inputType on DOM input event.

var ev = new InputEvent('beforeinput', {'inputType' : 'deleteContentBackward'})
ev.inputType // always prints ""

InputEventInit.inputType has been only supported on Safari since this commit: WebKit/WebKit#3990. which didn't make it to Safari 15.

I don't know if it is reasonable to expect the CI to be updated to Safari 16 any time soon, but if not then these tests should maybe be disabled for Safari?

Renzo-Olivares · 2023-09-05T18:48:45Z

@knopp Thank you for looking into that! Given that information I think its okay to disable these tests on Safari for the time being and a TODO explaining the issue. Our use of inputType here is not introduced by this PR this is just the first time we have tested this code path. Do you have any opinions on doing this @htoor3?

htoor3 · 2023-09-07T15:43:59Z

@Renzo-Olivares I think that makes sense. Let me doublecheck with the team to determine what version of Safari is being used to run our CI tests and also what the version rolling process looks like.

Could you point me to the codepath where we're testing inputType?

knopp · 2023-09-07T15:51:29Z

@Renzo-Olivares I think that makes sense. Let me doublecheck with the team to determine what version of Safari is being used to run our CI tests and also what the version rolling process looks like.

Could you point me to the codepath where we're testing inputType?

Here we're creating a DomInputEvent with 'inputType': 'deleteContentBackward', which only works in Safari 16. In Safari 15 querying the inputType after creating the event always returns empty string.

htoor3 · 2023-09-07T17:22:58Z

Thank you @knopp! Regarding the CI Safari version, it seems like it's tied to the CI server MacOS version and probably needs manual intervention to roll.

I think in that case, it's fair to disable them for Safari so we can get this fix in. I would add a TODO that links to a newly created issue outlining the Safari issue, mentioning to re-enable these after the Safari version rolls to 16 in CI.

…is supported

knopp · 2023-09-08T13:10:53Z

Looks like the tests pass, can this be merged?

Renzo-Olivares · 2023-09-08T14:50:30Z

Yes I'll merge it. I created an issue to re-enable the tests flutter/flutter#134271.

…xtEditingDeltas (flutter/engine#44693)

…134306) flutter/engine@f09a139...6d6b448 2023-09-08 skia-flutter-autoroll@skia.org Roll Skia from 71e40cc0faf6 to c5e67d222f46 (4 revisions) (flutter/engine#45576) 2023-09-08 rmolivares@renzo-olivares.dev [Web] Fix insertions/deletions at inverted selection for TextEditingDeltas (flutter/engine#44693) 2023-09-08 skia-flutter-autoroll@skia.org Roll Skia from ece9f3a15b08 to 71e40cc0faf6 (1 revision) (flutter/engine#45575) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added the platform-web Code specifically for the web engine label Aug 14, 2023

Renzo-Olivares commented Aug 14, 2023

View reviewed changes

Renzo-Olivares mentioned this pull request Aug 14, 2023

[web] Assertion failure when inserting new lines flutter/flutter#131023

Closed

2 tasks

This was referenced Aug 16, 2023

[web] Incorrect selection affinity flutter/flutter#131906

Closed

[Web] Properly report inverted selection #44806

Merged

eyebrowsoffire mentioned this pull request Aug 18, 2023

runIf clauses in .ci.yaml don't actually do anything flutter/flutter#132858

Closed

Renzo-Olivares force-pushed the delta-web-inverted-fix branch from 80f13cd to ea53bab Compare August 21, 2023 19:03

knopp approved these changes Aug 21, 2023

View reviewed changes

Renzo-Olivares requested review from htoor3 and mdebbar August 23, 2023 18:14

htoor3 approved these changes Aug 25, 2023

View reviewed changes

Renzo-Olivares force-pushed the delta-web-inverted-fix branch 2 times, most recently from 954d2f6 to 1a0ed36 Compare August 30, 2023 20:01

Renzo-Olivares and others added 3 commits September 7, 2023 21:25

Fix insertions/deletions at inverted selection

227e50e

updates

ba74e2d

Fix tests

c640bc7

Renzo-Olivares added 6 commits September 7, 2023 21:25

Fix new line insertion at inverted selection

5fd5d00

fix analyzer

11241ab

fix analyzer

5b117c6

try suggestion to fix test

491a6fe

fix test

9835675

updates

136ce62

Renzo-Olivares mentioned this pull request Sep 8, 2023

[Web] Re-enable safari tests that utilize InputEvent.inputType flutter/flutter#134271

Open

Skip tests on safari until CI is rolled to Safari 16 where inputType …

c0a0a8a

…is supported

Renzo-Olivares force-pushed the delta-web-inverted-fix branch from 1a0ed36 to c0a0a8a Compare September 8, 2023 04:44

Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2023

auto-submit bot merged commit 019715e into flutter:main Sep 8, 2023
25 checks passed

engine-flutter-autoroll mentioned this pull request Sep 8, 2023

Roll Flutter Engine from f09a139101c3 to 019715e02ae6 (2 revisions) flutter/flutter#134303

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 8, 2023

019715e02 [Web] Fix insertions/deletions at inverted selection for Te…

7fed991

…xtEditingDeltas (flutter/engine#44693)

engine-flutter-autoroll mentioned this pull request Sep 8, 2023

Roll Flutter Engine from f09a139101c3 to 019715e02ae6 (2 revisions) flutter/flutter#134304

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 8, 2023

019715e02 [Web] Fix insertions/deletions at inverted selection for Te…

900fc38

…xtEditingDeltas (flutter/engine#44693)

zanderso mentioned this pull request Sep 8, 2023

Engine -> Framework autoroller attempting to roll to Engine commit that hasn't finished building flutter/flutter#134305

Open

engine-flutter-autoroll mentioned this pull request Sep 8, 2023

Roll Flutter Engine from f09a139101c3 to 6d6b44886175 (3 revisions) flutter/flutter#134306

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 8, 2023

019715e02 [Web] Fix insertions/deletions at inverted selection for Te…

94605e8

…xtEditingDeltas (flutter/engine#44693)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Web] Fix insertions/deletions at inverted selection for TextEditingDeltas #44693

[Web] Fix insertions/deletions at inverted selection for TextEditingDeltas #44693

Renzo-Olivares commented Aug 14, 2023

Renzo-Olivares Aug 14, 2023

knopp Aug 16, 2023

Renzo-Olivares Aug 17, 2023

knopp Aug 17, 2023

knopp Aug 21, 2023

Renzo-Olivares commented Aug 21, 2023

htoor3 commented Aug 23, 2023 •

edited

knopp commented Aug 24, 2023

Renzo-Olivares commented Aug 24, 2023

htoor3 commented Aug 24, 2023

knopp commented Aug 31, 2023 •

edited

knopp commented Aug 31, 2023

Renzo-Olivares commented Aug 31, 2023

knopp commented Aug 31, 2023 •

edited

knopp commented Sep 3, 2023 •

edited

Renzo-Olivares commented Sep 5, 2023

htoor3 commented Sep 7, 2023

knopp commented Sep 7, 2023

htoor3 commented Sep 7, 2023

knopp commented Sep 8, 2023

Renzo-Olivares commented Sep 8, 2023

[Web] Fix insertions/deletions at inverted selection for TextEditingDeltas #44693

[Web] Fix insertions/deletions at inverted selection for TextEditingDeltas #44693

Conversation

Renzo-Olivares commented Aug 14, 2023

Pre-launch Checklist

Renzo-Olivares Aug 14, 2023

Choose a reason for hiding this comment

knopp Aug 16, 2023

Choose a reason for hiding this comment

Renzo-Olivares Aug 17, 2023

Choose a reason for hiding this comment

knopp Aug 17, 2023

Choose a reason for hiding this comment

knopp Aug 21, 2023

Choose a reason for hiding this comment

Renzo-Olivares commented Aug 21, 2023

htoor3 commented Aug 23, 2023 • edited

knopp commented Aug 24, 2023

Renzo-Olivares commented Aug 24, 2023

htoor3 commented Aug 24, 2023

knopp commented Aug 31, 2023 • edited

knopp commented Aug 31, 2023

Renzo-Olivares commented Aug 31, 2023

knopp commented Aug 31, 2023 • edited

knopp commented Sep 3, 2023 • edited

Renzo-Olivares commented Sep 5, 2023

htoor3 commented Sep 7, 2023

knopp commented Sep 7, 2023

htoor3 commented Sep 7, 2023

knopp commented Sep 8, 2023

Renzo-Olivares commented Sep 8, 2023

htoor3 commented Aug 23, 2023 •

edited

knopp commented Aug 31, 2023 •

edited

knopp commented Aug 31, 2023 •

edited

knopp commented Sep 3, 2023 •

edited