Skip to content

[pointer_interceptor] Update app-facing package to accommodate new federated structure #5501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

LouiseHsu
Copy link
Contributor

Addresses flutter/flutter#30143 by adding an iOS implementation

This PR is Part 3 of #5233

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 [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
  • 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.


You can use this widget in a cross-platform app freely. In mobile, where the issue that this plugin fixes does not exist, the widget acts as a pass-through of its `children`, without adding anything to the render tree.
`PointerInterceptor` is a widget that prevents mouse events from being captured by an underlying [`HtmlElementView`](https://api.flutter.dev/flutter/widgets/HtmlElementView-class.html) in web, or an underlying [`PlatformView`](https://api.flutter.dev/flutter/widgets/PlatformViewLink-class.html) on iOS.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`PointerInterceptor` is a widget that prevents mouse events from being captured by an underlying [`HtmlElementView`](https://api.flutter.dev/flutter/widgets/HtmlElementView-class.html) in web, or an underlying [`PlatformView`](https://api.flutter.dev/flutter/widgets/PlatformViewLink-class.html) on iOS.
`PointerInterceptor` is a widget that prevents pointer events from being captured by an underlying [`HtmlElementView`](https://api.flutter.dev/flutter/widgets/HtmlElementView-class.html) in web, or an underlying [`PlatformView`](https://api.flutter.dev/flutter/widgets/PlatformViewLink-class.html) on iOS.


## What is the problem?

When overlaying Flutter widgets on top of `HtmlElementView` widgets that respond to mouse gestures (handle clicks, for example), the clicks will be consumed by the `HtmlElementView`, and not relayed to Flutter.
When overlaying Flutter widgets on top of `HtmlElementView`/`PlatformView` widgets that respond to mouse gestures (handle clicks, for example), the clicks will be consumed by the `HtmlElementView`/`PlatformView`, and not relayed to Flutter.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When overlaying Flutter widgets on top of `HtmlElementView`/`PlatformView` widgets that respond to mouse gestures (handle clicks, for example), the clicks will be consumed by the `HtmlElementView`/`PlatformView`, and not relayed to Flutter.
When overlaying Flutter widgets on top of `HtmlElementView`/`PlatformView` widgets that respond to pointer gestures (handle clicks, for example), the clicks will be consumed by the `HtmlElementView`/`PlatformView`, and not relayed to Flutter.

Comment on lines +12 to +14
// TODO(louisehsu): given the difficulty of making the same integration tests
// work for both web and ios implementations, please find tests in their respective
// platform implementation packages.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are not going to be tests here, I'd fully remove integration_test from the example app. I don't think it'll annoy the CI too much.

You already have the tests_exist_elsewhere_test.dart in the main directory, so as long as you point people to the implementation-specific tests from there, I think you can remove the ones here altogether!

Comment on lines +53 to +56
ui_web.platformViewRegistry.registerViewFactory(
_htmlElementViewType,
(int viewId) => htmlElement,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We normally recommend the registerViewFactory call to not be in the build function. Any chance this can be moved elsewhere earlier in the file? (Outside of the NativeWidget class, maybe?)

@@ -9,8 +9,7 @@ import 'package:flutter_test/flutter_test.dart';
void main() {
test('Tell the user where to find the real tests', () {
print('---');
print('This package uses integration_test for its tests.');
print('See `example/README.md` for more info.');
print('Please find platform tests in their respective packages.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -4,4 +4,5 @@

library pointer_interceptor;

export 'src/mobile.dart' if (dart.library.html) 'src/web.dart';
export 'package:pointer_interceptor/src/pointer_interceptor.dart';
export 'package:pointer_interceptor_platform_interface/pointer_interceptor_platform_interface.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this needs to re-export anything from the platform_interface package, right?

@stuartmorgan-g
Copy link
Contributor

This was obsoleted by #5640; closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants