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

Implement Pop-Up Noterefs #139

Closed
tooolbox opened this issue Apr 19, 2020 · 41 comments · Fixed by readium/r2-navigator-swift#118
Closed

Implement Pop-Up Noterefs #139

tooolbox opened this issue Apr 19, 2020 · 41 comments · Fixed by readium/r2-navigator-swift#118

Comments

@tooolbox
Copy link
Contributor

tooolbox commented Apr 19, 2020

The r2-navigator-kotlin has a click handler that is bridged from JS that checks for <a epub:type="noteref"> and displays the target content in a modal.

Relevant code:=

var handleClick = function(event) {
    Android.handleClick(event.target.outerHTML)
};

and

@android.webkit.JavascriptInterface
fun handleClick(html: String) {
    val doc = Jsoup.parse(html)
    val link = doc.select("a[epub:type=noteref]")?.first()
    link?.let { noteref ->
        val href = noteref.attr("href")
        if (href.indexOf("#") > 0) {
            val id = href.substring(href.indexOf('#') + 1)
            var absolute = getAbsolute(href, resourceUrl!!)
            absolute = absolute.substring(0, absolute.indexOf("#"))
            val document = Jsoup.connect(absolute).get()
            val aside = document.select("aside#$id").first()?.html()
            aside?.let {
                val safe = Jsoup.clean(aside, Whitelist.relaxed())

                // Initialize a new instance of LayoutInflater service
                val inflater = activity.getSystemService(Context.LAYOUT_INFLATER_SERVICE) as LayoutInflater

                // Inflate the custom layout/view
                val customView = inflater.inflate(R.layout.popup_footnote, null)

                // Initialize a new instance of popup window
                val mPopupWindow = PopupWindow(
                    customView,
                    ListPopupWindow.WRAP_CONTENT,
                    ListPopupWindow.WRAP_CONTENT
                )
                mPopupWindow.isOutsideTouchable = true
                mPopupWindow.isFocusable = true

                // Set an elevation value for popup window
                // Call requires API level 21
                if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
                    mPopupWindow.elevation = 5.0f
                }

                val textView = customView.findViewById(R.id.footnote) as TextView
                if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
                    textView.text = Html.fromHtml(safe, Html.FROM_HTML_MODE_COMPACT)
                } else {
                    textView.text = Html.fromHtml(safe)
                }

                // Get a reference for the custom view close button
                val closeButton = customView.findViewById(R.id.ib_close) as ImageButton

                // Set a click listener for the popup window close button
                closeButton.setOnClickListener {
                    // Dismiss the popup window
                    mPopupWindow.dismiss()
                }

                // Finally, show the popup window at the center location of root relative layout
                mPopupWindow.showAtLocation(this, Gravity.CENTER, 0, 0)

                overrideUrlLoading = false
            }
        }
    }
}

The same could be implemented for iOS.

Current click handling:

function onClick(event) {
  if (event.defaultPrevented || isInteractiveElement(event.target)) {
    return;
  }

  if (!window.getSelection().isCollapsed) {
    // There's an on-going selection, the tap will dismiss it so we don't forward it.
    return;
  }

  webkit.messageHandlers.tap.postMessage({
    "screenX": event.screenX,
    "screenY": event.screenY,
    "clientX": event.clientX,
    "clientY": event.clientY,
  });

In terms of implementation, I'll start with comparing to the Android version.

In reading through the behavior, it appears that all taps (on any element) call the Kotlin function before the event is propagated through the DOM. The Kotlin checks to see if the tapped element is a[epub:type=noteref] and if so, identifies the target material. It's important to do this in the host code, rather than JS, so that you can access whatever file is being targeted. If the Kotlin func successfully displays the modal, it does a little semaphore action (so to speak) to the WebView delegate to cancel the navigation. (That last bit is a little convoluted since shouldOverrideUrlLoading appears to be employed in reverse, but that's what I gather.)

In contrast, the iOS click handler explicitly avoids calling across the JS bridge to Swift for any interactive elements, including <a> tags. The recursive isInteractiveElement JS function references this https://github.com/JayPanoz/architecture/tree/touch-handling/misc/touch-handling regarding don't-mess-up-the-author's-scripts.

Links

This test contains 3 links with different behaviors:

  1. normal link, which should take the user to the previous test;
  2. scripted link, which should display a hidden element below the second paragraph;
  3. scripted link, which is using window.location to send the user to the next test.

The test is here and looks like:

<a href="../Text/test-005.xhtml">I’m a simple link to the previous chapter (Audio controls with buttons)</a>
<a id="anchor-hook" href="#note">I’m a link which will be used to display something below this paragraph</a>
<a id="anchor-programmatic" href="#anchor-programmatic">I’m a link using window.location to the next chapter (DOM Storage)</a>

along with

var hook = document.getElementById("anchor-hook"),
    prog = document.getElementById("anchor-programmatic"),
    note = document.getElementById("note");

hook.addEventListener("click", function(e) {
  e.preventDefault();
  if (note.style.display === "none") {
    note.style.display = "block";
  } else {
	note.style.display = "none";
  }
}, false);

prog.addEventListener("click", function(e) {
  e.preventDefault();
  window.location = "../Text/test-007.xhtml";
}, false);

I am tempted to check for epub:type=noteref in the JS and call a new, special Swift handler specifically for this purpose. I think that links of that type should be handled by the app and any user scripts are forfeit.

The other question in my mind is what to do on the targeted content, because typically (in my understanding of things) it should be hidden. Not sure if the author should have to display:none it or if that should be part of the styles injected by the app...I lean towards the latter.

I intend to do this work over the next day or two, but I wanted to ask for feedback before my PR leaps full-armed from the brow of Jove.

@llemeurfr
Copy link
Contributor

llemeurfr commented Apr 19, 2020

The popup footnotes feature was indeed implemented first in the Kotlin toolkit, by @aferditamuriqi. The original idea was to start the development of a JS module common to the Swift and Kotlin toolkits, a lib we called "Glue JS" at the time (as it was acting as a glue between native code and browsing actions). This Glue JS lib was never finalized, but the repo is present here. Touch handling would have moved to this repo if it had been made common btw the different toolkits. I don't know if this overarching goal can still be reached without large refactoring.

Not sure if the author should have to display:none it ...

Few reading systems implement EPUB 3 popup footnotes; iBooks is and the authoring part is correctly described by Apple here. It is said there that if the footnote is an aside element and no explicit display:none is needed on it. But if the author wants the content of the footnote to be visible a div or p element should be used instead.

Logically, the target footnote text is in the same resource as the hyperlink. But this is not explicitly stated in any guideline I know of.

@tooolbox
Copy link
Contributor Author

The popup footnotes feature was indeed implemented first in the Kotlin toolkit, by @aferditamuriqi. The original idea was to start the development of a JS module common to the Swift and Kotlin toolkits, a lib we called "Glue JS" at the time (as it was acting as a glue between native code and browsing actions). This Glue JS lib was never finalized, but the repo is present here. Touch handling would have moved to this repo if it had been made common btw the different toolkits. I don't know if this overarching goal can still be reached without large refactoring.

I don't think I will attempt this. Do you want a PR for this functionality, and then it can be refactored and centralized later, with everything taken into consideration? It might not be a short task given potential browser variations.

Few reading systems implement EPUB 3 popup footnotes; iBooks is and the authoring part is correctly described by Apple here. It is said there that if the footnote is an aside element and no explicit display:none is needed on it. But if the author wants the content of the footnote to be visible a div or p element should be used instead.

Okay great, so <aside> should be hidden.

Logically, the target footnote text is in the same resource as the hyperlink. But this is not explicitly stated in any guideline I know of.

Sure, you would think so logically, but as you say not specified anywhere. I have crafted books where they were in separate files. The Android implementation allows for them being in separate files so I will do the same for Swift.

@aferditamuriqi
Copy link
Member

Just to give some more information here, the entire purpose on the footnote when I implemented this in kotlin, was to do most if not all the rendering in kotlin vs in js. the javascript code simply passes the html through the click handler. and the rest is done in kotlin, this allows for native views to be shown, and dom manipulation avoided. Also, all html sanitizing is not in kotlin, which was an important part of footnotes.

At the time this was implemented there was no click handler whatsoever neither in swift or kotlin.

Since then the click handlers in Swift have evolved, but not including any footnote handling.

@tooolbox
Copy link
Contributor Author

tooolbox commented Apr 19, 2020

Thanks for jumping in @aferditamuriqi

Just to give some more information here, the entire purpose on the footnote when I implemented this in kotlin, was to do most if not all the rendering in kotlin vs in js. the javascript code simply passes the html through the click handler. and the rest is done in kotlin, this allows for native views to be shown, and dom manipulation avoided. Also, all html sanitizing is not in kotlin, which was an important part of footnotes.

Great, that makes sense and matches what I see in the code. I will do it the same in Swift, basically just try to port the Kotlin. (Figure I will use the SwiftSoup library, so I get a mostly direct translation from Jsoup.)

Since then the click handlers in Swift have evolved, but not including any footnote handling.

Indeed. The tap handler in iOS is engineered in a specific fashion and I don't think it makes sense to include noteref handling as part of it. The JS will need to scan the DOM, notice the epub:type=noteref attribute, cancel the default action and then perform a call across the bridge. This will be included in the JS click handler, but the content sent across the bridge and the subsequent Swift codepath is completely different, so those will be new bits.

@tooolbox
Copy link
Contributor Author

RFC:

Rather than taking the content of the <aside>, cleaning it, and displaying it in a native view, what do we think about this progression:

  1. Retrieve the relevant HTML resource & parse with (J|Swift)Soup.
  2. Locate the targeted <aside> in the document.
  3. Replace the children of <body> with the children of <aside>.
  4. Output to HTML and clean the result.

If you display the bare HTML, you're going to lose any styles that aren't inlined. By using the original document as a framework, you will keep any styles in the <head>. What do we think?

(Of course, depending on the exact implementation of step 3, selectors targeting <aside> may fail, but given the default rule of aside { display: none; } I don't know that we want to keep that element hanging around.)

@mickael-menu
Copy link
Member

@tooolbox Thanks for this great analysis and particularly for pursuing feedback before sending a PR 👍

The strategy implemented in Kotlin by Aferdita sounds good to me, from the native side. And the hiding approach described by Apple for iBooks should probably be followed as well.

If you display the bare HTML, you're going to lose any styles that aren't inlined. By using the original document as a framework, you will keep any styles in the . What do we think?

That's an interesting point. I tend to think that extracted notes live outside the rendering context of a publication, since they are presented in a native modal, and therefore should not be styled. But I would love to hear more opinions from other contributors.

On a side note, we're experimenting right now with a proposal format for changes adding non trivial features and that should be aligned across platforms. Would you consider writing one for handling EPUB noterefs? This doesn't prevent you from implementing a PR which could be used as a proof-of-concept companion to the proposal.

You can find a ready-to-use template for proposals here: readium/architecture#129 And a few examples of proposals in these PRs: https://github.com/readium/architecture/pulls

@mickael-menu
Copy link
Member

By the way, we don't want to create the modal directly in r2-navigator, because UI presentation is the responsibility of reading apps. You could add a new function in NavigatorDelegate, for example:

/// Called when the user activated a note reference.
/// You should present its content in a modal dialog.
/// Use `link.type` to know the format of the given `content`, e.g. `text/html`.
func navigator(_ navigator: Navigator, presentNote content: String, at link: Link)

And then display the pop-up in r2-testapp-swift.

With this, a reading app may decide to not show a pop-up, but instead to go to the location in the navigator. Assuming that we add a way to disable footnotes' hiding.

@tooolbox
Copy link
Contributor Author

Thanks for this great analysis and particularly for pursuing feedback before sending a PR

👍

That's an interesting point. I tend to think that extracted notes live outside the rendering context of a publication, since they are presented in a native modal, and therefore should not be styled. But I would love to hear more opinions from other contributors.

I know that iBooks (when I checked years ago) displays pop-ups without links to external CSS styles. However, I don't see why we shouldn't enable this.

Some years ago I had an author who wanted the footnote styles to be preserved, so I had to go through pain to inline styles into the HTML as part of the processing I did to the book. This problem can be avoided by using the original document as a context. Do we gain something by displaying the <aside> markup in a vacuum?

By the way, we don't want to create the modal directly in r2-navigator, because UI presentation is the responsibility of reading apps.

The existing pop-up functionality in Android is implemented in the Navigator. That doesn't seem particularly incorrect, because the Navigator is responsible for navigation and this is an alternate method of navigation. The Navigator is perceiving the hyperlink-being-navigated-to and deciding to do something besides shifting over the main viewing window.

Now, if we want the pop-up behavior to be customizable on a per-app basis, then to keep Navigator as a clean library, we should have a delegate method. But maybe there should be a default.

On a side note, we're experimenting right now with a proposal format for changes adding non trivial features and that should be aligned across platforms. Would you consider writing one for handling EPUB noterefs? This doesn't prevent you from implementing a PR which could be used as a proof-of-concept companion to the proposal.

Hm... Can't say that I'm overjoyed at the thought 😅 But sure, I'll put something together this afternoon. 👍

@mickael-menu
Copy link
Member

I know that iBooks (when I checked years ago) displays pop-ups without links to external CSS styles. However, I don't see why we shouldn't enable this.

It all depends how the notes will be presented, and this should be decided by reading apps. For example, a bottom sheet without any UX, dismissed when taping outside its content, would look great with the book's styling. But with other more native components, it would look weird in my opinion.

Some years ago I had an author who wanted the footnote styles to be preserved, so I had to go through pain to inline styles into the HTML as part of the processing I did to the book. This problem can be avoided by using the original document as a context. Do we gain something by displaying the <aside> markup in a vacuum?

This strategy sounds great to keep the styles, but it would be doing too much work and be too opinionated in the navigator IMHO.

With the navigator(Navigator, presentNote content: String, at link: Link) delegate function, it would be trivial for a reading app to read the content at the given link, and replace the <body> with content. By not doing this work early, we're leaving options to reading apps.

The existing pop-up functionality in Android is implemented in the Navigator. That doesn't seem particularly incorrect, because the Navigator is responsible for navigation and this is an alternate method of navigation. The Navigator is perceiving the hyperlink-being-navigated-to and deciding to do something besides shifting over the main viewing window.

We're moving away from having an opinionated UI in the navigator to have more flexibility in reading apps. That being said, having a default implementation with the most idiomatic native component for that would be perfectly fine.

The navigator is still responsible of intercepting the navigation to the footnote link, but the presentation is decoupled and handled by the reading app.

Hm... Can't say that I'm overjoyed at the thought 😅 But sure, I'll put something together this afternoon. 👍

That's really not necessary, I understand that integrating the feature would be more exciting to you ;)

@tooolbox
Copy link
Contributor Author

Great, your logic makes sense to me; I'll add a NavigatorDelegate method.

In terms of adding aside { display: none; } am I correct that the change should be done in https://github.com/readium/readium-css and then the generated stylesheets copied in?

This strategy sounds great to keep the styles, but it would be doing too much work and be too opinionated in the navigator IMHO.

That was something I did to hack around limitations in a reading app, it wasn't meant as a suggestion, more like a reason not to limit this app :)

@mickael-menu
Copy link
Member

In terms of adding aside { display: none; } am I correct that the change should be done in https://github.com/readium/readium-css and then the generated stylesheets copied in?

I'm not sure, maybe @JayPanoz has an opinion on this?

In any case, a reading app should be able to disable hiding the footnotes (we don't need to expose the API to disable it right now). So either the CSS is injected manually in the navigator, or we have a ReadiumCSS variable to enable this feature.

@JayPanoz
Copy link

JayPanoz commented Apr 21, 2020

That’s an interesting question as this will probably be the option chosen by a lot of implementations, but I’d be considering it as something different than the existing flags we have in ReadiumCSS as it’s relatively stable – except if it could change on user’s demand, for instance in advanced settings.

On first glance, seems to me this is something you likely don’t want to change, and it would be much better as something “static” in your own version of ReadiumCSS than something you have to enable on each load. But again, a CSS variable would help make it a setting if you wanted to let users decide what they want to make of these asides.

I prefer to mention that because it seems iBooks’ implementation has suffered from various accessibility issues over time – e.g. I can recall bug reports that Voice Over couldn’t even reach the popup footnote on one platform, so a publisher decided to move away from aside and use div so that they can still be accessible at the end of the chapter for instance.

More globally, I’d lean towards putting this question into context: we’ve regularly seen that CSS might be an issue for some implementers, and they are more likely to try doing things in the app than forking/cloning the repo, customising the modules, and building their own version. So I’m wondering whether we’re not at this point we should try to accommodate popular options such as this one, so that they can just set a CSS variable instead of customising ReadiumCSS.

@mickael-menu
Copy link
Member

mickael-menu commented Apr 21, 2020

I've been thinking that maybe navigating to the footnote in the main WebView should actually be the default behavior of the navigator. Since this is the least opinionated, and relies on standard HTML.

The delegate function would look like this instead:

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

And a reading app wanting to show the footnote as pop-ups would just return false to override the default behavior.

@mickael-menu
Copy link
Member

More globally, I’d lean towards putting this question into context: we’ve regularly seen that CSS might be an issue for some implementers, and they are more likely to try doing things in the app than forking/cloning the repo, customising the modules, and building their own version. So I’m wondering whether we’re not at this point we should try to accommodate popular options such as this one, so that they can just set a CSS variable instead of customising ReadiumCSS.

Also we'll need to add extensible CSS injection capabilities, which could also address this issue without modifying ReadiumCSS.

@tooolbox
Copy link
Contributor Author

In any case, a reading app should be able to disable hiding the footnotes (we don't need to expose the API to disable it right now). So either the CSS is injected manually in the navigator, or we have a ReadiumCSS variable to enable this feature.

Seems like you could give your <aside> a class and make a more specific selector to override the default in ReadiumCSS, no?

On first glance, seems to me this is something you likely don’t want to change, and it would be much better as something “static” in your own version of ReadiumCSS than something you have to enable on each load. But again, a CSS variable would help make it a setting if you wanted to let users decide what they want to make of these asides.

I'm honestly not familiar with CSS variables. Do you have an example of how this would go or what I would need to do to enable this?

I prefer to mention that because it seems iBooks’ implementation has suffered from various accessibility issues over time – e.g. I can recall bug reports that Voice Over couldn’t even reach the popup footnote on one platform, so a publisher decided to move away from aside and use div so that they can still be accessible at the end of the chapter for instance.

Interesting. Maybe if VoiceOver is enabled, the app should inject an override (or alter a variable) which reveals the <aside> tags?

Also, I'm not very familiar with Voice Over, so forgive me: could it not read the contents of our pop-up window? I.e. is there a reason our implementation would suffer from the same issue as iBooks, or is this a matter of compatibility with books that have been developed a certain way because of iBooks limitations? Also, what's the UX of a person using Voice Over clicking a noteref link; is it reasonable to expect them to be able to do that?

More globally, I’d lean towards putting this question into context: we’ve regularly seen that CSS might be an issue for some implementers, and they are more likely to try doing things in the app than forking/cloning the repo, customising the modules, and building their own version. So I’m wondering whether we’re not at this point we should try to accommodate popular options such as this one, so that they can just set a CSS variable instead of customising ReadiumCSS.

Seems like:

  • By default, the <aside> should be hidden because that's the most common scenario.
  • Publishers should be able to override that with styles if they really want to; they don't have to fork the app.
  • When we detect Voice Over we can un-hide the <aside> by default.

I've been thinking that maybe navigating to the footnote in the main WebView should actually be the default behavior of the navigator. Since this is the least opinionated, and relies on standard HTML.

I don't think this is good UX. You get dropped at the end of a chapter--maybe, if the note is in the same file, which they typically are for sure--and there may be 1 or 10 footnotes, and then you have to see which one is being referred to, read it, and then navigate back.

The reason why iBooks and some Kindles have pop-up footnotes is because is creates a better UX. I don't think we gain anything by settling for less or crippling the default, as long as we have Voice Over sorted out, right?

Maybe my perspective is off, but it seems like if the author puts <a epub:type="noteref"/> and <aside epub:type="footnote"/> in their markup, they're communicating to the reading system that they want a popup.

@mickael-menu
Copy link
Member

mickael-menu commented Apr 21, 2020

On first glance, seems to me this is something you likely don’t want to change, and it would be much better as something “static” in your own version of ReadiumCSS than something you have to enable on each load. But again, a CSS variable would help make it a setting if you wanted to let users decide what they want to make of these asides.

I was actually considering using this variable as a "constant", not necessarily exposed in the user settings. For example, an app could have its own CSS file injected after ReadiumCSS, that would override some of the default CSS variables. Some kind of ReadiumCSS configuration. So not as dynamic as the user settings.

Also, I'm not very familiar with Voice Over, so forgive me: could it not read the contents of our pop-up window?

I would think that should work fine, but maybe there's an issue with breaking the flow in the WebView, we'll need to experiment I guess. As an alternative, we could revert to a normal WebView jump when clicking on a noteref, with the aside visible, when VoiceOver is enabled. Another reason to be able to disable this feature.

I don't think this is good UX. You get dropped at the end of a chapter--maybe, if the note is in the same file, which they typically are for sure--and there may be 1 or 10 footnotes, and then you have to see which one is being referred to, read it, and then navigate back.

Definitely, I wouldn't expect an app not showing the footnotes in some kind of pop-up. I'm just thinking that this might not be the responsibility of the navigator, which should be as simple as possible – which is already pretty complex – but with a lot of extension points. So far we've used the testapp for "best practices" on the presentation side.

I'm just thinking out loud for the sake of brainstorming though, and I'm not sure what's the right solution yet.

Considering the 2 main responsibilities of the navigator:

  • Navigating across a publication (either jumping to a locator, or moving through scroll/pagination)
  • Rendering the content (including audio playback for an audiobook)

We could have something a bit more generic to intercept a navigation event with added context, and let the reading app decide what to do. Something like that:

enum NavigationLocation {
    case locator(Locator)
    case link(Link)
    case resource(Link)
    case internal(href: String, title: String)
    case external(url: String, title: String)
    case note(href: String, type: String, content: String)
}

func navigator(_ navigator: Navigator, shouldNavigateTo location: NavigationLocation) -> Bool

@HadrienGardeur
Copy link

I don't think this is good UX. You get dropped at the end of a chapter--maybe, if the note is in the same file, which they typically are for sure--and there may be 1 or 10 footnotes, and then you have to see which one is being referred to, read it, and then navigate back.

The reason why iBooks and some Kindles have pop-up footnotes is because is creates a better UX. I don't think we gain anything by settling for less or crippling the default, as long as we have Voice Over sorted out, right?

This is not specific to footnotes and this is something that needs to be addressed for any link.

If a user jumps ahead or backward in a publication while following a link, we need to:

  • emit an event that can be intercepted by the host app and contains a locator
  • this way an app can provide its own UI for going back to where the user was

IMO saying that a popup is better than jumping to a locator is quite subjective, which is why we should not take this decision on behalf of app developers.
That said, we should make it easy to:

  • intercept specific events
  • get raw text or HTML for specific fragments, especially when they have a fragment identifier

There are many use cases for these two features and this goes beyond footnotes.

@tooolbox
Copy link
Contributor Author

tooolbox commented Apr 21, 2020

I would think that should work fine, but maybe there's an issue with breaking the flow in the WebView, we'll need to experiment I guess. As an alternative, we could revert to a normal WebView jump when clicking on a noteref, with the aside visible, when VoiceOver is enabled. Another reason to be able to disable this feature.

I imagine that jumping somewhere else when clicking on a noteref would break the flow too, right?

I'll note that I don't think that the testapp has a Back button implemented right now, which might interfere with implementing the feature that way.

Definitely, I wouldn't expect an app not showing the footnotes in some kind of pop-up. I'm just thinking that this might not be the responsibility of the navigator, which should be as simple as possible – which is already pretty complex – but with a lot of extension points. So far we've used the testapp for "best practices" on the presentation side.

It hurts a little bit, but I can accept it. I do think a footnote popup is very germane to navigation and display of content (the two responsibilities of the navigator, as you say). But I have less experience with these different libraries and components so if you want it in the app, I can respect that.

We could have something a bit more generic to intercept a navigation event with added context, and let the reading app decide what to do.

(This is worth discussing, but in my case I would like to keep my PR focused so I am making a minimum of changes while adding this functionality to match Android. Obviously I don't want to inhibit what you guys are planning but I think this is out of scope for me.)

@tooolbox
Copy link
Contributor Author

tooolbox commented Apr 22, 2020

IMO saying that a popup is better than jumping to a locator is quite subjective, which is why we should not take this decision on behalf of app developers.

As an app developer, I don't think it's very subjective. If the markup is <a epub:type="noteref"/> linking to an <aside epub:type="footnote"/> then the author has very effectively and thoroughly communicated that he wants a popup if the reading system can support it. In what scenario is an app developer going to want to ignore that intent?

It's even discussed in the DAISY docs: http://kb.daisy.org/publishing/docs/html/notes.html#desc

Footnotes and endnotes have proven an impediment to the reading experience because they interrupt the narrative flow. When footnotes are placed immediately following the paragraph that references them, users had to manually navigate past them each time, as they are typically indistinguishable from text content. Even endnotes, grouped at the end of the section, require the user to jump past them.

The HTML5 structural elements, together with the role attribute, provide a means of alleviating this problem, by clearly marking individual footnotes and endnotes, and sections of them. Not only does this allow accessible user agents to ignore the notes except when followed from their referents, but it allows any user agent to handle them more intelligently (e.g., as pop-ups). (emphasis added)

Seems pretty straightforward.

There are many use cases for these two features and this goes beyond footnotes.

I'm sure you're right, but I'm curious, what are those use cases? I think listing them out would help give perspective.

@mickael-menu
Copy link
Member

I imagine that jumping somewhere else when clicking on a noteref would break the flow too, right?

I expect that the WebView will handle basic hyperlink features properly with VoiceOver.

I'll note that I don't think that the testapp has a Back button implemented right now, which might interfere with implementing the feature that way.

Yes, that's what @HadrienGardeur was talking about, this will be something needed soon. Although an app could already implement it by building a history of the changed locations. But it would be better if we have a way to differentiate if we jumped to a location, or simply turned a page.

It hurts a little bit, but I can accept it. I do think a footnote popup is very germane to navigation and display of content (the two responsibilities of the navigator, as you say). But I have less experience with these different libraries and components so if you want it in the app, I can respect that.

We're getting into a broader discussion, and I'm afraid it will be a bit too much off-topic. But here we go. We probably all have slightly different opinions about what should be in Readium and what should live in the apps, but we already defined pretty clearly the scope of the navigators. Now Readium is an ever-evolving project, and we're not yet aligned with this strategy on all platforms.

What we don't want to do is limit reading apps, to avoid having implementors forking Readium like with R1.

Now I hear the need for basic features such as footnote popups, of which a default, sound implementation could be provided by Readium. And two complementary ways of achieving that have been on my mind lately:

  • Streamer/Navigator Plugins, which would be fully encapsulated ways to extend the more lower-level Readium toolkit. Readium could definitely provide default plugins for basic features, such as an EPUBFootnotePopupPlugin, which would intercept noteref navigation and present a pop-up, optionally customizable by reading apps.
  • r2-ux, which would be an additional library containing higher level UX components: Basically a lot of what's currently in r2-testapp. This would serve as an example on how to use the lower-level components, but also as ready-to-use blocks for reading apps that want a basic reader without caring too much about its features or its appearance. Stuff that could be in this: user settings dialog, reader with a progress bar, OPDS catalog view, etc. This would also help implementors upgrading Readium without having to check the changes in r2-testapp all the time.

(This is worth discussing, but in my case I would like to keep my PR focused so I am making a minimum of changes while adding this functionality to match Android. Obviously I don't want to inhibit what you guys are planning but I think this is out of scope for me.)

Yes, and thank you for the brainstorming opportunity that your post created 👍

I think for this PR we can stick with:

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

And have the pop-up implemented in the test app. This way we can improve this later without breaking reading apps, by pulling back the pop-up code in a plugin or something else.

@HadrienGardeur
Copy link

As an app developer, I don't think it's very subjective. If the markup is linking to an

then the author has very effectively and thoroughly communicated that he wants a popup if the reading system can support it. In what scenario is an app developer going to want to ignore that intent?

Sorry but I strongly disagree. aside just means "aside" and there are many different ways this can be handled, popups being just one of them.

There's not much in terms of guidance in the EPUB spec, app developers have a lot of room to decide what's best.

The HTML5 structural elements, together with the role attribute, provide a means of alleviating this problem, by clearly marking individual footnotes and endnotes, and sections of them. Not only does this allow accessible user agents to ignore the notes except when followed from their referents, but it allows any user agent to handle them more intelligently (e.g., as pop-ups). (emphasis added)

Seems pretty straightforward.

"It allows them" being the most important of the sentence. You're certainly not forced to do it, and you're also free to do it the way you want (pop-up is just used as an example).

Don't get me wrong, I have nothing against pop-up footnotes, I just don't think that this is as universal as you think.
I also believe that we need to be extra careful regarding what the SDK offers vs what app developers want to do.

We're quite opiniated in this project, but when it comes to such UX, it's subjective and should be entirely in the hands of developers.

@llemeurfr
Copy link
Contributor

llemeurfr commented Apr 22, 2020

There is IMO a balance to find between the flexibility the toolkit must offer (and for that I'm 100% with Mickaël and Hadrien) and the fact that many developers wish to integrate a toolkit which provides most of what is needed by default, i.e. off the shelf features (and there I tend to follow Matt).

Could we find this balance by having a default behavior in the navigator (popup with raw text) that can be triggered by the app (which therefore keeps the control with few lines of code) or be unused if the app takes full control of the footnote layout?

At the VisualNavigator interface level, it could mean adding a method "displayPopup" with the id of the footnote as param (aïe, issue if the note is in a different resource).

@HadrienGardeur
Copy link

HadrienGardeur commented Apr 22, 2020

Could we find this balance by having a default behavior in the navigator (popup with raw text) that can be triggered by the app (which therefore keeps the control with few lines of code) or be unused if the app takes full control of the footnote layout?

I disagree. It's not the responsibility of the navigator to handle such UI.

From one app to another this could be extremely different: for example there's a move towards using bottom sheets recently on Android, which are modal but not really what one would call a "pop-up".
Just to illustrate that with a few native components on Android that could fit this use case:

Add on top of that all the custom components that app developers are free to build...

Each app developer will be opiniated not only about how this is displayed (modal dialog, jump to reference, side panel display) but also how each of these UI elements are handled.

We can illustrate how APIs can be leveraged to handle this in the test app, but we need to draw a clear line between the navigator concerns and the app concerns.

@mickael-menu
Copy link
Member

It seems to me that the plugins and r2-ux I introduced above would answer both sides of the argument? It would also be more convenient than the test app because easier to integrate and upgrade for unopinionated developers.

@llemeurfr
Copy link
Contributor

llemeurfr commented Apr 22, 2020

@tooolbox just a comment on:

By default, the <aside> should be hidden because that's the most common scenario.

From what I read in the Apple guidelines and in the Daisy guidelines, the author does not hide the aside element. iBooks hides it automatically when it recognizes a footnote in an aside. It's not a mandatory app behavior however, by EPUB 3 (absence of) rules.

Helicon guidelines add "It is advised to put the aside tag at the bottom of the HTML file." which solves a visual issue if the app does not treat the footnote is a smart manner.

@tooolbox
Copy link
Contributor Author

tooolbox commented Apr 22, 2020

I expect that the WebView will handle basic hyperlink features properly with VoiceOver.

I think we are debating ourselves into a corner, within a vacuum. 😄 If you think regular hyperlinks will be handled correctly by VoiceOver, I'm not sure how pop-ups would be different, but I think I just need stop talking and look at it.

We probably all have slightly different opinions about what should be in Readium and what should live in the apps, but we already defined pretty clearly the scope of the navigators.

Okay, I read the blurb you linked to and that's pretty clear. I will note that the NavigatorDelegate protocol has a default implementation for handling external links.

Streamer/Navigator Plugins

Sure :) Sounds like quite a bit of architectural work, but great.

I think for this PR we can stick with:
func navigator(_ navigator: Navigator, shouldNavigateToNoteAt link: Link, content: String) -> Bool

Great, I am more-or-less down with that, I will take up some technical points on that in another comment.

@tooolbox
Copy link
Contributor Author

tooolbox commented Apr 23, 2020

Sorry but I strongly disagree. aside just means "aside" and there are many different ways this can be handled, popups being just one of them.

I have been a little inconsistent throughout this rather long thread, so it's possible you missed it, but I'm not talking about a generic <aside>, I am talking about <a epub:type="noteref"/> linking to <aside epub:type="footnote"/>. Does that make things more clear?

"It allows them" being the most important of the sentence. You're certainly not forced to do it, and you're also free to do it the way you want (pop-up is just used as an example).
Don't get me wrong, I have nothing against pop-up footnotes, I just don't think that this is as universal as you think.
I also believe that we need to be extra careful regarding what the SDK offers vs what app developers want to do.
We're quite opiniated in this project, but when it comes to such UX, it's subjective and should be entirely in the hands of developers.

Maybe you missed it, but one of the last things I said on this was:

Now, if we want the pop-up behavior to be customizable on a per-app basis, then to keep Navigator as a clean library, we should have a delegate method. But maybe there should be a default.

So, I'm not trying to limit customization, I'm supporting a sensible default.

It would be great if you could give examples of other UX besides a popup (or similar UI) showing the footnote. You seem very much in support of customization and you seem to have specific scenarios in mind but I don't know what they are. (Note that I am specifically talking about the above markup.)

There is IMO a balance to find between the flexibility the toolkit must offer (and for that I'm 100% with Mickaël and Hadrien) and the fact that many developers wish to integrate a toolkit which provides most of what is needed by default, i.e. off the shelf features (and there I tend to follow Matt).
Could we find this balance by having a default behavior in the navigator (popup with raw text) that can be triggered by the app (which therefore keeps the control with few lines of code) or be unused if the app takes full control of the footnote layout?

Yes, this is what I was getting at. The prior art is that the NavigatorDelegate protocol has a default implementation for handling external links.

I disagree. It's not the responsibility of the navigator to handle such UI. ...

Sure, so not the Navigator, but how about the NavigatorDelegate protocol? That way an App Developer who's using R2 and implementing a NavigatorDelegate doesn't have to implement this UI (or copy-paste it from your implementation) if he doesn't need/want to.

You say you disagree, but nothing in your post actually shows why "a fully customizable UI/UX with a sensible default" is undesirable.

Hm, if the goal is to keep all UI code out of the r2-navigator-swift repo, then maybe in r2-testapp-swift we could make a protocol that inherits from NavigatorDelegate and has the default implementation, and use that as the required type somehow. Not entirely sure if that's possible, but it occurred to me.

It seems to me that the plugins and r2-ux I introduced above would answer both sides of the argument? It would also be more convenient than the test app because easier to integrate and upgrade for unopinionated developers.

Sure, IMO this is superior to a default method on the NavigatorDelegate protocol, for one reason: to me, the whole package consists of (1) the popup handler and (2) CSS to hide the <aside epub:type="noteref"> elements by default. A plugin could theoretically handle both of those together, so the behavior is a more packaged unit.

While I do agree with you, "plugins" is a vague concept in my mind and implementing a default method for a protocol is a few keystrokes away. :)

@tooolbox
Copy link
Contributor Author

@llemeurfr

From what I read in the Apple guidelines and in the Daisy guidelines, the author does not hide the aside element. iBooks hides it automatically when it recognizes a footnote in an aside. It's not a mandatory app behavior however, by EPUB 3 (absence of) rules.

You're absolutely right, the hiding is done by the app, not the author. And yeah, good point that it's not mandatory.

Helicon guidelines add "It is advised to put the aside tag at the bottom of the HTML file." which solves a visual issue if the app does not treat the footnote is a smart manner.

Yeah, that makes sense. If I'm authoring a general ePub and write that specialized markup, I am hoping for popups, but I'm definitely putting the <aside> elements at the end of a chapter as they say.

@tooolbox
Copy link
Contributor Author

Okay, so I've opened a formative PR which is what I had written last night, before all of this discussion :) Aside from anything else, it does work. @mickael-menu shortly I will give some more thoughts on that method signature we've been discussing.

@tooolbox
Copy link
Contributor Author

tooolbox commented Apr 23, 2020

Okay, I wanted to give some notes on the PR:

  • 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 this thread, (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.

EDIT: Having looked at this, it doesn't really make any sense. The r2-testapp-swift/EPUBViewController creates a r2-navigator-swift/EPUBNavigatorViewController and then assigns self to its delegate. Plus, I realize, if we expect an app dev to throw away the testapp and just use r2-shared, r2-navigator etc. then any such "default" would have to live in one of those frameworks and we are back to square one.

Summary

As I said, I had done this work, and it's functional, so I wanted to make it visible as a starting point.

@mickael-menu
Copy link
Member

@tooolbox Thanks for the detailed explanation of your PR, that's really useful.

For information, we discussed about this issue in our weekly team call yesterday. You can find a summary here. But basically, we agreed that such features should not be in R2Navigator directly, but the navigator should provide building blocks for reading apps to implement them.

Now to facilitate usage of default implementations of these features by app developers, a plugin architecture was deemed too complex at the moment. Instead, we considered splitting the R2TestApp project in 2 (but in the same repository):

  • A high-level UX library implementing all the default features you could expect from a reading app: footnote pop-ups, side-taps to turn pages, etc. It would also serve as an example on how to use the lower-level Navigator for advanced use cases. This library would be versioned and distributed to app developers, and componentized as much as possible to allow cherry-picking features.
  • A sample test app, much simpler than the current one, which would use this library and could serve as a starting point for developers.

I think we are debating ourselves into a corner, within a vacuum. 😄 If you think regular hyperlinks will be handled correctly by VoiceOver, I'm not sure how pop-ups would be different, but I think I just need stop talking and look at it.

Yes I think so too, maybe I was ambiguous.

Okay, I read the blurb you linked to and that's pretty clear. I will note that the NavigatorDelegate protocol has a default implementation for handling external links.

That's a bit different, there's nothing we can do in the navigator with external links, because opening them in the navigator's web view would break everything. You'll notice that the default implementation (opening the system default browser) is different from the recommended one we have in the test app (presenting a SFSafariViewController). I added the minimum behavior necessary to not break the external links if an app doesn't implement this delegate method.

In the case of footnotes, we already have a default behavior provided by the WebView: following the link, like any other internal link.

It would be great if you could give examples of other UX besides a popup (or similar UI) showing the footnote.

@HadrienGardeur gave a few here (albeit for Kotlin): #139 But to add different kind of examples, you could have different strategies on how to display the HTML: in a WKWebView, with or without the styles of the document, in a UILabel with an NSAttributedString, etc.

Sure, so not the Navigator, but how about the NavigatorDelegate protocol? That way an App Developer who's using R2 and implementing a NavigatorDelegate doesn't have to implement this UI (or copy-paste it from your implementation) if he doesn't need/want to.

Like you noticed, it gets complicated to present stuff from the context of the Navigator (which should not inherit from UIViewController). Also, having this as a default makes it complicated to rely on the default hyperlink behavior, since it's hijacked. I know you consider that this is not a viable way of navigating to footnotes, but this is really not as obvious. The Navigator is not meant to be used only with EPUB, for example, WebPub doesn't have such feature as noteref, as far as I know it would rely on default hyperlinks for footnotes.

About avoiding copy-pasting, I think that the UX library mentioned above should answer this?


About the PR:

SwiftSoup dependency added, mirroring the JSoup dep used in r2-navigator-kotlin.

Do we really need SwiftSoup or could Fuzi do the job as well? If we can avoid another dependency that would be best. If we can't, that's fine, especially since SwiftSoup might be useful to implement CSS/JS injection as well later on.

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.

Yes we shouldn't do that. I'm working right now on an implementation of an AudiobookNavigator, and it doesn't make sense to attach a view to it. That's another argument to avoid UX in the Navigator layer, since an Audio Navigator doesn't render anything visually.

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.

I agree.

In Swift, getting the suffix after a specific character is atrocious.

Swift String API is garbage... So complicated.

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

It seems like if we can't solve this problem, that's going to be complicated to not have the pop-up directly in the navigator. Let's focus on that before discussing the rest.

WKNavigationDelegate.webView(_:decidePolicyFor:decisionHandler:) seems like the correct way to handle that to me, we need to find a way to get more information about the link. Maybe having a JS script executed during onLoad which would query all epub:type="noteref" and send the list of href back to the navigator. That could fail in 2 ways:

  • If a book creates a noteref dynamically, but it would fall back on normal hyperlinking so not such a big issue.
  • If a link doesn't have epub:type="noteref" but still points to the footnote href. That sounds like an authoring error to me, and it would be presented as a pop-up which is probably not so bad.

Maybe we could also send information about the current link in onClick to the Swift side. But that's assuming that the information will be received before webView(_:decidePolicyFor:decisionHandler:). It would probably be the best/simplest, but it might also break in future versions of WKWebView. Breaking meaning reverting back to normal hyperlinking until we fix it, which is not too bad.

@llemeurfr
Copy link
Contributor

WebPub doesn't have such feature as noteref, as far as I know it would rely on default hyperlinks for footnotes.

Why? Web Publications are XHTML5 pages glued together with a JSON manifest. Many Web Publications will be dynamically created out of an EPUB via a Publication Server (i.e. a Streamer). So they may have footnotes using the epub:type notation.

@mickael-menu
Copy link
Member

You're right, many WebPubs will be repackaged EPUBs. However, the WebPub spec itself doesn't contain anything specific for footnotes, so it won't apply for WebPubs created outside an EPUB context, for example from webpages.

@tooolbox
Copy link
Contributor Author

@mickael-menu Thanks for the review & feedback!

For information, we discussed about this issue in our weekly team call yesterday. You can find a summary here. But basically, we agreed that such features should not be in R2Navigator directly, but the navigator should provide building blocks for reading apps to implement them.

Okay, tracking.

Now to facilitate usage of default implementations of these features by app developers, a plugin architecture was deemed too complex at the moment. Instead, we considered splitting the R2TestApp project in 2 (but in the same repository):

  • A high-level UX library implementing all the default features you could expect from a reading app: footnote pop-ups, side-taps to turn pages, etc. It would also serve as an example on how to use the lower-level Navigator for advanced use cases. This library would be versioned and distributed to app developers, and componentized as much as possible to allow cherry-picking features.
  • A sample test app, much simpler than the current one, which would use this library and could serve as a starting point for developers.

Sounds like a lot of work, but makes sense. I do think your team's goals with making this very customizable and helpful are great. I could probably give some feedback at some point or another on my experience with customizing R2, I had one point which I think would help DX but isn't germane to this thread.

So it seems like at this point most of the pop-up stuff will be handled in my App, meaning injecting CSS to hide the <aside epub:type="footnote"> and the note display UI.


That's a bit different, there's nothing we can do in the navigator with external links, because opening them in the navigator's web view would break everything. You'll notice that the default implementation (opening the system default browser) is different from the recommended one we have in the test app (presenting a SFSafariViewController). I added the minimum behavior necessary to not break the external links if an app doesn't implement this delegate method.

In the case of footnotes, we already have a default behavior provided by the WebView: following the link, like any other internal link.

Okay, tracking.

@HadrienGardeur gave a few here (albeit for Kotlin): #139 (comment) But to add different kind of examples, you could have different strategies on how to display the HTML: in a WKWebView, with or without the styles of the document, in a UILabel with an NSAttributedString, etc.

I mentioned this over in readium/readium-css#83 but those examples aren't what I was looking for; they're all basically the same UX of displaying the note in native UI, and as long as the dev can customize what the UI is, I was having a hard time seeing why you wouldn't want to use that UI in every case where the markup indicated it. The question was "in what example scenario, given the aforementioned markup, do you not want to display the note in the native UI of your choice?"

Regardless, @JayPanoz did provide an example of an interesting implementation where the use of a pop-up could vary based on the screen width. Actually, you would probably do a media query and set pointer-events: none; on your footnote hyperlinks, so you don't need to have different app behavior between the two modes, but the point stands.

Like you noticed, it gets complicated to present stuff from the context of the Navigator (which should not inherit from UIViewController).

Tracking, makes sense...

Also, having this as a default makes it complicated to rely on the default hyperlink behavior, since it's hijacked. I know you consider that this is not a viable way of navigating to footnotes, but this is really not as obvious. The Navigator is not meant to be used only with EPUB, for example, WebPub doesn't have such feature as noteref, as far as I know it would rely on default hyperlinks for footnotes.

I don't follow you here. I understand that the Navigator isn't meant to be used only with EPUB, but if the markup is <a epub:type="noteref"> then I think that's a strong indicator it's an EPUB, no? All hyperlinks except for this style are treated normally, so I think you can continue to rely on the default behavior, right?

About avoiding copy-pasting, I think that the UX library mentioned above should answer this?

👍


Do we really need SwiftSoup or could Fuzi do the job as well? If we can avoid another dependency that would be best. If we can't, that's fine, especially since SwiftSoup might be useful to implement CSS/JS injection as well later on.

Huh! I will scope out Fuzi and see if it will work.

Yes we shouldn't do that. I'm working right now on an implementation of an AudiobookNavigator, and it doesn't make sense to attach a view to it. That's another argument to avoid UX in the Navigator layer, since an Audio Navigator doesn't render anything visually.

Interesting... I mean, it must display something, right? But I get your point, we won't go this route.

I'll discuss the API/method signature next.

@mickael-menu
Copy link
Member

I could probably give some feedback at some point or another on my experience with customizing R2, I had one point which I think would help DX but isn't germane to this thread.

Any feedback, especially concerning pain points integrating/extending Readium, is most welcome! We're trying to improve a lot in this area right now, so it's the right time to take current issues into account.

We have Slack channels if you're interested in discussing more informally about that.

I mentioned this over in readium/readium-css#83 but those examples aren't what I was looking for; they're all basically the same UX of displaying the note in native UI, and as long as the dev can customize what the UI is, I was having a hard time seeing why you wouldn't want to use that UI in every case where the markup indicated it. The question was "in what example scenario, given the aforementioned markup, do you not want to display the note in the native UI of your choice?"

I see, then indeed I'm not sure if there are much more viable options besides normal webview jumps or footnote popups.

I don't follow you here. I understand that the Navigator isn't meant to be used only with EPUB, but if the markup is then I think that's a strong indicator it's an EPUB, no?

My bad, my point was unclear and ambiguous. It was one potential reason for a reading app to want to keep normal hyperlinking behavior: to have the same footnote behavior across all formats: EPUB, and the formats not having noteref, such as WebPub packaged from regular webpages. See the principle of least astonishment. If that's a good idea or not, that I do not know. You would need UX research like @JayPanoz mentioned in the ReadiumCSS PR.

Huh! I will scope out Fuzi and see if it will work.

Thanks, but don't worry too much if it doesn't, I considered integrating SwiftSoup at some point for the JS/CSS injection.

Interesting... I mean, it must display something, right? But I get your point, we won't go this route.

In the reading app, there's a UIViewController displaying buttons to control the Navigator playback and the cover. While footnote pop-ups could pass as being not too opinionated in the Navigator, a media player could be designed in hundred of ways, and you might even want to have a background playback with a "mini" version of the player view somewhere in your app.

@tooolbox
Copy link
Contributor Author

It seems like if we can't solve this problem, that's going to be complicated to not have the pop-up directly in the navigator. Let's focus on that before discussing the rest.

👍

WKNavigationDelegate.webView(_:decidePolicyFor:decisionHandler:) seems like the correct way to handle that to me, we need to find a way to get more information about the link. Maybe having a JS script executed during onLoad which would query all epub:type="noteref" and send the list of href back to the navigator. That could fail in 2 ways:

  • If a book creates a noteref dynamically, but it would fall back on normal hyperlinking so not such a big issue.
  • If a link doesn't have epub:type="noteref" but still points to the footnote href. That sounds like an authoring error to me, and it would be presented as a pop-up which is probably not so bad.

If it didn't have epub:type="noteref" then IMO we wouldn't be obligated to display a pop-up, so I don' t think we need to worry about that scenario.

Otherwise, while this is a bright idea, and I do think you're right about decidePolicyFor being the ideal hook-in point, I'm not terribly excited about the exact method you propose.

As an idea, we could perhaps somehow inject relevant data into the URL. Add a query param, change the scheme...

Actually, if the delegate method returned false we could inject the JS window.location=\(xyz) into the webview. That solves it, no?

Maybe we could also send information about the current link in onClick to the Swift side. But that's assuming that the information will be received before webView(_:decidePolicyFor:decisionHandler:). It would probably be the best/simplest, but it might also break in future versions of WKWebView. Breaking meaning reverting back to normal hyperlinking until we fix it, which is not too bad.

Yeah, I'm not too keen on the whole semaphore thing between a click handler & decidePolicyFor. We could but I want to check out other options first, as above.

@mickael-menu
Copy link
Member

Actually, if the delegate method returned false we could inject the JS window.location=(xyz) into the webview. That solves it, no?

Yes that's an interesting solution, definitely worth exploring.

Yeah, I'm not too keen on the whole semaphore thing between a click handler & decidePolicyFor. We could but I want to check out other options first, as above.

Actually I wasn't talking about a semaphore, but simply sending a message through webkit.messageHandlers.x.postMessage() and see if it gets predictably received before webView(_:decidePolicyFor:decisionHandler:) or not.

@mickael-menu
Copy link
Member

mickael-menu commented Apr 23, 2020

Actually I wasn't talking about a semaphore, but simply sending a message through webkit.messageHandlers.x.postMessage() and see if it gets predictably received before webView(_:decidePolicyFor:decisionHandler:) or not.

@tooolbox I just checked and it seems to be the case, so you could potentially send information about the upcoming navigation action right from the click handler. That might potentially break in a new version of the WKWebView though, since I don't think it's documented. But I think it's safe enough, and it wouldn't break normal hyperlinking.

@tooolbox
Copy link
Contributor Author

tooolbox commented Apr 23, 2020

Actually I wasn't talking about a semaphore, but simply sending a message through webkit.messageHandlers.x.postMessage() and see if it gets predictably received before webView(_:decidePolicyFor:decisionHandler:) or not.

Sorry, that's kinda what I mean by semaphore, i.e. a variable or flag shared between two different execution contexts—normally concurrently, but not in this case. A similar thing is implemented in Kotlin whereby the click handler sets overrideUrlLoading = false and the equivalent of decidePolicyFor checks that variable to determine whether to let that request through. (The code is a tad confusing, since it seems like it should be named doNotOverrideUrlLoading based on the behavior I see.) It seems to be doing what you're describing, and I'm calling that overrideUrlLoading a "semaphore" rightly or wrongly, because it's being used to share data between two different callbacks.

(Specifically, what I understand is that you're saying is to use postMessage to set a variable which is then read by decidePolicyFor and I'm calling that variable a "semaphore". Anyway, enough with my semantics. 😄 )

@tooolbox I just checked and it seems to be the case, so you could potentially send information about the upcoming navigation action right from the click handler. That might potentially break in a new version of the WKWebView though, since I don't think it's documented. But I think it's safe enough, and it wouldn't break normal hyperlinking.

Hm! Okay...okay...I think I get your concept and I'm liking it more. I felt weird about a simple "allow/disallow" sempahore between the click handler and decidePolicyFor but if we made a variable like "click data" so it's more generic, it becomes less hacky in my mind.

I'll take a shot at implementing it today and let you know!

tooolbox referenced this issue in tooolbox/r2-navigator-swift Apr 24, 2020
… navigator, made touch handling always send data across the bridge, noteref delegate method returns bool.
@tooolbox
Copy link
Contributor Author

Okay @mickael-menu so I re-worked it, a few points:

  • Using your suggestion of 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 what you & @HadrienGardeur wanted regarding 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.

I'm pretty happy with it at this point; if you're fine then we can move into a formal review from the code owner.

@mickael-menu
Copy link
Member

@tooolbox That sounds great! I'll follow up in the PR itself on minutiae, once I review the code.

I can see that being used by reading apps to intercept other kind of links 👍

Thanks a lot for this great contribution and interesting brainstorming.

@mickael-menu mickael-menu transferred this issue from readium/r2-navigator-swift Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants