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

Problem transitioning to `Plain` image from other image types #550

Closed
rgrempel opened this Issue Apr 5, 2016 · 1 comment

Comments

Projects
None yet
2 participants
@rgrempel
Contributor

rgrempel commented Apr 5, 2016

There is a bug here which would create a problem if someone were to try to transition to a Plain image type from any other image type (e.g. Fitted).

https://github.com/elm-lang/core/blob/3.0.0/src/Native/Graphics/Element.js#L486-L493

This code appears to optimize the case where it is only the src attribute which is changing. In that case, it is not necessary to recreate the DOM node -- once can just change the src attribute (and, of course, apply other property updates, but that is not relevant here).

The problem is that the code only checks the type of the image which we are transitioning to -- it ignores the type of the image we are transitioning from. The optimization only makes sense if we are transitioning from a Plain image to another Plain image. If, for instance, we are transitioning from a Fitted image to a Plain image, then the structure of the DOM is completely different -- updating the src attribute will not accomplish anything.

One way to fix this would be to change the conditional to something like:

if (nextE._0.ctor === 'Plain' && currE._0.ctor === 'Plain')

Of course, there may be other ways.

It is possible that no one has ever encountered this bug, since it is probably unusual to transition from one image type to another. Therefore, one might not consider this to be a high priority bug to fix.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz May 11, 2016

Member

Sorry for the slow reply. It has been busy times. All the Graphics.* modules have moved to evancz/elm-graphics so it makes sense to retarget stuff like this.

Member

evancz commented May 11, 2016

Sorry for the slow reply. It has been busy times. All the Graphics.* modules have moved to evancz/elm-graphics so it makes sense to retarget stuff like this.

@evancz evancz closed this May 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment