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 bug when scaling canvas with CSS #100

Closed

Conversation

richardcpeterson
Copy link

Previously, setting the canvas width and height via CSS "width" and "height" properties
would create a scale multiplier that SignaturePad did not account for. This caused the
line to show up offset from where the user attempted to draw.

To reproduce, open in browser and set

canvas {
    width: 200px;
    height: 100px;
}

with those widths and heights differing from the actual canvas width and height. Then
attempt to draw.

With this commit, the scale multiplier is taken into account, and scaling the canvas with
CSS works just fine, with drawn lines appearing directly under the cursor.

STILL NEEDS MINIFICATION

Previously, setting the canvas width and height via CSS "width" and "height" properties
would create a scale multiplier that SignaturePad did not account for. This caused the
line to show up offset from where the user attempted to draw.  

Now, the scale multiplier is taken into account, and scaling the canvas with CSS works
just fine, with lines appearing directly under the cursor.
@szimek
Copy link
Owner

szimek commented Apr 25, 2015

Does the demo app also have this issue? Could you prepare a demo that reproduces this issue e.g. on jsfiddle?

The demo app has some code that should handle this issue: https://github.com/szimek/signature_pad/blob/gh-pages/js/app.js#L10-L18.

@richardcpeterson
Copy link
Author

The demo app doesn't display the bug because of the window.onresize that
you set in the app.js, which resizes the actual canvas.

Resizing the canvas like SignaturePad's app.js won't work for my
application, since the saved images need to be a certain dimension, and I'm
using CSS to fit SignaturePad into a responsive layout.

Here's the bug in action:

http://jsfiddle.net/7v31o00t/

On Sat, Apr 25, 2015 at 4:39 AM, Szymon Nowak notifications@github.com
wrote:

Does the demo app http://szimek.github.io/signature_pad/ also have this
issue? Could you prepare a demo that reproduces this issue e.g. on jsfiddle?


Reply to this email directly or view it on GitHub
#100 (comment).

@szimek
Copy link
Owner

szimek commented Apr 25, 2015

I'd like to avoid adding any code that does something with canvas to the library, unless there's really good reason to. Would your change break the existing apps that use this library or would _getCanvasWidth/HeightScale return just 1 if somebody calls canvas.height = canvas.offsetHeight manually? It looks like you're using clientWidth/Height and the demo uses offsetWidth/Height - these probably would need to be the same to make sure nothing breaks.

  1. Regarding images with certain dimension - can't you simply resize images to required size either on the client side (e.g. by rendering to offscreen canvas of required size) or server side?
  2. I'm not really sure what responsive layout has to do with it - is the canvas resized when the window is resized as well? If that's the case, can't you simply handle it using window.onresize handler?

@richardcpeterson
Copy link
Author

On Apr 25, 2015, at 12:50 PM, Szymon Nowak notifications@github.com wrote:

I'd like to avoid adding any code that does something with canvas to the library, unless there's really good reason to. Would your change break the existing apps that use this library or would _getCanvasWidth/HeightScale return just 1 if somebody calls canvas.height = canvas.offsetHeight manually?

I don’t know. I’m not sure I understand your use case here.

It looks like you're using clientWidth/Height and the demo uses offsetWidth/Height - these probably would need to be the same to make sure nothing breaks.

Could be. I noticed several fields on the canvas object that had the identical “scaled” width / height values (as opposed to the true canvas width / height), and to be honest, I didn’t look under the hood or at the documentation to see which is most appropriate to use. I just picked the one that seemed to have a sensible name (“client” width. Seems like that should be the width of the canvas as displayed in the client...). Maybe I picked the wrong one. In any case, I think the underlying purpose of the patch is a sound one, even if I picked the wrong field out of haste.

Regarding images with certain dimension - can't you simply resize images to required size either on the client side (e.g. by rendering to offscreen canvas of required size) or server side?

It seems to me that the JSFiddle example I sent is glitchy. In my opinion, I should be able to do what I attempted to do in JSFiddle and have it work as expected. As a user of the library, I want it to be the case that when I draw, the ink goes where I click, and I want the library to handle that.

Having to handle this server-side is burdensome. I’m just saving out the data URLs to my database, which is a good solution for what I’m doing.

I'm not really sure what responsive layout has to do with it - is the canvas resized when the window is resized as well? If that's the case, can't you simply handle it using window.onresize handler?
By responsive layout, I just mean that when I stick a signature pad in a context that is likely to be scaled depending on the client viewport size, I still want things to work. In my case, I have a div containing the canvas, and the div has max-width set. The canvas inside has width set to 100% in CSS. Thus the canvas scales as the viewport shrinks. But I still want that underlying canvas to be the same pixel dimensions, just as if it were any other image.

The browser does good work scaling images in the sub-optimal case that they must be scaled. I’d rather have the option of letting the browser do its work than having to redo things in my application code.

In SignaturePad, the user is drawing in client coordinates, and it seems natural that it should be SignaturePad’s job to convert client coordinates to canvas coordinates, since the library is managing the interaction between events (client coordinate space) and the canvas quite intimately.

-Richard


Reply to this email directly or view it on GitHub.

@jlambert121
Copy link

Just curious if there's any update on this. We ran into this issue as well and the supplied patch resolves the issue we were seeing.

@szimek
Copy link
Owner

szimek commented Aug 5, 2015

@jlambert121 Thanks for the report that it fixes your issue. I don't have much free time now and I'd really like to test it out thoroughly before I merge it to make sure it doesn't break anything and doesn't cause any slowdowns on older devices (as it needs to get canvas size when creating every point).

@PerfectPixel
Copy link
Contributor

+1

This is vital for creating high res canvas elements on iOS devices. Otherwise everything is blurry. I could not find any issues yet. If you fear a performance issue, we could explore the options for caching this property. In case of an iOS device it is simple: canvas width and height are twice as big as the css width and height.

@szimek szimek added this to the 2.0 milestone Nov 6, 2015
@digi604
Copy link

digi604 commented Nov 12, 2015

i tested this branch it works as advertised. The code from the example does not work if there is other js resizing the stuff around. This branch works always for me.

@szimek
Copy link
Owner

szimek commented Nov 18, 2015

Just a small update - I'll try to take a look at it over the weekend.

@szimek
Copy link
Owner

szimek commented Dec 30, 2015

@richardcpeterson I've prepared a jsfiddle that includes this fix (it's v1.5.2 with this fix) - http://jsfiddle.net/jLm3nw2z/. Turns out that without scaling the canvas in any way, the width of lines is scaled instead. It might be something that you want or not, I'm not sure really. I'd expect lines to have the same width regardless of the canvas size.

@richardcpeterson
Copy link
Author

@szimek The behavior in the fiddle is what I expect.

The lines, line thickness, and the whole canvas is scaled with CSS just like a regular image would be scaled, with everything that entails. For instance, if I create a 50px x 50px canvas, and scale it up using CSS to 500x500, I'm going to see some artifacts. The width of the lines should change appropriately, just like with any image. Everything should scale, just like with a PNG, etc.

However, what's now fixed is that you can still draw on it, and the tracking of the cursor takes into account this scaling. So the Fiddle represents what I consider to be a solution that gives the user of your plugin the most predictable and consistent results. They can scale the canvas with CSS if needed, and still draw just fine.

@szimek
Copy link
Owner

szimek commented Jan 4, 2016

@richardcpeterson I'll have to check if it doesn't break anything else (the low/high DPI screen stuff is really confusing...). I don't really have any preference regarding this behaviour, but you're right that this fix at least allows users to draw on a canvas even if CSS and physical sizes don't match.

@szimek
Copy link
Owner

szimek commented Jan 4, 2016

@richardcpeterson If you drawn something on a canvas where sizes don't match, save it as data URL and then try to read it back - is it drawn correctly?

@PerfectPixel
Copy link
Contributor

I had no issues with reading / writing data URLs

@szimek
Copy link
Owner

szimek commented Jan 4, 2016

@PerfectPixel Here's a demo: http://jsfiddle.net/szimek/L1cdekw6/2/. If you draw something on the first canvas, it will be copied after 5 seconds to the second one, but it will not look correctly.

I'm not sure if it won't be confusing that you can draw on a canvas with non-matching size, but then can't properly read it back on the same canvas.

@PerfectPixel
Copy link
Contributor

@szimek Well I have two options for you: http://jsfiddle.net/L1cdekw6/3/ abolishes the ratio-scaling for width&height in loadFromDataURL or you need to properly size your canvas before using it http://jsfiddle.net/akxmz98o/

A small disclaimer though: I am sitting at a non-retina device at the moment and have a little headache. I can check it out at work tomorrow and give you feedback.

@Teemu
Copy link

Teemu commented Jun 27, 2016

I have done some work on my fork of angular-signature to fix this issue. It's probably not the right place for it as the underlying library should handle all the high DPI / CSS scaling issue stuff.

Btw, Has there been any attempts to use SVG instead? Performance?

@UziTech
Copy link
Collaborator

UziTech commented Jun 8, 2021

Closing this PR as stale

@UziTech UziTech closed this Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants