Skip to content

Reverts "Changing TextPainter.getOffsetForCaret implementation to remove the logarithmic search (#143281)" #143801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
auto-submit merged 1 commit into from
Feb 21, 2024
Merged

Reverts "Changing TextPainter.getOffsetForCaret implementation to remove the logarithmic search (#143281)" #143801

auto-submit merged 1 commit into from
Feb 21, 2024

Conversation

@auto-submit
Copy link
Contributor

@auto-submit auto-submit bot commented Feb 21, 2024

Reverts #143281

Initiated by: LongCatIsLooong

Reason for reverting: #143797

Original PR Author: LongCatIsLooong

Reviewed By: {justinmc, jason-simmons}

This change reverts the following previous change:
Original Description:
The behavior largely remains the same, except:

  1. The EOT cursor (textLength, downstream) for text ending in the opposite writing direction as the paragraph is now placed at the visual end of the last line.
    For example, in a LTR paragraph, the EOT cursor for aA (lowercase for LTR and uppercase for RTL) is placed to the right of the line: aA| (it was a|A before).
    This matches the behavior of most applications that do logical order arrow key navigation instead of visual order navigation.
    And it makes the navigation order consistent for aA\naA:
  |aA    =>  aA|  => aA|  => aA  => aA   => aA 
   aA        aA      aA     |aA     aA|     aA|     
   (1)       (2)     (3)    (4)    (5)      (6)

This is indeed still pretty confusing as (2) and (3), as well as (5) and (6) are hard to distinguish (when the I beam has a large width they are actually visually distinguishable -- they use the same anchor but one gets painted to the left and the other to the right. I noticed that emacs does the same).
But logical order navigation will always be confusing in bidi text, in one way or another.

Interestingly there are 3 different behaviors I've observed in chrome:

  • the chrome download dialog (which I think uses GTK text widgets but not sure which version) gives me 2 cursors when navigating bidi text, and
  • its HTML fields only show one, and presumably they place the I beam at the trailing edge of the character (which makes more sense for backspacing I guess).
  • On the other hand, its (new) omnibar seems to use visual order arrow navigation

Side note: we may need to update the "tap to place the caret here" logic to handle the case where the tap lands outside of the text and the text ends in the opposite writing direction.

  1. Removed the logarithmic search. The same could be done using the characters package but when glyphInfo tells you about the baseline location in the future we probably don't need the getBoxesForRange call. This should fix Caret misplaced in bidi text #123424.

Internal Tests

This is going to change the image output of some internal golden tests. I'm planning to merge #143281 before this to avoid updating the same golden files twice for invalid selections.

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.

Revert "Changing TextPainter.getOffsetForCaret implementation to re…
bc690e2 
…move the logarithmic search (#143281)"

This reverts commit 3538e4c.
@auto-submit auto-submit bot added the revert of Bot Only: Tracking label for bot. Tracks new revert of pull requests. label Feb 21, 2024
@github-actions 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: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Feb 21, 2024
@auto-submit auto-submit bot merged commit f9b3b84 into master Feb 21, 2024
@auto-submit auto-submit bot deleted the revert_3538e4c788e106b76649f99758c39ebafa03b8ea branch February 21, 2024 00:10
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 21, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 21, 2024
@engine-flutter-autoroll
f9b3b84 Reverts "Changing TextPainter.getOffsetForCaret implementat…
61d4e9e 
…ion to remove the logarithmic search (#143281)" (flutter/flutter#143801)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 21, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 21, 2024
@engine-flutter-autoroll
f9b3b84 Reverts "Changing TextPainter.getOffsetForCaret implementat…
0b0e49b 
…ion to remove the logarithmic search (#143281)" (flutter/flutter#143801)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 21, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 21, 2024
@engine-flutter-autoroll
f9b3b84 Reverts "Changing TextPainter.getOffsetForCaret implementat…
52a687c 
…ion to remove the logarithmic search (#143281)" (flutter/flutter#143801)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 21, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 21, 2024
@engine-flutter-autoroll
f9b3b84 Reverts "Changing TextPainter.getOffsetForCaret implementat…
aebe7e7 
…ion to remove the logarithmic search (#143281)" (flutter/flutter#143801)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 22, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 22, 2024
@engine-flutter-autoroll
f9b3b84 Reverts "Changing TextPainter.getOffsetForCaret implementat…
6cac55b 
…ion to remove the logarithmic search (#143281)" (flutter/flutter#143801)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Feb 22, 2024
@engine-flutter-autoroll
Manual roll Flutter from 5129806 to efee280 (47 revisions) (#6180)
078c2a3 
Manual roll Flutter from 5129806 to efee280 (47 revisions)

Manual roll requested by [email protected]

flutter/flutter@5129806...efee280

2024-02-22 [email protected] Roll Flutter Engine from bf5c003085fd to 7eeb697687d5 (16 revisions) (flutter/flutter#143911)
2024-02-22 [email protected] Update PR template for dart fix (flutter/flutter#143879)
2024-02-22 [email protected] Re-use methods to calculate leading and trailing garbage in RenderSliverMultiBoxAdaptor (flutter/flutter#143884)
2024-02-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Make impeller goldens test blocking. (#143864)" (flutter/flutter#143896)
2024-02-21 [email protected] [Impeller] Make impeller goldens test blocking. (flutter/flutter#143864)
2024-02-21 [email protected] Disable color filter sepia test for Impeller. (flutter/flutter#143861)
2024-02-21 [email protected] Roll Flutter Engine from 52ffcaadea41 to bf5c003085fd (12 revisions) (flutter/flutter#143875)
2024-02-21 [email protected] Roll Flutter Engine from 4128895d79a1 to 52ffcaadea41 (1 revision) (flutter/flutter#143862)
2024-02-21 [email protected] Deprecate redundant itemExtent in RenderSliverFixedExtentBoxAdaptor methods (flutter/flutter#143412)
2024-02-21 [email protected] Add aab as alias for appbundle (flutter/flutter#143855)
2024-02-21 [email protected] Roll Flutter Engine from e16f43eeaaa4 to 4128895d79a1 (1 revision) (flutter/flutter#143856)
2024-02-21 [email protected] Roll Packages from 8bba41b to 48048f6 (2 revisions) (flutter/flutter#143853)
2024-02-21 [email protected] Update `hourMinuteTextStyle` defaults for Material 3 Time Picker (flutter/flutter#143749)
2024-02-21 [email protected] Roll Flutter Engine from 93063f61943a to e16f43eeaaa4 (2 revisions) (flutter/flutter#143827)
2024-02-21 [email protected] Roll Flutter Engine from ed49634486e9 to 93063f61943a (1 revision) (flutter/flutter#143826)
2024-02-21 [email protected] `CalendarDatePicker` doesn't announce selected date on desktop (flutter/flutter#143583)
2024-02-21 [email protected] Roll Flutter Engine from 9100d326475a to ed49634486e9 (2 revisions) (flutter/flutter#143824)
2024-02-21 [email protected] Add `timeSelectorSeparatorColor` and `timeSelectorSeparatorTextStyle`  for Material 3 Time Picker (flutter/flutter#143739)
2024-02-21 [email protected] Roll Flutter Engine from efc69946cb1e to 9100d326475a (2 revisions) (flutter/flutter#143820)
2024-02-21 [email protected] Roll Flutter Engine from 700250436e3f to efc69946cb1e (2 revisions) (flutter/flutter#143816)
2024-02-21 [email protected] Roll Flutter Engine from 3557277c575c to 700250436e3f (1 revision) (flutter/flutter#143814)
2024-02-21 [email protected] more fixes to unstable impeller goldens. (flutter/flutter#143811)
2024-02-21 [email protected] Roll Flutter Engine from cb12a8cc97a1 to 3557277c575c (2 revisions) (flutter/flutter#143807)
2024-02-21 [email protected] [flutter_tools] enable wasm compile on beta channel (flutter/flutter#143779)
2024-02-21 [email protected] Fix initialization of time in repeat on AnimationController (flutter/flutter#142887)
2024-02-21 [email protected] Disable debug banner to stabilize impeller goldens. (flutter/flutter#143794)
2024-02-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Changing `TextPainter.getOffsetForCaret` implementation to remove the logarithmic search (#143281)" (flutter/flutter#143801)
2024-02-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add UI Benchmarks (#143542)" (flutter/flutter#143798)
2024-02-20 [email protected] Avoid applying partial dartfixes on CI (flutter/flutter#143551)
2024-02-20 [email protected] Add UI Benchmarks (flutter/flutter#143542)
2024-02-20 [email protected] Roll Flutter Engine from 1ae2c10e8071 to cb12a8cc97a1 (1 revision) (flutter/flutter#143791)
2024-02-20 [email protected] Implement `_suspendedNode` fix (flutter/flutter#143556)
2024-02-20 [email protected] Change `ItemExtentBuilder`'s return value nullable (flutter/flutter#142428)
2024-02-20 [email protected] Roll Flutter Engine from 27828054f07a to 1ae2c10e8071 (6 revisions) (flutter/flutter#143783)
2024-02-20 [email protected] Roll Flutter Engine from e16a260265ad to 27828054f07a (1 revision) (flutter/flutter#143769)
2024-02-20 [email protected] [gold] Always provide host ABI to gold config (flutter/flutter#143621)
2024-02-20 [email protected] instead of exiting the tool, print a warning when using --flavor with an incompatible device (flutter/flutter#143735)
2024-02-20 [email protected] Implementing `switch` expressions: everything in `flutter/lib/src/` (flutter/flutter#143634)
2024-02-20 [email protected] Changing `TextPainter.getOffsetForCaret` implementation to remove the logarithmic search (flutter/flutter#143281)
2024-02-20 [email protected] Delete local.properties that shouldn't have been pushed (flutter/flutter#143774)
2024-02-20 [email protected] Clean leaks. (flutter/flutter#142818)
2024-02-20 [email protected] Introduce tone-based surfaces and accent color add-ons - Part 2 (flutter/flutter#138521)
2024-02-20 [email protected] Explain when and why to use CrossAxisAlignment.baseline (flutter/flutter#143632)
2024-02-20 [email protected] Roll Flutter Engine from a41da3701923 to e16a260265ad (2 revisions) (flutter/flutter#143763)
...
LongCatIsLooong added a commit to LongCatIsLooong/flutter that referenced this pull request Feb 22, 2024
@LongCatIsLooong
Relands "Changing TextPainter.getOffsetForCaret implementation to
b6592af 
remove the logarithmic search (flutter#143281)" (reverted in flutter#143801)

This reverts commit f9b3b84.
auto-submit bot pushed a commit that referenced this pull request Feb 23, 2024
@LongCatIsLooong
Relands "Changing TextPainter.getOffsetForCaret implementation to r…
a0a854a 
…emove the logarithmic search (#143281)" (reverted in #143801) (#143954)

The original PR was reverted because the new caret positioning callpath triggered a skparagraph assert. The assert has been removed. Relanding the PR with no changes applied.
LouiseHsu pushed a commit to LouiseHsu/packages that referenced this pull request Mar 7, 2024
@engine-flutter-autoroll @LouiseHsu
Manual roll Flutter from 5129806 to efee280 (47 revisions) (flutter#6180
2e81f1d 
)

Manual roll Flutter from 5129806 to efee280 (47 revisions)

Manual roll requested by [email protected]

flutter/flutter@5129806...efee280

2024-02-22 [email protected] Roll Flutter Engine from bf5c003085fd to 7eeb697687d5 (16 revisions) (flutter/flutter#143911)
2024-02-22 [email protected] Update PR template for dart fix (flutter/flutter#143879)
2024-02-22 [email protected] Re-use methods to calculate leading and trailing garbage in RenderSliverMultiBoxAdaptor (flutter/flutter#143884)
2024-02-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Make impeller goldens test blocking. (#143864)" (flutter/flutter#143896)
2024-02-21 [email protected] [Impeller] Make impeller goldens test blocking. (flutter/flutter#143864)
2024-02-21 [email protected] Disable color filter sepia test for Impeller. (flutter/flutter#143861)
2024-02-21 [email protected] Roll Flutter Engine from 52ffcaadea41 to bf5c003085fd (12 revisions) (flutter/flutter#143875)
2024-02-21 [email protected] Roll Flutter Engine from 4128895d79a1 to 52ffcaadea41 (1 revision) (flutter/flutter#143862)
2024-02-21 [email protected] Deprecate redundant itemExtent in RenderSliverFixedExtentBoxAdaptor methods (flutter/flutter#143412)
2024-02-21 [email protected] Add aab as alias for appbundle (flutter/flutter#143855)
2024-02-21 [email protected] Roll Flutter Engine from e16f43eeaaa4 to 4128895d79a1 (1 revision) (flutter/flutter#143856)
2024-02-21 [email protected] Roll Packages from 8bba41b to 48048f6 (2 revisions) (flutter/flutter#143853)
2024-02-21 [email protected] Update `hourMinuteTextStyle` defaults for Material 3 Time Picker (flutter/flutter#143749)
2024-02-21 [email protected] Roll Flutter Engine from 93063f61943a to e16f43eeaaa4 (2 revisions) (flutter/flutter#143827)
2024-02-21 [email protected] Roll Flutter Engine from ed49634486e9 to 93063f61943a (1 revision) (flutter/flutter#143826)
2024-02-21 [email protected] `CalendarDatePicker` doesn't announce selected date on desktop (flutter/flutter#143583)
2024-02-21 [email protected] Roll Flutter Engine from 9100d326475a to ed49634486e9 (2 revisions) (flutter/flutter#143824)
2024-02-21 [email protected] Add `timeSelectorSeparatorColor` and `timeSelectorSeparatorTextStyle`  for Material 3 Time Picker (flutter/flutter#143739)
2024-02-21 [email protected] Roll Flutter Engine from efc69946cb1e to 9100d326475a (2 revisions) (flutter/flutter#143820)
2024-02-21 [email protected] Roll Flutter Engine from 700250436e3f to efc69946cb1e (2 revisions) (flutter/flutter#143816)
2024-02-21 [email protected] Roll Flutter Engine from 3557277c575c to 700250436e3f (1 revision) (flutter/flutter#143814)
2024-02-21 [email protected] more fixes to unstable impeller goldens. (flutter/flutter#143811)
2024-02-21 [email protected] Roll Flutter Engine from cb12a8cc97a1 to 3557277c575c (2 revisions) (flutter/flutter#143807)
2024-02-21 [email protected] [flutter_tools] enable wasm compile on beta channel (flutter/flutter#143779)
2024-02-21 [email protected] Fix initialization of time in repeat on AnimationController (flutter/flutter#142887)
2024-02-21 [email protected] Disable debug banner to stabilize impeller goldens. (flutter/flutter#143794)
2024-02-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Changing `TextPainter.getOffsetForCaret` implementation to remove the logarithmic search (#143281)" (flutter/flutter#143801)
2024-02-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add UI Benchmarks (#143542)" (flutter/flutter#143798)
2024-02-20 [email protected] Avoid applying partial dartfixes on CI (flutter/flutter#143551)
2024-02-20 [email protected] Add UI Benchmarks (flutter/flutter#143542)
2024-02-20 [email protected] Roll Flutter Engine from 1ae2c10e8071 to cb12a8cc97a1 (1 revision) (flutter/flutter#143791)
2024-02-20 [email protected] Implement `_suspendedNode` fix (flutter/flutter#143556)
2024-02-20 [email protected] Change `ItemExtentBuilder`'s return value nullable (flutter/flutter#142428)
2024-02-20 [email protected] Roll Flutter Engine from 27828054f07a to 1ae2c10e8071 (6 revisions) (flutter/flutter#143783)
2024-02-20 [email protected] Roll Flutter Engine from e16a260265ad to 27828054f07a (1 revision) (flutter/flutter#143769)
2024-02-20 [email protected] [gold] Always provide host ABI to gold config (flutter/flutter#143621)
2024-02-20 [email protected] instead of exiting the tool, print a warning when using --flavor with an incompatible device (flutter/flutter#143735)
2024-02-20 [email protected] Implementing `switch` expressions: everything in `flutter/lib/src/` (flutter/flutter#143634)
2024-02-20 [email protected] Changing `TextPainter.getOffsetForCaret` implementation to remove the logarithmic search (flutter/flutter#143281)
2024-02-20 [email protected] Delete local.properties that shouldn't have been pushed (flutter/flutter#143774)
2024-02-20 [email protected] Clean leaks. (flutter/flutter#142818)
2024-02-20 [email protected] Introduce tone-based surfaces and accent color add-ons - Part 2 (flutter/flutter#138521)
2024-02-20 [email protected] Explain when and why to use CrossAxisAlignment.baseline (flutter/flutter#143632)
2024-02-20 [email protected] Roll Flutter Engine from a41da3701923 to e16a260265ad (2 revisions) (flutter/flutter#143763)
...
justinmc added a commit to justinmc/flutter that referenced this pull request Apr 10, 2024
@justinmc
Revert "Relands "Changing TextPainter.getOffsetForCaret implementat…
e420099 
…ion to remove the logarithmic search (flutter#143281)" (reverted in flutter#143801) (flutter#143954)"

This reverts commit a0a854a.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
@engine-flutter-autoroll
f9b3b84 Reverts "Changing TextPainter.getOffsetForCaret implementat…
3c5f0dc 
…ion to remove the logarithmic search (#143281)" (flutter/flutter#143801)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fluttergithubbot fluttergithubbot fluttergithubbot approved these changes

Assignees

@LongCatIsLooong LongCatIsLooong

Labels

a: text input Entering text in a text field or keyboard related problems d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. revert of Bot Only: Tracking label for bot. Tracks new revert of pull requests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Caret misplaced in bidi text

2 participants

@fluttergithubbot @LongCatIsLooong