Fix cursor is not centered when cursorHeight is set (non-Apple platforms). #145829

bleroux · 2024-03-27T13:54:15Z

Description

This PRs fixes the cursor vertical position when a custom cursor height is set on non-Apple platforms (on Apple platforms the cursor is already centered) .

Before	After

Code sample used for the screenshots (cursorHeight 18, font size 16, line height 2)

import 'package:flutter/material.dart';

void main() async {
  runApp(
    const MaterialApp(
      title: 'Flutter Demo',
      home: MyApp(),
    ),
  );
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const Scaffold(
      body: Center(
        child: TextField(
          cursorHeight: 18,
          style: TextStyle(fontSize: 16, height: 2),
        ),
      ),
    );
  }
}

Related Issue

Fixes #143480

Tests

Adds 2 tests, updates 1.

flutter-dashboard · 2024-03-27T15:33:14Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #145829 at sha d1c847d

justinmc

I wonder why this was only done on Apple platforms before? I can't think of a reason why you wouldn't want this, though.

I see there are two goldens failures, but those look like an improvement if indeed no one wants a bottom aligned cursor. I've also run the Google tests, where there are probably more visual diff tests that will fail. I'll take a look at that and make sure there aren't any that look incorrect, otherwise this looks good.

justinmc · 2024-03-27T17:22:57Z

packages/flutter/test/widgets/editable_text_cursor_test.dart

@@ -1236,7 +1236,7 @@ void main() {
      backgroundCursorColor: Colors.grey,
      controller: controller,
      focusNode: focusNode,
-      style: const TextStyle(),
+      style: const TextStyle(fontSize: 17),


Why set this fontSize now?

Because otherwise this test is failing.
This test sets cursorHeight to 17 and did not set the font size (it defaults to 14).
In this case, because the cursor is bigger than the font size it will overflow the editable vertically. Before my PR the overflow was only on the bottom (the cursor was painted with a vertical position align to the top of the text and overflow on the bottom).
With my PR, when a cursorHeight is bigger than the font size, the cursor is centered so it overflows on the top and on the bottom).
I choose to change the fontSize in this test instead of changing the rect that it tested some lines later because I don't think that checking that the cursor did not overflow vertically was the goal of this test (and also because I added a test for this particular case) .

Thanks for the thorough explanation, I agree that this looks to be in line with the original intentions of this test 👍

bleroux · 2024-03-27T18:34:01Z

I wonder why this was only done on Apple platforms before? I can't think of a reason why you wouldn't want this, though.

Centering the caret on Apple platforms was added in #31687 (at that time cursorHeight property did not exist). On Apple platforms because it was chosen to make the cursor bigger than prefferedLineHeight it was a nice idea to introduce centering logic. On Android, this same PR forced the cursor height to _textPainter.getFullHeightForCaret so the cursor did not need centering logic because it was admitted that the cursor height is the same as the editable height.
Custom cursorHeight was introduced later in #61714.
Unfortunately this PR added one test in editable_text_cursor_test.dart and that test restricted its target platforms to iOS and macOS. I’m not sure it was on purpose but it probably did not help to figure out that the centering behavior was missing on non Apple platforms.

…rms)

bleroux · 2024-04-02T13:23:10Z

I've also run the Google tests, where there are probably more visual diff tests that will fail. I'll take a look at that and make sure there aren't any that look incorrect, otherwise this looks good.

@justinmc
I rebased the PR, all checks are green but it seems that the 'Google testing' step did not run this time? Do you have to start it manually? (last week, I saw it failing on this PR, but it seems that there were some issues with CI).

justinmc

Thanks for explaining the history of the cursor centering. I've restarted the Google tests and it looks like they're working now.

justinmc · 2024-04-03T20:54:18Z

packages/flutter/test/widgets/editable_text_cursor_test.dart

@@ -1236,7 +1236,7 @@ void main() {
      backgroundCursorColor: Colors.grey,
      controller: controller,
      focusNode: focusNode,
-      style: const TextStyle(),
+      style: const TextStyle(fontSize: 17),


Thanks for the thorough explanation, I agree that this looks to be in line with the original intentions of this test 👍

bleroux · 2024-04-04T18:42:06Z

@justinmc
It looks like Google testing is failing consistently on this PR.
Thank you in advance for sharing any information abouth those failing tests 🙏

justinmc · 2024-04-05T00:18:07Z

I've approved the failing Google image diff tests. Actually what caused most of these to fail here was things like bringIntoView, which scrolls to a text position based on what the caret rect would be. Because we changed the position of the caret in this PR, the scroll position changed slightly whenever these kinds of things were used. Thanks to @LongCatIsLooong for figuring that out.

justinmc

LGTM 👍

…ple platforms). (flutter/flutter#145829)

Roll Flutter from ac2ca9347cf9 to 477ebd831c12 (32 revisions) flutter/flutter@ac2ca93...477ebd8 2024-04-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 95ffe73ac2a8 to 6974dbac35a1 (2 revisions) (flutter/flutter#146350) 2024-04-05 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#146348) 2024-04-05 engine-flutter-autoroll@skia.org Roll Packages from dce6f0c to 764d997 (4 revisions) (flutter/flutter#146344) 2024-04-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 292c48b9af5e to 95ffe73ac2a8 (1 revision) (flutter/flutter#146343) 2024-04-05 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#146331) 2024-04-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 23a1d0d2fce6 to 292c48b9af5e (1 revision) (flutter/flutter#146330) 2024-04-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 597d33f57183 to 23a1d0d2fce6 (1 revision) (flutter/flutter#146327) 2024-04-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1fa0623bb07f to 597d33f57183 (1 revision) (flutter/flutter#146325) 2024-04-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from f17d586de6f1 to 1fa0623bb07f (1 revision) (flutter/flutter#146324) 2024-04-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from d44462a42d73 to f17d586de6f1 (4 revisions) (flutter/flutter#146322) 2024-04-05 leroux_bruno@yahoo.fr Fix cursor is not centered when cursorHeight is set (non-Apple platforms). (flutter/flutter#145829) 2024-04-05 git@reb0.org refactor: Perform plugin resolution per platform (flutter/flutter#144506) 2024-04-04 Michal-MK@users.noreply.github.com `ExpansionTile` Unable to remove right padding from title (flutter/flutter#145271) 2024-04-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from dcc99d652154 to d44462a42d73 (1 revision) (flutter/flutter#146311) 2024-04-04 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.1.1 to 4.2.0 (flutter/flutter#146310) 2024-04-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from e3104af94328 to dcc99d652154 (4 revisions) (flutter/flutter#146308) 2024-04-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from fa4d97e363ff to e3104af94328 (2 revisions) (flutter/flutter#146300) 2024-04-04 magder@google.com Remove dead `compareIosVersions` function (flutter/flutter#146298) 2024-04-04 philipfranchi@gmail.com Adds semanticsLabel to MenuItemButton (flutter/flutter#145846) 2024-04-04 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Bump to AGP 8.1/Gradle 8.3 (almost) everywhere (#146181)" (flutter/flutter#146305) 2024-04-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 918c72b803cc to fa4d97e363ff (2 revisions) (flutter/flutter#146299) 2024-04-04 34871572+gmackall@users.noreply.github.com Bump to AGP 8.1/Gradle 8.3 (almost) everywhere (flutter/flutter#146181) 2024-04-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 03cd54d4285a to 918c72b803cc (2 revisions) (flutter/flutter#146296) 2024-04-04 tessertaha@gmail.com Fix InputDecorator suffix and prefix IconButtons ignore `IconButtonTheme` (flutter/flutter#145473) 2024-04-04 34871572+gmackall@users.noreply.github.com Give `generate_gradle_lockfiles.dart` functionality to exclude certain subdirectories (flutter/flutter#146228) 2024-04-04 victorsanniay@gmail.com Update documentation to discourage using the TextEditingController.text setter (flutter/flutter#146151) 2024-04-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from b2ceabf7acce to 03cd54d4285a (1 revision) (flutter/flutter#146292) 2024-04-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from b435a8a87df0 to b2ceabf7acce (1 revision) (flutter/flutter#146283) 2024-04-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from bd51ee80d74b to b435a8a87df0 (1 revision) (flutter/flutter#146279) 2024-04-04 engine-flutter-autoroll@skia.org Roll Packages from 0e848fa to dce6f0c (4 revisions) (flutter/flutter#146276) 2024-04-04 dacoharkes@google.com Flutter templates example app Gradle memory settings (flutter/flutter#146275) 2024-04-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4303dc0d7a73 to bd51ee80d74b (2 revisions) (flutter/flutter#146273) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md ...

@OverRide

…rms). (flutter#145829) ## Description This PRs fixes the cursor vertical position when a custom cursor height is set on non-Apple platforms (on Apple platforms the cursor is already centered) . | Before | After | |--------|--------| | ![image](https://github.com/flutter/flutter/assets/840911/2d1b855d-d36c-4941-85be-5044ea0e9bb2) | ![image](https://github.com/flutter/flutter/assets/840911/306510c8-42ca-45c7-8c25-ddfa2e22c7f3) | <details><summary>Code sample used for the screenshots (cursorHeight 18, font size 16, line height 2)</summary> ```dart import 'package:flutter/material.dart'; void main() async { runApp( const MaterialApp( title: 'Flutter Demo', home: MyApp(), ), ); } class MyApp extends StatelessWidget { const MyApp({super.key}); @OverRide Widget build(BuildContext context) { return const Scaffold( body: Center( child: TextField( cursorHeight: 18, style: TextStyle(fontSize: 16, height: 2), ), ), ); } } ``` </details> ## Related Issue Fixes flutter#143480 ## Tests Adds 2 tests, updates 1.

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. labels Mar 27, 2024

flutter-dashboard bot added the will affect goldens Changes to golden files label Mar 27, 2024

bleroux requested a review from justinmc March 27, 2024 16:06

justinmc reviewed Mar 27, 2024

View reviewed changes

bleroux force-pushed the fix_cursor_not_centered_on_non_apple_platforms branch from d1c847d to ca2c61b Compare March 28, 2024 16:18

bleroux requested a review from justinmc March 28, 2024 19:03

bleroux force-pushed the fix_cursor_not_centered_on_non_apple_platforms branch from ca2c61b to 9a5e6dd Compare March 29, 2024 20:49

Fix cursor is not centered when cursorHeight is set (non-Apple platfo…

eb61288

…rms)

bleroux force-pushed the fix_cursor_not_centered_on_non_apple_platforms branch from 9a5e6dd to eb61288 Compare April 2, 2024 12:55

justinmc reviewed Apr 3, 2024

View reviewed changes

justinmc approved these changes Apr 5, 2024

View reviewed changes

bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2024

auto-submit bot merged commit a5ea2ef into flutter:master Apr 5, 2024
67 checks passed

bleroux deleted the fix_cursor_not_centered_on_non_apple_platforms branch April 5, 2024 04:03

engine-flutter-autoroll mentioned this pull request Apr 5, 2024

Roll Flutter from ac2ca9347cf9 to 45deb49716b4 (28 revisions) flutter/packages#6465

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2024

a5ea2ef1d Fix cursor is not centered when cursorHeight is set (non-Ap…

c024bb6

…ple platforms). (flutter/flutter#145829)

engine-flutter-autoroll mentioned this pull request Apr 5, 2024

Roll Flutter from ac2ca9347cf9 to 2ed0b4fd9bfe (29 revisions) flutter/packages#6466

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2024

a5ea2ef1d Fix cursor is not centered when cursorHeight is set (non-Ap…

dddc12d

…ple platforms). (flutter/flutter#145829)

engine-flutter-autoroll mentioned this pull request Apr 5, 2024

Roll Flutter from ac2ca9347cf9 to 1f64be86810a (30 revisions) flutter/packages#6468

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2024

a5ea2ef1d Fix cursor is not centered when cursorHeight is set (non-Ap…

0b0d9d1

…ple platforms). (flutter/flutter#145829)

engine-flutter-autoroll mentioned this pull request Apr 5, 2024

Roll Flutter from ac2ca9347cf9 to b0f52b5be29c (31 revisions) flutter/packages#6469

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2024

a5ea2ef1d Fix cursor is not centered when cursorHeight is set (non-Ap…

95a1ba6

…ple platforms). (flutter/flutter#145829)

engine-flutter-autoroll mentioned this pull request Apr 5, 2024

Roll Flutter from ac2ca9347cf9 to 477ebd831c12 (32 revisions) flutter/packages#6470

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2024

a5ea2ef1d Fix cursor is not centered when cursorHeight is set (non-Ap…

76d12d4

…ple platforms). (flutter/flutter#145829)

HansMuller mentioned this pull request Apr 5, 2024

Flutter Framework Team: Notable Commits #121415

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cursor is not centered when cursorHeight is set (non-Apple platforms). #145829

Fix cursor is not centered when cursorHeight is set (non-Apple platforms). #145829

bleroux commented Mar 27, 2024

flutter-dashboard bot commented Mar 27, 2024

justinmc left a comment

justinmc Mar 27, 2024

bleroux Mar 27, 2024 •

edited

justinmc Apr 3, 2024

bleroux commented Mar 27, 2024

bleroux commented Apr 2, 2024

justinmc left a comment

justinmc Apr 3, 2024

bleroux commented Apr 4, 2024

justinmc commented Apr 5, 2024

justinmc left a comment

Fix cursor is not centered when cursorHeight is set (non-Apple platforms). #145829

Fix cursor is not centered when cursorHeight is set (non-Apple platforms). #145829

Conversation

bleroux commented Mar 27, 2024

Description

Related Issue

Tests

flutter-dashboard bot commented Mar 27, 2024

justinmc left a comment

Choose a reason for hiding this comment

justinmc Mar 27, 2024

Choose a reason for hiding this comment

bleroux Mar 27, 2024 • edited

Choose a reason for hiding this comment

justinmc Apr 3, 2024

Choose a reason for hiding this comment

bleroux commented Mar 27, 2024

bleroux commented Apr 2, 2024

justinmc left a comment

Choose a reason for hiding this comment

justinmc Apr 3, 2024

Choose a reason for hiding this comment

bleroux commented Apr 4, 2024

justinmc commented Apr 5, 2024

justinmc left a comment

Choose a reason for hiding this comment

bleroux Mar 27, 2024 •

edited