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

#8380: add isInViewport property to element reader #8381

Merged
merged 4 commits into from
May 1, 2024
Merged

#8380: add isInViewport property to element reader #8381

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cece926
#8380: add isInViewport to element reader
twschiller Apr 30, 2024
92b27f9
Merge branch 'main' into feature/8380-in-viewport
fungairino May 1, 2024
1166461
#8380: improve method documentation
twschiller May 1, 2024
d519b16
Merge branch 'feature/8380-in-viewport' of github.com:pixiebrix/pixie…
twschiller May 1, 2024
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
View file
Open in desktop
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
View file
Open in desktop
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
16 changes: 16 additions & 0 deletions src/utils/domUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,27 @@ 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
*/
export function isVisible(element: HTMLElement): boolean {
return element.checkVisibility();
}

/**
* Returns true if the element is completely in the viewport.
*/
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 •
edited
Loading

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 •
edited
Loading

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