-
Notifications
You must be signed in to change notification settings - Fork 772
Feature/wkwebview #386
base: master
Are you sure you want to change the base?
Feature/wkwebview #386
Conversation
Hi @thilakks, It is working without problems? Or is WIP? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ricardohg and @thilakks, thanks for contributing.
I have added some suggestions and comments to your PR. This is by far the most asked change by lots of people, thanks for working on that.
Few things I noticed on the behaviour and we need to figure out how to fix that:
- The "pages left" count is not working properly;
- The vertical scroll (and horizontal) changes to the next chapter instead of scrolling the content of the chapter, the ideal behaviour is that at the end of the content it should scroll to the next chapter. This probably has to do with the nested scroll views;
- The base font size is wrong, this is probably some weird WKWebView behaviour and we have to figure out how to reset that to the base size.
open func js(_ script: String, completion: @escaping JSCallback) { | ||
|
||
self.evaluateJavaScript(script) { (result, error) in | ||
completion(result as? String) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making the completion optional will allow you to avoid having to implement it.
Like this.js("resetCurrentSentenceIndex()")
instead of .js("resetCurrentSentenceIndex()") { _ in }
open func js(_ script: String, completion: @escaping JSCallback) { | |
self.evaluateJavaScript(script) { (result, error) in | |
completion(result as? String) | |
} | |
open func js(_ script: String, completion: JSCallback? = nil) { | |
evaluateJavaScript(script) { (result, error) in | |
completion?(result as? String) | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -67,7 +67,7 @@ class FolioReaderAddHighlightNote: UIViewController { | |||
|
|||
if !highlightSaved && !isEditHighlight { | |||
guard let currentPage = folioReader.readerCenter?.currentPage else { return } | |||
currentPage.webView?.js("removeThisHighlight()") | |||
currentPage.webView?.js("removeThisHighlight()") { _ in } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentPage.webView?.js("removeThisHighlight()") { _ in } | |
currentPage.webView?.js("removeThisHighlight()") |
@@ -176,7 +176,7 @@ open class FolioReaderAudioPlayer: NSObject { | |||
@objc func play() { | |||
if book.hasAudio { | |||
guard let currentPage = self.folioReader.readerCenter?.currentPage else { return } | |||
currentPage.webView?.js("playAudio()") | |||
currentPage.webView?.js("playAudio()") { _ in } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentPage.webView?.js("playAudio()") { _ in } | |
currentPage.webView?.js("playAudio()") |
@@ -410,7 +415,7 @@ open class FolioReaderAudioPlayer: NSObject { | |||
if synthesizer.isSpeaking { | |||
stopSynthesizer(immediate: false, completion: { | |||
if let currentPage = self.folioReader.readerCenter?.currentPage { | |||
currentPage.webView?.js("resetCurrentSentenceIndex()") | |||
currentPage.webView?.js("resetCurrentSentenceIndex()") { _ in } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentPage.webView?.js("resetCurrentSentenceIndex()") { _ in } | |
currentPage.webView?.js("resetCurrentSentenceIndex()") |
@@ -185,7 +185,7 @@ extension FolioReader { | |||
|
|||
if let readerCenter = self.readerCenter { | |||
UIView.animate(withDuration: 0.6, animations: { | |||
_ = readerCenter.currentPage?.webView?.js("nightMode(\(self.nightMode))") | |||
_ = readerCenter.currentPage?.webView?.js("nightMode(\(self.nightMode))") { _ in } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ = readerCenter.currentPage?.webView?.js("nightMode(\(self.nightMode))") { _ in } | |
_ = readerCenter.currentPage?.webView?.js("nightMode(\(self.nightMode))") |
|
||
open func webView(_ webView: UIWebView, shouldStartLoadWith request: URLRequest, navigationType: UIWebView.NavigationType) -> Bool { | ||
|
||
public func webView(_ webView: WKWebView, decidePolicyFor navigationAction: WKNavigationAction, decisionHandler: @escaping (WKNavigationActionPolicy) -> Void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, keep it open as well...
// let minutesRemaining = Int(ceil(CGFloat((pagesRemaining * totalMinutes)/totalPages))) | ||
// if minutesRemaining > 1 { | ||
// minutesLabel.text = "\(minutesRemaining) " + self.readerConfig.localizedReaderManyMinutes+" ·" | ||
// } else if minutesRemaining == 1 { | ||
// minutesLabel.text = self.readerConfig.localizedReaderOneMinute+" ·" | ||
// } else { | ||
// minutesLabel.text = self.readerConfig.localizedReaderLessThanOneMinute+" ·" | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The time left is always 0 or negative, I think you need this calculation.
paginationMode = .leftToRight | ||
paginationBreakingMode = .page | ||
//paginationMode = .leftToRight | ||
//paginationBreakingMode = .page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning on fixing this?
public static func removeFromHTMLById(withinPage page: FolioReaderPage?, highlightId: String, completion: @escaping JSCallback) { | ||
guard let currentPage = page else { return } | ||
|
||
currentPage.webView?.js("removeHighlightById('\(highlightId)')", completion: completion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, if the completion is optional you can ommit it.
public static func removeFromHTMLById(withinPage page: FolioReaderPage?, highlightId: String, completion: @escaping JSCallback) { | |
guard let currentPage = page else { return } | |
currentPage.webView?.js("removeHighlightById('\(highlightId)')", completion: completion) | |
public static func removeFromHTMLById(withinPage page: FolioReaderPage?, highlightId: String, completion: JSCallback? = nil) { | |
guard let currentPage = page else { return } | |
currentPage.webView?.js("removeHighlightById('\(highlightId)')", completion: completion) |
} | ||
|
||
let frameHeight = webView.frame.height | ||
// let lastPageHeight = frameHeight * CGFloat(webView.pageCount) - CGFloat(Double(contentHeight!)!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
Hello @hebertialmeida , thanks for the review!, I need to clarify that we were working in this feature for a specific project, so some stuff we have here are project specific, for instance we only support fixed-layout ebooks, so we did some changes that may break regular epubs. The project is on stand by, so we're not working on this at the moment. however, due to increased interest ( I just saw apple is not accepting UIWebKit anymore) and your code review I think we should end this WKWebView migration. :) |
Hi all, have you planning in the near future to finish the migration to WKWebview? And also this scrolling issue, how to fix it? I saw on google some suggestions via scroll issue, posted on this blog https://ahmedk92.github.io/2017/11/03/WKWebView-Horizontal-Paging.html |
As apple is rejecting apps if they are using the UIWebview, this library won't work till the migration to the WKWebview completed. So put this task on high priority otherwise in spite of being the best library, doesn't sense anything. |
I also find this repo where is the migration to WKWebview. Maybe to you guys who knows more about webviews can help to speed up process of migrations |
@ricardohg Yeah, after I posted my review I saw that it was opened by someone using your branch. I lost track of how many people have been asking about this but I haven't had time to work on that, but I can definitely help to review it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Can you please help me out I have implemented the same and also I got crashed but I have resolved;lved that now my project is working but I think CSS and js file is not loaded properly in the collection view cell WKwebview , it looks weird can you please let me know how to resolve that issue . here is screen shot
Which method are you using to load the html?, it is working on a real device ? |
Same method that is mentioned in the file and I only changed UIwebview TO WkwebView |
Please help me out its urgent |
Hello everyone, does somebody know how to download epub book, save it to path and load it and then present it in FolioReader? Please help. |
Hi, everyone. If you need to fix the issue with font you need do this: |
@hebertialmeida @ricardohg |
@hebertialmeida What do you think? When will you be able to publish an update with WKWebview? As I am working on one project in which this SDK is the heart of it. So now I am not able to launch that app due to UIWebView. Please do the needful. Thanks. |
It's a great library. Please don't let it die. |
Just thought of sharing these two forked repos - https://github.com/drasdasdasd/FolioReaderKit The font size issue is solved in this repo. There are minor issues with both of these repos. |
Since the paginationMode is missing in WkWebview, you need to add this code in your CSS file:
|
If you're in a rush, consider sponsoring the repository so the contributors can spend more time during these wild times. |
We have very recently started the work on our iOS app to integrate folioreader. Came across this issue. We did contact @ricardohg to check how we can get his services to get the issue fixed. We are waiting on the response. |
@mooka-ventures Check out @hebertialmeida's page: https://github.com/sponsors/hebertialmeida |
Thanks, yes we are aware of that page. @Parsida we are already doing a tiny bit of sponsorship starting this month.. |
Small donations from several developers that are using this repository in their projects adds up 🙌🏽 |
We don't see any other sponsorships to the author @hebertialmeida by other developers. |
Hey guys, I'm open to keeping code reviewing this but at this time I can't do this implementation. If anyone wants to continue this implementation on another PR feel free to open and I can review. |
Alright guys. I've managed to fix the scrolling/pagination issue by injecting style tag (scrolling direction dependent) into an html. But I still have to fix the page counting issue and media overlay handling issues. I can post a PR after I will make it work, ok? |
Hello!, are you working on my branch?, did you check it is working on a real device ? |
Hello @ricardohg! Well, I used the most of your code - just edited it according to @hebertialmeida remarks on this PR and fixed the scrolling issue (credits to @emirchelsea). I haven't pushed it anywhere yet. I plan to push it on my fork first. https://github.com/OrbitalMan/FolioReaderKit
Also I don't see images embedded in epub content in webview. You can check by opening a book from example project app and looking at empty rect on the first page where the book cover is supposed to be shown. (I don't yet have an idea how to fix it, nor I need to fix it urgently) |
@OrbitalMan Thanks for contributing! will you open a new PR then? If so, I can review that and we can go from there. |
Sure @hebertialmeida! On the first commit I will make as few changes as possible for your convenience. Later I will refactor the code. |
Hey guys, I really think that WebKit/CSS/Javascript approach is overkill (at least for my project requirements - at least for now) - but the problem that migration just screwed up and doesn't work acceptable way. I probably will continue to fix this monster later, some day-ish...
All you need is just locate your epub's .xhtml files! (you have to unzip the epub - it's up to you how to) Credits for the snippet go to the Hacking with Swift: |
https://developer.apple.com/forums/thread/100125 |
|
@OrbitalMan I converted this to SwiftUI and was able to get everything working; however, I see that the latest version of this is still using UIWebView. I used https://github.com/OrbitalMan/FolioReaderKit Do you by any chance have a repo that uses WKWebview? I tried https://github.com/drasdasdasd/FolioReaderKit but scrollViewDidEndDecelerating is still using UIWebView. |
Hey guys, who solved a problem with images? Right now WKWebView doesn't load images from epub. |
@hellbit , I ran into the same issue and was able to resolve it by adding loadFileURL Go to FolioReaderPage.swift and make sure your loadHTMLString function looks like this:
|
@mspusta78 oh, you save my time. )) Thank you, man. I really appreciate it. I will connect with you in Ln. |
Hi We are looking for our project folio reader with wkWebview