Reland "Single tap on the previous selection should toggle the toolbar on iOS #108913" #111995

Renzo-Olivares · 2022-09-20T19:23:27Z

Was failing due to a call to EditableText.toggleToolbar which does a forced null dereference on SelectionOverlay. There is an assert that checks if the SelectionOverlay exists, but this does not work on production. This change creates a SelectionOverlay if it does not exist when toggleToolbar is called and adds a regression test.

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], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
All existing and new tests are passing.

…e toolbar on iOS…" (flutter#111662)" This reverts commit 61db3bc.

LongCatIsLooong · 2022-09-26T17:44:13Z

packages/flutter/lib/src/widgets/text_selection.dart

+      position,
+    );
+
+    return renderEditable.selection!.start < textPosition.offset


when offset == start why is it not considered not on selection?

Ah I saw the comment from the previous pull request. But you may still want to check the affinity of the incoming position?

What should the TextAffinity be checked for?

Oh nvm you don't have to since the logic will be identical.

LongCatIsLooong · 2022-09-26T17:44:48Z

packages/flutter/lib/src/widgets/text_selection.dart

  }

+  bool _tapWasOnSelection(Offset position) {
+    if (renderEditable.selection == null) {


consider creating a local variable for selection so you can promote it to non null.

LongCatIsLooong · 2022-09-26T18:07:06Z

packages/flutter/lib/src/widgets/editable_text.dart

+  void toggleToolbar([bool hideHandles = true]) {
+    if (_selectionOverlay == null) {
+      _createSelectionOverlay();
+    }


final selectionOverlay = _selectionOverlay ??= createSelectionOverlay() to avoid the force cast?

LongCatIsLooong · 2022-09-26T18:13:07Z

packages/flutter/lib/src/widgets/text_selection.dart

+                renderEditable.selectWordEdge(cause: SelectionChangedCause.tap);
+              }
+              if (previousSelection == editableText.textEditingValue.selection && renderEditable.hasFocus) {
+                editableText.toggleToolbar(false);


Could this be combined with the previous if clause?

Not sure if it can, but I'll try it out.

…-ios

justinmc

LGTM 👍

justinmc · 2022-09-28T17:30:53Z

packages/flutter/lib/src/widgets/editable_text.dart


  /// Toggles the visibility of the toolbar.
-  void toggleToolbar() {
-    assert(_selectionOverlay != null);


I don't know why we didn't create the selection overlay here in the first place. Seems fine to me.

justinmc · 2022-09-28T17:31:09Z

packages/flutter/lib/src/widgets/editable_text.dart


-  void _createSelectionOverlay() {
-    _selectionOverlay = TextSelectionOverlay(
+  TextSelectionOverlay _createSelectionOverlay() {


I think I like this pattern better 👍

justinmc · 2022-09-28T17:32:11Z

packages/flutter/lib/src/widgets/text_selection.dart

+    );
+
+    return selection.start < textPosition.offset
+        && selection.end > textPosition.offset;


Not inclusive?

I updated the logic.

…-ios

justinmc

Renewing my LGTM after the previous changes 👍

justinmc · 2022-10-04T19:52:32Z

packages/flutter/test/material/text_field_test.dart

      await tester.tapAt(textfieldStart + const Offset(150.0, 9.0));
      await tester.pump(const Duration(milliseconds: 50));
-      // First tap moved the cursor.
+      // First tap moved the cursor and hides the toolbar.


"hides" => "hid"

LongCatIsLooong · 2022-10-04T20:26:00Z

packages/flutter/lib/src/widgets/text_selection.dart

+              // If the tap was neither on the selection exclusively or inclusively then we select the word edge. After
+              // selecting the word edge, if our selection remains the same, then we toggle the toolbar. If the selection changes
+              // then we hide the toolbar.
+              if (((_tapWasOnSelectionExclusive(details.globalPosition) && !previousSelection.isCollapsed)


This is a bit hard to read. Could you simplify this a bit?

Also you probably don't need _tapWasOnSelectionInclusive? if previousSelection == editableText.textEditingValue.selection then we'll toggle the toolbar anyway?

I think there is still the case where the cursor is in the middle of a word wor|d, and we tap it.

If we remove the inclusive check, then the exclusive one will not catch it, and the cursor will move to the word edge.

The expected behavior would be to toggle the toolbar when we tap the cursor.

if () { // If the tap was on the selection (different meaning for collapsed vs uncollapsed selection) // and the editable was focused, then toggle the toolbar. } else if () { // Tap was not on the selection at all. // We should still toggle the toolbar, if the selection remains the same after the word edge is selected. // If the selection changes then we hide the toolbar. }

I updated the logic a little, getting rid of the else if.

Yeah I’ll keep a lookout of affinity issues.

On native if the renderEditable doesn’t have focus we always select the closest word edge. And the toolbar is not toggled unless the editable has focus.

Could you check if you tap the latter half of the character before the caret? I think that will give you (x-1, upstream) instead of (x, downstream) so wasTapOnSelection will return false? But that should also toggle the toolbar right?

Good point, If i do the later half of the character before the character the selection is upstream. If I then follow that up with a tap on the former half of the character after the caret, then that selection is downstream. Then because of previousSelection == editableText.textEditingValue.selection, the toolbar is not toggled in this case. On native it looks like it toggles the toolbar regardless of the affinity.

In the case where the text is wrapping, then if we place the cursor at the end of the first line, and then try to tap at the beginning of the second line, then the toolbar will be toggled because wasTapOnSelectionInclusive returns true.

I think in the case where we select the word edge the affinity does not matter since a word will not be wrapped between two lines (will either fall on first or second line).

In the case where we check for wasTapOnSelectionInclusive we do care about the affinity because we want to be able to select the text at beginning of the next line. So we want it to return false when the affinity is different.

There might be a trade off we have to make here since we cannot differentiate why the TextAffinity changed.

We want to toggle the toolbar when

We call selectWordEdge and the selection after the call is the same as the previous selection (including TextAffinity).

We call selectWordEdge and the selection after the call is the same as the previous selection (excluding TextAffinity), this can happen when we tap

We want to hide the toolbar when

We call selectWordEdge and the selection after the call is the same as the previous selection (excluding TextAffinity). This can happen when there is a multi-line text field, there is word wrap and we want to tap on the boundary. Let's say the cursor is at the end of the first line. Tapping on the first line boundary should toggle the toolbar, tapping at the beginning of the second line should hide the toolbar.

We can't differentiate whether TextAffinity changed due to tapping on a new line but same selection offset, or if it occurred from tapping on the same line with the same selection offset.

justinmc

👍 I'm on board with the compromise made here, it seems like the best option in the circumstances. But I want to explore this a little farther just to make sure.

You probably already saw this but the logic that gives the affinity based on the side of a character is all the way down in paragraph_txt.cc:

  double glyph_center = (gp->x_pos.start + gp->x_pos.end) / 2;
  if ((direction == TextDirection::ltr && dx < glyph_center) ||
      (direction == TextDirection::rtl && dx >= glyph_center)) {
    return PositionWithAffinity(gp->code_units.start, DOWNSTREAM);
  } else {
    return PositionWithAffinity(gp->code_units.end, UPSTREAM);
  }

So that probably shouldn't change. Instead, the ideal logic is that on tap, we only care about affinity for toggling the toolbar if the tap is at a soft wordwrap.

Can we know about the location of soft wordwraps at this point in the pipeline? The text has already been laid out since this is a tap callback, and getPositionForPoint (called already on line 1948) requires the text to be laid out too. So the information we need is there, but if I'm not mistaken, there is no good way to get this from RenderEditable.

Tl;dr: The alternative to this compromise

You could add a method to RenderEditable that tells you one way or another if a given TextPosition is at a soft wordwrap or not. I think you can figure this out using LineMetrics. If the tap is at a soft wordwrap, only toggle the toolbar if the TextAffinity matches. If it's not at a soft wordwrap, toggle it regardless of the affinity.

Curious about your thoughts on if this is a correct analysis, and if so if it's worth it or not! But I'm on board with this PR either way. Maybe this could even be done in a follow-up PR.

LongCatIsLooong · 2022-10-07T22:20:20Z

packages/flutter/lib/src/widgets/text_selection.dart

+              // toggle the toolbar. If the selection changes then we hide the toolbar.
+              final TextSelection previousSelection = renderEditable.selection ?? editableText.textEditingValue.selection;
+              final bool isAffinityTheSame = renderEditable.getPositionForPoint(details.globalPosition).affinity == previousSelection.affinity;
+              if (((_tapWasOnSelectionExclusive(details.globalPosition) && !previousSelection.isCollapsed)


nit: You're potentially calling getPositionForPoint 3 times. The call is via FFI so could be a bit costly than usual dart calls. So maybe store that in a local variable and that might help simplify the code too.

Renzo-Olivares · 2022-10-10T19:28:55Z

👍 I'm on board with the compromise made here, it seems like the best option in the circumstances. But I want to explore this a little farther just to make sure.

You probably already saw this but the logic that gives the affinity based on the side of a character is all the way down in paragraph_txt.cc:
  double glyph_center = (gp->x_pos.start + gp->x_pos.end) / 2;
  if ((direction == TextDirection::ltr && dx < glyph_center) ||
      (direction == TextDirection::rtl && dx >= glyph_center)) {
    return PositionWithAffinity(gp->code_units.start, DOWNSTREAM);
  } else {
    return PositionWithAffinity(gp->code_units.end, UPSTREAM);
  }
So that probably shouldn't change. Instead, the ideal logic is that on tap, we only care about affinity for toggling the toolbar if the tap is at a soft wordwrap.

Can we know about the location of soft wordwraps at this point in the pipeline? The text has already been laid out since this is a tap callback, and getPositionForPoint (called already on line 1948) requires the text to be laid out too. So the information we need is there, but if I'm not mistaken, there is no good way to get this from RenderEditable.

Tl;dr: The alternative to this compromise

You could add a method to RenderEditable that tells you one way or another if a given TextPosition is at a soft wordwrap or not. I think you can figure this out using LineMetrics. If the tap is at a soft wordwrap, only toggle the toolbar if the TextAffinity matches. If it's not at a soft wordwrap, toggle it regardless of the affinity.

Curious about your thoughts on if this is a correct analysis, and if so if it's worth it or not! But I'm on board with this PR either way. Maybe this could even be done in a follow-up PR.

I 100% agree with your analysis. I will investigate further on how feasible adding such an API to renderEditable is, but it definitely sounds worth it to add if its possible. That will be in a separate PR. I created an issue for this #113228.

…-ios

…e toolbar on iOS #108913" (flutter/flutter#111995)

…e toolbar on iOS flutter#108913" (flutter#111995)" This reverts commit 4abe6fd.

Renzo-Olivares closed this Sep 20, 2022

Renzo-Olivares force-pushed the reland-single-tap-ios branch from 2c4ef21 to 90cabbd Compare September 20, 2022 19:25

Revert "Revert "Single tap on the previous selection should toggle th…

cd6451c

…e toolbar on iOS…" (flutter#111662)" This reverts commit 61db3bc.

Renzo-Olivares reopened this Sep 20, 2022

add regression test and fix test

8e5a514

Renzo-Olivares requested a review from justinmc September 20, 2022 19:44

Renzo Olivares added 3 commits September 20, 2022 13:01

fix test

d4f6408

fix

14753a9

fix analyzer

6731ae8

Renzo-Olivares requested a review from LongCatIsLooong September 23, 2022 20:38

LongCatIsLooong reviewed Sep 26, 2022

View reviewed changes

Renzo Olivares added 4 commits September 26, 2022 11:21

avoid selectionoverlay force cast

8204925

fix last commit

11cff93

local variable for selection

a21b1d5

Merge remote-tracking branch 'upstream/master' into reland-single-tap…

ff06c6c

…-ios

Renzo-Olivares mentioned this pull request Sep 27, 2022

Forced Null Dereference causing crash in editableTextState #111660

Closed

justinmc approved these changes Sep 28, 2022

View reviewed changes

Renzo Olivares added 4 commits October 4, 2022 11:10

update tests

16e9e81

Merge remote-tracking branch 'upstream/master' into reland-single-tap…

f534f1d

…-ios

fix tests

f10576d

Add comments

ab234cd

Renzo-Olivares requested review from LongCatIsLooong and justinmc October 4, 2022 19:11

justinmc approved these changes Oct 4, 2022

View reviewed changes

Address comments

202976a

LongCatIsLooong reviewed Oct 4, 2022

View reviewed changes

Renzo Olivares added 3 commits October 4, 2022 14:55

simplify logic

3fd2d8e

Updates to comments

cbafc98

updates

6c1f26f

Renzo-Olivares requested a review from LongCatIsLooong October 4, 2022 22:38

Renzo Olivares added 2 commits October 6, 2022 12:51

Fix TextAffinity behavior with a trade off

7247ae4

fix test

0e7d904

Renzo-Olivares requested a review from justinmc October 6, 2022 21:58

justinmc approved these changes Oct 7, 2022

View reviewed changes

LongCatIsLooong approved these changes Oct 7, 2022

View reviewed changes

Avoid calling renderEditable.getPositionForPoint multiple times

c642bc1

Merge remote-tracking branch 'upstream/master' into reland-single-tap…

bd3260f

…-ios

Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 17, 2022

auto-submit bot merged commit 4abe6fd into flutter:master Oct 17, 2022

engine-flutter-autoroll mentioned this pull request Oct 18, 2022

Roll Flutter from f26dd3730df8 to 0ad71e872793 (14 revisions) flutter/plugins#6586

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 18, 2022

4abe6fd Reland "Single tap on the previous selection should toggle th…

dd9eb9e

…e toolbar on iOS #108913" (flutter/flutter#111995)

engine-flutter-autoroll mentioned this pull request Oct 18, 2022

Roll Flutter from f26dd3730df8 to 4fa91c949aae (15 revisions) flutter/plugins#6587

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 18, 2022

4abe6fd Reland "Single tap on the previous selection should toggle th…

13d8204

…e toolbar on iOS #108913" (flutter/flutter#111995)

engine-flutter-autoroll mentioned this pull request Oct 18, 2022

Roll Flutter from f26dd3730df8 to eea60f86f1b1 (19 revisions) flutter/plugins#6588

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 18, 2022

4abe6fd Reland "Single tap on the previous selection should toggle th…

db880db

…e toolbar on iOS #108913" (flutter/flutter#111995)

engine-flutter-autoroll mentioned this pull request Oct 19, 2022

Roll Flutter from a707d05e9d4c to abfaec68114b (39 revisions) flutter/packages#2718

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 19, 2022

4abe6fd Reland "Single tap on the previous selection should toggle th…

d4cf14d

…e toolbar on iOS #108913" (flutter/flutter#111995)

This was referenced Oct 24, 2022

Single Tap on TextField does not display Toolbar #48434

Closed

Text input has different gesture/cursor interaction compared to the native one #62252

Closed

Renzo-Olivares pushed a commit to Renzo-Olivares/flutter that referenced this pull request Nov 28, 2022

Revert "Reland "Single tap on the previous selection should toggle th…

29ce977

…e toolbar on iOS flutter#108913" (flutter#111995)" This reverts commit 4abe6fd.

gnprice mentioned this pull request Feb 18, 2023

Error: The getter 'toolbarIsVisible' was called on null #64471

Closed

Reland "Single tap on the previous selection should toggle the toolbar on iOS #108913" #111995

Reland "Single tap on the previous selection should toggle the toolbar on iOS #108913" #111995

Uh oh!

Conversation

Renzo-Olivares commented Sep 20, 2022

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Tl;dr: The alternative to this compromise

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares commented Oct 10, 2022

Tl;dr: The alternative to this compromise

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Renzo-Olivares Oct 4, 2022 •

edited

Loading

Renzo-Olivares Oct 5, 2022 •

edited

Loading

Renzo-Olivares Oct 6, 2022 •

edited

Loading