Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion navigator-html-injectables/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
"devDependencies": {
"@juggle/resize-observer": "^3.4.0",
"@readium/shared": "workspace:*",
"css-selector-generator": "^3.6.4",
"css-selector-generator": "^3.6.9",
"tslib": "^2.6.1",
"typescript": "^5.4.5",
"vite": "^4.5.5"
Expand Down
33 changes: 30 additions & 3 deletions navigator-html-injectables/src/helpers/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,19 @@ export function nearestInteractiveElement(element: Element): Element | null {
/// Returns the `Locator` object to the first block element that is visible on
/// the screen.
export function findFirstVisibleLocator(wnd: ReadiumWindow, scrolling: boolean) {
const element = findElement(wnd, wnd.document.body, scrolling);
const element = findElement(wnd, wnd.document.body, scrolling) as HTMLElement;

// Use only the allowed selectors to generate the cssSelector and avoid a crash
const cssSelector = wnd._readium_cssSelectorGenerator.getCssSelector(element, {
selectors: ["tag", "id", "class", "nthchild", "nthoftype", "attribute"]
});

return new Locator({
href: "#",
type: "application/xhtml+xml",
locations: new LocatorLocations({
otherLocations: new Map([
["cssSelector", wnd._readium_cssSelectorGenerator.getCssSelector(element)]
["cssSelector", cssSelector]
])
}),
text: new LocatorText({
Expand All @@ -87,6 +93,9 @@ function findElement(wnd: ReadiumWindow, rootElement: Element, scrolling: boolea
for (var i = 0; i < rootElement.children.length; i++) {
const child = rootElement.children[i];
if (!shouldIgnoreElement(child) && isElementVisible(wnd, child, scrolling)) {
// Once we get a fully visible element, return it
if (isElementFullyVisible(wnd, child)) return child;
// if the parent is not fully visible, search in the childs
return findElement(wnd, child, scrolling);
}
}
Expand All @@ -111,11 +120,29 @@ function isElementVisible(wnd: ReadiumWindow, element: Element, scrolling: boole
}
}

/**
* Check if an element is fully visible in the current viewport.
* @param wnd Window instance to operate on
* @param element Element to check visibility of
* @returns True if the element is fully visible, false otherwise
*/
function isElementFullyVisible(wnd: ReadiumWindow, element: Element): boolean {
const rect = element.getBoundingClientRect();
const isFullyVisible =
rect.top >= 0 &&
rect.left >= 0 &&
rect.bottom <= wnd.innerHeight &&
rect.right <= wnd.innerWidth;
return isFullyVisible;
}

function shouldIgnoreElement(element: Element) {
const elStyle = getComputedStyle(element);
if (elStyle) {
const display = elStyle.getPropertyValue("display");
if (display != "block") {
// Added list-item as it is a common display property for list items
// TODO: Check if there are other display properties that should be ignored/considered
if (display != "block" && display != "list-item") {
Copy link
Member

Choose a reason for hiding this comment

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

@oscar-rivera-demarque can you explain why we can only allow block and and list-item as the display values? For example, why not inline-block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chocolatkey well, that was actually the question I had about why the display block was necessary here? (As it was working with that validation before).
I'm not sure what we're validating using the display != "block" line. I assumed it's issued because at some point the elements had the display behavior changed based on if they were loaded/allowed, or not. But this time I was not able to confirm if that was the expectation.
That's why I just simply added the display list-item, as a new allowed property, so that way I don't interfere in the current working behavior, but for sure we can review if maybe this check is not even necessary.

return true;
}
// Cannot be relied upon, because web browser engine reports invisible when out of view in
Expand Down
2 changes: 1 addition & 1 deletion navigator-html-injectables/src/modules/Peripherals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export class Peripherals extends Module {
targetFrameSrc: this.wnd.location.href,
targetElement: (event.target as Element).outerHTML,
interactiveElement: nearestInteractiveElement(event.target as Element)?.outerHTML,
cssSelector: this.wnd._readium_cssSelectorGenerator.getCssSelector(event.target),
cssSelector: this.wnd._readium_cssSelectorGenerator.getCssSelector(event.target as Element),
} as FrameClickEvent);

this.pointerMoved = false;
Expand Down
4 changes: 2 additions & 2 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.