Enable TapRegion to detect all mouse button click

[anidotnet]

With this PR, TapRegion would be able to detect any kind of mouse button events. Earlier, TapRegion used to detect only left mouse button event.

Fixes #136706

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.

[dkwingsmt]

This doesn't look right, since it changes the behavior of TapRegion.onTapOutside. Our typical convention is to have a different callback for different buttons, such as TapGestureRecognizer has onTap and onSecondaryTap and onTertiaryTapDown. Do you think this will be a better idea, i.e. will this make your app cleaner to write?

[anidotnet]

@dkwingsmt The documentation of both TapRegion and RenderTapRegionSurface says,

/// A render object that provides notification of a tap inside or outside of a
/// set of registered regions, without participating in the [gesture
/// disambiguation](https://flutter.dev/gestures/#gesture-disambiguation)
/// system.

Here the keyword is - without participating in the gesture disambiguation. As I understood, the responsibility of these 2 widgets is not to detect and identify gestures or taps, but just to notify if a tap happens inside or outside of a specific region. So does it make sense to differentiate between mouse button taps?

Moreover, if the app developer wants to identify which mouse button has triggered the tap event, he/she can get the details from the PointerDownEvent object passed into the TapRegionCallback.

[anidotnet]

Hi @dkwingsmt, I am waiting to hear your thoughts on this.

[dkwingsmt]

I think the change of design makes sense, although I'd like to hear @gspencergoog's thoughts, since he wrote both the code and the doc.

I'm not in favor of the way you change the unit tests. I think the better way is to keep the current test cases as much as possible, and add new test cases, even if it's just copy pasting the majority of the one for the primary button (but usually you can skip some of the details). This avoids braiding multiple features into one (so that it's hard to find errors if it is broken) and avoids a test case going too long.

[anidotnet]

Thanks @dkwingsmt for your input. I'll make changes to the test cases as suggested once @gspencergoog confirms the code changes.

[gspencergoog]

This is not a bad idea, but the problem I see is that it will subtly break developer's code. It widens the allowed set of pointers to all pointers, which means that secondary clicking inside or outside will now also do whatever primary clicking did.

I can see the argument that this is the right thing to do anyhow, since TapRegion is mostly used to dismiss things when they are clicked outside of, and it's probably the case that usually a secondary click and a primary click should do the same thing in this case, but it's not always the case, especially for the onTapInside case. And, yes, they can interrogate the event to see which pointer was pressed, but right now they don't have to do that, so they probably haven't, and it will change the behavior of their app.

The non-breaking alternative is to create new optional callback(s) that provide information for other pointers, but that seems like an overly complicated API, and it complicates the implementation too.

Overall, I think I'm probably OK with the change as-is (well, I'd also like to see the testing changes Tong suggested), but if there were a way to make it less "stealth" of a break without complicating the API too much, then I'd prefer that (I can't think of one at the moment, though).

[dkwingsmt]

Usually we'd solve this by renaming symbols. What about creating a new method such as onPointerDownOutside that is called by all kinds of buttons, and deprecating onTapOutside? This also solves a concern of mine that the term "tap" should refer to the gesture instead of the pointer events, while this class works pre-gesture.

[anidotnet]

@dkwingsmt Then again, you have to deprecate onTapInside to onPointerDownInside. So what next on this PR now?

[dkwingsmt]

@anidotnet Can you change this PR so that instead of changing the behavior of onTapOutside, it keeps the behavior of onTapOutside but deprecate it, and creates a new method onPointerDownOutside that does not check pointer buttons? The same should be done to onTapInside (since we also want it to avoid checking pointer buttons.)

@gspencergoog What do you think?

[anidotnet]

@dkwingsmt there is another problem with this approach. There are other built-in widgets which are using TapRegion, but due to this bug their behaviors are not as expected. One example is MenuAnchor.

Let's say you are using it to show a context menu, you right click, it opens the menu. If you right click to another region, it will open another menu without closing the earlier one. Which is a weird behavior for a context menu in a desktop app, isn't it? How do you plan to fix these?

[dkwingsmt]

We can declare these behaviors as bugs and directly change these built-in widgets' use of TapRegion. This tedious process is mostly to ensure that apps who directly use TapRegion are not affected without a warning. (I agree that it might seem a bit arbitrary on whether we declare a behavior as a bug or as a feature...)

[anidotnet]

Upon a quick review, here are the built-in widgets that either uses TapRegion or extends it.

• _Submenu
• MenuAnchor
• TextFieldTapRegion

Among them TextFieldTapRegion extends TapRegion, and below is the widget uses the onTapOutside method of it.

• EditableText

So there also we have to introduce onPointerDownOutside and onPointerDownInside.

I am wondering is this tedious process worth all the trouble? Instead, can we not change the behavior of onTapOutside and mention it as a breaking change in changelog?

[dkwingsmt]

Flutter has been doing this "tedious process", and much more than what we've talked about here, since the beginning.
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes

Because as a library, Flutter promised backward compatibility, and maintaining this backward compatibility builds trust. Developers expect that what worked in earlier version continue to work in newer versions. Your apps have also benefitted from this for many times.

[anidotnet]

Understood. So how do you want to handle the changes for each widget? In the current PR only or separate issue+PR for each widget?

I want to bring to your notice that I may not have bandwidth to work on all these changes. I can't guarantee on the timeline, but a helping hand would make things faster.

[dkwingsmt]

I talked with gspencergoog offline and we agreed that we can make the change in-place (i.e. as the current PR). Since the document has long stated that this widget detects all pointer down events without considering buttons, this change can be considered a bug fix instead of a breaking change.

If you make the test changes (as suggested above), we can get this PR merged. :) Thanks for your contribution.

[dkwingsmt]

No, we'll just modify the behavior or onTapInside and onTapOutside (by changeing handleEvent) just like how this PR is doing and not introduce new methods.

[anidotnet]

@dkwingsmt I have updated the test cases. Sorry for the delay.

[dkwingsmt]

LGTM, sorry for the late review!

[anidotnet]

@dkwingsmt Thanks a lot. Now should I merge this PR or somebody else will do it?