joakin / elm-canvas

A canvas drawing library for Elm
https://package.elm-lang.org/packages/joakin/elm-canvas/latest/
BSD 3-Clause "New" or "Revised" License
163 stars 30 forks source link

Canvas element doesn't update when width and height change #7

Closed dkodaj closed 5 years ago

dkodaj commented 5 years ago

Canvas.toHtml seems unresponsive to changes in the model: https://ellie-app.com/4kq2Hz8699za1

In the Ellie example, the canvas element is initialized with width = 0 and it stays that way even when model.width is updated.

joakin commented 5 years ago

It indeed only reads the width and height on creation, so changing it from Elm will be problematic as you are seeing.

This happens in the JS web-component: https://github.com/joakin/elm-canvas/blob/master/elm-canvas/elm-canvas.js#L15-L26

I believe there is some sort of web-component magic (attributeChanged methods or something like this) where we could probably listen to attribute changes and re-run the width/height dance that happens on those lines. I'd be happy to review a PR. May have a look myself at some point.

For now, you can force re-creating the whole canvas on resize by using Keyed in the parent div and using the width and height as the key for the canvas node. See this example:

https://ellie-app.com/4kSVhq5cx6da1

dkodaj commented 5 years ago

Great, thanks! Didn't know about the Keyed.node trick.

lazamar commented 5 years ago

Would this be solved if we just add the width and height style attributes here https://github.com/joakin/elm-canvas/blob/master/src/Canvas.elm#L167 ?

Something like this

canvas (height h :: style "height" (String.fromInt h) :: width w :: style "width" (String.fromInt w) :: attrs) []
joakin commented 5 years ago

I guess it could, but then the device pixel ratio tricks for HDPI screens would be messed up because we don't have access to those in Elm. That is why width and height styles get reset when the custom element initializes (see here).

For now thankfully there is a workaround:

And I think the best way to fix this would be:

  1. Extract the height/width/devicePixelRatio logic into a setCanvasDimensions(w, h) method and call it in connectedCallback
  2. Implement observedAttributes and attributeChangedCallback (here is an example) for width and height, and re-run setCanvasDimensions if they changed

Once that is done we can publish a major version of the npm elm-canvas web component and this will be fixed for existing uses if the version of the JS library is updated.

I'd be happy to review a PR if anyone is interested, otherwise I'll get to it at some point.

joakin commented 5 years ago

This is fixed, but not released, will close when released

joakin commented 5 years ago

Published:

The full screen example works, and it is an example in the repo.

benjajaja commented 4 years ago

@joakin I'm having some issues with the canvas.width = ... assignment clearing my canvas. It seems that when the canvas custom-element sets the width, this clears the canvas, and it relies on the Elm code redrawing some time after, usually from a continuous onAnimationFrame that MUST cause a call to Canvas.toHtml. If there is any Html.Lazy in between the onAnimationFrame update and the canvas being drawn, this breaks down and leaves a transparent canvas.

Proof: https://ellie-app.com/8R6NhyxrwbTa1 If you remove the lazy then it works.

I see no easy solution, as the only guarantee is that the custom-element's size adjustments will be called after Elm, but I think there is no safe way to execute e.g. a redraw in Elm after the custom-element is done. Could elm-canvas.js be hacked to temporarily copy the canvas contents into a buffer, resize, and then copy back?

Maybe you can guide me a bit on this. Before using canvas, I was using SVG so it was crucial to use lazy and render as little as possible. Perhaps with canvas it doesn't matter, and I should simply render constantly onAnimationFrame?

joakin commented 4 years ago

@gipsy-king Hey, I've looked at it and it is a bug, given we schedule the change of dimensions with requestAnimationFrame but don't re-render when we apply the changes.

Thanks for making the Ellie, extremely helpful to debug.

I've embedded the JS library and fixed it, see L53 and L87, if you want to keep going for now. I'll see if I can make a commit soon and release a new version to npm.

Next time please make a new issue and link to this old one, it is better for keeping track of what needs to be done 👍

joakin commented 4 years ago

Released 2.2.4 of the JS library with the fix.

See your ellie with the latest version fixed: https://ellie-app.com/8R9xRJzKbmYa1

benjajaja commented 4 years ago

Wow! It didn't occur to me that the js part could also re-render, of course!

This is great!

I will make sure to open a new issue and link next time :)