Add fallback method for CSV download #8452
Conversation
| }) | ||
| // Write row to CSV: | ||
| await writer.write(textEncoder.encode(toCsvRow(rowData))) | ||
| const url = URL.createObjectURL(blob) |
There was a problem hiding this comment.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
| @@ -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.
nit: I believe we can also just call that within the useDataExporter. Thought, I don't have any strong opinion if its better to get the context here or within the useDataExporter hook.
There was a problem hiding this comment.
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.
| const url = URL.createObjectURL(blob) | ||
| const link = document.createElement("a") | ||
| if (enforceDownloadInNewTab) { | ||
| link.setAttribute("target", "_blank") |
There was a problem hiding this comment.
nit: maybe add a one-liner comment here why we are doing this
There was a problem hiding this comment.
I meant more like answering why we are doing that: "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)."
There was a problem hiding this comment.
I rephrased with your suggestion!
| <iframe | ||
| src="{app_url}" | ||
| title="Iframed Streamlit App" | ||
| sandbox="allow-popups allow-same-origin allow-scripts allow-downloads" |
There was a problem hiding this comment.
nit: are these also the same permissions we use in SIS / community cloud?
There was a problem hiding this comment.
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.
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
| timeout=10000, | ||
| ): | ||
| frame_locator: FrameLocator = iframed_app.open_app() | ||
| _test_csv_download(page, frame_locator) |
There was a problem hiding this comment.
In this case, its using the fallback and not the file picker API or?
There was a problem hiding this comment.
yep it looks like in all iframe cases the fallback is used
There was a problem hiding this comment.
I have added another comment here to make this clear
Describe your changes
This PR adds a fallback to an old browser download method for CSV dataframe download feature. Some browser or situations might not correctly support the file picker feature, in these cases it will fallback to an older more commonly supported download method.
We also add iframe tests together with CSPs here. This can be used in the future also by other tests.
GitHub Issue Link (if applicable)
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.