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

Navigator.go results in misaligned view #449

Open
domkm opened this issue Jun 4, 2024 · 9 comments
Open

Navigator.go results in misaligned view #449

domkm opened this issue Jun 4, 2024 · 9 comments
Labels
bug Something isn't working triage Triage needed by maintainers

Comments

@domkm
Copy link
Contributor

domkm commented Jun 4, 2024

Describe the bug

I'm synchronizing EPUB location and decoration with audiobook narration and finding that using go on the EPUB navigator can result in a view that is not center aligned and is therefore cut off. As one navigates through the cut off view, the view can quickly swap back and forth between the two sides of the cut page. However, swiping causes the pages to transition correctly. I'm attaching a screen recording below showing the correct swiping behavior followed by the cut off go behavior.

RPReplay_Final1717532314.mov

How to reproduce?

  1. Open an EPUB
  2. Use go to navigate to locators currently displaying or about to display on the next page

Though not the same EPUB as in the video, I also noticed the issue using this one (zipped because GitHub won't permit EPUB upload): 2 B R 0 2 B.epub.zip

Readium version

develop 8fad4d5

OS version

iOS 17.5.1

Testing device

iPhone 11 (also noticing it in simulator)

Environment

macOS: 14.5
platform: arm64
Xcode 15.4
Build version 15F31d

Additional context

No response

@domkm domkm added bug Something isn't working triage Triage needed by maintainers labels Jun 4, 2024
@mickael-menu
Copy link
Member

Do you have a way to reproduce this problem in the Test App, maybe after applying some code patch?
go() is already used with the TTS in the Test App and don't seem to show this issue.

@domkm
Copy link
Contributor Author

domkm commented Jun 6, 2024

I'm having trouble reproducing this in the Test App. Would it be okay to zip up my project and send it to you privately?

@mickael-menu
Copy link
Member

Unfortunately I can't spend time debugging individual projects.

In your screencast the navigator is not spanning the whole screen, I wonder if that could have an impact on this bug?

@domkm
Copy link
Contributor Author

domkm commented Jun 6, 2024

It's totally understanding that you can't debug individual projects. I just assumed that this was a bug in swift-toolkit, not specifically in my implementation. The code I'm using is pretty much directly pulled from this documentation and I am not doing anything else to limit the width of the display, just the height.

If it is a bug in swift-toolkit, I believe the reason I am unable to replicate it is due to:

  1. I am using develop but had to switch to main to attempt to reproduce because Test App fails to compile on develop. Perhaps the issue was introduced in the transition from 2 to 3.
  2. My project uses SwiftUI and Test App uses UIKit, which I am not familiar with beyond the bare minimum I used to wrap EPUBNavigationViewController. As such, I am not able to copy the relevant parts of my UI into Test App.

One thing which does work around this issue is setting scroll: true in EPUBPreferences. However, that causes go to scroll such that the target locator is at the very top of the screen. Is there any way to make to go scroll such that the target locator is vertically centered?

@mickael-menu
Copy link
Member

I am not doing anything else to limit the width of the display, just the height.

We can see left and right margins around the navigator in your video. I wonder if you have the same issue if you remove them.

I am using develop but had to switch to main to attempt to reproduce because Test App fails to compile on develop. Perhaps the issue was introduced in the transition from 2 to 3.

Did you try to compile it after running make dev? (make spm/make carthage/make cocoapods only work properly from a released version). What errors did you get? If the GitHub checks pass, the Test App should not have build issues.

One thing which does work around this issue is setting scroll: true in EPUBPreferences. However, that causes go to scroll such that the target locator is at the very top of the screen. Is there any way to make to go scroll such that the target locator is vertically centered?

Not at the moment, but that could be an EPUBNavigatorViewController.Configuration setting if you're interested in contributing it?

@domkm
Copy link
Contributor Author

domkm commented Jun 7, 2024

We can see left and right margins around the navigator in your video. I wonder if you have the same issue if you remove them.

You're right! I didn't realize that I had done that. And you're also right about that causing this issue. Thanks! Is there an easy way for me to add horizontal padding in Test App to attempt to reproduce this?

Did you try to compile it after running make dev? (make spm/make carthage/make cocoapods only work properly from a released version). What errors did you get? If the GitHub checks pass, the Test App should not have build issues.

I did not and that fixed it. Thanks. Looks like I didn't read the README very thoroughly. My mistake.

Not at the moment, but that could be an EPUBNavigatorViewController.Configuration setting if you're interested in contributing it?

Even though this is no longer my top priority, I would be interested in this at some future point. If you point me in the right direction as to how to implement this, I will try to get to it eventually.

@mickael-menu
Copy link
Member

Is there an easy way for me to add horizontal padding in Test App to attempt to reproduce this?

You might be able to do it here:

navigator.view.frame = view.bounds
navigator.view.autoresizingMask = [.flexibleWidth, .flexibleHeight]

I did not and that fixed it. Thanks. Looks like I didn't read the README very thoroughly. My mistake.

You're not the only one, I'll see if we can't have a guard in the makefile to check this 😄

Even though this is no longer my top priority, I would be interested in this at some future point. If you point me in the right direction as to how to implement this, I will try to get to it eventually.

That would be in the JavaScript layer, every time scrollTop is set, for example here:

document.scrollingElement.scrollTop = offset;

Take a look at this guide if you want to modify the JavaScript files: https://github.com/readium/swift-toolkit/blob/develop/CONTRIBUTING.md#modifying-the-epub-navigators-javascript-layer

@domkm
Copy link
Contributor Author

domkm commented Jun 8, 2024

You might be able to do it here:

navigator.view.frame = view.bounds
navigator.view.autoresizingMask = [.flexibleWidth, .flexibleHeight]

That's it! To reproduce the issue, change the code like this:

- navigator.view.frame = view.bounds
+ let padding: CGFloat = 25
+ navigator.view.frame = CGRect(x: view.bounds.origin.x + padding, y: view.bounds.origin.y, width: view.bounds.width - 2 * padding, height: view.bounds.height)

Then add some bookmarks at various places and navigate directly to them. It seems like bookmarks on the first page of a chapter will display correctly while others will be cut off.

@domkm
Copy link
Contributor Author

domkm commented Jun 15, 2024

Actually, this is possible to replicate without modifying the Test App. If you open it in an iPad or Mac you can resize the window and that will cause navigating to arbitrary bookmarks to break. I just reproduced this on unmodified develop (91bc1592).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage Triage needed by maintainers
Projects
None yet
Development

No branches or pull requests

2 participants