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

Image Resize Plugin breaks after History or Paste Action #901

Closed
realulim opened this issue Dec 19, 2018 · 9 comments
Closed

Image Resize Plugin breaks after History or Paste Action #901

realulim opened this issue Dec 19, 2018 · 9 comments
Labels
bug:plugin Bug in a plugin

Comments

@realulim
Copy link
Contributor

Informations

Tested with newest Firefox and Chrome on MacOS.

How to reproduce the bug?

  1. Go to a Page with activated Resize Plugin (Live Demo page at https://alex-d.github.io/Trumbowyg/demos/#plugins-resizimg will do)
  2. Delete Image (place cursor behind it and press Backspace)
  3. Undo delete via History plugin (if you have History plugin installed)
  4. If you don't have the History plugin installed, you can also simulate the effect with ctrl-x ctrl-v (cut and paste)
  5. Image is restored after either undelete or paste, but it cannot be resized anymore

I could not see a relevant difference between the two DOMs (resizable and not resizable), so I suspect that some Javascript initialisation code would have to re-run to make the image resizable again. If true, then this would probably mean that images would also not be resizable after saving and loading the editor content.

@realulim
Copy link
Contributor Author

realulim commented Dec 27, 2018

My last assumption turned out to be correct. Even when reloading the page with the editor, which should cause all the JavaScript to run again, the image - once unresizable - never becomes resizable again.
This discovery led me to a workaround: a non-resizable image has the CSS class "resizable". Oddly enough, removing this class in the developer tools makes the image resizable again, even though the CSS class is immediately reassigned. I do have to click once on the image for that to happen.

@realulim
Copy link
Contributor Author

Looking at the code (https://github.com/Alex-D/Trumbowyg/blob/develop/plugins/resizimg/trumbowyg.resizimg.js) it seems line 48 is to blame. Only images that do not already have the class "resizable" are initialised by calling the resizable function from the jquery-resizable plugin. So if you save a resized image and load it later, it won't be initialised, because it already has the CSS class. Workaround is obviously to strip the class before saving.
I'm not sure what the best way to fix this bug is, otherwise I'd submit a pull request. But I suppose that there should be a better way to check, whether an image has already been initialised other than checking for the existence of the CSS class "resizable".

@Alex-D
Copy link
Owner

Alex-D commented Dec 28, 2018

I assume it's done like that to avoid re-init the resizable plugin on each image each time.

And yes, remove all resizable class should be a good solution: maybe at plugin init. Also it should be renamed with trumbowyg prefix to avoid conflicts.

@realulim
Copy link
Contributor Author

The resizable class is supplied by the jquery-resizable plugin, so there's nothing we can do about the name. If we want to fix this bug, then it's not enough to just remove the class at plugin init, because as soon as an image is deleted and restored or cut and pasted, the resizable class will reappear.

The interesting part is that the editor actually doesn't contain this class, only the DOM does. The way the class gets into the editor (which subsequently triggers the bug) is precisely by "history undo" or "paste" command. At that point the image is apparently restored from the previous DOM, not from the previous editor content. So perhaps this needs fixing in another place?

@Alex-D
Copy link
Owner

Alex-D commented Dec 28, 2018

There's already a jQuery data object attached to img element: https://github.com/RickStrahl/jquery-resizable/blob/master/src/jquery-resizable.js#L76

So we just need to change the selector from :not(.resizable) to just img then filter on data exists or not.

Should works with history/cut events, no?

@Alex-D Alex-D added the bug:plugin Bug in a plugin label Dec 28, 2018
@realulim
Copy link
Contributor Author

My other fix from #903 fixes half of this one as well. So let's get the other fix in first (or decide against it) in order to get a definite base for this one.

@realulim
Copy link
Contributor Author

I didn't get this to work, checking for data gave me erratic results. As well as removing the check entirely and just re-initialising every image every time.

The most reliable option at this point is to strip CSS classes when saving the editor content (which might be a best practice anyway). That way all images become resizable again after loading.

@realulim
Copy link
Contributor Author

Ok, so the "half" that is fixed is that the History-Back function now works fine. The image is restored without the "resizable" class. The part that doesn't work right yet is cut&paste - in that case the image is pasted with the "resizable" class.

@realulim
Copy link
Contributor Author

realulim commented Jan 26, 2019

Just created another pull request #919, I hope this fixes it completely.

Alex-D pushed a commit that referenced this issue Jan 27, 2019
Upon destruction of the Resizable, we need to remove the class 'resizable', otherwise a subsequent init will not work

Fix #901
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:plugin Bug in a plugin
Projects
None yet
Development

No branches or pull requests

2 participants