Skip to content

Commit

Permalink
fix: Don't AutoScroll to embed when multiple-duration event's booking…
Browse files Browse the repository at this point in the history
… page is embedded (#16411)

* Fix and add test and eslint rule

* Fix eslint errors

---------

Co-authored-by: Peer Richelsen <[email protected]>
  • Loading branch information
hariombalhara and PeerRich authored Sep 27, 2024
1 parent 38fa0fa commit d836497
Show file tree
Hide file tree
Showing 17 changed files with 129 additions and 21 deletions.
1 change: 1 addition & 0 deletions apps/web/components/booking/CancelBooking.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export default function CancelBooking(props: Props) {

const cancelBookingRef = useCallback((node: HTMLTextAreaElement) => {
if (node !== null) {
// eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- CancelBooking is not usually used in embed mode
node.scrollIntoView({ behavior: "smooth" });
node.focus();
}
Expand Down
6 changes: 6 additions & 0 deletions apps/web/playwright/fixtures/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,12 @@ export const createUsersFixture = (
{ title: "Paid", slug: "paid", length: 30, price: 1000 },
{ title: "Opt in", slug: "opt-in", requiresConfirmation: true, length: 30 },
{ title: "Seated", slug: "seated", seatsPerTimeSlot: 2, length: 30 },
{
title: "Multiple duration",
slug: "multiple-duration",
length: 30,
metadata: { multipleDuration: [30, 60, 90] },
},
];

if (opts?.eventTypes) defaultEventTypes = defaultEventTypes.concat(opts.eventTypes);
Expand Down
4 changes: 4 additions & 0 deletions packages/embeds/embed-core/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,10 @@ <h3><a href="?only=ns:weekView">Test Week View</a></h3>
<h3><a href="?only=ns:columnView">Test Column View</a></h3>
<div class="place" style="width: 100%"></div>
</div>
<div class="inline-embed-container" id="cal-booking-place-autoScrollTest">
<h3><a href="?only=ns:autoScrollTest">Test Auto Scroll</a></h3>
<div class="place" style="width: 100%"></div>
</div>
<script type="module" src="./playground.ts"></script>
</body>
</html>
16 changes: 16 additions & 0 deletions packages/embeds/embed-core/playground.ts
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,22 @@ if (only === "all" || only == "ns:columnView") {
});
}

if (only === "all" || only == "ns:autoScrollTest") {
if (!calLink) {
throw new Error("cal-link parameter is required for autoScrollTest");
}
Cal("init", "autoScrollTest", {
debug: true,
origin: origin,
});
Cal.ns.autoScrollTest("inline", {
elementOrSelector: "#cal-booking-place-autoScrollTest .place",
calLink: calLink,
config: {
"flag.coep": "true",
},
});
}
// Verifies that the type of e.detail.data is valid. type-check will fail if we accidentally break it.
const bookingSuccessfulV2Callback = (e: EmbedEvent<"bookingSuccessfulV2">) => {
const data = e.detail.data;
Expand Down
19 changes: 19 additions & 0 deletions packages/embeds/embed-core/playwright/lib/testUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ export const getBooking = async (bookingId: string) => {
return booking;
};

/**
* @deprecated use ensureEmbedIframe instead.
*/
export const getEmbedIframe = async ({
calNamespace,
page,
Expand Down Expand Up @@ -59,6 +62,22 @@ export const getEmbedIframe = async ({
return null;
};

export const ensureEmbedIframe = async ({
calNamespace,
page,
pathname,
}: {
calNamespace: string;
page: Page;
pathname: string;
}) => {
const embedIframe = await getEmbedIframe({ calNamespace, page, pathname });
if (!embedIframe) {
throw new Error("Embed iframe not found");
}
return embedIframe;
};

async function selectFirstAvailableTimeSlotNextMonth(frame: Frame, page: Page) {
await frame.click('[data-testid="incrementMonth"]');

Expand Down
29 changes: 17 additions & 12 deletions packages/embeds/embed-core/playwright/tests/inline.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,23 @@ import {
assertNoRequestIsBlocked,
bookFirstEvent,
deleteAllBookingsByEmail,
getEmbedIframe,
ensureEmbedIframe,
} from "../lib/testUtils";

test.describe("Inline Iframe", () => {
test("Inline Iframe - Configured with Dark Theme. Do booking and verify that COEP/CORP headers are correctly set", async ({
test("Configured with Dark Theme. Do booking and verify that COEP/CORP headers are correctly set", async ({
page,
embeds: { addEmbedListeners, getActionFiredDetails },
embeds,
}) => {
await deleteAllBookingsByEmail("[email protected]");
await addEmbedListeners("");
await page.goto("/?only=ns:default");
await embeds.gotoPlayground({ calNamespace: "", url: "/?only=ns:default" });
const calNamespace = "";
const embedIframe = await getEmbedIframe({ calNamespace, page, pathname: "/pro" });
expect(embedIframe).toBeEmbedCalLink(calNamespace, getActionFiredDetails, {
pathname: "/pro",
const embedIframe = await ensureEmbedIframe({ calNamespace, page, pathname: "/pro" });
expect(embedIframe).toBeEmbedCalLink(calNamespace, embeds.getActionFiredDetails, {
searchParams: {
theme: "dark",
},
});
// expect(await page.screenshot()).toMatchSnapshot("event-types-list.png");
if (!embedIframe) {
throw new Error("Embed iframe not found");
}

assertNoRequestIsBlocked(page);

Expand Down Expand Up @@ -57,6 +51,17 @@ test.describe("Inline Iframe", () => {
});
});

test("Ensure iframe doesn't hijack scroll in embed mode", async ({ page, embeds, users }) => {
const user = await users.create();
const calNamespace = "autoScrollTest";
await embeds.gotoPlayground({ calNamespace, url: `?only=ns:autoScrollTest` });
const calLink = `${user.username}/multiple-duration`;
await page.goto(`/?only=ns:autoScrollTest&cal-link=${calLink}`);
const embedIframe = await ensureEmbedIframe({ calNamespace, page, pathname: `/${calLink}` });
const finalScrollPosition = await page.evaluate(() => window.scrollY);
expect(finalScrollPosition).toBe(0);
});

todo(
"Ensure that on all pages - [user], [user]/[type], team/[slug], team/[slug]/book, UI styling works if these pages are directly linked in embed"
);
Expand Down
1 change: 1 addition & 0 deletions packages/embeds/embed-core/src/embed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ export class Cal {
// Try to readjust and scroll into view if more than 25% is hidden.
// Otherwise we assume that user might have positioned the content appropriately already
if (top < 0 && Math.abs(top / height) >= 0.25) {
// eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- Intentionally done
this.inlineEl.scrollIntoView({ behavior: "smooth" });
}
});
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/recommended.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const recommended = {
"@calcom/eslint/avoid-web-storage": "error",
"@calcom/eslint/avoid-prisma-client-import-for-enums": "error",
"@calcom/eslint/no-prisma-include-true": "warn",
"@calcom/eslint/no-scroll-into-view-embed": "error",
},
};

Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ export default {
"avoid-prisma-client-import-for-enums": require("./avoid-prisma-client-import-for-enums").default,
"no-prisma-include-true": require("./no-prisma-include-true").default,
"deprecated-imports-next-router": require("./deprecated-imports-next-router").default,
"no-scroll-into-view-embed": require("./no-scroll-into-view-embed").default,
} as ESLint.Plugin["rules"];
42 changes: 42 additions & 0 deletions packages/eslint-plugin/src/rules/no-scroll-into-view-embed.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import type { TSESTree } from "@typescript-eslint/utils";
import { ESLintUtils } from "@typescript-eslint/utils";

const createRule = ESLintUtils.RuleCreator((name) => `https://developer.cal.com/eslint/rule/${name}`);

export default createRule({
name: "no-scroll-into-view-embed",
meta: {
docs: {
description: "Disallow usage of scrollIntoView in embed mode",
recommended: "error",
},
messages: {
noScrollIntoViewForEmbed:
"Make sure to call scrollIntoView conditionally if it is called without user action. Use useIsEmbed() to detect if embed mode and then don't call it for embed case.",
},
type: "problem",
schema: [],
},
defaultOptions: [],
create(context) {
return {
CallExpression(node: TSESTree.CallExpression) {
const { callee } = node;

if (callee.type === "MemberExpression") {
if (callee.property.type === "Identifier" && callee.property.name === "scrollIntoView") {
context.report({
node,
messageId: "noScrollIntoViewForEmbed",
});
}
} else if (callee.type === "Identifier" && callee.name === "scrollIntoView") {
context.report({
node,
messageId: "noScrollIntoViewForEmbed",
});
}
},
};
},
});
1 change: 1 addition & 0 deletions packages/features/bookings/Booker/Booker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ const BookerComponent = ({
const scrolledToTimeslotsOnce = useRef(false);
const scrollToTimeSlots = () => {
if (isMobile && !isEmbed && !scrolledToTimeslotsOnce.current) {
// eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- Conditional within !isEmbed condition
timeslotsRef.current?.scrollIntoView({ behavior: "smooth" });
scrolledToTimeslotsOnce.current = true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ export const useBookings = ({ event, hashedLink, bookingForm, metadata, teamMemb
});
},
onError: (err, _, ctx) => {
// eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- It is only called when user takes an action in embed
bookerFormErrorRef && bookerFormErrorRef.current?.scrollIntoView({ behavior: "smooth" });
},
});
Expand All @@ -243,7 +244,7 @@ export const useBookings = ({ event, hashedLink, bookingForm, metadata, teamMemb
},
onError: (err, _, ctx) => {
console.error("Error creating instant booking", err);

// eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- It is only called when user takes an action in embed
bookerFormErrorRef && bookerFormErrorRef.current?.scrollIntoView({ behavior: "smooth" });
},
});
Expand Down
7 changes: 5 additions & 2 deletions packages/features/bookings/components/event-meta/Duration.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { TFunction } from "next-i18next";
import { useEffect, useRef } from "react";

import { useIsPlatform } from "@calcom/atoms/monorepo";
import { useIsEmbed } from "@calcom/embed-core/embed-iframe";
import { useBookerStore } from "@calcom/features/bookings/Booker/store";
import type { BookerEvent } from "@calcom/features/bookings/types";
import { classNames } from "@calcom/lib";
Expand Down Expand Up @@ -64,7 +65,7 @@ export const EventDuration = ({
};

const isDynamicEvent = "isDynamic" in event && event.isDynamic;

const isEmbed = useIsEmbed();
// Sets initial value of selected duration to the default duration.
useEffect(() => {
// Only store event duration in url if event has multiple durations.
Expand All @@ -74,7 +75,9 @@ export const EventDuration = ({

useEffect(() => {
const timeout = setTimeout(() => {
if (isEmbed) return;
if (selectedDuration && itemRefs.current[selectedDuration]) {
// eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- Called on !isEmbed case
itemRefs.current[selectedDuration]?.scrollIntoView({
behavior: "smooth",
block: "center",
Expand All @@ -83,7 +86,7 @@ export const EventDuration = ({
}
}, 100);
return () => clearTimeout(timeout);
}, [selectedDuration]);
}, [selectedDuration, isEmbed]);

if ((!event?.metadata?.multipleDuration && !isDynamicEvent) || isPlatform)
return <>{getDurationFormatted(event.length, t)}</>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export function CurrentTime() {
if (!currentTimeRef.current || scrolledIntoView) return;
// Within a small timeout so element has time to render.
setTimeout(() => {
// eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- Does't seem to cause any issue. Put it under condition if needed
currentTimeRef?.current?.scrollIntoView({ block: "center" });
setScrolledIntoView(true);
}, 100);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ const TeamListCollapsible = () => {
const tabMembers = Array.from(document.getElementsByTagName("a")).filter(
(bottom) => bottom.dataset.testid === "vertical-tab-Members"
)[1];
// eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- Settings layout isn't embedded
tabMembers?.scrollIntoView({ behavior: "smooth" });
}, 100);
}
Expand Down Expand Up @@ -418,6 +419,7 @@ const SettingsSidebarContainer = ({
const tabMembers = Array.from(document.getElementsByTagName("a")).filter(
(bottom) => bottom.dataset.testid === "vertical-tab-Members"
)[1];
// eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- Settings layout isn't embedded
tabMembers?.scrollIntoView({ behavior: "smooth" });
}, 100);
}
Expand Down
2 changes: 2 additions & 0 deletions packages/features/settings/layouts/SettingsLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ const TeamListCollapsible = () => {
const tabMembers = Array.from(document.getElementsByTagName("a")).filter(
(bottom) => bottom.dataset.testid === "vertical-tab-Members"
)[1];
// eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- Settings layout isn't embedded
tabMembers?.scrollIntoView({ behavior: "smooth" });
}, 100);
}
Expand Down Expand Up @@ -424,6 +425,7 @@ const SettingsSidebarContainer = ({
const tabMembers = Array.from(document.getElementsByTagName("a")).filter(
(bottom) => bottom.dataset.testid === "vertical-tab-Members"
)[1];
// eslint-disable-next-line @calcom/eslint/no-scroll-into-view-embed -- Settings layout isn't embedded
tabMembers?.scrollIntoView({ behavior: "smooth" });
}, 100);
}
Expand Down
14 changes: 8 additions & 6 deletions playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,14 @@ expect.extend({
const u = new URL(iframe.url());

const pathname = u.pathname;
const expectedPathname = `${expectedUrlDetails.pathname}/embed`;
if (expectedPathname && expectedPathname !== pathname) {
return {
pass: false,
message: () => `Expected pathname to be ${expectedPathname} but got ${pathname}`,
};
if (expectedUrlDetails.pathname) {
const expectedPathname = `${expectedUrlDetails.pathname}/embed`;
if (pathname !== expectedPathname) {
return {
pass: false,
message: () => `Expected pathname to be ${expectedPathname} but got ${pathname}`,
};
}
}

const origin = u.origin;
Expand Down

0 comments on commit d836497

Please sign in to comment.