-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add fallback method for CSV download #8452
Changes from 19 commits
7319601
cc40bff
8f70472
f941238
c84dffd
9d3bc23
8393618
a29ca4c
82e6d68
ab7c47f
2f7acf5
712024b
003c9f2
31c7915
814bc76
3542ada
385eae3
eda37e6
93a82af
be8fe8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,100 @@ def test_data_editor_keeps_state_after_unmounting( | |
) | ||
|
||
|
||
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") | ||
|
||
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") | ||
|
||
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: | ||
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 | ||
raethlein marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
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 | ||
# check has to be updated; or maybe playwright will support the file-access APIs better. | ||
# 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) | ||
|
||
|
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, its using the fallback and not the file picker API or? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep it looks like in all iframe cases the fallback is used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added another comment here to make this clear |
||
|
||
|
||
# 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I believe we can also just call that within the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I just thought that not having nested hooks could be easier to comprehend, so keep it in DataFrame component, and pass to useDataExperter as parameter a little bit more preferable for me, although I also don't have super strong opinion here. |
||
} = React.useContext(LibContext) | ||
|
||
const [isFocused, setIsFocused] = React.useState<boolean>(true) | ||
const [showSearch, setShowSearch] = React.useState(false) | ||
const [hasVerticalScroll, setHasVerticalScroll] = | ||
|
@@ -475,7 +480,12 @@ function DataFrame({ | |
] | ||
) | ||
|
||
const { exportToCsv } = useDataExporter(getCellContent, columns, numRows) | ||
const { exportToCsv } = useDataExporter( | ||
getCellContent, | ||
columns, | ||
numRows, | ||
enforceDownloadInNewTab | ||
) | ||
|
||
const { onCellEdited, onPaste, onRowAppended, onDelete, validateCell } = | ||
useDataEditor( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: are these also the same permissions we use in SIS / community cloud?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I believe so, at least they are the ones @sfc-gh-jkinkead posted in the example in #8528
I will double-check!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added
allow="clipboard-write;"
to the iframe which is used in SiS. CC has some more things allowed, but I guess its good to use the more locked-down one