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.

Sign up for GitHub

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

add findElementLocator #273

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

add findElementLocator #273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions Sources/Navigator/EPUB/Assets/Static/scripts/readium-fixed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1622,7 +1622,8 @@ window.addEventListener("load", function () {
__webpack_require__.r(__webpack_exports__);
/* harmony export */ __webpack_require__.d(__webpack_exports__, {
/* harmony export */ "findNearestInteractiveElement": () => (/* binding */ findNearestInteractiveElement),
/* harmony export */ "findFirstVisibleLocator": () => (/* binding */ findFirstVisibleLocator)
/* harmony export */ "findFirstVisibleLocator": () => (/* binding */ findFirstVisibleLocator),
/* harmony export */ "findLocatorAtPoint": () => (/* binding */ findLocatorAtPoint)
/* harmony export */ });
/* harmony import */ var _utils__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! ./utils */ "./src/utils.js");
/* harmony import */ var css_selector_generator__WEBPACK_IMPORTED_MODULE_1__ = __webpack_require__(/*! css-selector-generator */ "./node_modules/css-selector-generator/build/index.js");
Expand Down Expand Up @@ -1681,6 +1682,24 @@ function findFirstVisibleLocator() {
}
};
}
function findLocatorAtPoint(x, y) {
var element = document.elementFromPoint(x, y);

if (!element) {
return undefined;
}

return {
href: "#",
type: "application/xhtml+xml",
locations: {
cssSelector: (0,css_selector_generator__WEBPACK_IMPORTED_MODULE_1__.getCssSelector)(element)
},
text: {
highlight: element.textContent
}
};
}

function findElement(rootElement) {
var foundElement = undefined;
Expand Down Expand Up @@ -1863,7 +1882,8 @@ __webpack_require__.g.readium = {
registerDecorationTemplates: _decorator__WEBPACK_IMPORTED_MODULE_4__.registerTemplates,
getDecorations: _decorator__WEBPACK_IMPORTED_MODULE_4__.getDecorations,
// DOM
findFirstVisibleLocator: _dom__WEBPACK_IMPORTED_MODULE_2__.findFirstVisibleLocator
findFirstVisibleLocator: _dom__WEBPACK_IMPORTED_MODULE_2__.findFirstVisibleLocator,
findLocatorAtPoint: _dom__WEBPACK_IMPORTED_MODULE_2__.findLocatorAtPoint
};

/***/ }),
Expand Down
24 changes: 22 additions & 2 deletions Sources/Navigator/EPUB/Assets/Static/scripts/readium-reflowable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1622,7 +1622,8 @@ window.addEventListener("load", function () {
__webpack_require__.r(__webpack_exports__);
/* harmony export */ __webpack_require__.d(__webpack_exports__, {
/* harmony export */ "findNearestInteractiveElement": () => (/* binding */ findNearestInteractiveElement),
/* harmony export */ "findFirstVisibleLocator": () => (/* binding */ findFirstVisibleLocator)
/* harmony export */ "findFirstVisibleLocator": () => (/* binding */ findFirstVisibleLocator),
/* harmony export */ "findLocatorAtPoint": () => (/* binding */ findLocatorAtPoint)
/* harmony export */ });
/* harmony import */ var _utils__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! ./utils */ "./src/utils.js");
/* harmony import */ var css_selector_generator__WEBPACK_IMPORTED_MODULE_1__ = __webpack_require__(/*! css-selector-generator */ "./node_modules/css-selector-generator/build/index.js");
Expand Down Expand Up @@ -1681,6 +1682,24 @@ function findFirstVisibleLocator() {
}
};
}
function findLocatorAtPoint(x, y) {
var element = document.elementFromPoint(x, y);

if (!element) {
return undefined;
}

return {
href: "#",
type: "application/xhtml+xml",
locations: {
cssSelector: (0,css_selector_generator__WEBPACK_IMPORTED_MODULE_1__.getCssSelector)(element)
},
text: {
highlight: element.textContent
}
};
}

function findElement(rootElement) {
var foundElement = undefined;
Expand Down Expand Up @@ -1863,7 +1882,8 @@ __webpack_require__.g.readium = {
registerDecorationTemplates: _decorator__WEBPACK_IMPORTED_MODULE_4__.registerTemplates,
getDecorations: _decorator__WEBPACK_IMPORTED_MODULE_4__.getDecorations,
// DOM
findFirstVisibleLocator: _dom__WEBPACK_IMPORTED_MODULE_2__.findFirstVisibleLocator
findFirstVisibleLocator: _dom__WEBPACK_IMPORTED_MODULE_2__.findFirstVisibleLocator,
findLocatorAtPoint: _dom__WEBPACK_IMPORTED_MODULE_2__.findLocatorAtPoint
};

/***/ }),
Expand Down
8 changes: 8 additions & 0 deletions Sources/Navigator/EPUB/EPUBNavigatorViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,14 @@ open class EPUBNavigatorViewController: UIViewController, VisualNavigator, Selec
}
spreadView.findFirstVisibleElementLocator(completion: completion)
}

public func elementLocator(at point: CGPoint, completion: @escaping (Locator?) -> Void) {
guard let spreadView = paginationView.currentView as? EPUBSpreadView else {
DispatchQueue.main.async { completion(nil) }
return
}
spreadView.findElementLocator(at: point, completion: completion)
}

/// Last current location notified to the delegate.
/// Used to avoid sending twice the same location.
Expand Down
18 changes: 18 additions & 0 deletions Sources/Navigator/EPUB/EPUBSpreadView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,24 @@ class EPUBSpreadView: UIView, Loggable, PageView {
}
}


func findElementLocator(at point: CGPoint, completion: @escaping (Locator?) -> Void) {
let x = Int(point.x - webView.frame.minX)
let y = Int(point.y - webView.frame.minY)
Comment on lines +342 to +343
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is useful stuff but also easy to get it wrong, depending on the iOS version or FXL vs reflowable layouts.

Maybe we could add an equivalent to these two APIs but in the other direction (fromNavigatorSpace)?

swift-toolkit/Sources/Navigator/EPUB/EPUBSpreadView.swift

Lines 216 to 226 in 4f082ee

/// Converts the given JavaScript point into a point in the webview's coordinate space.
func convertPointToNavigatorSpace(_ point: CGPoint) -> CGPoint {
// To override in subclasses.
return point
}
/// Converts the given JavaScript rect into a rect in the webview's coordinate space.
func convertRectToNavigatorSpace(_ rect: CGRect) -> CGRect {
// To override in subclasses.
return rect
}

For inspiration, here's the reflowable implementation:

swift-toolkit/Sources/Navigator/EPUB/EPUBReflowableSpreadView.swift

Lines 133 to 158 in 4f082ee

override func convertPointToNavigatorSpace(_ point: CGPoint) -> CGPoint {
var point = point
if isScrollEnabled {
// Starting from iOS 12, the contentInset are not taken into account in the JS touch event.
if #available(iOS 12.0, *) {
if scrollView.contentOffset.x < 0 {
point.x += abs(scrollView.contentOffset.x)
}
if scrollView.contentOffset.y < 0 {
point.y += abs(scrollView.contentOffset.y)
}
} else {
point.x += scrollView.contentInset.left
point.y += scrollView.contentInset.top
}
}
point.x += webView.frame.minX
point.y += webView.frame.minY
return point
}
override func convertRectToNavigatorSpace(_ rect: CGRect) -> CGRect {
var rect = rect
rect.origin = convertPointToNavigatorSpace(rect.origin)
return rect
}

and the FXL implementation:

swift-toolkit/Sources/Navigator/EPUB/EPUBFixedSpreadView.swift

Lines 100 to 115 in 4f082ee

override func convertPointToNavigatorSpace(_ point: CGPoint) -> CGPoint {
CGPoint(
x: point.x * scrollView.zoomScale - scrollView.contentOffset.x + webView.frame.minX,
y: point.y * scrollView.zoomScale - scrollView.contentOffset.y + webView.frame.minY
)
}
override func convertRectToNavigatorSpace(_ rect: CGRect) -> CGRect {
var rect = rect
rect.origin = convertPointToNavigatorSpace(rect.origin)
rect.size = CGSize(
width: rect.width * scrollView.zoomScale,
height: rect.height * scrollView.zoomScale
)
return rect
}

evaluateScript("readium.findLocatorAtPoint(\(x), \(y))") { result in
DispatchQueue.main.async {
do {
let resource = self.spread.leading
let locator = try Locator(json: result.get())?
.copy(href: resource.href, type: resource.type ?? MediaType.xhtml.string)
completion(locator)
} catch {
self.log(.error, error)
completion(nil)
}
}
}
}

// MARK: - JS Messages

Expand Down
18 changes: 18 additions & 0 deletions Sources/Navigator/EPUB/Scripts/src/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,24 @@ export function findFirstVisibleLocator() {
};
}

export function findLocatorAtPoint(x, y) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind refactoring to avoid the code duplication with findFirstVisibleLocator? Maybe adding a locatorToElement() function.

const element = document.elementFromPoint(x, y);
if (!element) {
return undefined;
}

return {
href: "#",
type: "application/xhtml+xml",
locations: {
cssSelector: getCssSelector(element),
},
text: {
highlight: element.textContent,
},
};
}

function findElement(rootElement) {
var foundElement = undefined;
for (var i = rootElement.children.length - 1; i >= 0; i--) {
Expand Down
7 changes: 6 additions & 1 deletion Sources/Navigator/EPUB/Scripts/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@

import "./gestures";
import "./keyboard";
import { findFirstVisibleLocator } from "./dom";
import {
findFirstVisibleLocator,
findLocatorAtPoint,
} from "./dom";

import {
removeProperty,
scrollLeft,
Expand Down Expand Up @@ -37,4 +41,5 @@ global.readium = {

// DOM
findFirstVisibleLocator: findFirstVisibleLocator,
findLocatorAtPoint: findLocatorAtPoint,
};
9 changes: 9 additions & 0 deletions Sources/Navigator/VisualNavigator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ public protocol VisualNavigator: Navigator {

/// Returns the `Locator` to the first content element that begins on the current screen.
func firstVisibleElementLocator(completion: @escaping (Locator?) -> Void)

/// Returns the `Locator` to the first content element located at the given point
func elementLocator(at point: CGPoint, completion: @escaping (Locator?) -> Void)
Comment on lines +35 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest renaming elementLocator() to just locator(). In firstVisibleElementLocator, the "element" part was related to "first visible element".

Suggested change
/// Returns the `Locator` to the first content element located at the given point
func elementLocator(at point: CGPoint, completion: @escaping (Locator?) -> Void)
/// Returns the `Locator` to the content visible at the given view coordinates.
func locator(at point: CGPoint, completion: @escaping (Locator?) -> Void)

}

public extension VisualNavigator {
Expand All @@ -40,6 +43,12 @@ public extension VisualNavigator {
completion(currentLocation)
}
}

func elementLocator(at point: CGPoint, completion: @escaping (Locator?) -> Void) {
DispatchQueue.main.async {
completion(currentLocation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize you reused the default logic from firstVisibleElementLocator, but I wonder if it wouldn't be better to return nil if it's not implemented in a navigator, for this API?

When an app calls firstVisibleElementLocator(), it's to get more or less to the top of the page, so currentLocation is alright (although maybe it should have been nil too). But when you intend to get a Locator to a specific point, it might be confusing if you get the top of the page.

Apps can still fallback on currentLocation manually when receiving nil.

}
}

@discardableResult
func goLeft(animated: Bool = false, completion: @escaping () -> Void = {}) -> Bool {
Expand Down