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

#8380: add isInViewport property to element reader #8381

Merged
merged 4 commits into from
May 1, 2024
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
34 changes: 34 additions & 0 deletions src/bricks/readers/ElementReader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,19 @@

import { ElementReader } from "@/bricks/readers/ElementReader";
import { validateUUID } from "@/types/helpers";
import { rectFactory } from "@/testUtils/factories/domFactories";

const reader = new ElementReader();

describe("ElementReader", () => {
beforeEach(() => {
// `jsdom` does not implement full layout engine
// https://github.com/jsdom/jsdom#unimplemented-parts-of-the-web-platform
(Element.prototype.getBoundingClientRect as any) = jest.fn(() =>
rectFactory(),
);
});

test("it produces valid element reference", async () => {
const div = document.createElement("div");
const { ref } = await reader.read(div);
Expand All @@ -44,4 +53,29 @@ describe("ElementReader", () => {
const { isVisible } = await reader.read(div);
expect(isVisible).toBe(true);
});

test("isInViewport: true for element in document", async () => {
const div = document.createElement("div");
div.innerHTML = "<p>Some text</p>";
document.body.append(div);

const { isInViewport } = await reader.read(div);
expect(isInViewport).toBe(true);
});

test("isInViewport: false for element partially outside of document", async () => {
(Element.prototype.getBoundingClientRect as any) = jest.fn(() =>
rectFactory({
width: window.innerWidth + 1,
right: window.innerWidth + 1,
}),
);

const div = document.createElement("div");
div.innerHTML = "<p>Some text</p>";
document.body.append(div);

const { isInViewport } = await reader.read(div);
expect(isInViewport).toBe(false);
});
});
17 changes: 15 additions & 2 deletions src/bricks/readers/ElementReader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { getReferenceForElement } from "@/contentScript/elementReference";
import { ReaderABC } from "@/types/bricks/readerTypes";
import { type SelectorRoot } from "@/types/runtimeTypes";
import { type Schema } from "@/types/schemaTypes";
import { isVisible } from "@/utils/domUtils";
import { isInViewport, isVisible } from "@/utils/domUtils";

/**
* Read attributes, text, etc. from an HTML element.
Expand Down Expand Up @@ -49,6 +49,7 @@ export class ElementReader extends ReaderABC {
return {
ref: getReferenceForElement(element),
isVisible: isVisible(element),
isInViewport: isInViewport(element),
tagName: element.tagName,
attrs: Object.fromEntries(
Object.values(element.attributes).map((x) => [x.name, x.value]),
Expand Down Expand Up @@ -89,8 +90,20 @@ export class ElementReader extends ReaderABC {
type: "boolean",
description: "True if the element is visible",
},
isInViewport: {
type: "boolean",
description: "True if element is completely in the viewport",
},
},
required: ["tagName", "attrs", "data", "text", "ref", "isVisible"],
required: [
"tagName",
"attrs",
"data",
"text",
"ref",
"isVisible",
"isInViewport",
],
additionalProperties: false,
};

Expand Down
29 changes: 29 additions & 0 deletions src/utils/domUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,40 @@ export async function waitForBody(): Promise<void> {

/**
* Return true if the element is visible (i.e. not in `display: none`), even if outside the viewport
* See https://developer.mozilla.org/en-US/docs/Web/API/Element/checkVisibility
*
* To determine if an element is fully contained within the viewport, use `isInViewport`.
*
* @see isInViewport
*/
export function isVisible(element: HTMLElement): boolean {
return element.checkVisibility();
}

/**
* Returns true if the element is completely in the viewport. The method does not consider the element's
* opacity, z-index, or other CSS properties that may affect visibility.
*
* Example scenarios where the method may return counterintuitive results:
* - Elements hidden by z-index
* - If they are hidden by overflow-scroll in element's container
* - Things hidden by relative/absolute positioning
* - Elements within an iframe that is not entirely visible in the top frame
*
* @see isVisible
*/
export function isInViewport(element: HTMLElement): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should note on this method that this doesn't work for elements hidden by z-index overlapped, or if they are hidden by overflow-scroll in element's container. This may also have issues with things hidden by relative/absolute positioning, and if they are within an iframe that is not entirely visible in the top frame.

// https://stackoverflow.com/a/125106
const rect = element.getBoundingClientRect();
return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also check isVisible?

Copy link
Contributor Author

@twschiller twschiller May 1, 2024

Choose a reason for hiding this comment

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

We also have isVisible exposed on ElementReader. So I think it's good to keep them separate.

Although, related to your comment here: #8381 (comment), the devil's advocate is that ideally for the customer's use case we'd expose a property that captures whether or not the element would be fully captured by our Screenshot Brick.

We just happen to know there's no funny business going on the site they need to screenshot, so the method as-is would be sufficient.

Perhaps there's a better name we could use the method/output property? Although, from the Google/SO Results, this is what most developers seem to call it. (Although a good proportion of them seem to use it to mean the element is at least partially in the viewport)

Some other name options for brainstorming:

  • isFullyWithinViewport
  • isPositionedWithinViewport
  • isFullyPositionedWithinViewport

Although "position" might be confusing w.r.t. CSS position properties

Copy link
Collaborator

@fungairino fungairino May 1, 2024

Choose a reason for hiding this comment

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

We just happen to know there's no funny business going on the site they need to screenshot

Maybe I'm missing the context, but is this for a specific customer?

I think it's fine to leave the methodName and functionality as-is, but I would recommend updating the jsdoc to be more comprehensive on the behavior and limitations of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I'm missing the context, but is this for a specific customer?

Yes. To see that you can look for the "customer" label and user story in the issue: #8380. Adding the customer name epic now

I think it's fine to leave the methodName and functionality as-is, but I would recommend updating the jsdoc to be more comprehensive on the behavior and limitations of this function.

👍

rect.top >= 0 &&
rect.left >= 0 &&
rect.bottom <=
(window.innerHeight || document.documentElement.clientHeight) &&
rect.right <= (window.innerWidth || document.documentElement.clientWidth)
);
}

/**
* Returns a callback that runs only when the document is visible.
* - If the document is visible, runs immediately
Expand Down
Loading