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

Comment Detail: Render content in web view #17129

Merged
merged 29 commits into from
Sep 9, 2021

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Sep 7, 2021

Refs #17108, #17087, aXjNk0P-zpl

This adds the web view implementation to render the comment content. Some notes:

  • Fixed a little bug when fetching the user's avatar URL.
  • richCommentTemplate.html and richCommentStyle.css were added to stylize the contents according to the designs.
  • Navigation requests from the web view are temporarily disabled for now. i.e. links look clickable but will do nothing (for now). It'll be implemented in the next PR.
  • To handle content resize on orientation change, I ended up reloading the contents (i.e. calling loadHTMLString on the web view) again.
    • I've tried other methods (reload(), through javascript's location.reload(), re-querying scrollHeight, ...) to no avail. The resize only works by calling loadHTMLString again. Somewhat expensive, I know. 😅 But it's what works so far and I've yet to find a better way.
  • All resources within the comment content (images, video thumbnails) are loaded synchronously for now. For heavy comments (e.g. with >5 images), it may take a couple of seconds for the web view to fully render the content.
    • I'll take a look at loading the images asynchronously in the next PR. My assumption is it'll at least cause the table to recalculate the cell height after each image completes loading, but if the method proves too tricky, I'll defer the async improvement and instead show a loading animation on top of the web view for the much simpler approach.
  • Lastly, cell/component visibility based on user permission will be implemented in the upcoming PR.

To test

  • Ensure that the New Comment Detail flag is enabled.
  • Go to My Site > Comments, and pick any comment.
  • Verify that the comment content is displayed correctly (and beautifully ✨).
    • Note that you may need to jump to multiple sites and hunt for tricky comments that may break the layout or appear strange. If you find them, please let me know!
  • Try changing to landscape orientation, and verify that the content is resized properly.

Regression Notes

  1. Potential unintended areas of impact
    n/a. Feature is still hidden behind a flag.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    n/a. Feature is still hidden behind a flag.

  3. What automated tests I added (or what prevented me from doing so)
    n/a. Feature is still hidden behind a flag.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • 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

peril-wordpress-mobile bot commented Sep 7, 2021

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 7, 2021

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

Copy link
Member

@aerych aerych left a comment

Choose a reason for hiding this comment

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

Heya @dvdchr 👋
Looking sharp! I tested in in the iPhone Pro 11 simulator and the iPad Pro 9.7 simulator and things mostly looked good. I did spot a couple of things and left comments about them. Lemme know what you think.
Back to you sir!

@aerych
Copy link
Member

aerych commented Sep 7, 2021

For some light stress testing I created this comment on a test self-hosted site that includes (I think) all of the default allowed tags for comments, plus images and YouTube embeds (which are not allowed by default). The comment looks pretty hideous in a browser but it looks rather nice in the app. :)
Simulator Screen Shot - iPhone 11 Pro - 2021-09-07 at 18 11 51
Simulator Screen Shot - iPhone 11 Pro - 2021-09-07 at 18 11 55
Simulator Screen Shot - iPhone 11 Pro - 2021-09-07 at 18 12 04

I did notice there is some trouble with the the YouTube embed sizing properly. I'm not sure if that's because of the error screen or something else. Something we'll want to look into. Probably we'll have a period of finding lots of one-off quirky things (typical of this sort of change and to be expected) but we'll get 'em sorted and then we'll have some rock solid webview comments. :)

Additionally, the RowType enum is refactored; the .text case no longer provides a closure as associated value, in order to make the enum class conform to Equatable without manual implementation. Row actions are now implemented in the didSelectRow method instead.
* All elements now have max-width: 100vw to prevent content overflow, unless explicitly overridden (e.g. table). This should fix unexpected content overflows e.g. from YouTube embeds.
* Stylize img tags that aren't part of Gutenberg blocks.
The web view needs to be translucent initially, because otherwise the web view might show a short, white flash. This is noticeable in dark color schemes, and a tad annoying. On the other hand, a translucent webview causes scroll indicator to be invisible, regardless of the indicator style.

Therefore, the solution is to set the webview to translucent initially, and set it back to opaque once the content has finished loading.
@dvdchr
Copy link
Contributor Author

dvdchr commented Sep 8, 2021

Appreciate the testing effort! 👍 Thank you.

Probably we'll have a period of finding lots of one-off quirky things ...

Yeah... Also, considering how many blocks there are, I'm not sure if they would all look great, since the CSS is homegrown and thus, limited. 😆

Additionally, I've found that embedded tweets are causing problems with height calculation since it asynchronously fetches Twitter's widget.js library and resizes the tweet component. This is rather tricky since I've yet to find a way to block async Javascript without affecting the Javascript code that's used to calculate document height.

The content is still vertically scrollable, it still doesn't look good since the detail screen now has two vertically scrollable components. Although this only applies to comments with embedded Tweet blocks. I'll take a look at this in the next PR, and will address it if it's not taking up too much time.

The rough idea is instead of using evaluateJavascript, I should use WKUserContentController and move the height calculation there. Then, I should be able to hook into Twitter's widget library and listen after the tweet component has finished rendering. Finally, call the height calculation function so it could trigger the table view to resize the cell.

I did notice there is some trouble with the YouTube embed sizing properly.

Yeah, for this I figured it's better to restrict everything from the start, and allow elements to scale past 100vw explicitly (e.g. tables, custom emoji images):

* {
  max-width: 100vw;
}

Update: this is now addressed in 1337847.

@dvdchr dvdchr requested a review from aerych September 8, 2021 12:23
@dvdchr
Copy link
Contributor Author

dvdchr commented Sep 8, 2021

@aerych this is ready for a second round of review! 🙂

Copy link
Member

@aerych aerych left a comment

Choose a reason for hiding this comment

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

Heya @dvdchr 👋
Changes look good! Confirming portrait images and youtube embeds are sizing better. :) The styles are preserved when rotating now and sizes are updated.

Simulator Screen Shot - iPhone 11 Pro - 2021-09-08 at 13 41 17

I noticed if I got very aggressive with rotation I could get into a state where the content height was higher than the actual content. I think it would be pretty hard to do this with an actual device so not a blocker imho.
Simulator Screen Shot - iPhone 11 Pro - 2021-09-08 at 13 43 05

One thing we might add to a list of things to improve as we go is centering embedded content. What do you think?
Simulator Screen Shot - iPhone 11 Pro - 2021-09-08 at 13 41 56

Nice work sir. Let's :shipit: !

super.viewWillTransition(to: size, with: coordinator)

// when an orientation change is triggered, recalculate the content cell's height.
guard let contentRowIndex = rows.firstIndex(where: { $0 == .content }) else {
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

@dvdchr
Copy link
Contributor Author

dvdchr commented Sep 9, 2021

Thanks @aerych !

I noticed if I got very aggressive with rotation I could get into a state where the content height was higher than the actual content. I think it would be pretty hard to do this with an actual device so not a blocker imho.

Good catch, and I agree with you. While this is not something crucial to address, I'll note it down with low priority for future reference. Or at least to keep the issue tracked.

One thing we might add to a list of things to improve as we go is centering embedded content.

It looks like this is because the reply content is not using blocks. Here's an example HTML content:

Here\'s an example of a YouTube embed:

<iframe width=\"560\" height=\"315\" src=\"https://www.youtube.com/embed/dQw4w9WgXcQ\" title=\"YouTube video player\" frameborder=\"0\" allow=\"accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture\" allowfullscreen></iframe>

Let\'s see how this will be rendered on the mobile side.

Unlike blocks, the texts are not wrapped in a paragraph element, thus kinda mucking up with the spacing. I'll try to explore if I can improve the spacing for plain text / non-Gutenberg block comments in the next PR.

@dvdchr dvdchr merged commit 931a2cf into develop Sep 9, 2021
@dvdchr dvdchr deleted the issue/17087-content-cell-webview branch September 9, 2021 12:05
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.

2 participants