[Material] Implement TooltipTheme and Tooltip.textStyle, fix Tooltip debugLabel, update Tooltip defaults #36030

shihaohong · 2019-07-12T00:31:14Z

Description

Implements a TooltipTheme widget that allows all existing properties to be themeable.

Introduces the textStyle property, granting more fine-grained control to developers to update the tooltip's text message.

Fix debugLabel to use the correct color when reporting the message's textStyle
Breaking change: Updating default tooltip decoration to the following:

Brightness.light

BoxDecoration(
  color: Colors.grey[700].withOpacity(0.9),
  borderRadius: const BorderRadius.all(Radius.circular(4)),
);

Brightness.dark

BoxDecoration(
  color: Colors.white.withOpacity(0.9), // new
  borderRadius: const BorderRadius.all(Radius.circular(4)),
);

Breaking change: Updating default textStyle to the following:

Brightness.light

theme.textTheme.body1.copyWith(
  color: Colors.white, // new
  debugLabel: theme.textTheme.body1.debugLabel.replaceAll('white', 'black'), // fixes debugLabel
);

Brightness.dark

theme.textTheme.body1.copyWith(
  color: Colors.black, // new
  debugLabel: theme.textTheme.body1.debugLabel.replaceAll('white', 'black'), // fixes debugLabel
);

Tooltip Before Breaking change:

Tooltip After Breaking change:

Related Issues

Fixes #35513
Related to #33654
Related to #25844

Tests

I added the following tests:

Several tests verifying passing down of TooltipTheme properties (color, location, height, decoration, semantics, show and wait durations to reveal and hide tooltip)
Tests for textStyle implementation
Test to verify debugLabel for light and dark theme

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). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

…property Not documented yet, still needs tests for TooltipTheme.

…oltip-theme

HansMuller

This all looks good although I'd like to see if we can avoid the Theme brightness dependency. Most of feedback is trivial wordsmithing.

Please add:

DebugFillProperties tests like https://github.com/flutter/flutter/blob/master/packages/flutter/test/material/bottom_sheet_theme_test.dart#L22 and
https://github.com/flutter/flutter/blob/master/packages/flutter/test/material/bottom_sheet_theme_test.dart#L34

A copyWith/hashCode/== test like this: https://github.com/flutter/flutter/blob/master/packages/flutter/test/material/bottom_sheet_theme_test.dart#L10

A test that verifies the theme's default values (even though they're all null; so we don't regress) like this:
https://github.com/flutter/flutter/blob/master/packages/flutter/test/material/button_theme_test.dart#L9

HansMuller · 2019-07-15T18:18:30Z

packages/flutter/lib/src/material/theme_data.dart

  /// A theme for customizing the size, shape, and color of the tab bar indicator.
  final TabBarTheme tabBarTheme;

+  /// A theme for customizing the various properties of [Tooltip]s.


Instead of "various", maybe visual.

HansMuller · 2019-07-15T18:21:53Z

packages/flutter/lib/src/material/tooltip.dart

  /// user long presses on the widget.
  ///
  /// All of the arguments except [child] and [decoration] must not be null.
  const Tooltip({


The constructor doc isn't right (anymore):

/// All of the arguments except [child] and [decoration] must not be null.

Instead of this point about tooltip positioning, which probably belongs somewhere else, you could make a brief point about how the constructor parameters relate to TooltipTheme.

/// By default, tooltips prefer to appear below the [child] widget when the /// user long presses on the widget.

HansMuller · 2019-07-15T18:26:17Z

packages/flutter/lib/src/material/tooltip.dart


+  /// The style to use for the message of the tooltip.
+  ///
+  /// If null, the message's [TextStyle] will be determined based on


It would be really nice if this bright/dark dependency wasn't needed. Can't we just use ThemeData.textTheme? It appears to be black/white already - https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/material/theme_data.dart#L224

Should we do the following instead:

final TextStyle defaultTextStyle = theme.textTheme.body1.copyWith( color: theme.colorScheme.onSurface, );

As discussed offline, it'd be a breaking change, but it'd be closer aligned to the material spec and removes the dependency on Brightness.

Theming is specified by the material spec here: https://material.io/design/components/tooltips.html#theming

I've updated the textTheme to correspond with what is expected by Material

HansMuller · 2019-07-15T18:27:56Z

packages/flutter/lib/src/material/tooltip.dart

+    final TooltipThemeData tooltipTheme = TooltipTheme.of(context);
+    final bool excludeFromSemantics = widget.excludeFromSemantics ?? tooltipTheme.excludeFromSemantics ?? _defaultExcludeFromSemantics;
+
+    waitDuration = widget.waitDuration ?? tooltipTheme.waitDuration ?? _defaultWaitDuration;


HansMuller · 2019-07-15T18:35:30Z

packages/flutter/lib/src/material/tooltip.dart

+      ? theme.typography.white.merge(theme.textTheme).body1
+      : theme.typography.white.merge(theme.primaryTextTheme).body1;
+
+    final BoxDecoration defaultDecoration = BoxDecoration(


Can this be a static const _defaultDecoration? Note that

BorderRadius.circular(4) == const BorderRadius.all(Radius.circular(4))

How can this be achieved for the color Colors.grey[700].withOpacity(0.9)? I could use Color(0xFF616161), but calling .withOpacity() makes it a non-const value.

Actually, after our discussion, if we decide to do a breaking change, the defaults would change as follows:

final BoxDecoration defaultDecoration = BoxDecoration( color: theme.colorScheme.surface, borderRadius: const BorderRadius.all(Radius.circular(4)), );

HansMuller · 2019-07-15T19:05:42Z

packages/flutter/lib/src/material/tooltip_theme.dart

+    assert(t != null);
+    assert(a?.preferBelow == b?.preferBelow);
+    assert(a?.excludeFromSemantics == b?.excludeFromSemantics);
+    if (a == null && b == null)


It seems like this statement should be first.

HansMuller · 2019-07-15T19:09:14Z

packages/flutter/lib/src/material/tooltip_theme.dart

+    );
+  }
+
+  /// Linearly interpolate between two tooltip themes.


If we're going to prohibit changing preferBelow and excludeFromSemantics, we should document that. It might be better to use this kind of "interpolation" instead:

preferBelow: t < 0.5 ? a.preferBelow: b.preferBelow

Ideally this stuff wouldn't change but it doesn't seem worth creating a corner case for the odd case where it does.

I'll use that interpolation style instead. It seems to be better than just blocking them outright.

HansMuller · 2019-07-15T19:12:16Z

packages/flutter/lib/src/material/tooltip_theme.dart

+       ),
+       super(key: key, child: child);
+
+  /// Specifies the properties for descendant [Tooltip] widgets.


Specifies the => The

HansMuller · 2019-07-15T19:13:44Z

packages/flutter/lib/src/material/tooltip_theme.dart

+  /// Typical usage is as follows:
+  ///
+  /// ```dart
+  /// TooltipTheme theme = TooltipTheme.of(context);


TooltipThemeData theme = ...

HansMuller · 2019-07-15T19:18:31Z

packages/flutter/lib/src/material/tooltip_theme.dart

+  /// Specifies the properties for descendant [Tooltip] widgets.
+  final TooltipThemeData data;
+
+  /// The closest instance of this class that encloses the given context.


This is the conventional boilerplate for an inherited widget; it's not correct for this theme. This of method returns the closest TooltipTheme ancestor's [data] value, and if there is no TooltipTheme ancestor, then it returns the current [Theme]'s tooltipTheme. Apps can assume that the returned value is not null.

…ialization

shihaohong · 2019-07-15T23:12:45Z

I've implemented most of the code review suggestions and added the default tests for TooltipThemeData. I'm still currently waiting on what the default values for the border radii, background color, text color and opacity values should be.

shihaohong · 2019-07-17T15:50:03Z

I've received feedback on what the default tooltip decoration and textStyle should be, and this PR is now ready for review.

…Theme

HansMuller

LGTM!

HansMuller · 2019-07-19T19:59:23Z

packages/flutter/lib/src/material/theme_data.dart


+  /// A theme for customizing the visual properties of [Tooltip]s.
+  ///
+  /// This is the value returned from [Tooltip.of].


from [TooltipTheme.of]

HansMuller · 2019-07-19T20:15:14Z

packages/flutter/lib/src/material/tooltip.dart

  /// tree.
+  ///
+  /// Defaults to false. A tooltip will add a [Semantics.label] that is set to
+  /// [Tooltip.message]. Set this property to true if a custom


I think you mean ... true if the app is going to provide its own custom semantics label?

Yes! I'll update that

HansMuller · 2019-07-19T20:18:04Z

packages/flutter/lib/src/material/tooltip_theme.dart

+  /// tree.
+  ///
+  /// By default, [Tooltip]s will add a [Semantics.label] that is set to
+  /// [Tooltip.message]. Set this property to true if a custom


same comment as earlier

HansMuller · 2019-07-19T20:19:31Z

packages/flutter/lib/src/material/tooltip_theme.dart

+  /// The properties for descendant [Tooltip] widgets.
+  final TooltipThemeData data;
+
+  /// The closest instance of this class' [data] value that encloses the given


class' => class's

Maybe this could be a little clearer.

Returns the [data] from the closest [TooltipTheme] ancestor.

HansMuller · 2019-07-19T20:22:25Z

packages/flutter/lib/src/material/tooltip.dart

-  /// By default, tooltips prefer to appear below the [child] widget when the
-  /// user long presses on the widget.
+  /// By default, tooltips should adhere to the
+  /// [Material specifications](https://material.io/design/components/tooltips.html#spec).


specifications => specification

…oltip-theme

…Tooltip debugLabel, update Tooltip defaults (#36030)" This reverts commit 62158a1.

…Tooltip debugLabel, update Tooltip defaults (#36030)" (#36851) This reverts commit 62158a1.

…debugLabel, update Tooltip defaults (flutter#36030) * Implement TooltipThemeData and TooltipTheme * Add tooltip text style property * Update default tooltip text style and decoration to match Material specification * Fix debugLabel incorrectly labeling "white" as "black", and vice versa by default

…Tooltip debugLabel, update Tooltip defaults (flutter#36030)" (flutter#36851) This reverts commit 62158a1.

shihaohong added 9 commits July 10, 2019 15:58

Added TODO comments

5674921

Implement TooltipThemeData and TooltipTheme

4f2d993

WIP - Wire TooltipTheme through Tooltip and ThemeData, add TextStyle …

7718cc4

…property Not documented yet, still needs tests for TooltipTheme.

Add API documentation for textStyle property

fa682df

Fix wiring of default tooltip values

b875b85

Add height and Padding tests for TooltipTheme

12db4ac

Add preferBelow and verticalOffset theme tests

6cadb1f

Add semantics tests for Tooltip themes

5a87cd5

Add tooltip theme waitDuration and showDuration tests

b0b963f

shihaohong added c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 12, 2019

googlebot added the cla: yes label Jul 12, 2019

Add custom tooltip TextStyle tests, fix implementation bug

64166b9

shihaohong changed the title ~~WIP - Tooltip Theme~~ Tooltip Theme Jul 12, 2019

shihaohong requested a review from HansMuller July 12, 2019 00:51

Shi-Hao Hong and others added 5 commits July 12, 2019 22:24

Add const where needed in test file

137f64d

Merge branch 'master' of github.com:flutter/flutter into tooltip-theme

317616d

Add custom tooltip TextStyle theme tests

cafa9b8

Merge branch 'tooltip-theme' of github.com:shihaohong/flutter into to…

e39fd45

…oltip-theme

Add const to both tests where missing

6929760

HansMuller reviewed Jul 15, 2019

View reviewed changes

shihaohong added 3 commits July 15, 2019 14:01

Incorporate doc improvement and const declaration code review feedback

a1232e9

add debugFillProperties tests to tooltip_theme_test.dart

2b83cf2

Add tests on TooltipThemeData copyWith, ==, hashCode and default init…

04edf97

…ialization

shihaohong added 2 commits July 15, 2019 16:44

Add missing text style and decoration debugFillProperties

f700671

Update default colors, add test for debugLabel and dark theme

71063db

shihaohong added the c: API break Backwards-incompatible API changes label Jul 17, 2019

Updated documentation to reflect updated defaults

a789fb9

shihaohong changed the title ~~Tooltip Theme~~ [Material] Implement Tooltip Theme, along with fixes to default Tooltip properties Jul 17, 2019

shihaohong requested a review from HansMuller July 18, 2019 04:36

Shi-Hao Hong and others added 6 commits July 17, 2019 22:24

Incorporate API documentation feedback

bfce9be

Fix grammar on waitDuration doc

c5eed47

Add doc change to Tooltip.preferBelow that reflects change in Tooltip…

030a871

…Theme

Remove whitespace, add lerp macro doc

181dc7b

Fix properties not properly passing to the _TooltipOverlay

7e3dfd7

Make sample more fun

82ae05a

HansMuller mentioned this pull request Jul 18, 2019

[Material] ToggleButtons #34599

Merged

9 tasks

shihaohong changed the title ~~[Material] Implement Tooltip Theme, along with fixes to default Tooltip properties~~ [Material] Implement Tooltip Jul 19, 2019

shihaohong changed the title ~~[Material] Implement Tooltip~~ [Material] Implement TooltipTheme, fix Tooltip bugs, update Tooltip defaults Jul 19, 2019

HansMuller approved these changes Jul 19, 2019

View reviewed changes

Documentation improvements per feedback by HansMuller

e7df499

shihaohong changed the title ~~[Material] Implement TooltipTheme, fix Tooltip bugs, update Tooltip defaults~~ [Material] Implement TooltipTheme and Tooltip.textStyle, fix Tooltip debugLabel, update Tooltip defaults Jul 22, 2019

shihaohong added 5 commits July 22, 2019 15:30

Separate tests to ensure that textStyle is properly reflected

916bee7

Merge branch 'tooltip-theme' of github.com:shihaohong/flutter into to…

921266f

…oltip-theme

Merge branch 'master' of github.com:flutter/flutter into tooltip-theme

a6061a6

Fix tooltipTheme parameter with wrong type

f79f514

Merge branch 'tooltip-theme' of github.com:shihaohong/flutter into to…

408bf25

…oltip-theme

shihaohong merged commit 62158a1 into flutter:master Jul 24, 2019

shihaohong deleted the tooltip-theme branch July 24, 2019 15:24

shihaohong pushed a commit that referenced this pull request Jul 24, 2019

Revert "[Material] Implement TooltipTheme and Tooltip.textStyle, fix …

00e4d38

…Tooltip debugLabel, update Tooltip defaults (#36030)" This reverts commit 62158a1.

shihaohong mentioned this pull request Jul 24, 2019

Revert "[Material] Implement TooltipTheme and Tooltip.textStyle, fix Tooltip debugLabel, update Tooltip defaults" #36851

Merged

shihaohong pushed a commit that referenced this pull request Jul 24, 2019

Revert "[Material] Implement TooltipTheme and Tooltip.textStyle, fix …

aa6384c

…Tooltip debugLabel, update Tooltip defaults (#36030)" (#36851) This reverts commit 62158a1.

shihaohong mentioned this pull request Jul 24, 2019

[Material] Implement TooltipTheme and Tooltip.textStyle, update Tooltip defaults #36856

Merged

9 tasks

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

Revert "[Material] Implement TooltipTheme and Tooltip.textStyle, fix …

68565db

…Tooltip debugLabel, update Tooltip defaults (flutter#36030)" (flutter#36851) This reverts commit 62158a1.

HansMuller mentioned this pull request Jul 31, 2019

Update constructor APIs TooltipTheme, ToggleButtonsTheme, PopupMenuTheme #37338

Merged

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

[Material] Implement TooltipTheme and Tooltip.textStyle, fix Tooltip debugLabel, update Tooltip defaults #36030

[Material] Implement TooltipTheme and Tooltip.textStyle, fix Tooltip debugLabel, update Tooltip defaults #36030

Uh oh!

Conversation

shihaohong commented Jul 12, 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!

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

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!

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!

shihaohong commented Jul 15, 2019

Uh oh!

shihaohong commented Jul 17, 2019

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shihaohong commented Jul 12, 2019 •

edited

Loading