From 73196012918dd4e3c2a54e91af78d545ddcb3af5 Mon Sep 17 00:00:00 2001 From: lukasmasuch Date: Sun, 7 Apr 2024 00:18:33 +0200 Subject: [PATCH 01/15] Update native files system adapter --- frontend/lib/package.json | 2 +- frontend/yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/frontend/lib/package.json b/frontend/lib/package.json index a0b211cbd85f..5bfa36412a1f 100644 --- a/frontend/lib/package.json +++ b/frontend/lib/package.json @@ -68,7 +68,7 @@ "moment": "^2.29.4", "moment-duration-format": "^2.3.2", "moment-timezone": "^0.5.40", - "native-file-system-adapter": "^3.0.0", + "native-file-system-adapter": "^3.0.1", "node-emoji": "^1.11.0", "numbro": "^2.3.6", "plotly.js": "^2.30.1", diff --git a/frontend/yarn.lock b/frontend/yarn.lock index 5f50d283a074..bf92c5e54a90 100644 --- a/frontend/yarn.lock +++ b/frontend/yarn.lock @@ -11999,10 +11999,10 @@ nanoid@^3.3.4: resolved "https://registry.yarnpkg.com/nanoid/-/nanoid-3.3.4.tgz#730b67e3cd09e2deacf03c027c81c9d9dbc5e8ab" integrity sha512-MqBkQh/OHTS2egovRtLk45wEyNXwF+cokD+1YPf9u5VfJiRdAiRwB2froX5Co9Rh20xs4siNPm8naNotSD6RBw== -native-file-system-adapter@^3.0.0: - version "3.0.0" - resolved "https://registry.yarnpkg.com/native-file-system-adapter/-/native-file-system-adapter-3.0.0.tgz#602317f30fdc2495efe598055a0cda75c1c6e051" - integrity sha512-IXwQiLiS7UlrlUetr9rHp+6uAAHT3291w2FGse5rdrQ7YXKtZHaqC4+tEXHMXVcevwsJNB3c7q9KO1Iu3IxgLw== +native-file-system-adapter@^3.0.1: + version "3.0.1" + resolved "https://registry.yarnpkg.com/native-file-system-adapter/-/native-file-system-adapter-3.0.1.tgz#d8881903a94e17eac368c4c0e8805f98a4e2ddc1" + integrity sha512-ocuhsYk2SY0906LPc3QIMW+rCV3MdhqGiy7wV5Bf0e8/5TsMjDdyIwhNiVPiKxzTJLDrLT6h8BoV9ERfJscKhw== optionalDependencies: fetch-blob "^3.2.0" From cc40bff5ce002d67d001c0189a9e2ddbad07c011 Mon Sep 17 00:00:00 2001 From: lukasmasuch Date: Sun, 7 Apr 2024 01:25:20 +0200 Subject: [PATCH 02/15] Add fallback method for CSV download --- .../DataFrame/hooks/useDataExporter.ts | 103 ++++++++++++++---- 1 file changed, 82 insertions(+), 21 deletions(-) diff --git a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts index e51aec78e8ea..b63f307cce60 100644 --- a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts +++ b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts @@ -23,7 +23,7 @@ import { toSafeString, } from "@streamlit/lib/src/components/widgets/DataFrame/columns" import { isNullOrUndefined } from "@streamlit/lib/src/util/utils" -import { logWarning } from "@streamlit/lib/src/util/log" +import { logError, logWarning } from "@streamlit/lib/src/util/log" // Delimiter between cells const CSV_DELIMITER = "," @@ -76,6 +76,45 @@ type DataExporterReturn = { exportToCsv: () => void } +/** + * Writes CSV data to a specified writable stream using provided data table parameters. + * Initiates by writing a UTF-8 Byte Order Mark (BOM) for Excel compatibility, followed by + * column headers and rows constructed from the cell values obtained through `getCellContent`. + * The function handles encoding and CSV formatting, concluding by closing the writable stream. + * + * @param {WritableStreamDefaultWriter} writable - Target stream for CSV data. + * @param {DataEditorProps["getCellContent"]} getCellContent - The cell content getter compatible with glide-data-grid. + * @param {BaseColumn[]} columns - The columns of the table. + * @param {number} numRows - The number of rows of the current state. + * + * @returns {Promise} Promise that resolves when the CSV has been fully written. + */ +async function writeCsv( + writable: WritableStreamDefaultWriter, + getCellContent: DataEditorProps["getCellContent"], + columns: BaseColumn[], + numRows: number +): Promise { + const textEncoder = new TextEncoder() + + // Write UTF-8 BOM for excel compatibility: + await writable.write(textEncoder.encode(CSV_UTF8_BOM)) + + // Write headers: + const headers: string[] = columns.map(column => column.name) + await writable.write(textEncoder.encode(toCsvRow(headers))) + + for (let row = 0; row < numRows; row++) { + const rowData: any[] = [] + columns.forEach((column: BaseColumn, col: number, _map) => { + rowData.push(column.getCellValue(getCellContent([col, row]))) + }) + // Write row to CSV: + await writable.write(textEncoder.encode(toCsvRow(rowData))) + } + + await writable.close() +} /** * Custom hook that handles all the data export/download logic. * @@ -91,6 +130,8 @@ function useDataExporter( numRows: number ): DataExporterReturn { const exportToCsv = React.useCallback(async () => { + const timestamp = new Date().toISOString().slice(0, 16).replace(":", "-") + const suggestedName = `${timestamp}_export.csv` try { // Lazy import to prevent weird breakage in some niche cases // (e.g. usage within the replay.io browser). The package works well @@ -100,38 +141,58 @@ function useDataExporter( const nativeFileSystemAdapter = await import( "native-file-system-adapter" ) - - const timestamp = new Date().toISOString().slice(0, 16).replace(":", "-") - const suggestedName = `${timestamp}_export.csv` - const fileHandle = await nativeFileSystemAdapter.showSaveFilePicker({ suggestedName, types: [{ accept: { "text/csv": [".csv"] } }], excludeAcceptAllOption: false, }) - const textEncoder = new TextEncoder() const writer = await fileHandle.createWritable() + await writeCsv(writer, getCellContent, columns, numRows) + } catch (error) { + if (error instanceof Error && error.name === "AbortError") { + // The user has canceled the save dialog. Do nothing. + return + } - // Write UTF-8 BOM for excel compatibility: - await writer.write(textEncoder.encode(CSV_UTF8_BOM)) + try { + logWarning( + "Failed to export data as CSV with FileSystem API, trying fallback method", + error + ) + // Simulated WritableStream that builds CSV content in-memory for the Blob fallback method + let csvContent = "" + + const inMemoryWriter = new WritableStream({ + write: async chunk => { + csvContent += new TextDecoder("utf-8").decode(chunk) + }, + close: async () => {}, + }) - // Write headers: - const headers: string[] = columns.map(column => column.name) - await writer.write(textEncoder.encode(toCsvRow(headers))) + await writeCsv( + inMemoryWriter.getWriter(), + getCellContent, + columns, + numRows + ) - for (let row = 0; row < numRows; row++) { - const rowData: any[] = [] - columns.forEach((column: BaseColumn, col: number, _map) => { - rowData.push(column.getCellValue(getCellContent([col, row]))) + // Fallback to the old browser download method: + const blob = new Blob([csvContent], { + type: "text/csv;charset=utf-8;", }) - // Write row to CSV: - await writer.write(textEncoder.encode(toCsvRow(rowData))) + const url = URL.createObjectURL(blob) + const link = document.createElement("a") + link.style.display = "none" + link.href = url + link.download = suggestedName + document.body.appendChild(link) // Required for FF + link.click() + document.body.removeChild(link) // Clean up + URL.revokeObjectURL(url) // Free up memory + } catch (error) { + logError("Failed to export data as CSV", error) } - - await writer.close() - } catch (error) { - logWarning("Failed to export data as CSV", error) } }, [columns, numRows, getCellContent]) From f941238703fd6885133e2aa65336aa21a356650b Mon Sep 17 00:00:00 2001 From: Karen Javadyan Date: Tue, 14 May 2024 16:33:31 +0400 Subject: [PATCH 03/15] configure fallback csv download behaviour baaed on `enforceDownloadInNewTab` host config --- .../src/components/widgets/DataFrame/DataFrame.tsx | 12 +++++++++++- .../widgets/DataFrame/hooks/useDataExporter.ts | 10 +++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/frontend/lib/src/components/widgets/DataFrame/DataFrame.tsx b/frontend/lib/src/components/widgets/DataFrame/DataFrame.tsx index ec9c41667155..26d1eed59a77 100644 --- a/frontend/lib/src/components/widgets/DataFrame/DataFrame.tsx +++ b/frontend/lib/src/components/widgets/DataFrame/DataFrame.tsx @@ -45,6 +45,7 @@ import { debounce, isNullOrUndefined } from "@streamlit/lib/src/util/utils" import Toolbar, { ToolbarAction, } from "@streamlit/lib/src/components/shared/Toolbar" +import { LibContext } from "@streamlit/lib/src/components/core/LibContext" import EditingState, { getColumnName } from "./EditingState" import { @@ -145,6 +146,10 @@ function DataFrame({ const { theme, headerIcons, tableBorderRadius } = useCustomTheme() + const { + libConfig: { enforceDownloadInNewTab = false }, // Default to false, if no libConfig, e.g. for tests + } = React.useContext(LibContext) + const [isFocused, setIsFocused] = React.useState(true) const [showSearch, setShowSearch] = React.useState(false) const [hasVerticalScroll, setHasVerticalScroll] = @@ -469,7 +474,12 @@ function DataFrame({ ] ) - const { exportToCsv } = useDataExporter(getCellContent, columns, numRows) + const { exportToCsv } = useDataExporter( + getCellContent, + columns, + numRows, + enforceDownloadInNewTab + ) const { onCellEdited, onPaste, onRowAppended, onDelete, validateCell } = useDataEditor( diff --git a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts index b63f307cce60..ae3a1e69f66e 100644 --- a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts +++ b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts @@ -127,7 +127,8 @@ async function writeCsv( function useDataExporter( getCellContent: DataEditorProps["getCellContent"], columns: BaseColumn[], - numRows: number + numRows: number, + enforceDownloadInNewTab: boolean ): DataExporterReturn { const exportToCsv = React.useCallback(async () => { const timestamp = new Date().toISOString().slice(0, 16).replace(":", "-") @@ -148,6 +149,7 @@ function useDataExporter( }) const writer = await fileHandle.createWritable() + await writeCsv(writer, getCellContent, columns, numRows) } catch (error) { if (error instanceof Error && error.name === "AbortError") { @@ -183,6 +185,12 @@ function useDataExporter( }) const url = URL.createObjectURL(blob) const link = document.createElement("a") + if (enforceDownloadInNewTab) { + link.setAttribute("target", "_blank") + } else { + link.setAttribute("target", "_self") + } + link.style.display = "none" link.href = url link.download = suggestedName From c84dffdc21a01ae2c51ecf1e38c0196b6f9a76d3 Mon Sep 17 00:00:00 2001 From: Karen Javadyan Date: Tue, 14 May 2024 16:38:17 +0400 Subject: [PATCH 04/15] fix failing tests --- .../widgets/DataFrame/hooks/useDataExporter.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.test.ts b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.test.ts index 6f051b055e7b..967273d81cc5 100644 --- a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.test.ts +++ b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.test.ts @@ -108,7 +108,7 @@ describe("useDataExporter hook", () => { it("correctly writes data row-by-row to writable", async () => { const { result } = renderHook(() => { - return useDataExporter(getCellContentMock, MOCK_COLUMNS, NUM_ROWS) + return useDataExporter(getCellContentMock, MOCK_COLUMNS, NUM_ROWS, false) }) if (typeof result.current.exportToCsv !== "function") { @@ -130,7 +130,7 @@ describe("useDataExporter hook", () => { it("correctly creates a file picker", async () => { const { result } = renderHook(() => { - return useDataExporter(getCellContentMock, MOCK_COLUMNS, NUM_ROWS) + return useDataExporter(getCellContentMock, MOCK_COLUMNS, NUM_ROWS, false) }) if (typeof result.current.exportToCsv !== "function") { From 9d3bc230b1b28a415aadfd11517e810fa0752c84 Mon Sep 17 00:00:00 2001 From: Karen Javadyan Date: Tue, 14 May 2024 16:48:43 +0400 Subject: [PATCH 05/15] make linter happy --- .../src/components/widgets/DataFrame/hooks/useDataExporter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts index ae3a1e69f66e..8b2287ba7450 100644 --- a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts +++ b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts @@ -202,7 +202,7 @@ function useDataExporter( logError("Failed to export data as CSV", error) } } - }, [columns, numRows, getCellContent]) + }, [columns, numRows, getCellContent, enforceDownloadInNewTab]) return { exportToCsv, From 83936183ce49a17381c8d030bec317f840507dec Mon Sep 17 00:00:00 2001 From: Karen Javadyan Date: Tue, 14 May 2024 19:15:33 +0400 Subject: [PATCH 06/15] add explanatory comment --- .../src/components/widgets/DataFrame/hooks/useDataExporter.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts index 8b2287ba7450..c8a863128939 100644 --- a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts +++ b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts @@ -185,6 +185,7 @@ function useDataExporter( }) const url = URL.createObjectURL(blob) const link = document.createElement("a") + // Open link in new tab if enforceDownloadInNewTab host config is true if (enforceDownloadInNewTab) { link.setAttribute("target", "_blank") } else { From a29ca4c1f57bc9a61f90c856ceef51588cf90de3 Mon Sep 17 00:00:00 2001 From: Karen Javadyan Date: Tue, 14 May 2024 20:38:53 +0400 Subject: [PATCH 07/15] better explanatory comment --- .../src/components/widgets/DataFrame/hooks/useDataExporter.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts index c8a863128939..2fe8f00fb277 100644 --- a/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts +++ b/frontend/lib/src/components/widgets/DataFrame/hooks/useDataExporter.ts @@ -185,7 +185,8 @@ function useDataExporter( }) const url = URL.createObjectURL(blob) const link = document.createElement("a") - // Open link in new tab if enforceDownloadInNewTab host config is true + // Open the download link in a new tab to ensure that this is working in embedded + // setups that limit the URL that an iframe can navigate to (e.g. via CSP) if (enforceDownloadInNewTab) { link.setAttribute("target", "_blank") } else { From 2f7acf58f6b4cb0cf1c52792f84dfb0931721f72 Mon Sep 17 00:00:00 2001 From: braethlein Date: Wed, 15 May 2024 17:44:08 +0200 Subject: [PATCH 08/15] Add tests for download button with and without iframe --- e2e_playwright/conftest.py | 107 +++++++++++++++++- .../st_dataframe_interactions_test.py | 68 ++++++++++- 2 files changed, 169 insertions(+), 6 deletions(-) diff --git a/e2e_playwright/conftest.py b/e2e_playwright/conftest.py index 819da0d82b2c..a74e006df73c 100644 --- a/e2e_playwright/conftest.py +++ b/e2e_playwright/conftest.py @@ -27,18 +27,26 @@ import subprocess import sys import time +from dataclasses import dataclass from io import BytesIO from pathlib import Path from random import randint from tempfile import TemporaryFile from types import ModuleType -from typing import Any, Dict, Generator, List, Literal, Protocol, Tuple +from typing import Any, Callable, Dict, Generator, List, Literal, Protocol, Tuple from urllib import parse import pytest import requests from PIL import Image -from playwright.sync_api import ElementHandle, Locator, Page +from playwright.sync_api import ( + ElementHandle, + FrameLocator, + Locator, + Page, + Response, + Route, +) from pytest import FixtureRequest @@ -124,6 +132,11 @@ def resolve_test_to_script(test_module: ModuleType) -> str: return test_module.__file__.replace("_test.py", ".py") +def get_iframe_html_path() -> str: + """Get the absolute path of the given path.""" + return f"file://{os.getcwd()}/test_assets/iframed_app.html" + + def hash_to_range( text: str, min: int = 10000, @@ -258,6 +271,96 @@ def app_with_query_params( return page, query_params +@dataclass +class IframedPage: + # the page to configure + page: Page + # opens the configured page via the iframe URL and returns the frame_locator pointing to the iframe + open_app: Callable[[], FrameLocator] + + +@pytest.fixture(scope="function") +def iframed_app(page: Page, app_port: int) -> IframedPage: + """Fixture that returns an IframedPage. + + The page object can be used to configure additional routes, for example to override the host-config. + The open_app function triggers the opening of the app in an iframe. + """ + # we are going to intercept the request, so the address is arbitrarily chose and does not have to exist + fake_iframe_server_origin = "http://localhost:1345" + fake_iframe_server_route = f"{fake_iframe_server_origin}/iframed_app.html" + # the url where the Streamlit server is reachable + app_url = f"http://localhost:{app_port}" + # the CSP header returned for the Streamlit index.html loaded in the iframe. This is similar to a common CSP we have seen in the wild. + app_csp_header = f"default-src 'none'; worker-src blob:; form-action 'none'; connect-src ws://localhost:{app_port}/_stcore/stream http://localhost:{app_port}/_stcore/allowed-message-origins http://localhost:{app_port}/_stcore/host-config http://localhost:{app_port}/_stcore/health; script-src 'unsafe-inline' 'unsafe-eval' {app_url}/static/js/; style-src 'unsafe-inline' {app_url}/static/css/; img-src data: {app_url}/favicon.png {app_url}/favicon.ico; font-src {app_url}/static/fonts/ {app_url}/static/media/; frame-ancestors {fake_iframe_server_origin};" + + def fulfill_iframe_request(route: Route) -> None: + """Return as response an iframe that loads the actual Streamlit app.""" + + browser = page.context.browser + # webkit requires the iframe's parent to have "blob:" set, for example if we want to download a CSV via the blob: url + # chrome seems to be more lax + frame_src_blob = "" + if browser is not None and ( + browser.browser_type.name == "webkit" + or browser.browser_type.name == "firefox" + ): + frame_src_blob = "blob:" + + route.fulfill( + status=200, + body=f""" + + """, + headers={ + "Content-Type": "text/html", + "Content-Security-Policy": f"frame-src {frame_src_blob} {app_url};", + }, + ) + + # intercept all requests to the fake iframe server and fullfil the request in playwright + page.route(fake_iframe_server_route, fulfill_iframe_request) + + def fullfill_streamlit_app_request(route: Route) -> None: + response = route.fetch() + route.fulfill( + body=response.body(), + headers={**response.headers, "Content-Security-Policy": app_csp_header}, + ) + + page.route(f"{app_url}/", fullfill_streamlit_app_request) + + def _open_app() -> FrameLocator: + def _expect_streamlit_app_loaded_in_iframe_with_added_header( + response: Response, + ) -> bool: + """Ensure that the routing-interception worked and that Streamlit app is indeed loaded with the CSP header we expect""" + + return ( + response.url == f"{app_url}/" + and response.headers["content-security-policy"] == app_csp_header + ) + + with page.expect_event( + "response", + predicate=_expect_streamlit_app_loaded_in_iframe_with_added_header, + ): + page.goto(fake_iframe_server_route, wait_until="domcontentloaded") + frame_locator = page.frame_locator("iframe") + frame_locator.nth(0).locator("[data-testid='stAppViewContainer']").wait_for( + timeout=30000, state="attached" + ) + return frame_locator + + return IframedPage(page, _open_app) + + @pytest.fixture(scope="session") def browser_type_launch_args(browser_type_launch_args: Dict, browser_name: str): """Fixture that adds the fake device and ui args to the browser type launch args.""" diff --git a/e2e_playwright/st_dataframe_interactions_test.py b/e2e_playwright/st_dataframe_interactions_test.py index 720a4f03b1d4..e6fb06fbf428 100644 --- a/e2e_playwright/st_dataframe_interactions_test.py +++ b/e2e_playwright/st_dataframe_interactions_test.py @@ -12,10 +12,11 @@ # See the License for the specific language governing permissions and # limitations under the License. + import pytest -from playwright.sync_api import Page, expect +from playwright.sync_api import FrameLocator, Locator, Page, Route, expect -from e2e_playwright.conftest import ImageCompareFunction, wait_for_app_run +from e2e_playwright.conftest import IframedPage, ImageCompareFunction, wait_for_app_run # This test suite covers all interactions of dataframe & data_editor @@ -265,11 +266,70 @@ def test_data_editor_keeps_state_after_unmounting( ) +def _test_csv_download(page: Page, locator: FrameLocator | Locator): + dataframe_element = locator.get_by_test_id("stDataFrame").nth(0) + dataframe_toolbar = dataframe_element.get_by_test_id("stElementToolbar") + + download_csv_toolbar_button = dataframe_toolbar.get_by_test_id( + "stElementToolbarButton" + ).first + + # Activate toolbar: + dataframe_element.hover() + # Check that it is visible + expect(dataframe_toolbar).to_have_css("opacity", "1") + + # Click on download csv button: + with page.expect_download(timeout=5000) as download_info: + download_csv_toolbar_button.click() + + download = download_info.value + download_path = download.path() + with open(download_path, "r", encoding="UTF-8") as f: + content = f.read() + # the app uses a fixed seed, so the data is always the same. This is the reason why we can check it here. + some_row = "1,-0.977277879876411,0.9500884175255894,-0.1513572082976979,-0.10321885179355784,0.41059850193837233" + assert some_row in content + + +def test_csv_download_button(app: Page): + _test_csv_download(app, app.locator("body")) + + +def test_csv_download_button_in_iframe(iframed_app: IframedPage): + page: Page = iframed_app.page + frame_locator: FrameLocator = iframed_app.open_app() + + _test_csv_download(page, frame_locator) + + +def test_csv_download_button_in_iframe_with_new_tab_host_config( + iframed_app: IframedPage, +): + page: Page = iframed_app.page + + def fulfill_host_config_request(route: Route): + response = route.fetch() + result = response.json() + result["enforceDownloadInNewTab"] = True + route.fulfill(json=result) + + page.route("**/_stcore/host-config", fulfill_host_config_request) + + # ensure that the route interception works and we get the correct enforceDownloadInNewTab config + with page.expect_event( + "response", + lambda response: response.url.endswith("_stcore/host-config") + and response.json()["enforceDownloadInNewTab"] == True, + timeout=10000, + ): + frame_locator: FrameLocator = iframed_app.open_app() + _test_csv_download(page, frame_locator) + + # TODO(lukasmasuch): Add additional interactive tests: # - Selecting a cell # - Opening a cell # - Applying a cell edit # - Copy data to clipboard # - Paste in data -# - Download data via toolbar: I wasn't able to find out how to detect the -# showSaveFilePicker the filechooser doesn't work. From 712024b090c1232327c067679fa51f0efdbc5ea1 Mon Sep 17 00:00:00 2001 From: braethlein Date: Wed, 15 May 2024 19:05:50 +0200 Subject: [PATCH 09/15] Fix text in Chrome --- .../st_dataframe_interactions_test.py | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/e2e_playwright/st_dataframe_interactions_test.py b/e2e_playwright/st_dataframe_interactions_test.py index e6fb06fbf428..919b265befdb 100644 --- a/e2e_playwright/st_dataframe_interactions_test.py +++ b/e2e_playwright/st_dataframe_interactions_test.py @@ -266,7 +266,11 @@ def test_data_editor_keeps_state_after_unmounting( ) -def _test_csv_download(page: Page, locator: FrameLocator | Locator): +def _test_csv_download( + page: Page, + locator: FrameLocator | Locator, + click_enter_on_file_picker: bool = False, +): dataframe_element = locator.get_by_test_id("stDataFrame").nth(0) dataframe_toolbar = dataframe_element.get_by_test_id("stElementToolbar") @@ -279,10 +283,15 @@ def _test_csv_download(page: Page, locator: FrameLocator | Locator): # Check that it is visible expect(dataframe_toolbar).to_have_css("opacity", "1") - # Click on download csv button: - with page.expect_download(timeout=5000) as download_info: + with page.expect_download(timeout=10000) as download_info: download_csv_toolbar_button.click() + # playwright does not support all fileaccess APIs yet (see this issue: https://github.com/microsoft/playwright/issues/8850) + # this means we don't know if the system dialog opened to pick a location (expect_file_chooser does not work). So as a workaround, we wait for now and then press enter. + if click_enter_on_file_picker: + page.wait_for_timeout(1000) + page.keyboard.press("Enter") + download = download_info.value download_path = download.path() with open(download_path, "r", encoding="UTF-8") as f: @@ -293,7 +302,12 @@ def _test_csv_download(page: Page, locator: FrameLocator | Locator): def test_csv_download_button(app: Page): - _test_csv_download(app, app.locator("body")) + click_enter_on_file_picker = False + # right now the filechooser will only be opened on Chrome. Maybe this will change in the future and the + # check has to be updated; or maybe playwright will support the file-access APIs better. + if app.context.browser and app.context.browser.browser_type.name == "chromium": + click_enter_on_file_picker = True + _test_csv_download(app, app.locator("body"), click_enter_on_file_picker) def test_csv_download_button_in_iframe(iframed_app: IframedPage): From 003c9f265ef229493941fff01a89812f3009d2ca Mon Sep 17 00:00:00 2001 From: braethlein Date: Wed, 15 May 2024 23:20:38 +0200 Subject: [PATCH 10/15] Fix test on Chromium headless --- e2e_playwright/st_dataframe_interactions_test.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/e2e_playwright/st_dataframe_interactions_test.py b/e2e_playwright/st_dataframe_interactions_test.py index 919b265befdb..d62e9541f653 100644 --- a/e2e_playwright/st_dataframe_interactions_test.py +++ b/e2e_playwright/st_dataframe_interactions_test.py @@ -301,12 +301,21 @@ def _test_csv_download( assert some_row in content -def test_csv_download_button(app: Page): +def test_csv_download_button( + app: Page, browser_name: str, browser_type_launch_args: dict +): click_enter_on_file_picker = False + # right now the filechooser will only be opened on Chrome. Maybe this will change in the future and the # check has to be updated; or maybe playwright will support the file-access APIs better. - if app.context.browser and app.context.browser.browser_type.name == "chromium": - click_enter_on_file_picker = True + # In headless mode, the file-access API our csv-download button uses under-the-hood does not work. So we monkey-patch it to throw an error and trigger our alternative download logic. + if browser_name == "chromium": + if browser_type_launch_args.get("headless", False): + click_enter_on_file_picker = True + else: + app.evaluate( + "() => window.showSaveFilePicker = () => {throw new Error('Monkey-patched showOpenFilePicker')}", + ) _test_csv_download(app, app.locator("body"), click_enter_on_file_picker) From 3542ada0220803b5bad37a763af747f05c4eb97f Mon Sep 17 00:00:00 2001 From: braethlein Date: Thu, 16 May 2024 10:30:11 +0200 Subject: [PATCH 11/15] Add docstring comment --- e2e_playwright/st_dataframe_interactions_test.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/e2e_playwright/st_dataframe_interactions_test.py b/e2e_playwright/st_dataframe_interactions_test.py index d62e9541f653..191f2f96aded 100644 --- a/e2e_playwright/st_dataframe_interactions_test.py +++ b/e2e_playwright/st_dataframe_interactions_test.py @@ -304,6 +304,12 @@ def _test_csv_download( def test_csv_download_button( app: Page, browser_name: str, browser_type_launch_args: dict ): + """Test that the csv download button works. + + Note that the library we are using calls the file picker API to download the file. This is not supported in headless mode. Hence, the test + triggers different code paths in the app depending on the browser and the launch arguments. + """ + click_enter_on_file_picker = False # right now the filechooser will only be opened on Chrome. Maybe this will change in the future and the From 385eae379795d7d306f19878cf6ea3f9fb74e3fa Mon Sep 17 00:00:00 2001 From: braethlein Date: Thu, 16 May 2024 11:00:24 +0200 Subject: [PATCH 12/15] Address feedback --- e2e_playwright/conftest.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/e2e_playwright/conftest.py b/e2e_playwright/conftest.py index a74e006df73c..ec1c18d67312 100644 --- a/e2e_playwright/conftest.py +++ b/e2e_playwright/conftest.py @@ -132,11 +132,6 @@ def resolve_test_to_script(test_module: ModuleType) -> str: return test_module.__file__.replace("_test.py", ".py") -def get_iframe_html_path() -> str: - """Get the absolute path of the given path.""" - return f"file://{os.getcwd()}/test_assets/iframed_app.html" - - def hash_to_range( text: str, min: int = 10000, @@ -353,7 +348,7 @@ def _expect_streamlit_app_loaded_in_iframe_with_added_header( ): page.goto(fake_iframe_server_route, wait_until="domcontentloaded") frame_locator = page.frame_locator("iframe") - frame_locator.nth(0).locator("[data-testid='stAppViewContainer']").wait_for( + frame_locator.nth(0).get_by_test_id("stAppViewContainer").wait_for( timeout=30000, state="attached" ) return frame_locator From eda37e6c720a4d617994303b24785299113da51d Mon Sep 17 00:00:00 2001 From: braethlein Date: Thu, 16 May 2024 11:03:18 +0200 Subject: [PATCH 13/15] Add comment --- e2e_playwright/st_dataframe_interactions_test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/e2e_playwright/st_dataframe_interactions_test.py b/e2e_playwright/st_dataframe_interactions_test.py index 191f2f96aded..aa20a584ca18 100644 --- a/e2e_playwright/st_dataframe_interactions_test.py +++ b/e2e_playwright/st_dataframe_interactions_test.py @@ -298,6 +298,7 @@ def _test_csv_download( content = f.read() # the app uses a fixed seed, so the data is always the same. This is the reason why we can check it here. some_row = "1,-0.977277879876411,0.9500884175255894,-0.1513572082976979,-0.10321885179355784,0.41059850193837233" + # we usually try to avoid assert in playwright tests, but since we don't have to wait for any UI interaction or DOM state, it's ok here assert some_row in content From 93a82afdf1feb63db1e5413a8e54ed1cc8ec457d Mon Sep 17 00:00:00 2001 From: braethlein Date: Thu, 16 May 2024 11:19:46 +0200 Subject: [PATCH 14/15] Add allow-directive to iframe --- e2e_playwright/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/e2e_playwright/conftest.py b/e2e_playwright/conftest.py index ec1c18d67312..df99c92b88f0 100644 --- a/e2e_playwright/conftest.py +++ b/e2e_playwright/conftest.py @@ -308,6 +308,7 @@ def fulfill_iframe_request(route: Route) -> None: