Skip to content
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.

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

Fix SegmentedButton default size and default tappable size #142243

Merged
merged 10 commits into from Jan 26, 2024

Conversation

QuncCccccc
Copy link
Contributor

@QuncCccccc QuncCccccc commented Jan 25, 2024

fix #121493

SegmentedButton uses TextButton for each segments. When we have MaterialTapTargetSize.padded for TextButton, we make sure the minimum tap target size is 48.0( this value can be adjusted by visual density), even tough the actual button size is smaller. When SegmentedButton paints segments by using MultiChildRenderObjectWidget, it also includes the tap target size so the button that it actually draws always has the same height as the height of the tap target size.

To fix it, this PR firstly calculate the actual height of a text button in SegmentedButton class, then we can get the height delta if there is. Then the the value of (Segmented button render box height - the delta) would be the actual button size that we should see.

For now, we are not able to customize the min, max, fixed size in SegmentedButton style. So the standard button height is always 40 and can only be customized by style.visualDensity and style.tapTargetSize; SegmentedButton only simulates the TextButton behavior when TextButton's height is its default value.

Screenshot 2024-01-25 at 11 45 42 AM

SegmentedButtonDemo.mov

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jan 25, 2024
@QuncCccccc QuncCccccc marked this pull request as ready for review January 25, 2024 20:15
@QuncCccccc QuncCccccc changed the title Fix SegmentedButton default size and default tapTargetSize Fix SegmentedButton default size and default tappable size Jan 25, 2024
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

final double adjustButtonMinHeight = textButtonMinHeight + densityAdjustment.dy;
final double effectiveVerticalPadding = resolvedPadding.vertical + densityAdjustment.dy * 2;
final double effectedButtonHeight = max(fontSize + effectiveVerticalPadding, adjustButtonMinHeight);
final double tapTargetVerticalPadding;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be initialized using the new switch expression:

    final double tapTargetVerticalPadding = switch (resolvedTargetSize) {
      MaterialTapTargetSize.shrinkWrap => 0,
      MaterialTapTargetSize.padded => max(0, kMinInteractiveDimension + densityAdjustment.dy - effectedButtonHeight);
    };

@@ -813,6 +812,48 @@ void main() {
state = tester.state(find.byType(SegmentedButton<int>));
expect(state.statesControllers.values.first.value, states);
});

testWidgets('Min button height is 48.0 with standard density and MaterialTapTargetSize.padded', (WidgetTester tester) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to not that the button's height is 40 but its tap target's height is 48. Like:
'Min button hit target height is 48 and min (painted) button height is 40'

paints..rrect(
style: PaintingStyle.stroke,
strokeWidth: 1.0,
// Button border height is 43.5 - 4.5 + stoke width(1) = 40.
Copy link
Contributor

Choose a reason for hiding this comment

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

If 43.5 and 4.5 correspond to something can identify here, then best to include that info in the comment.

@QuncCccccc QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 26, 2024
@auto-submit auto-submit bot merged commit 7ff5f81 into flutter:master Jan 26, 2024
70 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 29, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jan 29, 2024
…6008)

Manual roll requested by stuartmorgan@google.com

flutter/flutter@a8efa77...2f6fdf2

2024-01-26 matanlurey@users.noreply.github.com Start renaming by adding a new `bringup: true` as an Android emulator. (flutter/flutter#142257)
2024-01-26 polinach@google.com Instrument ImageInfo. (flutter/flutter#141411)
2024-01-26 36861262+QuncCccccc@users.noreply.github.com Fix `SegmentedButton` default size and default tappable size (flutter/flutter#142243)
2024-01-26 godofredoc@google.com Update name for android_defines_test. (flutter/flutter#142273)
2024-01-26 101587250+pbo-linaro@users.noreply.github.com Enable native compilation for windows-arm64 (flutter/flutter#141930)
2024-01-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 4b145d041560 to 44f26274bfbe (6 revisions)" (flutter/flutter#142274)
2024-01-25 godofredoc@google.com Run a few mac tests only on arm. (flutter/flutter#142188)
2024-01-25 82763757+NobodyForNothing@users.noreply.github.com fix Ink not updating on TextField newline (flutter/flutter#140700)
2024-01-25 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 3.1.4 to 3.1.5 (flutter/flutter#142259)
2024-01-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4b145d041560 to 44f26274bfbe (6 revisions) (flutter/flutter#142264)
2024-01-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reverts "Rename `integration_tests/external_ui` but do not touch anything else..."" (flutter/flutter#142268)
2024-01-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Rename `integration_tests/external_ui` but do not touch anything else..." (flutter/flutter#142265)
2024-01-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from b2167a93c1a0 to 4b145d041560 (3 revisions) (flutter/flutter#142256)
2024-01-25 nathan.wilson1232@gmail.com Implementing `switch` expressions in the `cupertino/` directory (flutter/flutter#141591)
2024-01-25 matanlurey@users.noreply.github.com Rename `integration_tests/external_ui` but do not touch anything else... (flutter/flutter#142238)
2024-01-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 55eefd5bd255 to b2167a93c1a0 (2 revisions) (flutter/flutter#142252)
2024-01-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3b4779324b44 to 55eefd5bd255 (6 revisions) (flutter/flutter#142245)
2024-01-25 louisehsu@google.com Fix incorrect zh-cn translation for Look Up Label in selection controls (flutter/flutter#142158)
2024-01-25 jmccandless@google.com PopScope example improvements (flutter/flutter#142163)
2024-01-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1d3f16b0d62e to 3b4779324b44 (1 revision) (flutter/flutter#142225)
2024-01-25 engine-flutter-autoroll@skia.org Roll Packages from 8fbdf65 to 21b5abb (6 revisions) (flutter/flutter#142224)
2024-01-25 sigurdm@google.com Don't show legacy welcome message when analytics are disabled (flutter/flutter#140956)
2024-01-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7c4ed15cb271 to 1d3f16b0d62e (1 revision) (flutter/flutter#142223)

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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Height of SegmentedButton does not match Material3 spec and cannot be changed
2 participants