Skip to content
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

[TIMOB-24581] Android: Support WebView.setMediaPlaybackRequiresUserGesture #8967

Closed
wants to merge 5 commits into from

Conversation

bduyng
Copy link
Contributor

@bduyng bduyng commented Apr 16, 2017

  • Implement Titanium.UI.WebView.mediaPlaybackRequiredUserGesture property
TEST CASE
var win = Ti.UI.createWindow({backgroundColor: 'gray'}),
    webView = Ti.UI.createWebView({
        mediaPlaybackRequiresUserGesture: false, // on iOS with `true` we need `controls` parameter in `video` tag so user can play the video
        html: "<video autoplay playsinline style=\"width:100%;\"><source src=\"https://www.w3schools.com/html/mov_bbb.mp4\">"
});

win.add(webView);
win.open();

JIRA Ticket

@hansemannn
Copy link
Collaborator

Pretty sure this does work without the kroll method annotation, did you test this before? Also missing docs and test-case.

if (Build.VERSION.SDK_INT >= 17) {
getWebView().getSettings().setMediaPlaybackRequiresUserGesture(enabled);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This causes an "opcode" warning in the Android log on API Level 16 and lower. While harmless, we don't want to add any more noise to the log. Doing the following would be better...
`

public void setMediaPlaybackRequiresUserGesture(boolean enabled)
{
	if (Build.VERSION.SDK_INT >= 17) {
		TiUIWebView webView = getWebView();
		if (webView != null) {
			ApiLevel17.setMediaPlaybackRequiresUserGesture(webView.getSettings(), enabled);
		}
	}
}

private static class ApiLevel17
{
	private ApiLevel17() {}

	public static boolean setMediaPlaybackRequiresUserGesture(
		android.webkit.WebSettings settings, boolean enabled)
	{
		if (settings != null) {
			settings.setMediaPlaybackRequiresUserGesture(enabled);
		}
	}
}

`

The above solution is based on Google's blog post "How to have your (Cup)cake and eat it too". It takes advantage of Java's lazy class loading to avoid the opcode warning. Google's Android Supports library does something very similar to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, ideally, this should be a property (getter/setter) in JavaScript and not a method. Having a setter without a getter is not good design (but a getter-only API is okay).

@bduyng
Copy link
Contributor Author

bduyng commented May 25, 2017

@hansemannn @jquick-axway I just did an update that define mediaPlaybackRequiresUserGesture as a property of TiWebView and included docs. I also add here a test-case. Please check

var win = Ti.UI.createWindow({
  backgroundColor: "#fff"
});

var webView = Ti.UI.createWebView({
  mediaPlaybackRequiresUserGesture: false,
  html: "<video autoplay playsinline style=\"width:100%;\"><source src=\"https://www.w3schools.com/html/mov_bbb.mp4\">"
});

win.add(webView);
win.open();

@garymathews garymathews changed the title [AC-4905] Android: WebView: Support setMediaPlaybackRequiresUserGesture setting [TIMOB-24581] Android: Support WebView.setMediaPlaybackRequiresUserGesture Aug 14, 2017
Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

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

Cleaned PR

CR: PASS

👍

@ssjsamir ssjsamir self-requested a review August 14, 2017 20:57
@ssjsamir
Copy link
Contributor

@bduyng @garymathews The test case is a bit unclear to me what should I be looking out for. I looked at the documentation as well but I am still confused on how to test this.

@lokeshchdhry
Copy link
Contributor

@bduyng , @garymathews - when I have the property to false & I do a tap, swipe etc in the webview the video does not play. What's it supposed to do ?

@jquick-axway
Copy link
Contributor

@lokeshchdhry, @ssjsamir,

This is what I understand... and I could be wrong...

On Android, a WebView cannot play an embedded video via the JavaScript in the HTML while the setMediaPlaybackRequiresUserGesture() is true until the end-user taps on the video. The idea is the end-user must opt-in to the download of the video due to data usage concerns over a cellular network. This setting is true by default.

So, setting this to false will allow the HTML's JavaScript to auto-play the video without needing the user to tap the video.

@hansemannn hansemannn added this to the 7.0.0 milestone Aug 30, 2017
@bduyng
Copy link
Contributor Author

bduyng commented Sep 27, 2017

@hansemannn can I know the reason why this PR still not merge in 6.2.2.GA? are there any things that I can do here to have it in 6.3.0 on the end of the month? Thanks!!

@hansemannn
Copy link
Collaborator

hansemannn commented Sep 27, 2017

@bduyng The QE-testing was not completed and then overwhelmed by the other releases (Android O, iOS 11). Also it does not have parity with iOS so far, which we are trying to focus on more before adding anything platform-specific. Isn't it something like "autoplay"? If so, we could probably do an iOS / Windows parity easily and get it done. Sorry for the inconvenience!

EDIT: There is mediaPlaybackRequiresUserAction:

A Boolean value that determines whether HTML5 videos can play automatically or require the user to start playing them.

This is exactly what we need if I'm correct?

@bduyng
Copy link
Contributor Author

bduyng commented Sep 27, 2017

@hansemannn you mean it can be done quickly if we support for both iOS and Android? I think I can add mediaPlaybackRequiresUserAction for iOS easily

@build
Copy link
Contributor

build commented Sep 27, 2017

Warnings
⚠️

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

@bduyng
Copy link
Contributor Author

bduyng commented Sep 27, 2017

@hansemannn I updated the code to support mediaPlaybackRequiresUserGesture for iOS. please check

Copy link
Collaborator

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

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

Please fix the docs, then it should be good to go.

summary: Sets whether the WebView requires a user gesture to play media.
type: Boolean
platforms: [android, iphone, ipad]
since: "6.2.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use "7.0.0" instead. 6.2.0 is already shipped, 6.3.0 is already in RC next week + iOS 11 focussed and the next one after that is 7.0.0.

@lokeshchdhry
Copy link
Contributor

This does not work on android. After the app launches & clicking the webview nothing happens.
On IOS it works fine.
@bduyng , If I am missing something please let me know.

@hansemannn hansemannn removed this from the 7.0.0 milestone Nov 20, 2017
@hansemannn hansemannn added this to the 7.1.0 milestone Nov 20, 2017
@hansemannn hansemannn modified the milestones: 7.1.0, 7.2.0 Feb 26, 2018
@richardkalunda
Copy link

Upgrade and support for me the new version features

@sgtcoolguy sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 2018
@hansemannn
Copy link
Collaborator

@bduyng @garymathews Are you able to address the QE comments? I will need to reschedule it for now.

@hansemannn hansemannn modified the milestones: 7.3.0, 7.4.0 May 18, 2018
@hansemannn
Copy link
Collaborator

Closing pull request due to inactivity for now. @bduyng If you are able to address the review comments, we are happy to re-schedule it again!

@hansemannn hansemannn removed this from the 7.4.0 milestone Jun 22, 2018
@hansemannn hansemannn closed this Jun 22, 2018
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.

None yet

9 participants