SelectionArea cursor should only change when over text #131640

hannah-hyj · 2023-08-01T00:01:09Z

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Renzo-Olivares · 2023-08-02T21:15:55Z

packages/flutter/lib/src/painting/text_painter.dart

Nit: I wonder if we should move some of the documentation from getBoxesForSelection to getBoxesForRange and add a mention to each other in the docs.

consider using macro template to share document between these two methods

Renzo-Olivares · 2023-08-02T21:17:18Z

packages/flutter/test/widgets/text_test.dart

Why is this Center needed?

add center so when add point : await gesture.addPointer(location: tester.getCenter(find.byType(Text)));

the center of the text widget is within the actual text.

Renzo-Olivares · 2023-08-02T21:20:21Z

packages/flutter/test/widgets/slivers_padding_test.dart

Can you explain the change to this test, I don't fully understand. Thank you!

this is to make the text as big as the text widget, so the hittest will hit the textspan for these tests.

chunhtai · 2023-08-02T21:29:42Z

packages/flutter/test/widgets/text_test.dart

Is the original test no longer valid? otherwise, we should create a new test

yes, the original test is no longer valid

chunhtai · 2023-08-02T21:35:50Z

packages/flutter/lib/src/rendering/paragraph.dart

weird it is declared as Object? it is probably a mistake I think

chunhtai · 2023-08-02T21:38:37Z

packages/flutter/lib/src/rendering/paragraph.dart

If we really want to be certain, this range should be coming from the span, since this is where the hittest will be sent to

There doesn't seem to be an existing api to get the range of a span in text painter.

I think boxes for (textPosition.offset - 1, textPosition.offset +1) should cover all cases.

If there is no easy way to do that, I am fine with using textPosition.offset +-1

Also this can be
max(0, textPosition.offset - 1)

min(_textPainter.text.length, textPosition.offset + 1)

chunhtai · 2023-08-02T21:41:16Z

packages/flutter/test/widgets/text_test.dart

can you write a test that selectable textspan with recognizer will still show SystemMouseCursors.click when hover over the recognizer?

Renzo-Olivares

LGTM after google testing passes, it looks like some related tests are failing.

chunhtai

LGTM, just some nits

packages/flutter/lib/src/rendering/paragraph.dart

chunhtai · 2023-08-03T19:06:41Z

packages/flutter/lib/src/painting/text_painter.dart

consider using macro template to share document between these two methods

Co-authored-by: chunhtai <[email protected]>

auto-submit · 2023-08-03T20:14:14Z

auto label is removed for flutter/flutter, pr: 131640, due to - The status or check suite ci.yaml validation has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2023-08-04T04:53:50Z

auto label is removed for flutter/flutter, pr: 131640, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

LongCatIsLooong · 2023-08-04T21:21:07Z

packages/flutter/lib/src/rendering/paragraph.dart

-      result.add(HitTestEntry(span));
-      return true;
+    if (_positionIsWithinText(position: position, textPosition: textPosition)) {
+      final InlineSpan? span = _textPainter.text!.getSpanForPosition(textPosition);


The reason span was typed Object? was to avoid the down-casting below (i.e. span! as HitTestTarget).

Makes sense, please also add a comment on this line to explain this.

LongCatIsLooong · 2023-08-04T21:28:58Z

packages/flutter/lib/src/rendering/paragraph.dart

+      math.max(0, textPosition.offset - 1),
+      math.min(_textPainter.plainText.length, textPosition.offset + 1),
+    );
+    return boxes.where((ui.TextBox box) => box.toRect().contains(position)).isNotEmpty;


boxes.any so it stops when it finds a box.

I suspect this is going to be slow if the method gets called frequently. There's a new Skia API that will allow us to do this correctly and efficiently. Unfortunately it's not exposed in text painter yet. If this fix is not needed immediately would it be ok to wait until the new APIs are available?

This is not needed immediately, I can close this pr and wait for the new APIs.

@LongCatIsLooong What is the timeline on the Skia API? I have a need for this in Linkify (#125927). I filed #131435 and verified that this PR did fix the issue for me locally.

Even though I'm excited for this to fix my Linkify problem, it's not really super urgent. Linkify is not yet ready to merge, and merging it without this fix wouldn't even be terrible. I just want to make sure it's not going to be months or something for the Skia change.

The skia API is already available it just needs to be exposed (but the HTML renderer needs a separate implementation). I'll start working on that once the rounding thing is removed.

Sounds good, thanks!

Addresses #131640 (comment)

github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Aug 1, 2023

hannah-hyj force-pushed the 104594 branch from 007dd2b to ff6dbcd Compare August 2, 2023 01:42

hannah-hyj requested review from Renzo-Olivares and chunhtai August 2, 2023 17:55

flutter deleted a comment from flutter-dashboard bot Aug 2, 2023

Renzo-Olivares reviewed Aug 2, 2023

View reviewed changes

chunhtai reviewed Aug 2, 2023

View reviewed changes

hannah-hyj requested review from Renzo-Olivares and chunhtai August 2, 2023 22:42

Renzo-Olivares approved these changes Aug 3, 2023

View reviewed changes

chunhtai approved these changes Aug 3, 2023

View reviewed changes

hannah-hyj and others added 17 commits August 3, 2023 13:11

Update text.dart

896a05d

1

1dc182d

update

4855b71

test

e4012fb

test

3408b3a

test

d463edc

Update text_test.dart

433ef23

Update text_test.dart

b7333b1

test

cd4c239

Update hyperlink_test.dart

2bc62d1

Update hyperlink_test.dart

26438b5

Update paragraph.dart

e094163

Update hyperlink_test.dart

0e21e28

Update paragraph.dart

6e61657

update

45222e2

Update paragraph.dart

8fba6b1

Update packages/flutter/lib/src/rendering/paragraph.dart

1efcb7b

Co-authored-by: chunhtai <[email protected]>

hannah-hyj and others added 2 commits August 3, 2023 13:11

Update packages/flutter/lib/src/rendering/paragraph.dart

8828b4d

Co-authored-by: chunhtai <[email protected]>

Update text_painter.dart

d97a8da

hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 3, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 3, 2023

hannah-hyj force-pushed the 104594 branch from 92050f9 to d97a8da Compare August 3, 2023 20:16

hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 3, 2023

justinmc mentioned this pull request Aug 3, 2023

InlineText's GestureDetector is always full-width #131435

Closed

2 tasks

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 4, 2023

LongCatIsLooong reviewed Aug 4, 2023

View reviewed changes

LongCatIsLooong mentioned this pull request Aug 4, 2023

Use pattern matching to avoid strange type annotations #131964

Merged

8 tasks

hannah-hyj closed this Aug 7, 2023

fluttermirroringbot pushed a commit that referenced this pull request Aug 9, 2023

Use pattern matching to avoid strange type annotations (#131964)

aac0189

Addresses #131640 (comment)

SelectionArea cursor should only change when over text #131640

SelectionArea cursor should only change when over text #131640

Uh oh!

Conversation

hannah-hyj commented Aug 1, 2023

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!

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!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Aug 3, 2023

Uh oh!

auto-submit bot commented Aug 4, 2023

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!

LongCatIsLooong Aug 4, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

LongCatIsLooong Aug 4, 2023 •

edited

Loading