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

Fixes for downloading PNG #84

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

louh
Copy link
Contributor

@louh louh commented Nov 25, 2019

Hi there,

I'd like to open my PR to further the discussion on #75. This fixes two big issues for us:

  1. If the DOM containing element has a variable width and height, and it contains an <svg> element with a viewBox property, then the resulting downloaded PNG will render the SVG at its own viewBox dimensions, which will mismatch from the DOM dimensions. This proposal renders the resulting PNG at the source SVG's viewBox dimensions.

  2. If the user downloads a PNG from a monitor with a high pixel density (such as a Macbook with a Retina screen) then the resulting PNG will be pixelated. This PR renders the resulting PNG at the same pixel density as the user's monitor.

There is a broken test because reading baseVal from an empty SVG throws an error. This might take time to fix, so I'd like to open this for further conversation so I can decide how to approach this. Thanks!

@cy6erskunk
Copy link
Owner

Hi, thanks for the PR and sorry for the long radio silence!
This looks really interesting, could you give en axample of the first case, it's really hard to understand what's going on there from the text description.

@louh
Copy link
Contributor Author

louh commented May 8, 2020

Hi @cy6erskunk, apologies for my own long radio silence! I'm finding it hard to set up a simple test case for the first point, and I need to do a little more experimentation on my side to see whether it's just an issue with our own implementation.

In the meantime, can I break out the device pixel ratio change as a separate, standalone PR? I imagine this would be an easier merge to make and shouldn't be blocked while I work on the first point. Thanks!

@@ -38,8 +38,8 @@ function getSource(svg) {
const result = {
top: rect.top,
left: rect.left,
width: rect.width,
height: rect.height,
width: svg.width.baseVal.value,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, it's a bit more complicated than I thought. Chrome and Firefox approach this value a bit differently. I'm trying to set up some tests as it feels that solution gonna be a bit more complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed. I do have a bit of time to help investigate this in the next few weeks, but that's also why I'm proposing that I break out commit 432f198 into a separate PR, since it's a different issue. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let’s do it!

@louh
Copy link
Contributor Author

louh commented May 26, 2020

@cy6erskunk I'm putting an example for the first case in the original issue (#75).

@louh louh force-pushed the louh/png-fixes branch 2 times, most recently from 987825e to 0237aa1 Compare December 6, 2020 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants