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

[webview_flutter] Added 'allowsInlineMediaPlayback' property #2227

Closed
wants to merge 5 commits into from
Closed

[webview_flutter] Added 'allowsInlineMediaPlayback' property #2227

wants to merge 5 commits into from

Conversation

sarbagyastha
Copy link
Contributor

@sarbagyastha sarbagyastha commented Oct 22, 2019

Description

The PR includes following changes:

  • adds allowsInlineMediaPlayback property to WebView to support inline media playback in iOS.
  • switched to using collection literals for Set.

Related Issues

#25630
#42099

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.

@vlowe85
Copy link

vlowe85 commented Nov 12, 2019

Nice, waiting for this too

@alexmarkley
Copy link

Hey, I just created a (much simpler) PR to address this issue as well: #2309

Personally I would like to see your approach get merged because it gives the developer more control over the widget from Flutter code.

Out of curiosity, may I ask what is holding up this PR?

Copy link
Contributor

@ened ened left a comment

Choose a reason for hiding this comment

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

Hi @sarbagyastha thank you for the PR! I think this may be the way to go.

There is only 1 comment on the e2e test, really. Even a property is no-op, we should still somewhat test it to be sure nothing breaks in the future.

The PR also changes some of the Set syntax. It has to be done at some point, but we should mention that in the change log, along with the increment of the Dart version constraints.
Could you please address those items & rebase to latest master? Thank you!

@@ -456,6 +457,69 @@ void main() {
isPaused = await controller.evaluateJavascript('isPaused();');
expect(isPaused, _webviewBool(false));
});

test('Video plays inline when allowsInlineMediaPlayback is true', () async {
if (Platform.isIOS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should run on all platforms (so that we can be sure future plugin / platform versions don't break).

Can you clearly observe a different behavior on Android? If so, perhaps this could be a separate else branch to verify that.

@blasten
Copy link

blasten commented Dec 15, 2020

Thanks for the PR! We have migrated this plugin to null safe, would you be able to rebase and re-apply your patch?

@mehmetf
Copy link
Contributor

mehmetf commented Dec 16, 2020

My git-fu isn't very strong but given that this now says "unknown repository" on top, I assume the fork (and likely branch) was deleted. I will rebase this and open a new PR.

@mehmetf mehmetf closed this Dec 16, 2020
@mehmetf
Copy link
Contributor

mehmetf commented Dec 16, 2020

Moved over to: #3334

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.

7 participants