Remove iPadOS mouse pointer if no longer connected #28319

moffatman · 2021-08-26T05:00:44Z

On iPadOS, if you are using a flutter application with a mouse or trackpad connected, but then disconnect that device, the pointer will stay in the framework at its previous position. So any Widget which interacts on PointerEnter/PointerExit will behave strangely. with no way to resolve it.
There is a way to query whether the pointer should be shown any more, but no way to get a callback from the system when this happens. So I perform the check when the user touches the screen, this should still provide a much better experience than before.

The test doesn't pass on the most recent Xcode 13 betas, the hover selector is broken in the simulator. I filed radar FB9427343 so hopefully that will be addressed. But CI is not running the iPadGestureTests yet anyways.

Fixes flutter/flutter#89878

chinmaygarde

cc @jmagman @gaaclarke

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

testing/scenario_app/lib/src/touches_scenario.dart

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m

gaaclarke · 2021-09-10T17:33:34Z

Can you file an issue and link it in the description please? That helps other people that run into this problem discover it.

What do you think about an approach that would fire an NSTimer to poll for the presence of the device every 500ms one a device is added and remove it when it's no longer present, instead of relying on the next touch event to clear it out?

moffatman · 2021-09-10T19:13:37Z

What do you think about an approach that would fire an NSTimer to poll for the presence of the device every 500ms one a device is added and remove it when it's no longer present, instead of relying on the next touch event to clear it out?

I thought about it, and initially didn't like polling approach. And also, iOS removes the cursor/focus environment when the user hasn't moved the mouse for only a few seconds (I measured it at ~3.5 seconds). I suppose it's iOS policy that the cursor should be removed when inactive, so maybe it should be followed. If the user was actively hovering something when the cursor went inactive, though, it might be unexpected. They would see the cursor fade out, then ~0-500ms later, the hover stop.

In one of my Flutter apps, I have some content that appears on hover, sometimes I leave the mouse inactive while I read the content. If the Flutter cursor was also removed, it would be annoying, but I suppose it's rare to use that desktop UI paradigm on the iPad. Mostly on iPad hover is just used to do the fancy cursor effects.

gaaclarke · 2021-09-15T17:27:59Z

Couldn't we mimic that behavior though, right? Anytime there is cursor activity we reset the timer that is set for ~3.5 seconds. That way we are matching the behavior of UIKit apps, no?

moffatman · 2021-09-16T04:46:07Z

It's a good idea, I implemented it but I realized that the UIFocusSystemCheck doesn't care about whether the cursor is active, it's just if the mouse is connected or not. I think I was confused with some other APIs I was trying out to check the pointer status, there isn't a great official one. But this means I can implement your original suggestion, check every 500ms if the mouse is plugged in.

chinmaygarde · 2021-09-30T20:29:09Z

Just a drive by comment about "but then disconnect that device, the pointer will stay in the framework at its previous position". Isn't it the case that devices when disconnected broadcast a cancelled pointer event? The framework should be able to detect that instead of there being a need to poll for the active pointers.

moffatman · 2021-09-30T20:41:58Z

@chinmaygarde
The hover pointer is not like other pointers, Flutter's use case is not well-supported. We only know its position by intercepting UIKit's request for a rectangle to apply cursor effects. And we can only tell if it's no longer connected by polling this side channel.
@gaaclarke
I would say this is ready for review but I don't know if it's testable, I have found the iPad simulator to be broken for testing pointer interactions since ~Xcode 13 beta 2.

chinmaygarde · 2021-10-14T20:20:04Z

If @gaaclarke is busy, I can take a stab at reviewing this. It looks like there is consensus on this approach being the right way forward.

chinmaygarde

The guide for tests discourages timeouts but I am not able to provide better guidance on how to test this. LGTM.

moffatman · 2021-11-05T05:05:52Z

I found that this is currently not testable, the pointerInteraction here does not get triggered by synthetic hover events in recent versions of Xcode (it did at one point work during the beta cycle for Xcode 13). I discovered that when using UIHoverGestureRecognizer, it does get triggered properly. If we use that class instead of PointerInteraction we will also get a state-change when the pointer is disconnected, so it's overall a better solution for this issue in general. I will update this PR with that change, let's not merge this.

chinmaygarde · 2021-11-11T21:03:31Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+      pointer_data.change = flutter::PointerData::Change::kRemove;
+      break;
+    default:
+      FML_LOG(ERROR) << "Unhandled hover phase: " << _hoverGestureRecognizer.get().state;


This log is not user actionable. Let's remove it. Also, flutter::PointerData::Change::kCancel seems more apt here.

kCancel is meant for when the pointer is down, since this pointer is never down it will cause failed assertions during processing.
Since the remaining unhandled and unexpected states are all a kind of semi-recognized I thought that kHover was the closest choice. And it's the only one that will not cause a fatal double add or double remove.

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

jmagman · 2021-11-15T17:06:03Z

Merge conflict with https://github.com/flutter/engine/pull/29623/files#diff-365b13daa5168d36d5824f220bc472a4a90f4685761947f8daf9dc76e7f8d962R793

jmagman

The view controller code looks good. Had some nits about the test, and suggestions for making what it is expecting clearer.
I don't have a bluetooth mouse to test this with at the moment.

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m

jmagman · 2021-11-15T18:07:49Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m

+  NSMutableDictionary* messages = [NSMutableDictionary dictionary];
+  for (XCUIElement* element in [app.textFields allElementsBoundByIndex]) {
+    NSString* rawMessage = element.value;
+    NSUInteger commaIndex = [rawMessage rangeOfString:@","].location;


This section is hard to understand, what is it doing, exactly? Does -componentsSeparatedByString make it easier to read?

I just need the first component, I don't want to split the whole string by commas since there are more commas later.

jmagman · 2021-11-15T18:09:11Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m

-                @"PointerChange.up event did not occur for a right-click");
+  int rightClickAddedSeqNo, rightClickDownSeqNo, rightClickUpSeqNo;
+  if ([messages objectForKey:@"PointerChange.add,device=2,buttons=0"] == nil) {
+    // Sometimes the tap pointer has the same device as the right-click (the UITouch is reused)


This test generally may be easier to read as a series of expectations, with ordering either enforced or not depending on what you're checking.

I observed a lot of different permutations of event order, even though this is somewhat convoluted I don't think the alternative is better.

gaaclarke · 2021-11-29T21:58:22Z

@jmagman friendly ping. I looked through your feedback and their response, this looks good to me now.

jmagman

LGTM

jmagman · 2021-11-29T22:41:57Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/iPadGestureTests.m

+      [app.textFields[@"1,PointerChange.hover,device=0,buttons=0"] waitForExistenceWithTimeout:1],
+      @"PointerChange.hover event did not occur for a hover");
+  // The number of hover events fired is not always the same
+  NSInteger lastHoverSequenceNumber = -1;


Initializing to NSNotFound and checking (lastHoverSequenceNumber == NSNotFound) || (messageSequenceNumber > lastHoverSequenceNumber) is probably more idiomatic than -1, but this is fine.

…ngine#28319)

flutter-dashboard bot added the platform-ios label Aug 26, 2021

google-cla bot added the cla: yes label Aug 26, 2021

moffatman force-pushed the ipad_remove_cursor branch from aacdf5f to 95c3bb3 Compare August 27, 2021 03:09

chinmaygarde reviewed Sep 9, 2021

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm Outdated Show resolved Hide resolved

gaaclarke reviewed Sep 9, 2021

View reviewed changes

moffatman force-pushed the ipad_remove_cursor branch from 95c3bb3 to fc98152 Compare September 10, 2021 05:48

moffatman force-pushed the ipad_remove_cursor branch from fc98152 to 6b9240b Compare September 11, 2021 01:18

moffatman force-pushed the ipad_remove_cursor branch from 4e7429f to 8191524 Compare September 19, 2021 21:06

zanderso requested a review from gaaclarke October 7, 2021 20:15

moffatman requested a review from chinmaygarde October 14, 2021 20:41

chinmaygarde approved these changes Nov 4, 2021

View reviewed changes

jmagman added the waiting for customer response label Nov 5, 2021

moffatman force-pushed the ipad_remove_cursor branch from de5e6a0 to e5ade78 Compare November 7, 2021 23:18

moffatman requested a review from chinmaygarde November 8, 2021 18:03

chinmaygarde reviewed Nov 11, 2021

View reviewed changes

moffatman added 6 commits November 15, 2021 12:45

Remove iPadOS mouse pointer if no longer connected

bc964cf

Update touches_scenario data format

29185ad

Fix hover pointer add/remove logic

3845191

Poll to remove mouse pointer instead of checking on tap

eeb5e97

Fix retain cycle

fa6b846

Use UIHoverGestureRecognizer instead of UIPointerInteraction

7d9c4fd

Remove log message, clear delegates on dealloc

2b26b23

moffatman force-pushed the ipad_remove_cursor branch from a85eb94 to 2b26b23 Compare November 15, 2021 17:48

jmagman suggested changes Nov 15, 2021

View reviewed changes

CaseyHillers changed the base branch from master to main November 15, 2021 18:16

fluttergithubbot force-pushed the master branch from b1a8771 to 2962099 Compare November 15, 2021 18:30

Address test feedback about syntax and readability

821c4b5

moffatman force-pushed the ipad_remove_cursor branch from 0a210ec to 821c4b5 Compare November 15, 2021 22:50

moffatman requested a review from jmagman November 17, 2021 17:13

godofredoc changed the base branch from master to main November 24, 2021 07:21

Use NSPredicate to locate hover events

1752ef5

jmagman approved these changes Nov 29, 2021

View reviewed changes

jmagman added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. and removed waiting for customer response labels Nov 29, 2021

fluttergithubbot merged commit 0236853 into flutter:main Nov 29, 2021

engine-flutter-autoroll mentioned this pull request Nov 30, 2021

Roll Engine from 7ba215c2c6a0 to f37334353f48 (25 revisions) flutter/flutter#94435

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 30, 2021

0236853 Remove iPadOS mouse pointer if no longer connected (flutter/e…

dca12e2

…ngine#28319)

Remove iPadOS mouse pointer if no longer connected #28319

Remove iPadOS mouse pointer if no longer connected #28319

Uh oh!

Conversation

moffatman commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaaclarke commented Sep 10, 2021

Uh oh!

moffatman commented Sep 10, 2021

Uh oh!

gaaclarke commented Sep 15, 2021

Uh oh!

moffatman commented Sep 16, 2021

Uh oh!

chinmaygarde commented Sep 30, 2021

Uh oh!

moffatman commented Sep 30, 2021

Uh oh!

chinmaygarde commented Oct 14, 2021

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

moffatman commented Nov 5, 2021

Uh oh!

chinmaygarde Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

moffatman Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmagman commented Nov 15, 2021

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmagman Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

moffatman Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

moffatman Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Nov 29, 2021

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

moffatman commented Aug 26, 2021 •

edited

Loading