Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@justinmc
Copy link
Contributor

@justinmc justinmc commented Aug 10, 2021

Paragraph.getLineBoundary accepts a TextPosition, but it disregards the included TextAffinity. This causes it to give the wrong answer at downstream wordwrap boundaries (see the linked issue for an example).

This PR makes it give the right response for all TextAffinities.

Fixes flutter/flutter#87970

@justinmc justinmc force-pushed the get-line-boundary-affinity branch from fdf33e6 to 0c92a2b Compare August 11, 2021 00:53
@justinmc justinmc marked this pull request as ready for review August 11, 2021 00:54
lib/ui/text.dart Outdated

// _getLineBoundary only considers the offset and assumes that the
// TextAffinity is upstream. In the case that TextPosition is just after a
// wordwrap (downstream), we need to get the line for the next offset.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the line ends with a RTL run then the _getLineBoundary function will still report the same line for position.offset + 1 I think?

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Aug 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nvm _getLineBoundary reports the logical start and end of a line so +1 can't be on the same line, from the tests it looks like position.offset + 1 works even if it exceeds the text length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's what I'm seeing, but I'm glad you brought this up. I just took a dive into Flutter RTL keyboard support while investigating this and it seems very broken. I don't know anything about any RTL languages unfortunately, but the behavior is very different from the browser. I will probably create a new issue for that.

@justinmc justinmc force-pushed the get-line-boundary-affinity branch from c0135ba to ea255c7 Compare August 11, 2021 17:27
@justinmc
Copy link
Contributor Author

justinmc commented Aug 11, 2021

There is one test failure that is expected. I'll fix it in flutter/flutter#85653 and get that ready to merge before I merge this. If anyone knows of a better process for this situation let me know!

Nevermind, that was red due to a real edge case that I fixed. Merging.

@justinmc justinmc force-pushed the get-line-boundary-affinity branch from 019fbbf to 794ca14 Compare August 11, 2021 21:51
@justinmc justinmc merged commit 06da8b0 into flutter:master Aug 11, 2021
@justinmc justinmc deleted the get-line-boundary-affinity branch August 11, 2021 23:04
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 12, 2021
filmil pushed a commit to filmil/engine that referenced this pull request Apr 21, 2022
Fix a bug in a calculation when selection is at a wordwrap
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Paragraph.getLineBoundary doesn't consider TextAffinity

2 participants