pocket-casts-android icon indicating copy to clipboard operation
pocket-casts-android copied to clipboard

Fixes some strange layout issues in EpisodeFragment

Open VGJohn opened this issue 2 years ago • 5 comments

Fix for some strange but pretty serious layout issues in the EpisodeFragment seen in the linked issues.

All the issues are stemming from the WebView with height=WRAP_CONTENT that displays the episode notes. The height of the WebView causes different issues on different episodes, if the notes in the WebView fit on screen then the WebView flickers and disappears and then if the title of the episode fits on a single line then the entire screen is blank.

I've attached videos showing the before and after with fix. I'm testing on a Pixel 6a with my language set to German with default font size setting. I'm also forcing the notes to just be "Testing" and the title of every episode to just be "Testing" which causes the strange layout issues seen in the video.

NOTE: There is one drawback to this fix, if the episode notes are short then they display centred at the end of the screen and same thing with the progress spinner that shows while the notes are loading. This can be seen in the after video. I think this is an acceptable trade off considering users would see no notes or a blank screen before.

Fixes #162 #163

Checklist

  • [ ] Should this change be included in the release notes? If yes, please add a line in CHANGELOG.md
  • [x] Have you tested in landscape?
  • [ ] Have you tested accessibility with TalkBack?
  • [x] Have you tested in different themes?
  • [x] Does the change work with a large display font?
  • [x] Are all the strings localized?
  • [ ] Could you have written any new tests?
  • [ ] Did you include Compose previews with any components?

VGJohn avatar Oct 22 '22 20:10 VGJohn

Before video showing the issues on various episodes

https://user-images.githubusercontent.com/22520267/197360413-b81e488e-c19b-4017-8020-edba59ce897e.mp4

VGJohn avatar Oct 22 '22 20:10 VGJohn

After video showing the fix on the same episodes (can also see the effect the fix has on the loading spinner and placement of short episode notes, again I feel like this is an acceptable tradeoff)

https://user-images.githubusercontent.com/22520267/197360636-1150403d-1b5a-48da-a587-47ad40ff8e5c.mp4

VGJohn avatar Oct 22 '22 20:10 VGJohn

Thank you for the fix @VGJohn and sorry about the delay in reviewing it. While your solution looks fine but as you stated, it comes with a drawback that'll impact episodes with shorter notes.

I wonder if the issue could be because of how webViewShowNotes is constrained w.r.t. a group element lblDate. lblDate is a loadingGroup element aligned to bottomDivider which depends on another group: errorLayout. Both loadingGroup and errorLayout group visibility is controlled dynamically.

There's a known Android issue that affects the visibility of group elements and I suspect it could affect elements constrained to Group or their elements.

I wasn't able to reproduce the issue (tested on Pixel 5, Pixel 3a devices, and Pixel 6 emulator after force changing notes and title to Testing on the main branch) so I could not verify. By any chance, do you have steps to reproduce the issue on an emulator?

ashiagr avatar Oct 28 '22 06:10 ashiagr

@ashiagr sorry about the delay as well!

Thanks for the tip about the Group, I tried removing them although that didn't change the behaviour I was seeing but it did make me suspicious of the Barrier. The issue was arising from the middle part of the layout with all the buttons, instead of using the Barrier I switched to just wrapping the buttons in a nested ConstraintLayout.

The result is that the issues from the before video are fixed and the layout remains unchanged 🎉

I tried to find another device or emulator that I could reproduce the issue on but it seems like it only happens on the Pixel 6a which is very strange, sorry about that!

https://user-images.githubusercontent.com/22520267/198861357-5302dc95-fe9f-42bc-ab8c-3a881de36ee2.mp4

VGJohn avatar Oct 30 '22 03:10 VGJohn

Attaching a video of the episode screen with the actual episode title and notes instead of them being hardcoded to "Testing" so you can see the layout remains the same with this fix 😄

https://user-images.githubusercontent.com/22520267/198861497-8250297f-f917-4e4a-8d71-c8fb797a8f08.mp4

VGJohn avatar Oct 30 '22 03:10 VGJohn

Cheers @ashiagr

I think I just got lucky for the most part haha but hopefully it fixes the issues for the german users that reported it ❤️

Let me know if there's anything I can do to fix the spotless check failing, I run the spotless gradle task before opening any PRs and I didn't see it complain about any of the changes I made but I can't view the failure details from buildkite so maybe its something else 😕

VGJohn avatar Oct 31 '22 05:10 VGJohn

Spotless is working fine on the local copy. It looks like some caching issue, I re-triggered the build to see if it fixes it.

I don't think you need to do anything, but I'll keep you informed. 🤞

ashiagr avatar Oct 31 '22 05:10 ashiagr

CI got working this time, it was a temporary glitch.

hopefully it fixes the issues for the german users that reported it

I hope so too 🤞

I merged your PR, only to realize that there was no ChangeLog entry to this fix. Could you add it in your other open PR or a new one if you like?

ashiagr avatar Oct 31 '22 12:10 ashiagr

@ashiagr sorry about that, I added the changelog entry to my other open PR 😄

VGJohn avatar Oct 31 '22 14:10 VGJohn