Text selection handle color respects Theme.textSelectionHandleColor by LongCatIsLooong · Pull Request #36313 · flutter/flutter

LongCatIsLooong · 2019-07-16T21:55:16Z

Description

Added a new parameter to EditableText's constructor, as well as TextSelectionControls.buildHandle.

Note that even with this fix, if the selection handles are currently visible, ThemeData.textSelectionHandleColor change won't immediately affect the color of the handles, until the handles are rebuilt. This is because selection handles might be higher up in the tree than the Theme it is depending on (which happens to selectionColor as well). See #36220.

Related Issues

Fixes #34640

Tests

I added the following tests:

selection handles respect Theme
text selection handle color

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 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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Will send out the link after initial review.

justinmc

I called out a bunch of places that contain breaking changes just as a heads up. I think we have to announce those if they go into the final version of this PR.

See my question about the original approach to handle color in material though. I need some help understanding how you were able to solve the problem.

justinmc · 2019-07-16T22:59:52Z

    bool showCursor,
    this.showSelectionHandles = false,
    this.selectionColor,
+    @required this.textSelectionHandleColor,


This is a breaking change

justinmc · 2019-07-16T23:03:27Z

+import 'package:flutter/widgets.dart';
+import 'package:flutter_test/flutter_test.dart';
 import 'package:mockito/mockito.dart';
-import 'package:flutter/foundation.dart';


Thanks for alphabetizing 👍

justinmc · 2019-07-16T23:06:55Z

 class _TextSelectionHandlePainter extends CustomPainter {
-  const _TextSelectionHandlePainter();
+  const _TextSelectionHandlePainter(this.color);
+  final Color color;


This parameter in material's _TextSelectionHandlePainter is a named parameter, so probably make this one named too for consistency.

Also, maybe they should both be required and non-null? I'm not sure why the material one isn't like that already, but maybe there's a reason I'm not seeing.

justinmc · 2019-07-16T23:11:23Z

  /// The top left corner of this widget is positioned at the bottom of the
  /// selection position.
-  Widget buildHandle(BuildContext context, TextSelectionHandleType type, double textLineHeight);
+  Widget buildHandle(BuildContext context, TextSelectionHandleType type, Color color, double textLineHeight);


This is also a breaking change.

justinmc · 2019-07-16T23:11:39Z

    @required this.endHandleLayerLink,
    @required this.renderObject,
    this.selectionControls,
+    @required this.textSelectionHandleColor,


Breaking change

justinmc · 2019-07-16T23:34:43Z

      height: _kHandleSize,
      child: CustomPaint(
-        painter: _TextSelectionHandlePainter(
-          color: Theme.of(context).textSelectionHandleColor


Was something about this original approach causing the color to not change when the theme was updated? I'm trying to understand the cause of the problem and why the breaking changes are needed. Like the parameter in EditableText. Does passing it through there somehow make the handles rebuild when they weren't with this original code?

The problem is text selection handles are inserted onto an Overlay as OverlayEntrys:

flutter/packages/flutter/lib/src/widgets/text_selection.dart

Lines 404 to 409 in 93a3ce7

void showHandles() {

assert(_handles == null);

_handles = <OverlayEntry>[

OverlayEntry(builder: (BuildContext context) => _buildHandle(context, _TextSelectionHandlePosition.start)),

OverlayEntry(builder: (BuildContext context) => _buildHandle(context, _TextSelectionHandlePosition.end)),

];

So calling Theme.of(context) returns the closest ThemeData above the Overlay, which might be different from the ThemeData that sits above the EditableText, e.g.:

/ TextSelectionHandle Theme0 - Overlay < \ Theme1 - TextField - EditableText

Instead of Theme1, currently Theme0 is inherited by TextSelectionHandle.

The easy fix is to let EditableText take in textSelectionHandleColor as a parameter, and let TextField inherits from Theme (which it already does) to get the correct ThemeData.textSelectionHandleColor to build the text selection handles.

The fix doesn't solve the problem entirely as one of the tests demonstrated, because selection handles don't listen to Theme directly ("listen to" as in an inheritor listens to an InheritedWidget, not sure what the correct term is).

Oh I see, thanks for the clear explanation!

Would it be possible to make the handles use Theme1? Like what if you passed this.context from TextSelectionOverlay into _buildHandle instead of the given context and let the handles use that? Is that a major sin in Flutter?

Haven't tried it but I think that probably won't work. An InheritedWidget's inheritors have to be its descendant widgets, but Theme1 and TextSelectionHandle are sibling nodes. Moreover if the BuildContext (i.e. Element) we passed to the handle is unmounted (e.g., replaced by a new Element node) at some point, the app may crash.

Good point... What if you wrap the handles built in showHandles with a Theme widget? I guess that wouldn't update the handle color when Theme1 changes either, so that's not really a complete solution.

Just trying to brainstorm ways to solve this problem with our existing theme infrastructure to avoid having a redundant textSelectionHandleColor parameter.

codecov · 2019-07-17T00:15:51Z

Codecov Report

Merging #36313 into master will increase coverage by 1.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #36313      +/-   ##
==========================================
+ Coverage   53.69%   54.83%   +1.14%     
==========================================
  Files         187      187              
  Lines       17248    17237      -11     
==========================================
+ Hits         9261     9452     +191     
+ Misses       7987     7785     -202

Flag	Coverage Δ
#flutter_tool	`54.83% <0%> (+1.14%)`	⬆️

Impacted Files	Coverage Δ
...lutter_tools/lib/src/build_system/targets/ios.dart	`0% <0%> (-100%)`	⬇️
packages/flutter_tools/lib/src/web/compile.dart	`39.39% <0%> (-39.4%)`	⬇️
...tter_tools/lib/src/build_system/targets/macos.dart	`64.7% <0%> (-11.77%)`	⬇️
packages/flutter_tools/lib/src/cache.dart	`44.41% <0%> (-5.98%)`	⬇️
...ckages/flutter_tools/lib/src/ios/code_signing.dart	`96.87% <0%> (-1.57%)`	⬇️
...utter_tools/lib/src/build_system/targets/dart.dart	`83.33% <0%> (-1.29%)`	⬇️
packages/flutter_tools/lib/src/emulator.dart	`69.49% <0%> (-0.85%)`	⬇️
packages/flutter_tools/lib/src/build_info.dart	`70.16% <0%> (-0.56%)`	⬇️
packages/flutter_tools/lib/src/run_hot.dart	`45.83% <0%> (+0.21%)`	⬆️
...ckages/flutter_tools/lib/src/flutter_manifest.dart	`82.63% <0%> (+0.52%)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a60713...bac4c1c. Read the comment docs.

LongCatIsLooong · 2019-07-18T21:40:45Z

Since the PR introduces multiple breaking changes, I'm going to close it in favor of the attempt to resolve #36220.

LongCatIsLooong added 4 commits July 16, 2019 11:16

add new parameter

779ea9a

EditableText changes

5fc5bce

add test for selection handles

11bd783

IDE please

29009cc

LongCatIsLooong added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages labels Jul 16, 2019

LongCatIsLooong requested review from HansMuller and justinmc July 16, 2019 21:55

fluttergithubbot added the p: cupertino_ui cupertino_ui package in flutter/packages label Jul 16, 2019

googlebot added the cla: yes label Jul 16, 2019

LongCatIsLooong added the c: API break Backwards-incompatible API changes label Jul 16, 2019

justinmc reviewed Jul 16, 2019

View reviewed changes

review

bac4c1c

LongCatIsLooong force-pushed the text-selection-handle-color branch from c2b33b4 to bac4c1c Compare July 17, 2019 00:20

LongCatIsLooong closed this Jul 18, 2019

LongCatIsLooong mentioned this pull request Aug 1, 2019

Unable to Change TextSelectionHandleColor using Theme Widget #34640

Closed

github-actions Bot locked as resolved and limited conversation to collaborators Aug 5, 2021

Navigation Menu

Repository navigation

Text selection handle color respects Theme.textSelectionHandleColor - #36313

Text selection handle color respects Theme.textSelectionHandleColor#36313
LongCatIsLooong wants to merge 5 commits into
flutter:masterfrom
LongCatIsLooong:text-selection-handle-color

LongCatIsLooong commented Jul 16, 2019 •

edited

Loading

Uh oh!

justinmc left a comment

Uh oh!

justinmc Jul 16, 2019

Uh oh!

justinmc Jul 16, 2019

Uh oh!

justinmc Jul 16, 2019

Uh oh!

justinmc Jul 16, 2019

Uh oh!

justinmc Jul 16, 2019

Uh oh!

justinmc Jul 16, 2019

Uh oh!

LongCatIsLooong Jul 17, 2019 •

edited

Loading

Uh oh!

justinmc Jul 17, 2019

Uh oh!

LongCatIsLooong Jul 17, 2019 •

edited

Loading

Uh oh!

justinmc Jul 17, 2019

Uh oh!

codecov Bot commented Jul 17, 2019 •

edited

Loading

Uh oh!

LongCatIsLooong commented Jul 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	void showHandles() {
	assert(_handles == null);
	_handles = <OverlayEntry>[
	OverlayEntry(builder: (BuildContext context) => _buildHandle(context, _TextSelectionHandlePosition.start)),
	OverlayEntry(builder: (BuildContext context) => _buildHandle(context, _TextSelectionHandlePosition.end)),
	];

Conversation

LongCatIsLooong commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

justinmc left a comment

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!

LongCatIsLooong Jul 17, 2019 • 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!

LongCatIsLooong Jul 17, 2019 • 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!

codecov Bot commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LongCatIsLooong commented Jul 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LongCatIsLooong commented Jul 16, 2019 •

edited

Loading

LongCatIsLooong Jul 17, 2019 •

edited

Loading

LongCatIsLooong Jul 17, 2019 •

edited

Loading

codecov Bot commented Jul 17, 2019 •

edited

Loading