Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[webview_flutter] Add new entrypoint that uses hybrid composition on Android #2883

Merged
merged 12 commits into from
Sep 21, 2020

Conversation

bparrishMines
Copy link
Contributor

@bparrishMines bparrishMines commented Jul 17, 2020

Related Issues

Fixes flutter/flutter#61133

Tests on stable will fail until hybrid views support moves to stable

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@bparrishMines bparrishMines requested a review from amirh as a code owner July 17, 2020 03:38
@bparrishMines bparrishMines changed the title [webview_flutter] Add WebView entrypoint [webview_flutter] Add new entrypoint that uses hybrid composition on Android Jul 17, 2020
@bparrishMines bparrishMines requested a review from blasten July 17, 2020 20:56
/// This flag is temporary and is highly subject to removal. One should only
/// use this if they are willing to test [WebView] features that aren't
/// available in the standard version and acknowledges that it is possible
/// this can have slower performance and/or unpredictable bugs.
Copy link

Choose a reason for hiding this comment

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

I don't think it has slower performance necessarily. It depends on what is being measured and how relevant that is.

For example, this mode makes the webview itself much faster. The Flutter UI might be slower, but only on devices below Android 10. Only while the webview is rendered, and Flutter is producing a ton of frames. e.g. animations. That's really it. For the most part, these are pretty good trade-off for webview.

I'd change this description to point out that it's an experimental feature that appends the webview to the Android view hierarchy for better performance and compatibility.

That's it for now. Let me know what you think.

Copy link

Choose a reason for hiding this comment

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

Also, re: unpredictable bug. This is true for any new features, and it's definitely true for the current webview, which is the motivation behind this feature anyways. I think I will rephrase it as "this mode fixes many issues while interacting with the Android webview. For more, see flutter/flutter#61133"

/// this can have slower performance and/or unpredictable bugs.
///
/// Defaults to false.
final bool useExperimentalAndroidSurfaceView;
Copy link

Choose a reason for hiding this comment

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

What do you think about a separate .dart file that has this feature on by default? This way, we have a BUILD target that automatically adds the metadata to the Android Manifest.

@mehmetf do you have any preference about how to add this feature?

Copy link
Contributor Author

@bparrishMines bparrishMines Jul 20, 2020

Choose a reason for hiding this comment

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

I tried this out, but this file has a lot of private methods/constructors that needed to be copy pasted to the new file. I went with a different approach and decided just create to a new WebViewPlatform implementation that extends AndroidWebView. So, now all a user needs to do is: WebView.platform = SurfaceAndroidWebView(). What are your thoughts on that approach?

@pichillilorenzo
Copy link

When hybrid views support will move to stable? Thanks!

@blasten
Copy link

blasten commented Aug 5, 2020

@pichillilorenzo It's on stable now

@blasten
Copy link

blasten commented Aug 6, 2020

FYI, the documentation is available on https://github.com/flutter/flutter/wiki/Hybrid-Composition

@blasten
Copy link

blasten commented Aug 6, 2020

This PR is waiting for dynamic thread merging. FYI @cyanglaz

@Doflatango
Copy link

@pichillilorenzo It's on stable now

So this PR has been released in 1.20.0?

Comment on lines 5 to 16
`android/app/src/main/AndroidManifest.xml`:
```xml
<application>
.
.
<meta-data
android:name="io.flutter.embedded_views_preview"
android:value="true" />
.
.
</application>
```
Copy link

Choose a reason for hiding this comment

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

This won't be needed in 1.22.

@blasten
Copy link

blasten commented Sep 1, 2020

@amirh would you mind reviewing this PR as well?

Here's what I had in mind so far:

  1. Have this PR ready, and merge it when the 1.22 version comes out.
  2. Decide if a separate entrypoint or feature flag is preferred. I don't have strong preference, but Maurice said that a flag is easier to implement.
  3. Follow up on each issue fixed by this PR.

Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

Code overall looks good, left a few nits mostly about documentation

@@ -64,6 +67,60 @@ enum NavigationDecision {
navigate,
}

/// Android [WebViewPlatform] that uses [AndroidViewSurface] to build the [WebView] widget.
///
/// To use this, set [WebView.platform] to an instance of this class.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add some guidance on when should one use it and what are the costs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. I added it below. @blasten What are your thoughts on the explanation below?

/// Android [WebViewPlatform] that uses [AndroidViewSurface] to build the [WebView] widget.
///
/// To use this, set [WebView.platform] to an instance of this class.
class SurfaceAndroidWebView extends AndroidWebView {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the AndroidManifest change required? if so we should probably mention it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is no longer required, so I removed it from the changelog and example

@fddsocool
Copy link

This change worked well for my problem. When will the webview_flutter be updated? Thanks!

/// This implementation uses Hybrid Composition to render the [WebView] on
/// Android. It solves multiple issues related to accessibility and interaction
/// with the [WebView] at the cost of some performance on Android versions below
/// 10. See https://github.com/flutter/flutter/issues/61133 for more
Copy link

Choose a reason for hiding this comment

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

///
/// To use this, set [WebView.platform] to an instance of this class.
///
/// This implementation uses Hybrid Composition to render the [WebView] on
Copy link

Choose a reason for hiding this comment

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

Hybrid Composition -> hybrid composition

Comment on lines 3 to 4
* Add support for building `WebView` widget with Android hybrid views. To use this feature, set
`WebView.platform` to an instance of `SurfaceAndroidWebView`.
Copy link

Choose a reason for hiding this comment

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

What about this (note the link to hybrid composition and example):

  • Add support for building WebView widget with Android hybrid views.
    To use this feature, set WebView.platform to an instance of SurfaceAndroidWebView. For example:

    class WebViewExample extends StatefulWidget {
      @override
      void initState() {
        super.initState();
        WebView.platform = SurfaceAndroidWebView();
      }
    
      @override
      Widget build(BuildContext context) {
        return WebView(
          initialUrl: 'https://flutter.dev',
        );
      }
    }

Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

LGTM + nits

@bparrishMines bparrishMines merged commit 0d7a605 into flutter:master Sep 21, 2020
@bparrishMines bparrishMines deleted the webview_entrypoint branch September 21, 2020 20:54
@bparrishMines bparrishMines restored the webview_entrypoint branch September 21, 2020 20:59
bparrishMines added a commit that referenced this pull request Sep 21, 2020
bparrishMines added a commit that referenced this pull request Sep 21, 2020
@blasten
Copy link

blasten commented Sep 21, 2020

Offline chat: let's revert this PR and bump the Flutter SDK in pubspec.yaml to 1.22.

@bparrishMines
Copy link
Contributor Author

Moved to #3067

danielroek pushed a commit to Baseflow/flutter-plugins that referenced this pull request Oct 1, 2020
danielroek pushed a commit to Baseflow/flutter-plugins that referenced this pull request Oct 1, 2020
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[webview_flutter] Add new entrypoint that uses hybrid composition on Android
7 participants