Exposes onSecondaryTap in InkWell. #119058

chinmoy12c · 2023-01-24T12:32:32Z

This PR adds onSecondaryTap to InkWell.

Fixes: #118622

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.

Renzo-Olivares

Hello @chinmoy12c thank you for your contribution. This is a much needed PR and I'm happy to give some feedback.

The ink splash on secondary tap is not currently enabled. This is because the ink splash is created in handleTapDown but that is only called when a PointerDownEvent comes from a kPrimaryButton. We can take a look at handleTapDown for guidance on that and probably abstract some of the code there into something like this. For this to work we would also have to handle onSecondaryTapDown.

  void handleAnyDown(TapDownDetails details) {
    if (_anyChildInkResponsePressed) {
      return;
    }
    _startNewSplash(details: details);
  }

We should also call updateHighlight(_HighlightType.pressed, value: false); in handleSecondaryTap to cancel the ink splash when the click/tap is released.
I wonder if we should also provide onSecondaryTapUp (called at the same time as onSecondaryTap but provides a TapUpDetails) in this PR as well as on onSecondaryTapCancel to be consistent with the primary button gestures callbacks that InkWell provides.

Renzo-Olivares · 2023-01-25T17:30:38Z

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

If we look at this enabled property it looks like it includes all of the gesture callbacks. I think we should also add onSecondaryTap into this to correctly handle those cases.

bool isWidgetEnabled(_InkResponseStateWidget widget) { return widget.onTap != null || widget.onDoubleTap != null || widget.onLongPress != null || widget.onTapDown != null; }

Added callbacks like onSecondaryTap, onSecondaryTapUp, and onSecondaryTapDown should also go here.

I noticed that onTapUp was not included in this, so I added that too.

Renzo-Olivares · 2023-01-27T20:45:31Z

Google Test failures look unrelated.

Renzo-Olivares

https://github.com/chinmoy12c/flutter/blob/395c3d8abcc25ee3759f32d57179a7c4af371b06/packages/flutter/lib/src/material/ink_well.dart#L757

I think we should also add entries for the new callbacks here

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

Renzo-Olivares · 2023-01-30T21:31:50Z

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

I think we should also handle and expose secondary tap cancel in a similar way to other callbacks and handleTapCancel since it is possible a tap is cancelled and we want the splash to also cancel in that case.

Hey, @Renzo-Olivares I've updated the PR, sorry for the delay missed the notifications.

No worries! Sorry for the delay, i've been out of office for the past week as well.

Renzo-Olivares · 2023-02-17T01:25:54Z

Hi @chinmoy12c, where you still working on this / did you have any questions about any reviews?

Renzo-Olivares

LGTM other than my final comments. Thank you for your patience and for this awesome contribution!

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

Renzo-Olivares · 2023-02-27T17:32:00Z

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

No worries! Sorry for the delay, i've been out of office for the past week as well.

Renzo-Olivares · 2023-03-08T22:03:58Z

@chinmoy12c Looks like some tests are failing, but they seem unrelated. Maybe a sync up to master will fix them.

Piinks

Thank you!

justinmc

LGTM 👍. Just some nits. Thank you for the PR!

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

packages/flutter/test/material/ink_well_test.dart

justinmc · 2023-03-17T17:57:10Z

This has the potential to be a breaking change because InkWell will now steal secondary tap gestures from overlapping widgets. For example, I noticed this exact problem in data-table-2: maxim-saplin/data_table_2#176

The way to fix it is to simply migrate to the new secondary tap API on InkWell rather than using an overlapping widget to receive secondary taps.

maxim-saplin · 2023-03-18T09:18:20Z

It's worth to have the new events added to TableRowInkWell as well.

Sesa1988 · 2023-07-30T19:57:30Z

return InkWell(
      onSecondaryTap: isFollowedTo == true ? () => _openMenu(context) : null,
      child: ListTile(
        onTap: isFollowedTo == true ? () => _openMenu(context) : null,
        onLongPress: isFollowedTo == true ? () => _openMenu(context) : null,

onSecondaryTap still not working for me with trackpad two fingers click on macOS, the ripple effect is loading but then nothing is triggered, a long press is working.

[✓] Flutter (Channel stable, 3.10.6, on macOS 13.0 22A380 darwin-x64, locale en-DE)
[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.2)
[✓] Xcode - develop for iOS and macOS (Xcode 14.2)
[✓] Chrome - develop for the web
[✓] Android Studio (version 2022.2)
[✓] VS Code (version 1.80.2)
[✓] Connected device (2 available)
[✓] Network resources

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 24, 2023

Renzo-Olivares self-requested a review January 24, 2023 16:02

chinmoy12c force-pushed the issue_118622 branch from 17de905 to 97e7030 Compare January 24, 2023 19:42

Renzo-Olivares reviewed Jan 25, 2023

View reviewed changes

chinmoy12c force-pushed the issue_118622 branch from 97e7030 to 395c3d8 Compare January 27, 2023 15:43

chinmoy12c requested a review from Renzo-Olivares January 27, 2023 15:44

Renzo-Olivares reviewed Jan 27, 2023

View reviewed changes

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

Renzo-Olivares reviewed Jan 27, 2023

View reviewed changes

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

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

chinmoy12c force-pushed the issue_118622 branch 2 times, most recently from b3300ee to 4cca6b7 Compare January 28, 2023 08:36

Renzo-Olivares reviewed Jan 30, 2023

View reviewed changes

chinmoy12c force-pushed the issue_118622 branch 3 times, most recently from 08ce89e to c377ecc Compare February 18, 2023 11:47

AhmedLSayed9 mentioned this pull request Feb 18, 2023

added option to open with secondary tap AhmedLSayed9/dropdown_button2#125

Closed

Renzo-Olivares approved these changes Feb 27, 2023

View reviewed changes

chinmoy12c force-pushed the issue_118622 branch from c377ecc to ca1492f Compare March 6, 2023 20:29

chinmoy12c force-pushed the issue_118622 branch from ca1492f to ae0b394 Compare March 9, 2023 05:22

Piinks approved these changes Mar 9, 2023

View reviewed changes

justinmc approved these changes Mar 9, 2023

View reviewed changes

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

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

packages/flutter/test/material/ink_well_test.dart Outdated Show resolved Hide resolved

Added onSecondaryTap in InkWell.

a744165

chinmoy12c force-pushed the issue_118622 branch from ae0b394 to a744165 Compare March 11, 2023 19:51

Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 16, 2023

auto-submit bot merged commit e1eb1b9 into flutter:master Mar 16, 2023

engine-flutter-autoroll mentioned this pull request Mar 16, 2023

Roll Flutter from fb7e82826db3 to 4e4787ac7ebe (27 revisions) flutter/packages#3479

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2023

e1eb1b9 Added onSecondaryTap in InkWell. (flutter/flutter#119058)

3f7adca

engine-flutter-autoroll mentioned this pull request Mar 16, 2023

Roll Flutter from fb7e82826db3 to 9efbfb80de50 (28 revisions) flutter/packages#3480

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2023

e1eb1b9 Added onSecondaryTap in InkWell. (flutter/flutter#119058)

5d7ce0b

engine-flutter-autoroll mentioned this pull request Mar 17, 2023

Roll Flutter from 67e5f663d075 to 53dfd23b043f (39 revisions) flutter/packages#3484

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 17, 2023

e1eb1b9 Added onSecondaryTap in InkWell. (flutter/flutter#119058)

e8414f1

justinmc mentioned this pull request Mar 17, 2023

Fix secondary taps blocked by InkWell maxim-saplin/data_table_2#176

Merged

HansMuller mentioned this pull request Mar 17, 2023

Flutter Framework Team: Notable Commits #121415

Closed

justinmc mentioned this pull request Mar 20, 2023

Add secondary tap capabilities to TableRowInkWell #123036

Merged

Piinks mentioned this pull request Mar 20, 2023

InkResponse.onSecondary* docs are not clear #123049

Closed

huycozy mentioned this pull request Apr 7, 2023

[Bug Report][Desktop] Ink ripple is being displayed on right click (since Flutter 3.9.x) #124328

Closed

bleroux mentioned this pull request Apr 7, 2023

Fix InkWell ripple visible on right click when not expected #124386

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

e1eb1b9 Added onSecondaryTap in InkWell. (flutter/flutter#119058)

d2e3c3d

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

e1eb1b9 Added onSecondaryTap in InkWell. (flutter/flutter#119058)

9b7bc38

huycozy mentioned this pull request May 15, 2023

[3.10.0] GestureDetector/Inkwell onSecondaryTap #126780

Closed

2 tasks

millerovv mentioned this pull request Jun 13, 2023

ListTile blocks onSecondaryTap of parent GestureDetector #128779

Closed

2 tasks

Exposes onSecondaryTap in InkWell. #119058

Exposes onSecondaryTap in InkWell. #119058

Uh oh!

Conversation

chinmoy12c commented Jan 24, 2023

Pre-launch Checklist

Uh oh!

Renzo-Olivares left a comment • 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!

Renzo-Olivares Jan 25, 2023

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Jan 25, 2023 • 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!

chinmoy12c Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Renzo-Olivares left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Renzo-Olivares Jan 30, 2023 • 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!

chinmoy12c Feb 18, 2023

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares commented Feb 17, 2023

Uh oh!

Renzo-Olivares left a comment • 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!

Uh oh!

Renzo-Olivares Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares commented Mar 8, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinmc commented Mar 17, 2023

Uh oh!

maxim-saplin commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sesa1988 commented Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Renzo-Olivares left a comment •

edited

Loading

Renzo-Olivares Jan 25, 2023 •

edited

Loading

Renzo-Olivares commented Jan 27, 2023 •

edited

Loading

Renzo-Olivares Jan 30, 2023 •

edited

Loading

Renzo-Olivares left a comment •

edited

Loading

maxim-saplin commented Mar 18, 2023 •

edited

Loading

Sesa1988 commented Jul 30, 2023 •

edited

Loading