Pop-Up Footnotes

[tooolbox]

This PR is based on extensive discussion and closes readium/swift-toolkit#139

(Not ready for formal review yet.)

[tooolbox]

Cross-posting from readium/swift-toolkit#139

• This work was actually done on r2-navigator-swift@master and then I branched off of r2-navigator-swift@develop and cherry-picked the commit, so as of this writing I haven't checked to ensure it compiles there 😅 (I will, before this is done, but I wanted to note that if any eagle-eyed observers catch something out of sync).
• SwiftSoup dependency added, mirroring the JSoup dep used in r2-navigator-kotlin.
• On the JS side, I modified gestures.js to detect a <a epub:type="noteref"> link and call a special bridging method. The JS bridge doesn't let Swift return a value to the JS, so I couldn't easily do the pattern of "ask Swift if it wants to do something special with this hyperlink". More on that below.
• On the Swift side I mostly tried to mimic the logic from the Kotlin click handler by @aferditamuriqi
• The first chunk on the Swift side (finding the target file, pulling out the note, calling the delegate method) is implemented in the EPUBNavigatorViewController.
• The second chunk (default method for displaying the pop-up) is an extension on the NavigatorDelegate.
• To enable this default method on the NavigatorDelegate protocol, I made Navigator inherit from UIViewController. It works fine because everything implementing Navigator in the test app is a UIViewController but I have a feeling that's imperfect, will discuss below.
• In the default popup code, I opted to display it in a webview to preserve formatting from <strong> or <em> or any such things. We don't have to to do this but it takes so little effort that I don't see the point in stripping the HTML and throwing it into a UITextView. Open to discussion.
• BarButtonItem is a new little class to facilitate the default popup on the NavigatorDelegate.
• In Swift, getting the suffix after a specific character is atrocious: let id = String(href[href.index(hashIndex, offsetBy: 1)...]) 😅

JS Click Handler

As was mentioned in readium/swift-toolkit#139 (1, 2) it may be desirable moving forward to generically allow the App to intercept & control what occurs with tapping hyperlinks, and yet I didn't go this route; I wanted to explain my logic a bit more.

As I said above, the JS bridge doesn't normally allow Swift to return a value to JS, so you can't do this:

function onClick(event) {
  let eventWillBeHandledBySwift = webkit.messageHandlers.tap.postMessage(event);
  if (eventWillBeHandledBySwift) {
    event.preventDefault();
    event.stopPropagation();
    return;
  }
  // ...
}

Any other options? There's this method I found:

// js
function callNativeCode() {
    webkit.messageHandlers.callbackHandler.postMessage("Hello from JavaScript");
}

// this function will be later called by native code
function callNativeCodeReturn(returnedValue) {
    // do your stuff with returned value here
}

// swift
func userContentController(_ userContentController: WKUserContentController, didReceive message: WKScriptMessage) {
    if(message.name == "callbackHandler") {
        print("JavaScript is sending a message \(message.body)")
        webView.evaluateJavaScript("callNativeCodeReturn(0)")
    }
}

...Gross, and probably not terribly performant. Even if you use promises layered on this, which some people suggested.

You can also override the prompt() handler in Swift to use it as a crude method of synchronously getting a return value in JS...also gross.

On the other end of this, we have the actual link navigation which we can intercept with WKNavigationDelegate which is what the Navigator codebase uses for its internal and external link-tap delegate methods. This should work well for the use case of intercepting & deciding upon hyperlink clicks, but unfortunately it won't work for noterefs because at that point it's a WKNavigationAction so you can't check for epub:type="noteref" unless you want to parse the entire document again each time (yikes).

You could go the route of what was done in Android, whereby the Swift click handler sets a little semaphore variable that the navigation-policy delegate method uses to know if it should proceed...but that also feels icky (no offense).

The above is why I opted to purely detect this specific scenario in the JS: it's not generic, but it seemed the least hacky and most performant. However, it doesn't allow the signature suggested by @mickael-menu

func navigator(_ navigator: Navigator, shouldNavigateToNoteAt link: Link, content: String) -> Bool

Having that Bool return value necessitates one of the above workarounds, so I would prefer not to. Let's discuss :)

NavigatorDelegate

As I said, I feel like making the Navigator protocol inherit (or whatever it's called) from UIViewController is a move that will receive displeasure.

I could make it more specific by adding presentViewController as a method in Navigator or NavigatorDelegate but I'm not sure.

I could also attempt to call presentViewController via some global method or hack (UIApplication.shared.mainWindow.rootViewController or some such) but I feel like that's fragile.

Maybe this other thing I mentioned of somehow subclassing the NavigatorDelegate protocol in the App, that might be an option, but I'll need to scope it.

[tooolbox]

Cross-posting:

• Use postMessage to provide data about the click to decidePolicyFor
• Changed behavior of the postMessage so it's always called, even if the WebView is going to handle the click internally. I didn't want 2 calls across the bridge.
• I structured the tap data on the Swift side so it's more codified what's being received from the JS.
• Tap handling on the Swift side is pretty generic now, you get the content of the <a></a> tag if there was one, and you can do with it as you like. Should open the door to the App being able to dictate what's done with any kind of link navigation—although I didn't implement that.
• The actual popup implementation is moved out of Navigator.
• The delegate method returns Bool now to allow the Navigator to jump to the note as per usual.
• The default implementation of the delegate method returns true so the App doesn't have to implement it unless they like.

[mickael-menu]

@tooolbox Thanks again for this contribution, it will be very useful! I left a few comments in the PR.

Also I don't see any PR in r2-testapp-swift with the footnote pop-up, did you intend to send one?

[tooolbox]

@tooolbox Thanks again for this contribution, it will be very useful! I left a few comments in the PR.

Great review, appreciated 👍

Also I don't see any PR in r2-testapp-swift with the footnote pop-up, did you intend to send one?

I will need to see about extracting my implementation and making a feature branch to give you guys. Do you need to see that before merging this one?

[mickael-menu]

I will need to see about extracting my implementation and making a feature branch to give you guys. Do you need to see that before merging this one?

Not really, but it would help test the feature. Maybe we can wait until you're ready before merging this PR? I don't expect any changes in the files you modified in the short run.

[tooolbox]

Yeah, I can see how that would help with testing, so I'll get a PR together this afternoon. 🚀

[tooolbox]

Okay @mickael-menu I'm done with everything we discussed, plus a little more that I noticed during my tests, mainly to handle noterefs that are just #abc without a filepath.

Matching PR in the App is available here.

Whew.

[mickael-menu]

@tooolbox Thanks, great job! I'll test and merge it next week, but the code looks good.

[mickael-menu]

@tooolbox Thanks again for your contribution 👍