Support for MaterialTapTargetSize within ToggleButtons by rami-a · Pull Request #93259 · flutter/flutter

rami-a · 2021-11-08T16:57:53Z

This change adds a MaterialTapTargetSize param to the ToggleButtons widget (and also hooks into the ThemeData.materialTapTargetSize property) so that ToggleButtons can benefit from the same behavior as regular buttons (standard tap target size while being visibly smaller on screen).

Fixes #93249

Screen.Recording.2021-11-08.at.11.56.27.AM.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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

rami-a · 2021-11-08T18:51:04Z

+    switch (resolvedTapTargetSize) {
+      case MaterialTapTargetSize.padded:
+        return ConstrainedBox(
+          constraints: const BoxConstraints(),


This was necessary for some reason. @HansMuller thoughts?

I'm not sure. The implication is that the _InputPadding class is not respecting its incoming constraints, so adding this class forces that to happen.

HansMuller · 2021-11-08T20:22:59Z

+    switch (resolvedTapTargetSize) {
+      case MaterialTapTargetSize.padded:
+        return ConstrainedBox(
+          constraints: const BoxConstraints(),


I'm not sure. The implication is that the _InputPadding class is not respecting its incoming constraints, so adding this class forces that to happen.

HansMuller · 2021-11-18T00:19:30Z

I removed the ConstrainedBox noted in #93259 (comment)

      case MaterialTapTargetSize.padded:
        return _InputPadding(
          minSize: const Size(kMinInteractiveDimension, kMinInteractiveDimension),
          direction: direction,
          child: result,
        );

The demo below, which defines the height of the ToggleButtons with its constraints parameter, seems to work as expected.

Working Demo

import 'package:flutter/material.dart';

class Home extends StatefulWidget {
  const Home({ Key? key }) : super(key: key);

  @override
  _HomeState createState() => _HomeState();
}

class _HomeState extends State<Home> {
  final List<bool> isSelected = <bool>[false, false, false];

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: ToggleButtons(
          constraints: BoxConstraints(minWidth: 48.0, minHeight: 32.0),
          tapTargetSize: MaterialTapTargetSize.padded,
          isSelected: isSelected,
          onPressed: (int index) {
            setState(() {
              isSelected[index] = !isSelected[index];
            });
          },
          children: <Widget>[
            Icon(Icons.ac_unit),
            Icon(Icons.call),
            Icon(Icons.cake),
          ],
        ),
      ),
    );
  }
}

void main() {
  runApp(const MaterialApp(home: Home()));
}

A version of the demo that constrains the height SizedBox to be 32 using a SizedBox above the ToggleButtons does fail, but that's to be expected, since the ToggleButtons widget has to respect to the incoming constraints. This happens with or without the extra BoxConstraints widget.

Failing Demo

import 'package:flutter/material.dart';

class Home extends StatefulWidget {
  const Home({ Key? key }) : super(key: key);

  @override
  _HomeState createState() => _HomeState();
}

class _HomeState extends State<Home> {
  final List<bool> isSelected = <bool>[false, false, false];

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: SizedBox(
          height: 32,
          child: ToggleButtons(
            tapTargetSize: MaterialTapTargetSize.padded,
            isSelected: isSelected,
            onPressed: (int index) {
              setState(() {
                isSelected[index] = !isSelected[index];
              });
            },
            children: <Widget>[
              Icon(Icons.ac_unit),
              Icon(Icons.call),
              Icon(Icons.cake),
            ],
          ),
        ),
      ),
    );
  }
}

void main() {
  runApp(const MaterialApp(home: Home()));
}

What's the test case that caused you to add the extra BoxConstraints widget?

rami-a · 2021-11-18T13:50:51Z

Hmm when I tried just removing the constrained box, the demo you defined still allowed the buttons to be tapped even when tapping way outside the bounds (see attached video). I'm tapping way above/below the widget and still triggered the buttons.

Screen.Recording.2021-11-18.at.8.48.59.AM.mov

HansMuller · 2021-11-18T16:04:14Z

Sorry, I didn't realize that was the problem we were trying to resolve. Will investigate further.

HansMuller · 2021-11-18T23:13:44Z

The problem with the original version of the code was that _InputPadding.hitTest() was deferring to super.hitTest().

  @override
  bool hitTest(BoxHitTestResult result, { required Offset position }) {
    // The super.hitTest() method also checks hitTestChildren(). We don't
    // want that in this case because we've padded around the children per tapTargetSize.
    if (!size.contains(position))
      return false;

    // Only adjust one axis to ensure the correct button is tapped.
    Offset center;
    if (direction == Axis.horizontal) {
      center = Offset(position.dx, child!.size.height / 2);
    } else {
      center = Offset(child!.size.width / 2, position.dy);
    }
    return result.addWithRawTransform(
      transform: MatrixUtils.forceToPoint(center),
      position: center,
      hitTest: (BoxHitTestResult result, Offset position) {
        assert(position == center);
        return child!.hitTest(result, position: center);
      },
    );
  }

rami-a · 2021-11-19T14:32:46Z

The problem with the original version of the code was that _InputPadding.hitTest() was deferring to super.hitTest().

Ah that worked, thanks!

guidezpl

  _    ___ _____ __  __ 
 | |  / __|_   _|  \/  |
 | |_| (_ | | | | |\/| |
 |____\___| |_| |_|  |_|

guidezpl · 2021-11-19T14:48:35Z

+  /// If the [tapTargetSize] is larger than [constraints], the buttons will
+  /// include a transparent margin that responds to taps.
+  ///
+  /// Always defaults to [ThemeData.materialTapTargetSize].


Suggested change

/// Always defaults to [ThemeData.materialTapTargetSize].

/// Defaults to [ThemeData.materialTapTargetSize].

guidezpl · 2021-11-19T14:51:06Z

+
+/// A widget to pad the area around a [ToggleButtons]'s children.
+///
+/// This widget is based on a similar one using in [ButtonStyleButton] but it


there's an issue with this sentence. Overall I don't understand this comment. What does "tap target" in quotes mean? Which widget does "size of the widget" refer to?

Reworded the sentence, hopefully thats better.

Suggested change

/// This widget is based on a similar one using in [ButtonStyleButton] but it

/// This widget is based on a similar one used in [ButtonStyleButton] but it

oops, fixed

HansMuller

LGTM with a few small tweaks

HansMuller · 2021-11-19T16:17:52Z

+  }) : super(key: key, child: child);
+
+  final Size minSize;
+


extra blank line here

HansMuller · 2021-11-19T16:18:01Z

+
+  @override
+  void updateRenderObject(BuildContext context, covariant _RenderInputPadding renderObject) {
+    renderObject.minSize = minSize;


what about direction?

oops, added

HansMuller · 2021-11-19T16:24:01Z

+
+/// A widget to pad the area around a [ToggleButtons]'s children.
+///
+/// This widget is based on a similar one using in [ButtonStyleButton] but it


Rami Abou Ghanem added 3 commits November 3, 2021 10:34

Add tap target size to ToggleButtons

01ae3f8

Remove the max height constraint

a40f4af

Add tests for taptargetsize

eabfcb3

flutter-dashboard Bot added p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Nov 8, 2021

google-cla Bot added the cla: yes label Nov 8, 2021

rami-a requested a review from HansMuller November 8, 2021 16:59

Remove dead code

92c02fd

rami-a commented Nov 8, 2021

View reviewed changes

HansMuller reviewed Nov 8, 2021

View reviewed changes

Update comment in private class

a3b7366

Remove constrained box and fix hitTest override

1748894

rami-a requested a review from HansMuller November 19, 2021 14:33

guidezpl approved these changes Nov 19, 2021

View reviewed changes

Address feedback

36c4ff1

HansMuller approved these changes Nov 19, 2021

View reviewed changes

Address more feedback

7adeb51

rami-a added the waiting for tree to go green label Nov 19, 2021

fluttergithubbot merged commit a395c95 into flutter:master Nov 19, 2021

rami-a deleted the toggle_buttons_tap_target branch November 22, 2021 16:05

math1man mentioned this pull request Jan 26, 2022

ToggleButtons with MaterialTapTargetSize.padded and height constrained to 32 #97302

Closed

	/// Always defaults to [ThemeData.materialTapTargetSize].
	/// Defaults to [ThemeData.materialTapTargetSize].

	/// This widget is based on a similar one using in [ButtonStyleButton] but it
	/// This widget is based on a similar one used in [ButtonStyleButton] but it

Conversation

rami-a commented Nov 8, 2021

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HansMuller commented Nov 18, 2021

Uh oh!

rami-a commented Nov 18, 2021

Uh oh!

HansMuller commented Nov 18, 2021

Uh oh!

HansMuller commented Nov 18, 2021

Uh oh!

rami-a commented Nov 19, 2021

Uh oh!

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants