From da628b52381ba3ad2d81345b54476e74f2843bf4 Mon Sep 17 00:00:00 2001
From: "metabase-bot[bot]"
<109303359+metabase-bot[bot]@users.noreply.github.com>
Date: Thu, 4 Apr 2024 08:28:13 +0100
Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20backported=20"Fix=20embedding"?=
=?UTF-8?q?=20(#40969)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* Fix embedding (#40841)
* Fix overflow static dashboard
* Fix `frame` event's height for interactive dashboards
* Fix E2E tests
* Fix test names to match the correct GitHub issue numbers
* Use `data-element-id` for consistency
* Make the test selector more consistent
* Use the new backported util from master
---------
Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>
Co-authored-by: Mahatthana Nomsawadi <mahatthana.n@gmail.com>
---
e2e/support/helpers/e2e-embedding-helpers.js | 13 +++
.../helpers/e2e-ui-elements-helpers.js | 2 +-
...scalar-first-render-size-change.cy.spec.js | 17 +---
.../dashboard-filters-auto-apply.cy.spec.js | 13 +--
.../interactive-embedding.cy.spec.js | 91 ++++++++++++++++---
...-when-embedding-long-dashboards.cy.spec.js | 59 ++++++++++++
.../src/metabase-types/api/mocks/dashboard.ts | 9 +-
.../components/Dashboard/Dashboard.styled.tsx | 3 +
.../components/Dashboard/Dashboard.tsx | 5 +-
frontend/src/metabase/lib/embed.js | 24 ++++-
.../metabase/nav/components/AppBar/AppBar.tsx | 6 +-
.../EmbedFrame/EmbedFrame.styled.tsx | 1 +
12 files changed, 194 insertions(+), 49 deletions(-)
create mode 100644 e2e/test/scenarios/embedding/reproductions/40660-overflow-when-embedding-long-dashboards.cy.spec.js
diff --git a/e2e/support/helpers/e2e-embedding-helpers.js b/e2e/support/helpers/e2e-embedding-helpers.js
index 3fa187c63d1..a4f6efa7454 100644
--- a/e2e/support/helpers/e2e-embedding-helpers.js
+++ b/e2e/support/helpers/e2e-embedding-helpers.js
@@ -261,3 +261,16 @@ export function createPublicQuestionLink(questionId) {
export function createPublicDashboardLink(dashboardId) {
return cy.request("POST", `/api/dashboard/${dashboardId}/public_link`, {});
}
+
+export const visitFullAppEmbeddingUrl = ({ url, qs, onBeforeLoad }) => {
+ cy.visit({
+ url,
+ qs,
+ onBeforeLoad(window) {
+ // cypress runs all tests in an iframe and the app uses this property to avoid embedding mode for all tests
+ // by removing the property the app would work in embedding mode
+ window.Cypress = undefined;
+ onBeforeLoad?.(window);
+ },
+ });
+};
diff --git a/e2e/support/helpers/e2e-ui-elements-helpers.js b/e2e/support/helpers/e2e-ui-elements-helpers.js
index f998e154a16..539a4fa5699 100644
--- a/e2e/support/helpers/e2e-ui-elements-helpers.js
+++ b/e2e/support/helpers/e2e-ui-elements-helpers.js
@@ -201,5 +201,5 @@ export const undoToastList = () => {
};
export function dashboardCards() {
- return cy.get("#Dashboard-Cards-Container");
+ return cy.get("[data-element-id=dashboard-cards-container]");
}
diff --git a/e2e/test/scenarios/dashboard-cards/reproductions/29304-scalar-first-render-size-change.cy.spec.js b/e2e/test/scenarios/dashboard-cards/reproductions/29304-scalar-first-render-size-change.cy.spec.js
index 2f0784839d1..9f9f3e212e2 100644
--- a/e2e/test/scenarios/dashboard-cards/reproductions/29304-scalar-first-render-size-change.cy.spec.js
+++ b/e2e/test/scenarios/dashboard-cards/reproductions/29304-scalar-first-render-size-change.cy.spec.js
@@ -1,5 +1,5 @@
import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
-import { restore } from "e2e/support/helpers";
+import { restore, visitFullAppEmbeddingUrl } from "e2e/support/helpers";
// Couldn't import from `metabase/components/ExplicitSize` because dependency issue.
// It will fail Cypress tests.
@@ -102,18 +102,3 @@ describe("issue 29304", () => {
});
});
});
-
-// Use full-app embedding to test because `ExplicitSize` checks for `isCypressActive`,
-// which checks `window.Cypress`, and will disable the refresh mode on Cypress test.
-// If we test by simply visiting the dashboard, the refresh mode will be disabled,
-// and we won't be able to reproduce the problem.
-const visitFullAppEmbeddingUrl = ({ url }) => {
- cy.visit({
- url,
- onBeforeLoad(window) {
- // cypress runs all tests in an iframe and the app uses this property to avoid embedding mode for all tests
- // by removing the property the app would work in embedding mode
- window.Cypress = undefined;
- },
- });
-};
diff --git a/e2e/test/scenarios/dashboard-filters/dashboard-filters-auto-apply.cy.spec.js b/e2e/test/scenarios/dashboard-filters/dashboard-filters-auto-apply.cy.spec.js
index 6bcb1fb4b9e..eb3f132cf4b 100644
--- a/e2e/test/scenarios/dashboard-filters/dashboard-filters-auto-apply.cy.spec.js
+++ b/e2e/test/scenarios/dashboard-filters/dashboard-filters-auto-apply.cy.spec.js
@@ -19,6 +19,7 @@ import {
undoToast,
visitDashboard,
visitEmbeddedPage,
+ visitFullAppEmbeddingUrl,
visitPublicDashboard,
} from "e2e/support/helpers";
@@ -729,15 +730,3 @@ const openSlowFullAppEmbeddingDashboard = (params = {}) => {
getDashboardCard().should("be.visible");
};
-
-const visitFullAppEmbeddingUrl = ({ url, qs }) => {
- cy.visit({
- url,
- qs,
- onBeforeLoad(window) {
- // cypress runs all tests in an iframe and the app uses this property to avoid embedding mode for all tests
- // by removing the property the app would work in embedding mode
- window.Cypress = undefined;
- },
- });
-};
diff --git a/e2e/test/scenarios/embedding/interactive-embedding.cy.spec.js b/e2e/test/scenarios/embedding/interactive-embedding.cy.spec.js
index 2ac72dfad59..00d1aba3da3 100644
--- a/e2e/test/scenarios/embedding/interactive-embedding.cy.spec.js
+++ b/e2e/test/scenarios/embedding/interactive-embedding.cy.spec.js
@@ -18,8 +18,12 @@ import {
dashboardGrid,
createDashboardWithTabs,
goToTab,
+ visitFullAppEmbeddingUrl,
} from "e2e/support/helpers";
-import { createMockDashboardCard } from "metabase-types/api/mocks";
+import {
+ createMockDashboardCard,
+ createMockTextDashboardCard,
+} from "metabase-types/api/mocks";
const { ORDERS } = SAMPLE_DATABASE;
@@ -458,6 +462,79 @@ describeEE("scenarios > embedding > full app", () => {
},
);
});
+
+ it("should send `frame` message with dashboard height when the dashboard is resized (metabase#37437)", () => {
+ const TAB_1 = { id: 1, name: "Tab 1" };
+ const TAB_2 = { id: 2, name: "Tab 2" };
+ createDashboardWithTabs({
+ tabs: [TAB_1, TAB_2],
+ name: "Dashboard",
+ dashcards: [
+ createMockTextDashboardCard({
+ dashboard_tab_id: TAB_1.id,
+ size_x: 10,
+ size_y: 20,
+ text: "I am a text card",
+ }),
+ ],
+ }).then(dashboard => {
+ visitFullAppEmbeddingUrl({
+ url: `/dashboard/${dashboard.id}`,
+ onBeforeLoad(window) {
+ cy.spy(window.parent, "postMessage").as("postMessage");
+ },
+ });
+ });
+ cy.get("@postMessage")
+ .should("have.been.calledWith", {
+ metabase: {
+ type: "frame",
+ frame: {
+ mode: "fit",
+ height: Cypress.sinon.match(value => value > 1000),
+ },
+ },
+ })
+ .and("not.have.been.calledWith", {
+ metabase: {
+ type: "frame",
+ frame: {
+ mode: "fit",
+ height: Cypress.sinon.match(value => value < 400),
+ },
+ },
+ });
+
+ cy.findByRole("tab", { name: TAB_2.name }).click();
+ cy.get("@postMessage").should("have.been.calledWith", {
+ metabase: {
+ type: "frame",
+ frame: {
+ mode: "fit",
+ height: Cypress.sinon.match(value => value < 400),
+ },
+ },
+ });
+
+ cy.findByTestId("app-bar").findByText("Our analytics").click();
+
+ cy.findByRole("heading", { name: "Metabase analytics" }).should(
+ "be.visible",
+ );
+ cy.get("@postMessage").then(postMessage => {
+ expect(
+ postMessage.lastCall.calledWith({
+ metabase: {
+ type: "frame",
+ frame: {
+ mode: "fit",
+ height: 800,
+ },
+ },
+ }),
+ ).to.be.true;
+ });
+ });
});
describe("x-ray dashboards", () => {
@@ -480,18 +557,6 @@ describeEE("scenarios > embedding > full app", () => {
});
});
-const visitFullAppEmbeddingUrl = ({ url, qs }) => {
- cy.visit({
- url,
- qs,
- onBeforeLoad(window) {
- // cypress runs all tests in an iframe and the app uses this property to avoid embedding mode for all tests
- // by removing the property the app would work in embedding mode
- window.Cypress = undefined;
- },
- });
-};
-
const visitQuestionUrl = urlOptions => {
visitFullAppEmbeddingUrl(urlOptions);
cy.wait("@getCardQuery");
diff --git a/e2e/test/scenarios/embedding/reproductions/40660-overflow-when-embedding-long-dashboards.cy.spec.js b/e2e/test/scenarios/embedding/reproductions/40660-overflow-when-embedding-long-dashboards.cy.spec.js
new file mode 100644
index 00000000000..d0b68551d2f
--- /dev/null
+++ b/e2e/test/scenarios/embedding/reproductions/40660-overflow-when-embedding-long-dashboards.cy.spec.js
@@ -0,0 +1,59 @@
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+import {
+ getIframeBody,
+ openStaticEmbeddingModal,
+ restore,
+ updateDashboardCards,
+ visitDashboard,
+} from "e2e/support/helpers";
+
+const { PRODUCTS_ID } = SAMPLE_DATABASE;
+
+const questionDetails = {
+ name: "Products",
+ query: { "source-table": PRODUCTS_ID, limit: 2 },
+};
+
+const dashboardDetails = {
+ name: "long dashboard",
+ enable_embedding: true,
+};
+
+describe("issue 40660", () => {
+ beforeEach(() => {
+ cy.intercept("GET", "/api/preview_embed/dashboard/**").as(
+ "previewDashboard",
+ );
+ cy.intercept("GET", "/api/dashboard/**/params/**/values").as("values");
+
+ restore();
+ cy.signInAsAdmin();
+
+ cy.createQuestionAndDashboard({
+ questionDetails,
+ dashboardDetails,
+ }).then(({ body: { id, card_id, dashboard_id } }) => {
+ updateDashboardCards({
+ dashboard_id,
+ cards: [{ card_id }, { card_id }, { card_id }],
+ });
+
+ visitDashboard(dashboard_id);
+ });
+ });
+
+ it("static dashboard content shouldn't overflow its container (metabase#40660)", () => {
+ openStaticEmbeddingModal({
+ activeTab: "parameters",
+ previewMode: "preview",
+ });
+
+ getIframeBody().within(() => {
+ cy.findByTestId("embed-frame").scrollTo("bottom");
+
+ cy.findByRole("link", { name: "Powered by Metabase" }).should(
+ "be.visible",
+ );
+ });
+ });
+});
diff --git a/frontend/src/metabase-types/api/mocks/dashboard.ts b/frontend/src/metabase-types/api/mocks/dashboard.ts
index 211f9821015..5e95ad14e73 100644
--- a/frontend/src/metabase-types/api/mocks/dashboard.ts
+++ b/frontend/src/metabase-types/api/mocks/dashboard.ts
@@ -131,14 +131,15 @@ export const createMockVirtualDashCard = (
};
};
-export const createMockTextDashboardCard = (
- opts?: VirtualDashboardCardOpts & { text?: string },
-): VirtualDashboardCard =>
+export const createMockTextDashboardCard = ({
+ text,
+ ...opts
+}: VirtualDashboardCardOpts & { text?: string } = {}): VirtualDashboardCard =>
createMockVirtualDashCard({
...opts,
card: createMockVirtualCard({ display: "text" }),
visualization_settings: {
- text: opts?.text ?? "Body Text",
+ text: text ?? "Body Text",
},
});
diff --git a/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.styled.tsx b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.styled.tsx
index 79eec528a99..e2ec6045a10 100644
--- a/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.styled.tsx
+++ b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.styled.tsx
@@ -139,6 +139,9 @@ export const ParametersAndCardsContainer = styled.div<{
overflow-x: clip;
}
padding-bottom: 40px;
+ /* Makes sure it doesn't use all the height, so the actual content height could be used in embedding #37437 */
+ align-self: ${({ shouldMakeDashboardHeaderStickyAfterScrolling }) =>
+ !shouldMakeDashboardHeaderStickyAfterScrolling && "flex-start"};
&.${SAVING_DOM_IMAGE_CLASS} {
${ParametersWidgetContainer} {
diff --git a/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.tsx b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.tsx
index fe6d128bd98..852ee34c47a 100644
--- a/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.tsx
+++ b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.tsx
@@ -533,6 +533,8 @@ function DashboardInner(props: DashboardProps) {
{() => (
<DashboardStyled>
<DashboardHeaderContainer
+ data-element-id="dashboard-header-container"
+ id="Dashboard-Header-Container"
isFullscreen={isFullscreen}
isNightMode={shouldRenderAsNightMode}
>
@@ -553,13 +555,14 @@ function DashboardInner(props: DashboardProps) {
<DashboardBody isEditingOrSharing={isEditing || isSharing}>
<ParametersAndCardsContainer
id={DASHBOARD_PDF_EXPORT_ROOT_ID}
+ data-element-id="dashboard-parameters-and-cards"
data-testid="dashboard-parameters-and-cards"
shouldMakeDashboardHeaderStickyAfterScrolling={
!isFullscreen && (isEditing || isSharing)
}
>
{renderParameterList()}
- <CardsContainer id="Dashboard-Cards-Container">
+ <CardsContainer data-element-id="dashboard-cards-container">
{renderContent()}
</CardsContainer>
</ParametersAndCardsContainer>
diff --git a/frontend/src/metabase/lib/embed.js b/frontend/src/metabase/lib/embed.js
index 8d22fd6c9cf..760d1c7e8fe 100644
--- a/frontend/src/metabase/lib/embed.js
+++ b/frontend/src/metabase/lib/embed.js
@@ -59,8 +59,30 @@ function sendMessage(message) {
function getFrameSpec() {
if (isFitViewportMode()) {
- return { mode: "fit", height: document.body.scrollHeight };
+ return { mode: "fit", height: getScrollHeight() };
} else {
return { mode: "normal", height: document.body.scrollHeight };
}
}
+
+function defaultGetScrollHeight() {
+ return document.body.scrollHeight;
+}
+
+function getScrollHeight() {
+ const appBarHeight =
+ document.getElementById("[data-element-id=app-bar]")?.offsetHeight ?? 0;
+ const dashboardHeaderHeight =
+ document.querySelector("[data-element-id=dashboard-header-container]")
+ ?.offsetHeight ?? 0;
+ const dashboardContentHeight =
+ document.querySelector("[data-element-id=dashboard-parameters-and-cards]")
+ ?.scrollHeight ?? 0;
+ const dashboardHeight = dashboardHeaderHeight + dashboardContentHeight;
+
+ if (dashboardHeight > 0) {
+ return appBarHeight + dashboardHeight;
+ }
+
+ return defaultGetScrollHeight();
+}
diff --git a/frontend/src/metabase/nav/components/AppBar/AppBar.tsx b/frontend/src/metabase/nav/components/AppBar/AppBar.tsx
index 776e968c3ba..a5c97973616 100644
--- a/frontend/src/metabase/nav/components/AppBar/AppBar.tsx
+++ b/frontend/src/metabase/nav/components/AppBar/AppBar.tsx
@@ -28,7 +28,11 @@ const AppBar = (props: AppBarProps): JSX.Element => {
const isSmallScreen = useIsSmallScreen();
return (
- <AppBarRoot data-testid="app-bar" aria-label={t`Navigation bar`}>
+ <AppBarRoot
+ data-element-id="app-bar"
+ data-testid="app-bar"
+ aria-label={t`Navigation bar`}
+ >
<ErrorBoundary>
{isSmallScreen ? (
<AppBarSmall {...props} />
diff --git a/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.styled.tsx b/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.styled.tsx
index b1916b44389..f2c6108c47d 100644
--- a/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.styled.tsx
+++ b/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.styled.tsx
@@ -17,6 +17,7 @@ export const Root = styled.div<{
}>`
display: flex;
flex-direction: column;
+ overflow: auto;
${props =>
props.hasScroll &&
--
GitLab