Skip to content

Reader: Support Stories in detail view - #14965

Merged
aforcier merged 2 commits into
developfrom
feature/stories-reader-detail
Oct 1, 2020
Merged

Reader: Support Stories in detail view#14965
aforcier merged 2 commits into
developfrom
feature/stories-reader-detail

Conversation

@aforcier

@aforcier aforcier commented Sep 22, 2020

Copy link
Copy Markdown
Contributor

Tweaks the Reader to support the Story block in detail view:

Simulator Screen Shot - iPhone 11 Pro - 2020-09-22 at 14 31 32

The bulk of the work for this has been done in the shared CSS file in Calypso and already deployed: Automattic/wp-calypso#45397

Extra changes that were needed for WPiOS:

Image a href tag wrapping

The in-app custom CSS wraps all images with a link tag, which breaks the Story markup (and Stories have their own link set anyway). I added an exception for Stories in f7667f5.

(Incidentally the existing behavior also wraps images that already had custom links of their own, so the original link can no longer be visited -not sure if this was intentional, just noting it here.)

Story link handling

I had to modify the logic when tapping a Reader detail link. The idea is for tapping a story to open the WebView so the story can be loaded in fullscreen and autoplay. The link is to the post itself, with extra query parameters, like this:

https://site.wordpress.com/2020/09/02/a-story-post/?wp-story-load-in-fullscreen=true&wp-story-play-on-load=true

This worked correctly for Jetpack sites, however for WordPress.com sites the ReaderDetailCoordinator interprets the link as a WordPress.com post and opens it in a new detail screen. This looks like the same post being loaded again and again.

I added an exception for stories in 242781b, by filtering for a query param containing wp-story.

Android PR: wordpress-mobile/WordPress-Android#13002

To test:

  1. Proxy WP.com on your device (because the Story block is currently a beta block and poorly supported by the API)
  2. Start from a clean install of turn off the CSS cache to make sure you get a fresh copy of reader-mobile.css
  3. Follow a Jetpack and a WP.com site with Stories (DM me for some samples)
  4. Check that opening a story in Reader detail shows a properly formatted story, like the screenshot above
  5. Jetpack: Check that tapping the story opens it in the WebView, and it automatically plays once the page loads
  6. WP.com: Check that tapping the story opens it in the WebView (autoplay/fullscreen won't work at the moment due to an unrelated issue with Stories on WP.com)

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.

@aforcier aforcier added this to the 15.9 milestone Sep 22, 2020
@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.

@leandroalonso leandroalonso left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@aforcier it works like described!

Just a question though:

  1. The story doesn't open in fullscreen, is that expected?
  2. The video doesn't seem to play for me: https://d.pr/v/1YronD
  3. When I manually hit play, it shows as a default video (with OS controls). This is the default behavior of the web view, so there isn't much we can do here AFAIK. Just double-checking that you're aware of it.

view?.scroll(to: hash)
} else if url.pathExtension.contains("gif") || url.pathExtension.contains("jpg") || url.pathExtension.contains("jpeg") || url.pathExtension.contains("png") {
presentImage(url)
} else if url.query?.contains("wp-story") ?? false {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't we want to open that in fullscreen?

@leandroalonso

Copy link
Copy Markdown
Contributor

(Incidentally the existing behavior also wraps images that already had custom links of their own, so the original link can no longer be visited -not sure if this was intentional, just noting it here.)

This is def a bug. Would you mind opening an issue for it?

@aforcier

aforcier commented Sep 29, 2020

Copy link
Copy Markdown
Contributor Author

Thanks @leandroalonso!

  1. The story doesn't open in fullscreen, is that expected?

It's expected in the sense that I'd like it to open fullscreen but don't see a way to 😀. Visiting a story from iOS Safari directly also doesn't fully fullscreen and hide the controls so I thought this is as far as fullscreen goes.

Do you know of a way to fullscreen, either in the web view or in Safari itself (though I think it's not possible in Safari)?

  1. The video doesn't seem to play for me: https://d.pr/v/1YronD

Great find! I guess I hadn't tried a video on iOS. When I inspect the window I see an error in the console:

Unhandled Promise Rejection: NotAllowedError: The request is not allowed by the user agent or the platform in the current context, possibly because the user denied permission.

From some digging around it seems like autoplay for videos doesn't work for Safari because a user action is required to play audio by default.

I think there are three possible fixes here:

  1. Just remove the wp-story-play-on-load=true argument from the link before opening
  2. On the web end, catch that error and stop autoplay
  3. On the web end, catch that error and set the video to muted

I'll check in with @Tug and follow up on this one, might be a case of going with 1 for now and making the improvement from 2 or 3 later.

  1. When I manually hit play, it shows as a default video (with OS controls). This is the default behavior of the web view, so there isn't much we can do here AFAIK. Just double-checking that you're aware of it.

Hmmm. Seems specific to the web view as this works correctly on Safari. Is there a way to open Safari instead of a webview instead? (This is what we're also doing on Android but for different reasons.)

@aforcier aforcier self-assigned this Sep 29, 2020
@aforcier

Copy link
Copy Markdown
Contributor Author

This is def a bug. Would you mind opening an issue for it?

No problem! #15009

@leandroalonso

Copy link
Copy Markdown
Contributor

Do you know of a way to fullscreen, either in the web view or in Safari itself (though I think it's not possible in Safari)?

We have 2 options:

  1. Create a custom VC that contains a WKWebView and show it in fullscreen — but this comes with the issues, like the video one. I know WKWebView has some configurations and tricks, perhaps this might solve this issue and will play the video inline: https://stackoverflow.com/questions/43885705/how-to-play-video-inline-with-wkwebview
  2. Use the SFSafariViewController. This will be a way to open Safari instead of a WebView.

I guess we can try the playsinline=1 param and see if that's work. If not, then we can move to SFSafariViewController. Anyway, those changes might be addressed in another PR — up to you.

No problem! #15009

Thank you!

@aforcier

aforcier commented Oct 1, 2020

Copy link
Copy Markdown
Contributor Author

Thanks @leandroalonso! Still looking into this, I wrote up all the issues and options in #15026.

This PR is still good enough as a first iteration to actually get stories working in the Reader, so I'll merge it for now and follow up with improvements later.

@aforcier
aforcier merged commit 3ab10fd into develop Oct 1, 2020
@aforcier
aforcier deleted the feature/stories-reader-detail branch October 1, 2020 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants