Skip to content
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

Fix Safari Download #5609

Merged
merged 1 commit into from
Jul 19, 2021
Merged

Conversation

rlreamy
Copy link
Contributor

@rlreamy rlreamy commented Apr 26, 2021

Fix for issue #1227.

It appears that for Safari Version 14.0.1 (16610.2.11.51.8) the special code in fileSaver.js for handling Safari browsers is actually breaking the download. The filename is always "Unknown" and it doesn't download the entire image (in our case a scatter plot gave us only the axes and labels, but none of the points). Strangely a second try at the download will give the user the full plot, but the filename is still "Uknown".

Removing the special handling worked when testing with npm start.

…ri, and it appears that recent versions may not need it
@archmoj archmoj added bug something broken community community contribution labels Apr 26, 2021
@archmoj archmoj requested a review from alexcjohnson June 7, 2021 15:43
@alexcjohnson
Copy link
Collaborator

Thanks @rlreamy - it's interesting that this works now, but it clearly didn't in past versions of Safari (and some docs haven't caught up yet, like https://www.w3schools.com/jsref/prop_anchor_download.asp still says "Not Supported")

Can we instead move the special handling after the if(canUseSaveLink) block? That way older versions that fail this test will still at least have the previous behavior.

@archmoj
Copy link
Contributor

archmoj commented Jun 15, 2021

It would be really nice to get this PR to the finish line for the upcoming 2.1.0 release.
@rlreamy could you possibly address @alexcjohnson's comment above?

@archmoj archmoj added this to the NEXT milestone Jun 15, 2021
@rlreamy
Copy link
Contributor Author

rlreamy commented Jun 16, 2021

It is possible, but I cannot promise anything. This change was done as part of a work project, and I have not yet had time to revisit.
I can tell you that this change doesn't quite fix the issue, however. It does improve the situation, in that you will always get the correct filename now. However, there is a strange behavior that the first time you download the png, you get the axes, but not plot (for the scatter plots that we are generating with thousands of points). But, the second time on, it downloads everything as it should.
Another developer on my team will be revisiting this work in the next 2 weeks, and I will make sure to make a note of this request there.

@archmoj archmoj removed this from the v2.1.0 milestone Jun 17, 2021
@archmoj
Copy link
Contributor

archmoj commented Jun 17, 2021

Thanks very much @rlreamy for the information.
It looks something we could fix in a patch release.
So for now I dropped it from v2.1.0 milestone.
I am looking forward to the follow up whenever you had time.

@archmoj archmoj added this to the v2.3.0 milestone Jun 26, 2021
@archmoj archmoj changed the base branch from master to safari-download July 19, 2021 20:55
@archmoj
Copy link
Contributor

archmoj commented Jul 19, 2021

💃 Thanks for the PR.
Merging into safari-download branch.

@archmoj archmoj merged commit 0f9220c into plotly:safari-download Jul 19, 2021
@archmoj archmoj removed this from the NEXT milestone Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken community community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants