Add onLongPress to Buttons #40641

bernaferrari · 2019-09-17T01:40:17Z

Description

It is too hard to add onLongPress to Buttons. You need to add an InkWell that goes on top of another InkWell and this causes additional issues. You also need to deal with clipBehavior: Clip.antiAlias and onPressed from Button is required but needs to be empty. Two possible solutions to overcome the problem:

allow passing InkWell as a parameter (which would allow more advanced usages, like onDoubleTap or onHover).
just add a onLongPress parameter to buttons.

While I think there is room for both options, it was suggested I could pursue the second, which I really like and is good enough for me right now.

Related Issues

#40558

Tests

Should I add tests? If so, how? All of onPressed tests are focused on how it changes the color of Button, not on if it works or not.

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 report problems everywhere. I don't know how to use it, how do you use it? It looks like there are issues with const/icons/a lot of things. Not on this PR, but everywhere.
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.
No, this is not a breaking change.

fluttergithubbot · 2019-09-17T01:40:19Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

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

HansMuller · 2019-09-20T21:40:16Z

Material buttons, like most material components, handle long-press by showing a tooltip. It's not clear that it would be useful to override this behavior. What's the use case?

bernaferrari · 2019-09-21T13:04:46Z

There is no tooltip attribute for buttons. It only has a null onLongPress. I was making long press to show a Bottom Sheet with selectable options/settings.

HansMuller · 2019-09-24T17:50:25Z

You are right, buttons currently don't show a tooltip.

Should a button with only a longPress callback specified be disabled? I don't think so, but supporting that will complicate this PR somewhat.

Tests are needed which verify that longPress input gestures do what's expected. If the existing onPressed tests aren't verifying that their callback is actually running, then we might as well do that here as well.

bernaferrari · 2019-09-27T18:16:41Z

Done! @HansMuller I am having a hard time figuring out how to test my code (and the tests). I did two tests, but I'm not sure how to test them (weren't included on the previous commit). Could you please shine some light (or update the contributing wiki documentation)? flutter test only works for samples. I want to test the material code I just modified.

HansMuller · 2019-09-27T21:43:12Z

You'll want to extend the tests in packages/flutter/test/material/buttons_test.dart that verify that longPress is null by default. Similarly, add a test to the end of the file, like the one called 'MaterialButton disabled default is correct', which verifies that just setting longPress enables a button. This test should also check the enabled property.

HansMuller · 2019-09-27T21:44:21Z

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

 /// default will resemble a flat button in the [disabledColor]. If you are
 /// trying to change the button's [color] and it is not having any effect, check
-/// that you are passing a non-null [onPressed] handler.
+/// that you are passing a non-null [onPressed] or [onLongPress] handler.


I realize that this doc used to say "handler". Since we seem to be consistently referring to onPressed etc as "callbacks", please use callback here.

HansMuller · 2019-09-27T21:45:42Z

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

  final VoidCallback onPressed;

+  /// The callback that is called when the button is long-pressed.
+  ///
+  /// If this and [onPressed] are set to null, the button will be disabled.


If this callback and [onPressed] are null, then the button will be disabled.

HansMuller · 2019-09-27T21:46:26Z

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

@@ -88,9 +89,14 @@ class MaterialButton extends StatelessWidget {

  /// The callback that is called when the button is tapped or otherwise activated.
  ///
-  /// If this is set to null, the button will be disabled.
+  /// If this and [onLongPress] are set to null, the button will be disabled.


If this callback and [onLongPress] are null, then the button will be disabled.

HansMuller · 2019-09-27T21:46:59Z

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

@@ -21,8 +21,8 @@ import 'theme_data.dart';
 ///
 /// The button's size will expand to fit the child widget, if necessary.
 ///
-/// MaterialButtons whose [onPressed] handler is null will be disabled. To have
-/// an enabled button, make sure to pass a non-null value for onPressed.
+/// MaterialButtons whose [onPressed] and [onLongPress] handlers are null will be disabled. To have


handlers => callbacks

HansMuller · 2019-09-27T21:48:30Z

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

@@ -29,7 +29,7 @@ import 'theme_data.dart';
 /// interactive, with ink splashes, without also committing to these stylistic
 /// choices, consider using [InkWell] instead.
 ///
-/// If the [onPressed] callback is null, then the button will be disabled,
+/// If [onPressed] or [onLongPress] callbacks are null, then the button will be disabled,


If the [onPressed] and [onLongPress] callbacks are null, then this button will be disabled.

HansMuller · 2019-09-27T21:51:32Z

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

@@ -75,9 +76,14 @@ class RawMaterialButton extends StatefulWidget {

  /// Called when the button is tapped or otherwise activated.
  ///
-  /// If this is set to null, the button will be disabled, see [enabled].
+  /// If this and [onLongPress] are set to null, the button will be disabled, see [enabled].


If this callback and [onLongPress] are null, then this button will be disabled.

Here and elsewhere: to add a link a related method, include a "See also" section:

/// /// See also: /// /// * [enabled], which is true if the button is enabled.

bernaferrari · 2019-09-30T20:50:34Z

Tests are there. Please take a look.

I had a hard time testing the tests, because I don't know how to test them. I know there is no syntax issue in them, but I'm not sure if flutter tests is checking them or the multiple error messages (for unknown reasons) makes it exit before running the tests.

shihaohong · 2019-10-04T00:24:30Z

Based on the compiler errors, it might be from recent changes to flutter/dev/packages/flutter_test. Check to see if the Flutter repo you're working on is in sync with the current master branch

…ngpressed2

bernaferrari · 2019-10-04T01:11:08Z

Thanks, @shihaohong! All tests are now passing. :)

shihaohong · 2019-10-04T15:22:01Z

There seems to still be analyzer test failures. You might want to fix those before requesting another review :)

bernaferrari · 2019-10-04T17:14:29Z

Analyzer is fine. The issue is with: Whitespace detected at the end of source code lines. Please remove:

What is going on? Shouldn't there be a newline at the end of each file?

shihaohong · 2019-10-04T17:44:19Z

That refers to whitespace after a line, not a newline. For example

var someDartVar = "Hello"; [extra space here]

shihaohong · 2019-10-04T17:46:28Z

It seems to be these lines:

packages/flutter/lib/src/material/button.dart:80:  /// 
packages/flutter/lib/src/material/button.dart:87:  /// 
packages/flutter/lib/src/material/button.dart:89:  /// 
packages/flutter/lib/src/material/material_button.dart:93:  /// 
packages/flutter/lib/src/material/material_button.dart:102:  ///

bernaferrari · 2019-10-04T17:50:24Z

Oh, should work fine now, thanks! All of these are new to me.

shihaohong · 2019-10-04T17:53:01Z

No worries! I submitted an issue to improve the error message as well, since it was pretty confusing for me when I first bumped into this as well.

…Button.

shihaohong · 2019-10-15T15:35:56Z

It seems the failing tests are caused by changes to debugFillProperties. Also, you still have to remove the ObjectFlagProperty for onPressed and onLongPress

bernaferrari · 2019-10-15T16:53:44Z

lol, thanks! @shihaohong I am having a hard time figuring out stage vs stash vs commit on vscode and I guess that slipped. Should be fixed now.

shihaohong

This is getting very close! The only thing from my end would be adding tests to ensure that onLongPress's callbacks are properly invoked for buttons other than MaterialButton when a long press is detected.

shihaohong · 2019-10-15T21:37:58Z

packages/flutter/test/material/buttons_test.dart

+    expect(button.enabled, true);
+  });
+
+  testWidgets('FlatButton should be enabled when onLongPress is not null.', (WidgetTester tester) async {


I just realized that buttons_test.dart is an odd file and contains an amalgamation of tests for the different types of buttons, even though flat_button_test.dart, raised_button_test.dart, and material_button_test.dart exist.

It could be nice to have each button widget's tests in their own files, but maybe that's out of the scope of this PR since there are preexisting tests in this file that test for the different buttons as well.

Oh, you are right. For some reason I thought everything was on the same (because, well, there are everything on the same). I'm going to split those on the correct files.

Just the same question as I commented below. How many tests. 2 (onPressed true + onLongPress false / onPressed false + onLongPress true) or 4 (onPressed false + onLongPress false.. and so on)?

I think the following combinations will be important:

just onPressed to make sure that functionality works

just onLongPress to make sure that functionality works

both onPressed and onLongPress activated, then testing between different the two gestures to ensure that both are properly and distinctly recognized

~~both deactivated to ensure that neither callbacks are being invoked when they shouldn't be~~ Edit: I just realized that this doesn't make sense, since you cannot invoke a non-existent callback

Definitely check the other test files to see if some of this is already implemented -- I'd be surprised if the onPressed ones weren't already tested, but you never know :)

So.. outlineButton contained onTap test. I modified it to include onLongPress and replicated into the others. RawMaterialButton already contained one test, I kept it, but added the new one, more complex, in the end of the file. The others had no onTap/onPressed test.

TODO for you. Wish I could receive a notification when analyzer/CI/github/cirrus/whoever is testing finished the tests.

Also, running flutter analyze --flutter-repo will run the analyzer locally so you do not have to wait for the CI to finish running its tests

I had the issue where the repo version I was using was newer than flutter I had, therefore that wouldn't work. I needed to start using ../../bin/flutter instead. Wasnt trivial. And that doesn't check the whitespace thing.

shihaohong · 2019-10-15T21:39:01Z

packages/flutter/test/material/buttons_test.dart

+    expect(button.enabled, true);
+  });
+
+  testWidgets('MaterialButton onPressed callback', (WidgetTester tester) async {


If onPress callbacks of the other buttons haven't been tested, we should test them.

Ok, so do I add another 4 tests? Or even more? (onPressed true/onLongPress false, onPressed false/OnLongPress false, and so on)? I agree with you that file is a mess.

shihaohong · 2019-10-15T21:39:52Z

packages/flutter/test/material/buttons_test.dart

+    expect(didPressButton, isTrue);
+  });
+
+  testWidgets('MaterialButton onLongPress callback', (WidgetTester tester) async {


We should definitely add tests to ensure that onLongPress's callback works.

… replicated into others. Possibly WIP.

…ork locally.

shihaohong · 2019-10-18T14:45:56Z

packages/flutter/test/material/flat_button_test.dart

@@ -333,6 +333,79 @@ void main() {
        paintsExactlyCountTimes(#clipPath, 0),
    );
  });
+
+  testWidgets('FlatButton responds to tap and onLongPress when enabled', (WidgetTester tester) async {


nit for consistency

Suggested change

testWidgets('FlatButton responds to tap and onLongPress when enabled', (WidgetTester tester) async {

testWidgets('FlatButton onPressed and onLongPress callbacks are correctly called when non-null', (WidgetTester tester) async {

Thanks. Yeah. I had that question. It's the tap method that works with on Pressed, lol.

I'm traveling, I'll be back in about 8 days. See you soon, then. Have a nice week! (and please maybe review my other PR related to sliders)

shihaohong · 2019-10-18T14:54:40Z

packages/flutter/test/material/flat_button_test.dart

+    expect(pressedCount, 1);
+
+    // onPressed null, onLongPress not null.
+    pressedCount = 0;


I think it feels odd that pressedCount is reset after every test. The variable name implies that the number of taps/presses matters. However, in this case, it's only used to determine if the callback was invoked when the button was pressed.

A bool wasPressed would achieve the same effect, but be clearer to future readers of the code.

shihaohong · 2019-10-18T14:55:48Z

packages/flutter/test/material/flat_button_test.dart

+    expect(pressedCount, 0);
+  });
+
+  testWidgets('FlatButton onPressed and onLongPress callbacks are distincly recognized', (WidgetTester tester) async {


Suggested change

testWidgets('FlatButton onPressed and onLongPress callbacks are distincly recognized', (WidgetTester tester) async {

testWidgets('FlatButton onPressed and onLongPress callbacks are distinctly recognized', (WidgetTester tester) async {

shihaohong · 2019-10-18T14:58:36Z

packages/flutter/test/material/flat_button_test.dart

+    await tester.pumpWidget(
+      buildFrame(onPressed: null, onLongPress: () { pressedCount += 1; }),
+    );
+    expect(tester.widget<FlatButton>(find.byType(FlatButton)).enabled, true);


Splitting the call into multiple lines makes it easier to understand what is tested. That, or you could make the tester/find call its own separate line of code, just like you did for your other test. The same applies elsewhere

Suggested change

expect(tester.widget<FlatButton>(find.byType(FlatButton)).enabled, true);

expect(tester.widget<FlatButton>(

find.byType(FlatButton)

).enabled, isTrue);

shihaohong · 2019-10-18T15:00:16Z

packages/flutter/test/material/flat_button_test.dart

+      ),
+    );
+
+    final Finder flatButton = find.byType(FlatButton);


This is much easier to read 👍

Agree.. But there is a problem. I'm redefining the button 3 times in the other tests. So I would need to either use flatbutton1, flatbutton2, flatbutton.. or a var, which also doesn't sound good.

Ideas?

You could remove the final declaration and re-use the same variable, which is fine

shihaohong · 2019-10-18T15:41:08Z

packages/flutter/test/material/material_button_test.dart

+
+testWidgets('MaterialButton responds to tap and onLongPress when enabled', (WidgetTester tester) async {
+
+    int pressedCount = 0;


Same comment as above

shihaohong · 2019-10-18T15:41:39Z

packages/flutter/test/material/outline_button_test.dart

    );
    expect(tester.widget<OutlineButton>(find.byType(OutlineButton)).enabled, true);
    await tester.tap(find.byType(OutlineButton));
    await tester.pumpAndSettle();
    expect(pressedCount, 1);

+    // onPressed null, onLongPress not null.
+    pressedCount = 0;


Same comment as above

shihaohong · 2019-10-18T15:42:01Z

packages/flutter/test/material/raised_button_test.dart

+
+  testWidgets('RaisedButton responds to tap and onLongPress when enabled', (WidgetTester tester) async {
+
+    int pressedCount = 0;


Same comment as above

shihaohong · 2019-10-18T15:42:11Z

packages/flutter/test/material/raised_button_test.dart

+      buildFrame(onPressed: null, onLongPress: null),
+    );
+    expect(tester.widget<RaisedButton>(find.byType(RaisedButton)).enabled, false);
+    await tester.tap(find.byType(RaisedButton));


Same comment as above

shihaohong · 2019-10-18T15:42:21Z

packages/flutter/test/material/raw_material_button_test.dart

+
+  testWidgets('RawMaterialButton responds to tap and onLongPress when enabled', (WidgetTester tester) async {
+
+    int pressedCount = 0;


Same comment as above

shihaohong · 2019-10-28T19:40:55Z

packages/flutter/test/material/flat_button_test.dart

-    await tester.longPress(find.byType(FlatButton));
+    flatButton = find.byType(FlatButton);
+    expect(tester.widget<FlatButton>(flatButton).enabled, false);
+    await tester.tap(flatButton);


I think we discussed removing the tap and longPress checks here, since the callbacks are null anyway and don't really result in testing anything meaningful

oooops! Fixing it now, thanks for all the patience over the past weeks! I am learning a lot over how internal Flutter works.

No worries :) Thanks for addressing all the feedback comments!

shihaohong · 2019-10-28T19:41:44Z

packages/flutter/test/material/material_button_test.dart

-    await tester.longPress(find.byType(MaterialButton));
+    materialButton = find.byType(MaterialButton);
+    expect(tester.widget<MaterialButton>(materialButton).enabled, false);
+    await tester.tap(materialButton);


Same comment as above

shihaohong · 2019-10-28T19:42:02Z

packages/flutter/test/material/outline_button_test.dart

-    await tester.longPress(find.byType(OutlineButton));
+    outlineButton = find.byType(OutlineButton);
+    expect(tester.widget<OutlineButton>(outlineButton).enabled, false);
+    await tester.tap(outlineButton);


Same comment as above

shihaohong · 2019-10-28T19:42:15Z

packages/flutter/test/material/raised_button_test.dart

-    await tester.longPress(find.byType(RaisedButton));
+    raisedButton = find.byType(RaisedButton);
+    expect(tester.widget<RaisedButton>(raisedButton).enabled, false);
+    await tester.tap(raisedButton);


Same comment as above

shihaohong · 2019-10-28T19:42:28Z

packages/flutter/test/material/raw_material_button_test.dart

-    await tester.longPress(find.byType(RawMaterialButton));
+    rawMaterialButton = find.byType(RawMaterialButton);
+    expect(tester.widget<RawMaterialButton>(rawMaterialButton).enabled, false);
+    await tester.tap(rawMaterialButton);


Same comment as above

shihaohong

LGTM

HansMuller

LGTM too

shihaohong · 2019-10-29T21:57:14Z

I resolved a merge conflict caused by a separate PR that refactored some MaterialButton tests from buttons_test.dart into the material_button_test.dart. It should be good to go once the tree is green.

* Add onLongPress to Buttons. * Button enabled status will now respond to onLongPress

Add onLongPress to Buttons.

fe69fff

fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Sep 17, 2019

googlebot added the cla: yes label Sep 17, 2019

lucalves reviewed Sep 18, 2019

View reviewed changes

packages/flutter/lib/src/material/material_button.dart Outdated Show resolved Hide resolved

Button enabled status will now respond to onLongPress

e0c73e7

HansMuller reviewed Sep 27, 2019

View reviewed changes

bernaferrari added 2 commits September 27, 2019 19:10

Improve documentation as per Hans Muller review.

5151ca0

Tests

7db8303

bernaferrari added 2 commits October 3, 2019 22:04

Merge branch 'master' of https://github.com/flutter/flutter into onlo…

3135153

…ngpressed2

fix button onPressed test. Replace tester.pressed with tester.tap

36a633f

make tests full flutter analyze compliant

3640f39

Fix whitespace after comments line

49427e3

shihaohong mentioned this pull request Oct 4, 2019

Whitespace analyzer error message unclear #41995

Closed

Merge branch 'master' into onlongpressed2

c21d2dd

bernaferrari added 3 commits October 14, 2019 22:39

Add tests for Flatbutton, OutlineButton, RawMaterialButton and Raised…

5da494f

…Button.

Fix whitespace detected by analyzer

7180850

Replace ObjectFlagProperty with FlagProperty.

2ac3fc0

Fix last commit.

f681826

Merge branch 'master' into onlongpressed2

9c2a5b1

shihaohong reviewed Oct 15, 2019

View reviewed changes

bernaferrari added 7 commits October 16, 2019 15:21

Migrated tests to the correct files. Improved outline_button_test and…

3db4aa3

… replicated into others. Possibly WIP.

Fix additional ) which slipped.

837b2db

Fix analyzer issues

aaa268e

Fix additional analyzer issues.

a7d072d

Fix remaining issues with analyzer. Took me a while to make flutter w…

9766af2

…ork locally.

Last analyzer fix. Use const and single quotes.

3be4aeb

Fix whitespace thing.

95c6ff8

shihaohong reviewed Oct 18, 2019

View reviewed changes

Fix reviewed issues

bc6d7e5

shihaohong reviewed Oct 28, 2019

View reviewed changes

Remove pumpAndSettle, simplify onPressed null, onLongPress null.

577d1dd

shihaohong approved these changes Oct 28, 2019

View reviewed changes

HansMuller approved these changes Oct 29, 2019

View reviewed changes

Final commit, cosmetic improvement.

016da04

shihaohong added the waiting for tree to go green label Oct 29, 2019

Shi-Hao Hong added 2 commits October 29, 2019 14:54

Merge branch 'master' into onlongpressed2

a22c696

Remove whitespace

f03b92f

Remove more whitespace

d9acc3e

shihaohong merged commit 28a214b into flutter:master Oct 30, 2019

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019

Add onLongPress to Buttons (flutter#40641)

8c277d2

* Add onLongPress to Buttons. * Button enabled status will now respond to onLongPress

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

	testWidgets('FlatButton responds to tap and onLongPress when enabled', (WidgetTester tester) async {
	testWidgets('FlatButton onPressed and onLongPress callbacks are correctly called when non-null', (WidgetTester tester) async {


		testWidgets('MaterialButton responds to tap and onLongPress when enabled', (WidgetTester tester) async {

		int pressedCount = 0;


		testWidgets('RaisedButton responds to tap and onLongPress when enabled', (WidgetTester tester) async {

		int pressedCount = 0;


		testWidgets('RawMaterialButton responds to tap and onLongPress when enabled', (WidgetTester tester) async {

		int pressedCount = 0;

Add onLongPress to Buttons #40641

Add onLongPress to Buttons #40641

Conversation

bernaferrari commented Sep 17, 2019

Description

Related Issues

Tests

Checklist

Breaking Change

fluttergithubbot commented Sep 17, 2019

HansMuller commented Sep 20, 2019

bernaferrari commented Sep 21, 2019

HansMuller commented Sep 24, 2019

bernaferrari commented Sep 27, 2019

HansMuller commented Sep 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernaferrari commented Sep 30, 2019

shihaohong commented Oct 4, 2019 • edited

bernaferrari commented Oct 4, 2019

shihaohong commented Oct 4, 2019

bernaferrari commented Oct 4, 2019

shihaohong commented Oct 4, 2019 • edited

shihaohong commented Oct 4, 2019 • edited

bernaferrari commented Oct 4, 2019

shihaohong commented Oct 4, 2019

shihaohong commented Oct 15, 2019 • edited

bernaferrari commented Oct 15, 2019

shihaohong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shihaohong Oct 15, 2019 • edited

Choose a reason for hiding this comment

bernaferrari Oct 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shihaohong left a comment

Choose a reason for hiding this comment

HansMuller left a comment

Choose a reason for hiding this comment

shihaohong commented Oct 29, 2019

HansMuller commented Sep 27, 2019 •

edited

shihaohong commented Oct 4, 2019 •

edited

shihaohong commented Oct 4, 2019 •

edited

shihaohong commented Oct 4, 2019 •

edited

shihaohong commented Oct 15, 2019 •

edited

shihaohong Oct 15, 2019 •

edited

bernaferrari Oct 16, 2019 •

edited