[macOS] Implement hit testing and handle platform view cursor changes #43101

knopp · 2023-06-22T18:12:37Z

Changes to `FlutterMouseCursorPlugin`

none cursor is not handled by hiding the cursor anymore. That was too big of a hammer as it also affects other views (and platform views). Instead empty cursor is created from empty NSImage.
Cursor plugin now notifies the engine when cursor has changed. The engine forwards the change to last FlutterView that handled mouse event. This is necessary because on occasion FlutterView needs to be able to restore cursor without framework being involved.

Preventing PlatformView from changing cursor when it is obscured by Flutter Content.

Generally in Cocoa cursor changes are done as response to mouseMoved event, which is driven by a NSTrackingArea. The issue here is that this is not affected by hit testing and tracking areas form a hierarchy parallel to view hierarchy and are not affected by being obscured by another view (or tracking area). This means that platform view will receive mouseMoved event even when is obscured by Flutter content. To work around this, the mutator view puts a tracking area above platform view, which means it gets the mouseMove event first, and when it decides that mouse is over Flutter content, it will prevent platform view from changing the cursor for the rest of RunLoop turn (see NSCursor+IgnoreChange).

Actual hit testing

This part is rather straightforward, the area where FlutterContent obscures mutator view is provided to the mutator view, which will return nil from hitTest: when the point is in the obscured area.

Example of hit testing

flutter-hit-test1.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 and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

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

chinmaygarde · 2023-07-13T20:04:51Z

Ping @cbracken. Perhaps @cyanglaz also has some context.

cbracken

Looking at the embedder side changes right now, but a few comments on the pointer event/cursor side first.

cbracken · 2023-07-18T22:09:34Z

shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm

+      for (size_t j = i + 1; j < layers_count; j++) {
+        FlutterLayer* overlayLayer = (FlutterLayer*)layers[j];
+        if (overlayLayer->type == kFlutterLayerContentTypeBackingStore) {
+          auto present_info = overlayLayer->backing_store->present_info;
+          if (present_info != nullptr && present_info->paint_region != nullptr) {
+            for (size_t k = 0; k < present_info->paint_region->rects_count; k++) {
+              FlutterRect flutter_rect = present_info->paint_region->rects[k];
+              double scale = default_base_view.layer.contentsScale;
+              CGRect rect = CGRectMake(flutter_rect.left / scale, flutter_rect.top / scale,
+                                       (flutter_rect.right - flutter_rect.left) / scale,
+                                       (flutter_rect.bottom - flutter_rect.top) / scale);
+              CGRect intersection = CGRectIntersection(rect, mutator_view.frame);
+              if (!CGRectIsNull(intersection)) {
+                intersection.origin.x -= mutator_view.frame.origin.x;
+                intersection.origin.y -= mutator_view.frame.origin.y;
+                [mutator_view addHitTestIgnoreRegion:intersection];
+              }
+            }
+          }
+        }
+      }


Consider extracting this to its own method.

shell/platform/darwin/macos/framework/Source/FlutterMouseCursorPlugin.mm

cbracken · 2023-07-25T19:49:13Z

shell/platform/darwin/macos/framework/Source/NSCursor+IgnoreChange.mm

+  [self replaceSelector:@selector(set)
+              fromClass:[NSCursor class]
+           withSelector:@selector(_flutterSetCursorOverride)
+              fromClass:[NSCursor class]];


Avoid swizzling in all code except for tests. If it's not in the style guide already, I'll add it -- we've had this conversation many times in code reviews (including in one of mine from back in 2016 or so when I tried to get rid of FlutterAppDelegate) and in the end we always end up making the same call -- avoid it.

We don't allow swizzling AppKit classes for a few reasons:

it can be a blocker for shipping on the App Store.

we shouldn't apply Flutter-specific logic to non-Flutter parts of the app -- in add-to-app scenarios, developers may have just a single Flutter view in a massive app.

Swizzling in general does not prevent getting into AppStore. Using private / undocumented API does, which is not the case here. This also should not interfere with the rest of application, it just prevents changing cursor until the rest of RunLoop tick when Flutter content is over native view and already handled cursor change.

The application will be working without it, but with cursor flicker when moving over Flutter contents that have native view with active NSTrackingArea underneath (i.e. NSTextField).

cbracken · 2023-07-25T19:56:24Z

shell/platform/darwin/macos/framework/Source/NSCursor+IgnoreChange.mm

+  if (flutterIgnoreCursorChange) {
+    return;
+  }
+  [self _flutterSetCursorOverride];


For the sake of readability, at the very minimum this needs a comment pointing out that _flutterSetCursorOverride has been swizzled to use the original [NSCursor set] implementation and therefore this isn't an infinite loop. That said, see my comment on swizzling below.

flutter-dashboard · 2023-08-15T11:55:38Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

gspencergoog · 2023-09-14T18:26:10Z

@knopp Are you still planning on working on this? If not, could you either close it or mark it as draft so we can get it off the PR backlog? (Desktop Triage)

cbracken · 2023-09-14T20:07:21Z

This is pending another round of review from me, but been stuck working on two internal customer issues, I'll try to get back to this soon. Apologies for the delay.

gspencergoog · 2023-11-02T18:15:20Z

Converting this to Draft, as @cbracken is still looking at alternatives.

goderbauer · 2024-01-17T00:04:40Z

Spoke to @cbracken and he says this is still on his radar.

cbracken · 2024-04-08T18:01:10Z

shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm

+      if (present_info) {
+        for (size_t j = 0; j < present_info->paint_region->rects_count; j++) {
+          rects.push_back(present_info->paint_region->rects[j]);
+        }


Consider pre-reserving size of rects, and using std::copy which might have better performance:

rects.reserve(present_info->paint_region->rects_count); std::copy(present_info->paint_region->rects, present_info->paint_region->rects + present_info->paint_region->rects_count, std::back_inserter(rects));

cbracken

…or changes (flutter/engine#43101)

…146524) flutter/engine@5a824e2...5dca50d 2024-04-09 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Delete engine v1 android embedding (#51229)" (flutter/engine#51996) 2024-04-09 [email protected] Delete engine v1 android embedding (flutter/engine#51229) 2024-04-09 [email protected] better output from engine layer unit test failures (flutter/engine#51975) 2024-04-09 [email protected] Revert "Roll Dart SDK from 78174b41ab0f to 7a5e410f982e (1 revision) (#51980)" (flutter/engine#51990) 2024-04-09 [email protected] [skwasm] Reify the SkPicture pointer as the right type. (flutter/engine#51991) 2024-04-09 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Dart SDK from 7a5e410f982e to db99af14c4bc (1 revision) (#51986)" (flutter/engine#51989) 2024-04-09 [email protected] Move the Dart SDK to //flutter/third_party/dart (flutter/engine#51917) 2024-04-09 [email protected] [macOS] Implement hit testing and handle platform view cursor changes (flutter/engine#43101) 2024-04-09 [email protected] Roll Dart SDK from 7a5e410f982e to db99af14c4bc (1 revision) (flutter/engine#51986) 2024-04-09 [email protected] Roll Dart SDK from 78174b41ab0f to 7a5e410f982e (1 revision) (flutter/engine#51980) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This fixes an edge case that I've overlooked in #43101. It is possible to configure `FlutterViewController` tracking area with `FlutterMouseTrackingModeAlways` option, in which case the tracking area will call `[FlutterView cursorUpdate:]` unconditionally even if the mouse cursor is over platform view and the platform view has already handled the mouse update. The solution is to prevent the `FlutterView` from updating the cursor when the cursor is over a platform view. This can be accomplished by doing a hitTest inside `[FlutterView cursorUpdate:]`. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

…lutter#146524) flutter/engine@5a824e2...5dca50d 2024-04-09 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Delete engine v1 android embedding (flutter#51229)" (flutter/engine#51996) 2024-04-09 [email protected] Delete engine v1 android embedding (flutter/engine#51229) 2024-04-09 [email protected] better output from engine layer unit test failures (flutter/engine#51975) 2024-04-09 [email protected] Revert "Roll Dart SDK from 78174b41ab0f to 7a5e410f982e (1 revision) (flutter#51980)" (flutter/engine#51990) 2024-04-09 [email protected] [skwasm] Reify the SkPicture pointer as the right type. (flutter/engine#51991) 2024-04-09 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Dart SDK from 7a5e410f982e to db99af14c4bc (1 revision) (flutter#51986)" (flutter/engine#51989) 2024-04-09 [email protected] Move the Dart SDK to //flutter/third_party/dart (flutter/engine#51917) 2024-04-09 [email protected] [macOS] Implement hit testing and handle platform view cursor changes (flutter/engine#43101) 2024-04-09 [email protected] Roll Dart SDK from 7a5e410f982e to db99af14c4bc (1 revision) (flutter/engine#51986) 2024-04-09 [email protected] Roll Dart SDK from 78174b41ab0f to 7a5e410f982e (1 revision) (flutter/engine#51980) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

knopp requested review from cbracken and chinmaygarde as code owners June 22, 2023 18:12

github-actions bot added the platform-macos label Jun 22, 2023

knopp force-pushed the macos_hit_testing branch 3 times, most recently from f6ab057 to 8c76f8b Compare June 23, 2023 06:56

knopp changed the title ~~WIP: macOS: Implement hit testing and handle platform view cursor changes~~ WIP: [macOS] Implement hit testing and handle platform view cursor changes Jun 25, 2023

knopp force-pushed the macos_hit_testing branch 4 times, most recently from cd17508 to 6b81cd5 Compare July 2, 2023 08:38

chinmaygarde changed the title ~~WIP: [macOS] Implement hit testing and handle platform view cursor changes~~ [macOS] Implement hit testing and handle platform view cursor changes Jul 6, 2023

knopp force-pushed the macos_hit_testing branch from 6b81cd5 to 0443432 Compare July 9, 2023 11:49

chinmaygarde removed their request for review July 13, 2023 20:04

cbracken requested a review from cyanglaz July 18, 2023 23:28

cbracken reviewed Jul 25, 2023

View reviewed changes

knopp force-pushed the macos_hit_testing branch from 0443432 to 318bb0f Compare July 26, 2023 11:23

github-actions bot added the affects: desktop label Jul 26, 2023

knopp force-pushed the macos_hit_testing branch from 5f86374 to 8c2304d Compare September 1, 2023 17:37

knopp force-pushed the macos_hit_testing branch from 8c2304d to 6bc5add Compare September 28, 2023 21:45

gspencergoog marked this pull request as draft November 2, 2023 18:14

cyanglaz removed their request for review March 19, 2024 21:42

knopp force-pushed the macos_hit_testing branch from 6bc5add to eb819bd Compare April 7, 2024 19:05

macOS: Implement hit testing and handle platform view cursor changes

1114a10

cbracken reviewed Apr 8, 2024

View reviewed changes

cbracken approved these changes Apr 8, 2024

View reviewed changes

knopp added 2 commits April 9, 2024 10:14

Use std::copy instead

8a3e069

Add null checks

9256b1b

knopp merged commit 6280295 into flutter:main Apr 9, 2024

knopp deleted the macos_hit_testing branch April 9, 2024 10:00

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Flutter Engine from 5a824e22deb2 to 6280295af387 (3 revisions) flutter/flutter#146497

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

6280295af [macOS] Implement hit testing and handle platform view curs…

17913e3

…or changes (flutter/engine#43101)

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Flutter Engine from 5a824e22deb2 to 6280295af387 (3 revisions) flutter/flutter#146500

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

6280295af [macOS] Implement hit testing and handle platform view curs…

21a28c7

…or changes (flutter/engine#43101)

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Flutter Engine from 5a824e22deb2 to 9b1fdcb83f2f (4 revisions) flutter/flutter#146502

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

6280295af [macOS] Implement hit testing and handle platform view curs…

7f6d4b2

…or changes (flutter/engine#43101)

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Flutter Engine from 5a824e22deb2 to 9b1fdcb83f2f (4 revisions) flutter/flutter#146510

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

6280295af [macOS] Implement hit testing and handle platform view curs…

90909f0

…or changes (flutter/engine#43101)

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Flutter Engine from 5a824e22deb2 to bd171e73a302 (5 revisions) flutter/flutter#146514

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

6280295af [macOS] Implement hit testing and handle platform view curs…

6dfbb18

…or changes (flutter/engine#43101)

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Flutter Engine from 5a824e22deb2 to efd85b738aa8 (6 revisions) flutter/flutter#146519

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

6280295af [macOS] Implement hit testing and handle platform view curs…

35db413

…or changes (flutter/engine#43101)

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Flutter Engine from 5a824e22deb2 to a17f8d18f3a6 (8 revisions) flutter/flutter#146521

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

6280295af [macOS] Implement hit testing and handle platform view curs…

51833e5

…or changes (flutter/engine#43101)

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Flutter Engine from 5a824e22deb2 to b735b76bf376 (9 revisions) flutter/flutter#146522

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

6280295af [macOS] Implement hit testing and handle platform view curs…

e9e27df

…or changes (flutter/engine#43101)

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Flutter Engine from 5a824e22deb2 to 5dca50d3e268 (10 revisions) flutter/flutter#146524

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

6280295af [macOS] Implement hit testing and handle platform view curs…

b572817

…or changes (flutter/engine#43101)

cbracken mentioned this pull request Apr 10, 2024

[macOS][platform_view] Click event was not dispatched to PlatformView flutter/flutter#124492

Closed

knopp mentioned this pull request Apr 16, 2024

[macOS] FlutterView should not override platform view cursor #52159

Merged

8 tasks

bufffun mentioned this pull request Jun 3, 2024

[macOS] Fix FlutterView cursor hit testing #53171

Merged

8 tasks

[macOS] Implement hit testing and handle platform view cursor changes #43101

[macOS] Implement hit testing and handle platform view cursor changes #43101

Uh oh!

Conversation

knopp commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes to FlutterMouseCursorPlugin

Preventing PlatformView from changing cursor when it is obscured by Flutter Content.

Actual hit testing

Example of hit testing

Pre-launch Checklist

Uh oh!

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

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbracken Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

knopp Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

cbracken Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Aug 15, 2023

Uh oh!

gspencergoog commented Sep 14, 2023

Uh oh!

cbracken commented Sep 14, 2023

Uh oh!

gspencergoog commented Nov 2, 2023

Uh oh!

goderbauer commented Jan 17, 2024

Uh oh!

cbracken Apr 8, 2024 • 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!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

knopp commented Jun 22, 2023 •

edited

Loading

Changes to `FlutterMouseCursorPlugin`

chinmaygarde commented Jul 13, 2023 •

edited

Loading

cbracken Apr 8, 2024 •

edited

Loading