Skip to content

WebView: Configure WKWebView to support inline media playback on iPhones - #15029

Merged
aerych merged 1 commit into
developfrom
try/play-video-inline
Oct 5, 2020
Merged

WebView: Configure WKWebView to support inline media playback on iPhones#15029
aerych merged 1 commit into
developfrom
try/play-video-inline

Conversation

@aerych

@aerych aerych commented Oct 1, 2020

Copy link
Copy Markdown
Contributor

Refs #15026

This PR adds a WKWebViewConfiguration to the WebKitViewController. This allows web-based videos to be played inline on the iPhone if they are coded to do so. This is the default behavior on the iPad, and desired for WPStories regardless of the platform on which they are viewed.

With this change video tags that define the playsinline attribute will now play inline on the iPhone and iPad, while video tags that do not specify this attribute will continue to play full screen in the native player.

See comments below and the referenced issue for more details.

@aforcier, @leandroalonso would either of you care to review? 😁

To test:
View a WPStory with a video slide. Confirm the video plays inline vs opening the native viewer.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile

Copy link
Copy Markdown

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile

Copy link
Copy Markdown

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@aforcier

aforcier commented Oct 2, 2020

Copy link
Copy Markdown
Contributor

I gave this a try, it seems to be working great! 🎉

I checked that this works both from the Reader and when viewing a post with a story from the post list (in that case there's no autoplay, the post opens and you tap the story to open it). The inline playing works great.

I think this PR coupled with a fix for broken autoplay for videos with sound makes for a pretty solid v1 for story playback from WPiOS (leaving just fullscreen support from #15026, which is not critical).

Tug actually just added a fix for autoplay in Automattic/jetpack#16852 🎉 You can test this with this feature by following my sandbox site (I'll share the link with you). Note that there's currently an issue with unmuting, which we can hopefully get fixed soon.

@aerych

aerych commented Oct 2, 2020

Copy link
Copy Markdown
Contributor Author

Cool. :)
So the question I'd pose for general feedback is would we prefer to limit inline playback stories on iPhones, or support inline playback across the board.
On the one hand, stories are a special case and we're pretty sure they will be a good size already. Other videos might be rather small to play inline on the iPhone (depending on the phone).
On the other hand, the inconsistent video playing behavior in the reader could lead to some confusion about the experience.

Particularly interested to hear from @leandroalonso (who has some background already) and @emilylaguna @yaelirub and @mbshakti who have all been more invested in the reader side of things lately.

Depending on the consensus we can expand this proof of concept for special story handling or not.

@leandroalonso

Copy link
Copy Markdown
Contributor

@aerych the Reader uses it's own WKWebView, so it doesn't affect it. :)

I played with that when I was implementing the webView and I wasn't able to make the allowsInlineMediaPlayback to work. Tried again, still no lucky. You can try by change configureWebView method in ReaderDetailViewController:

    private func configureWebView() {
        webView.navigationDelegate = self
        webView.configuration.allowsInlineMediaPlayback = true
    }

I tried with both VideoPress and YouTube and when you tap play it goes to fullscreen. I wonder that maybe this is due to the content of the webview being loaded locally (using loadHTMLString).

@aerych

aerych commented Oct 2, 2020

Copy link
Copy Markdown
Contributor Author

the Reader uses it's own WKWebView, so it doesn't affect it

I should have guessed :)

I wonder that maybe this is due to the content of the webview being loaded locally

WKWebView configuration can't be modified after the WKWebView is created. Guessing that might be the issue? Per the docs:

WKWebViewConfiguration is only used when a web view is first initialized. You cannot use this class to change the web view's configuration after it has been created.

If the reader is creating the WKWebView via a Storyboard (I haven't looked to see) we'd need to change that to be instantiated programmatically -- assuming we want the behavior to be consistent everywhere. (Honestly not sure that we do.)

@leandroalonso

Copy link
Copy Markdown
Contributor

Hmmm, that might be the issue then! I'll check again, thanks for the heads up!

@leandroalonso

Copy link
Copy Markdown
Contributor

@aerych I changed in the Storyboard (there's an inline playback option) and the behavior is the same: https://d.pr/v/9Mh5vw

I wonder if maybe that's how Youtube/VideoPress behaves?

@aerych

aerych commented Oct 2, 2020

Copy link
Copy Markdown
Contributor Author

I wonder if maybe that's how Youtube/VideoPress behaves?

I think it depends on if the video specifies the "playsinline" attribute.

I made a test post with some video tags and some youtube embeds. Youtube doesn't want to play for me (I guess that's what I get for trying to rickroll 😆) but the video from the media library does.
The "clean" video tag displays full screen even after setting the flag on the web view (thanks for that tip!). Same with the video that sets playsinline as part of the URL query string. However, the video that specified playsinline as part of its html parameters does indeed play inline as expected.

And now that I think about it, maybe my original question about setting the WKWebView configuration is moot. The video markup has to specify if it can play inline or not so the experience will be inconsistent regardless. Thoughts?

@leandroalonso

Copy link
Copy Markdown
Contributor

@aerych Agree!

Anyway, I'd keep in the WebKitViewController. If the video markup specify that it should be played inline, we play it inline. :)

@aerych

aerych commented Oct 2, 2020

Copy link
Copy Markdown
Contributor Author

Sounds like a plan. :)

@aerych

aerych commented Oct 2, 2020

Copy link
Copy Markdown
Contributor Author

@aforcier how would you like to proceed? Shall we close this one in favor of one that includes some of the other changes you mentioned?

@aforcier

aforcier commented Oct 2, 2020

Copy link
Copy Markdown
Contributor

@aforcier how would you like to proceed? Shall we close this one in favor of one that includes some of the other changes you mentioned?

Just making sure I understand the outcome of the conversation:

  • This change doesn't affect the Reader detail view itself, which is a separate WebView (irrelevant for Stories but worth knowing)
  • allowsInlineMediaPlayback doesn't force inline playback, merely respects the playsinline attribute on the video tag if set (which it is for videos in a Story) - and we're okay with that

I think this PR coupled with a fix for broken autoplay for videos with sound makes for a pretty solid v1 for story playback from WPiOS (leaving just fullscreen support from #15026, which is not critical).

The autoplay fix is all taken care of server-side (@Tug just fixed the remaining issue I mentioned with the unmute button 🙇). That just leaves fullscreen support, which is a nice-to-have but not necessary for v1 (iOS Safari itself doesn't even support it as far as I can tell...).

So, I'd say this PR as-is is all we need, and can probably be un-drafted. WDYT @aerych?

Also thank you both @leandroalonso @aerych 🙇

@aerych

aerych commented Oct 3, 2020

Copy link
Copy Markdown
Contributor Author

@aforcier you have it exactly, and that sounds good to me. :)

@aerych aerych self-assigned this Oct 3, 2020
@aerych aerych added this to the 15.9 milestone Oct 3, 2020
@aerych
aerych marked this pull request as ready for review October 3, 2020 00:12
@aforcier

aforcier commented Oct 3, 2020

Copy link
Copy Markdown
Contributor

Looks good to me, and working well for opening Stories from the Reader and viewing story posts from the post list. :shipit:

@aerych
aerych merged commit 9552410 into develop Oct 5, 2020
@aerych

aerych commented Oct 5, 2020

Copy link
Copy Markdown
Contributor Author

Thanks all!

@aerych
aerych deleted the try/play-video-inline branch October 5, 2020 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants