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

#1511: Undo and change notification for object resizing in IE #88

Closed
wants to merge 2 commits into from

Conversation

bingnz
Copy link

@bingnz bingnz commented Mar 16, 2014

Native resizestart and resizeend events are now tracked in Internet Explorer to create undo snapshots when objects (e.g., images) have been resized.
This does not address the issue in other browsers.

…ot working correctly. Native resizestart and resizeend events are now tracked in IE to create undo snapshots when objects (e.g., images) have been resized. This does not address the issue in other browsers.
@Reinmar
Copy link
Member

Reinmar commented Mar 17, 2014

Thanks! :)

We'll review your PR for 4.4.1, because 4.3.4 is already frozen.

In the meantime, I noticed that your patch tries to add resizestart/end listeners to the selected element. AFAIK and AFAICS in https://github.com/ckeditor/ckeditor-dev/blob/6ce3f4e9a613032a63bb5d019486d449a99b76f8/plugins/wysiwygarea/plugin.js#L215 these events should bubble. Wouldn't it be possible to simply add listeners to the editable itself?

And the second thing - your patch fixes wysiwygarea only, so snapshots won't be created in inline editor and divarea. If you placed listeners in editable.js, then they would be always added.

@bingnz
Copy link
Author

bingnz commented Mar 17, 2014

Thanks for your comments! I did try to hook the bubbled events on the editable but it didn't work, at least in my version of IE. Using on( 'resizestart' ) or attachListener didn't work either, which is why I had to fall back to attachEvent. I'll have another go when I have time, and also move the patch to editable.js.

Thanks,
David

@bingnz
Copy link
Author

bingnz commented Mar 18, 2014

Updated patch to move code to editable.js and use bubbled events.

… not working correctly. Native resizestart and resizeend events are now tracked in IE to create undo snapshots when objects (e.g., images) have been resized. This does not address the issue in other browsers.

Moved original fix to editable.js so that it works in all editing modes.
@mlewand
Copy link
Contributor

mlewand commented May 5, 2014

We've reviewed your pull request with following results:

The general direction is very good, you've done good job with simplifying the code. We miss two things in your pull request:

  1. You should use this.on( 'resizestart', function( evt ) {... rather than this.$.attachEvent( 'onresizestart', function( evt ) {... - it will be simpler, and will bring support for IE11. As a result you might also then skip whole if ( this.$.attachEvent ) condition, which will simplify the code.
  2. We're missing also support for Mozilla Firefox, which also allows for resizing images - so we keep consistency between the browsers.

We're looking forward for your pull request with these issues fixed! :)

@Reinmar
Copy link
Member

Reinmar commented May 6, 2014

@mlewand This pull request is good enough. Please fix the minor issue mentioned in 1. and as for the 2. - Firefox is a separate problem. So please proceed with this pull request.

@Reinmar
Copy link
Member

Reinmar commented May 6, 2014

Surprise, surprise. During last review I noticed that when using addEventListener for listening on resizestart and resizeend events bubbling stops working on IE9+ ;|. Accepting this pull request with the attachEvent calls doesn't make much sense, because it'd be a temporary fix for IE8-10 only. So soon we would have to work on IE11+.

@bingnz: Sorry for the mess and wasting your time, because your first pull request was actually closer to what we need. However, there's also this ticket https://dev.ckeditor.com/ticket/9317 and they both need to be fixed by one patch, so I'm closing your pull request, because we'll have to refactor more code.

@Reinmar Reinmar closed this May 6, 2014
@bingnz
Copy link
Author

bingnz commented May 6, 2014

Thanks @Reinmar and @mlewand. I'm using CKEditor in a very constrained environment (embedded WebBrowser control in a WPF application) and hadn't had a chance to test IE11. I actually constrain the version to 9 with a META tag. I wonder if IE11 and FireFox could both use MutationObserver?

@Reinmar
Copy link
Member

Reinmar commented May 6, 2014

I've never had a chance to test MutationObserver in contenteditable. I'm never optimistic regarding a new feature's compatibility with contenteditable, but I know that the author of the onchange plugin was using it.

mdenburger pushed a commit to onehippo/ckeditor that referenced this pull request Oct 28, 2015
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.

None yet

3 participants