Skip to content

Implement customizable cursor height#61714

Merged
fluttergithubbot merged 8 commits intoflutter:masterfrom
guidezpl:text-field-cursor-height
Jul 21, 2020
Merged

Implement customizable cursor height#61714
fluttergithubbot merged 8 commits intoflutter:masterfrom
guidezpl:text-field-cursor-height

Conversation

@guidezpl
Copy link
Member

@guidezpl guidezpl commented Jul 17, 2020 •
edited
Loading

Description

Cursor width can currently be customized. This PR adds the ability to customize cursor height for Cupertino and Material text fields (as well as EditableText and SelectableText).

Related Issues

Closes #26870
#61715

Tests

I added the following tests:

  • new golden test for EditableText cursor height
    editable_text_test 3
  • new golden test for Material text field cursor height
    material text_field_cursor_width_test 2
  • updated existing default property, debugFillProperties and plumbing tests

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@fluttergithubbot fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jul 17, 2020
@skia-gold
Copy link

skia-gold commented Jul 17, 2020

Gold has detected about 4 untriaged digest(s) on patchset 6.
View them at https://flutter-gold.skia.org/cl/github/61714

@justinmc justinmc mentioned this pull request Jul 17, 2020
Closed
@guidezpl guidezpl requested a review from justinmc July 17, 2020 18:42
justinmc
justinmc approved these changes Jul 17, 2020
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM, just take a look at using pumpAndSettle as mentioned in a comment. Thanks for working on this!

packages/flutter/lib/src/cupertino/text_field.dart
properties.add(FlagProperty('maxLengthEnforced', value: maxLengthEnforced, ifTrue: 'max length enforced'));
properties.add(DoubleProperty('cursorWidth', cursorWidth, defaultValue: 2.0));
properties.add(DoubleProperty('cursorHeight', cursorHeight, defaultValue: null));
properties.add(DiagnosticsProperty<Radius>('cursorRadius', cursorRadius, defaultValue: null));
Copy link
Contributor

@justinmc justinmc Jul 17, 2020

Choose a reason for hiding this comment

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

Good catch adding these missing properties!

packages/flutter/test/widgets/editable_text_cursor_test.dart Outdated
tester.state<EditableTextState>(textFinder).showToolbar();
await tester.pump();
await tester.pump(const Duration(milliseconds: 500));
await tester.pump(const Duration(milliseconds: 500));
Copy link
Contributor

@justinmc justinmc Jul 17, 2020

Choose a reason for hiding this comment

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

You should be able to use a pumpAndSettle here and below instead of the multiple pumps and durations, if you haven't tried it already.

Copy link
Member Author

@guidezpl guidezpl Jul 20, 2020

Choose a reason for hiding this comment

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

I could only replace one, I think it has to do with the cursor blinking and the timing needing to be right

justinmc
justinmc reviewed Jul 20, 2020
View reviewed changes
packages/flutter/test/widgets/editable_text_cursor_test.dart Outdated
await tester.tap(find.text('Paste'));
await tester.pump();
await tester.pump(const Duration(milliseconds: 500));
await tester.pump(const Duration(milliseconds: 500));
Copy link
Contributor

@justinmc justinmc Jul 20, 2020

Choose a reason for hiding this comment

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

Strange, is the pump without a duration needed as well? And what if you combine the two with durations into one with Duration(seconds: 1)?

There are other tests that do this sort of thing, though not testing the cursor size.

Anyway I think the main thing is to just make sure it's clear to someone else reading this code why the pumps are there, so maybe just add a comment.

Copy link
Member Author

@guidezpl guidezpl Jul 21, 2020

Choose a reason for hiding this comment

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

It wasn't, I simplified this test and others doing the same thing, and added a comment

@fluttergithubbot fluttergithubbot merged commit 5d854f6 into flutter:master Jul 21, 2020
@guidezpl guidezpl deleted the text-field-cursor-height branch July 21, 2020 15:15
Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request Aug 11, 2020
@guidezpl @Pragya007
Implement customizable cursor height (flutter#61714)
5c9fad9
@vasilich6107 vasilich6107 mentioned this pull request Aug 18, 2020
Closed
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@guidezpl @mingwandroid
Implement customizable cursor height (flutter#61714)
2a2acc2
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@justinmc justinmc justinmc approved these changes

Assignees

No one assigned

Labels

f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TextField cursor height cannot be changed

5 participants

@guidezpl @skia-gold @justinmc @googlebot @fluttergithubbot