Opened 7 years ago

Closed 6 years ago

#10281 closed Bug (fixed)

jQuery integration issues

Reported by: Jakub Ś Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.2
Component: General Version:
Keywords: Cc:

Description

This is an umbrella ticket for jQuery problems and feature requests. Because requests mainly concern jQuery adapter I think this should be reported in #9790 however after consulting with @Reinmar I have decided to open new issue.

  • #9790 - create jQuery Adapter for CKE 4.x

jQuery adapter feature requests

  • #8261 - allow to work with existing instances
  • #9077 - use val Hooks

jQuery adapter bugs

  • #8710 - jQuery adapter doesn't support 'form' property
  • #8530 - jQuery adapter breaks save button
  • #9019 - jQuery Adapter version of jQuery.val() does not handle .val(undefined)
  • #6181 - jQuery adapter + jQuery ajaxForm plugin not working
  • #7778 - jQuery Adapter does not work with ckeditor_basic.js
  • #7876 - jQuery Adapter val() function handles multiple elements incorrectly

jQuery Can't Fix bugs

  • #10092 - Focus lost on IE when reopening in an iframe with a jQuery onload listener
  • #10269 - Dropdowns emptied on second click

jQuery tasks

  • #6906 - check if validation is correctly performed with jQuery

Change History (33)

comment:1 Changed 7 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 7 years ago by Piotrek Koszuliński

cc

comment:3 Changed 7 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:4 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.2

comment:5 Changed 6 years ago by Piotr Jasiun

Status: assignedreview

comment:6 Changed 6 years ago by Piotr Jasiun

What I've done:

  • support inline editing (frame editor will be created for textarea, inline editor for any other supported element),
  • new editor getter: use ckeditor().editor insteand of ckeditorGet (which now is deprecated),
  • separate setData and dataReady events (previously jQuery setData.ckeditor event was connected to CKEditor dataReady event; now setData.ckeditor is setData and dataReady.ckeditor is dataReady),
  • rewrite val() function using valHooks (what fixed #9077, #9019 and #7876),
  • add save event to save plugin and overwrite it in jQuery adapter; now save button in editor will call jQuery.submit and should work properly with jQuery.forms (tickets #6181 and #8530),
  • tests, tests and more tests,
  • docs in JSDuck style.
Last edited 6 years ago by Piotr Jasiun (previous) (diff)

comment:7 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_failed

I reviewed the code of the adapter.

  • I rebased both dev and test branches on latest major.
  • I also fixed lots of code-style issues, formatting, spelling and grammar. They've been pushed to dev.
  • I passed window.jQuery to the closure as $ to save bytes.

There are several issues visible in the adapter:

Collection handling

For the following HTML and JS three editors are created:

<div class="foo" contenteditable="true">#1</div>
<div class="foo" contenteditable="true">#2</div>
<div class="foo" contenteditable="true">#3</div>
$( '.foo' ).ckeditor();
  • There's no information in doc strings that such operation is possible. It should be documented.
  • Since $( '.foo' ).ckeditor() returns jQuery collection, $( '.foo' ).val() should return array of strings corresponding with editors data.
  • $( '.foo' ).ckeditor().editor should return array of editor objects. At the moment only the first one is returned.

setData

editor.setData() is asynchronous. It may seem to be synchronous, but it isn't. It takes callback argument if some operation needs to be done after data is set. This callback argument must be exposed and docummented in the adapter so users can use it.

Accordingly the following test is invalid:

$( '#editor1' ).ckeditor().editor.setData( ... );
assert.areSame ( ..., $( '#editor1' ).val(), ... ); // this may be executed before data is set!

Save event

Why don't we make it really useful? Let's think what other opportunities it may offer for end-users. Some data? Some info? Maybe it's possible to have it for more than aborting form submissions.

Tests

  • What we have in our fort is $().jquery == '1.7.2'. At the moment 2.0.2 is latest stable. And 1.10.1 is latest stable in 1.x. We need to decide which versions are supported by our adapter and how to test it. One things is sure: libraries got to be offline.
  • Lots of code style needs to be fixed. Brackets, spaces, new lines.
  • More "foo" and "bar". Less "lorem ipsum ..... ". Shorter strings are easier to read and debug.

val.html

  • You check only framed instances. What about inline ones and divarea?
  • You could've use a similar solution like setUpEditors from core/dt/filter/filter.html to initialize different editors (framed, inline...). Then you could write one super-test that iterates over all editors and checks all the things (get/set/multiple/undefined). Full coverage. Profit.
  • I don't see what happens if

test get value

  • Setting and checking value of #textarea must be explained in assertion. This <textarea> isn't bound to any editor and it's very hard to guess that this is a check for correct, non-greedy jQuery getter/setter behavior.
  • Counting getData.ckeditor isn't correct. What if getData is never fired? This should be more like:
    var eventCounter = 0;
    
    $( '#editor1' ).ckeditor( function() {
        ...
        assert.areSame( 1, eventCounter, 'There should be only one event.' );
        resume();
    } );
    
    $( '#editor1' ).ckeditor().on( 'getData.ckeditor', function( event, editor, data ) {
       ++eventCounter;
    } );
    
  • function( event, editor, data ) in event callback: those parameters must be checked. Check whether:
    • editor object equals that one from CKEDITOR.instances
    • event is a proper setData event (it got a name)
    • data is what we set

test set value

  • See: "setData" section above.

test multiple editors

  • See: "Collection handling" section above.

test val undefined

  • It's not enough to check if this is an object. Note that typeof null is Object :D Better check if returned object is what it should be.

form.html

  • The same thing like in val. All types of creators should be checked.

test form with textarea inside

  • Why not setData in the following?
    $( '#editor-inside' ).ckeditor().editor.editable().setHtml( LIPSUM_1 );
    

test save button with jQuery form

  • Better keep the following offline:
    <script src="//cdn.jsdelivr.net/jquery.form/3.24/jquery.form.js"></script>
    
  • Kiss:
    $( '#editor-save' ).ckeditor().editor.execCommand( 'save' );
    
    is
    this.execCommand( 'save' );
    

instance.html

  • You checked two instances:
assert.isUndefined( CKEDITOR.instances.editor2, 'editor2 should be undefined.' );
assert.isUndefined( CKEDITOR.instances.editor3, 'editor3 should be undefined.' );

Why not ckeditor 9, 11 or 42? Use CKEDITOR.tools.objectKeys( CKEDITOR.instances ) for faster and full check. Also use ArrayAssert.

  • See dt/core/creators/creators.html#test creator replace. Reuse some assertions.
  • Automate assertions. Your tests are very redundant here. Create input object(s) with element id's/classes as keys and desired elementModes, names, etc. as values. Call ckeditor() massively and assert massively in callbacks comparing with input object(s)'s values.

setUp

  • Why not CKEDITOR.destroy()? It's safer, isn't it?

We'll get back to that test file when it's automated for further polishing.

events.html

  • We should test all types of editor in terms of event emission. Automatically. See the comment in val.html.

setUp

  • See previous test file.

test event setData

  • See comment about asynchronous operations above.

test event destroy

  • Why not wait some time and check if editor is gone from CKEDITOR.instances?

Summary

If fact, it ain't bad. Really.

This a large piece of code. I'm sure that I missed lots of things in this review but we'll iterate (fix'n'review'n'fix'n'rev...) until it's perfect. I'm pretty sure that we'll see more architectural and test issues when things stabilize and get clean.

Happy coding ;)

comment:8 Changed 6 years ago by Piotrek Koszuliński

Collection handling

I'm not sure whether you're right Olek. In jQuery's world if you use a getter on a collection it is applied only to the first element. I don't like this personally, but it's how it works.

setData

This method should return promise in my opinion. This is how all async method are now implemented in jQuery, and promises are powerful. Perhaps .ckeditor() should also return promise.

Version

In case you didn't know - 1.10.* is latest stable with old IEs support. 2.0.* is AFAIR its equivalent without this support.

comment:9 Changed 6 years ago by Piotrek Koszuliński

Two more things.

event.removeListener( 'instanceReady', this.callee );

I think that this will be enough:

event.removeListener();

And second thing - we prefer to use evt over event.

I also pushed few docs style corrections.

comment:10 in reply to:  8 Changed 6 years ago by Olek Nowodziński

Replying to Reinmar:

Collection handling

I'm not sure whether you're right Olek. In jQuery's world if you use a getter on a collection it is applied only to the first element. I don't like this personally, but it's how it works.

setData

This method should return promise in my opinion. This is how all async method are now implemented in jQuery, and promises are powerful. Perhaps .ckeditor() should also return promise.

Version

In case you didn't know - 1.10.* is latest stable with old IEs support. 2.0.* is AFAIR its equivalent without this support.

Hawkeye Peter. Stronger than bears, faster than tornadoes.

Comments are correct. Still I find this getter solution ugly as hell. But if it's a common approach, we need to follow it.

comment:11 Changed 6 years ago by Olek Nowodziński

Also there's removeData method in jQuery which could be used in:

$element.data( 'ckeditorInstance', null );
$element.data( '_ckeditorInstanceLock', null );

Am I right?

comment:12 Changed 6 years ago by Piotr Jasiun

Collection handling

For sure .val function for collection should return first element. It's default behavior for textareas and it should works the same way for editor. There is even ticket for this: #7876 (and as I remember there was duplicate recently).

The another problem is behavior of ckeditor().editor for collection. My first idea was to return array (as Olek said) but now I think that we should do it in the jQuery way and return first (even if it seems to be stupid).

Version

jQuery version 2 does not support IE 6,7,8, so it's not for us. I thing we should test it on the oldest jQuery version which is in use, so it will be 1.3.2 (http://w3techs.com/technologies/details/js-jquery/1/all). I can find the way to use different version of jQuery for Fort and different for adapter, but I'm not sure if we should care so much.

Last edited 6 years ago by Piotr Jasiun (previous) (diff)

comment:13 Changed 6 years ago by Piotr Jasiun

About setData I agree that promise is the best solution. I'm working on this.

comment:14 in reply to:  12 Changed 6 years ago by Olek Nowodziński

Replying to pjasiun:

Version

jQuery version 2 does not support IE 6,7,8, so it's not for us. I thing we should test it on the oldest jQuery version which is in use, so it will be 1.3.2 (http://w3techs.com/technologies/details/js-jquery/1/all). I can find the way to use different version of jQuery for Fort and different for adapter, but I'm not sure if we should care so much.

The point is that users got to be informed which versions of jQuery are expected to be supported (info in docs).

We can arbitrarily choose the oldest and the newest one, test it once and say that all intermediate versions are just fine. Then KISS: we can keep going with our jQuery (1.7.x).

IMO, it's unlikely that some bugs emerge. And if so, we can restrict the supported scope later. That's all.

comment:15 Changed 6 years ago by Piotrek Koszuliński

This needs investigation, because none of us, as I see, is following what's going on with jQuery, but e.g. I remember that recently (in 1.9) bigger changes were made which caused compatibility issues. See here: https://github.com/jquery/jquery-migrate/#readme

Therefore we need to be very careful. We cannot release plugin which will only work in <1.9.x, but we also cannot support latest versions only, because great majority of websites still uses older. Thus, IMO we need to find a way to run tests automatically with both jQ - 1.8.x and 1.9.x.

comment:16 Changed 6 years ago by Piotr Jasiun

About setData

It is not so trival to support callback/promise for setData. valHooks doesn't allows us to pass more data to or return anything from setter (http://code.jquery.com/jquery-1.10.1.js line 4292).

Moreover, if we call val( "some data" ); on collection of editors we should call .done on promise when all off them are done. Something like this is already implemented for jQuery.when function so I can use it.

I don't see better solution than rewrite val without valHooks and implement proper promise. This needs some work (but not a lot) but I do not see better solution.

If anyone have some ideas please let me know.

comment:17 Changed 6 years ago by Piotr Jasiun

Or we can just don't care and keep setData as asynchronous function without callback. If someone need callback he can use ckeditor().editor.setData.

comment:18 in reply to:  7 Changed 6 years ago by Piotr Jasiun

Ok. I've done some changes.

setData & promise

I've removed valHooks and overwrite val function manually. Now if you use .val setter .val( 'some data' ) on editor or collection which contains editors val function will return jQuery promise instead of regular jQuery object. This promise will be resolved after all of setData callbacks.

Save event

Why don't we make it really useful? Let's think what other opportunities it may offer for end-users. Some data? Some info? Maybe it's possible to have it for more than aborting form submissions.

In my opinion it is. jQuery adapter catch save event on level 9, so if you want you can overwrite it anyway. There is no parameters, because this event only means that save button have been clicked, so there is no parameters we should pass. If you cancel that event you will cancel default behavior of the save button.

Tests

  • What we have in our fort is $().jquery == '1.7.2'. At the moment 2.0.2 is latest stable. And 1.10.1 is latest stable in 1.x. We need to decide which versions are supported by our adapter and how to test it.

I'm still working on this. The idea is to have meta tag to say to Fort "These tests should be run with numerous verions of jQuery." It seems to be more Fort modification then changes in tests files.

One things is sure: libraries got to be offline.

Done.

  • Lots of code style needs to be fixed. Brackets, spaces, new lines.

Done.

  • More "foo" and "bar". Less "lorem ipsum ..... ". Shorter strings are easier to read and debug.

Done.

val.html

  • You check only framed instances. What about inline ones and divarea?

In jQuery .val function is for form members and if you want to get value from div you should use .text or .html. So I think we should not support val function for inline and divarea.

test get value

  • Setting and checking value of #textarea must be explained in assertion. This <textarea> isn't bound to any editor and it's very hard to guess that this is a check for correct, non-greedy jQuery getter/setter behavior.

I've added comment to this test.

  • Counting getData.ckeditor isn't correct. What if getData is never fired? This should be more like...

Done.

  • function( event, editor, data ) in event callback: those parameters must be checked. Check whether:
    • editor object equals that one from CKEDITOR.instances
    • event is a proper setData event (it got a name)
    • data is what we set

Done.

test val undefined

  • It's not enough to check if this is an object. Note that typeof null is Object :D Better check if returned object is what it should be.

Done.

test form with textarea inside

  • Why not setData in the following?
    $( '#editor-inside' ).ckeditor().editor.editable().setHtml( LIPSUM_1 );
    

Because I need to set content without calling updateElement method.

  • Kiss:
    $( '#editor-save' ).ckeditor().editor.execCommand( 'save' );
    
    is
    this.execCommand( 'save' );
    

Good point. Done.

instance.html

  • You checked two instances:
assert.isUndefined( CKEDITOR.instances.editor2, 'editor2 should be undefined.' );
assert.isUndefined( CKEDITOR.instances.editor3, 'editor3 should be undefined.' );

Why not ckeditor 9, 11 or 42? Use CKEDITOR.tools.objectKeys( CKEDITOR.instances ) for faster and full check. Also use ArrayAssert.

  • See dt/core/creators/creators.html#test creator replace. Reuse some assertions.
  • Automate assertions. Your tests are very redundant here. Create input object(s) with element id's/classes as keys and desired elementModes, names, etc. as values. Call ckeditor() massively and assert massively in callbacks comparing with input object(s)'s values.

Done. Now with some helpers test looks much better.

setUp

  • Why not CKEDITOR.destroy()? It's safer, isn't it?

I've taken:

var allInstances = CKEDITOR.instances;
for( var i in allInstances ) {
	CKEDITOR.remove(  allInstances[ i ] );
}

from dt/core/ckeditor/ckeditor.html I will check the difference between remove and destroy and change it.

events.html

  • We should test all types of editor in terms of event emission.

I'm not sure about this. If you insist a will do it, but in my opinion test are for adapter and everything I want to test is if events from CKEditor are passed to jQuery. I think that adapter tests do not need to check if editor events work the same way in framed and inline editor.

test event destroy

  • Why not wait some time and check if editor is gone from CKEDITOR.instances?

The same what above. This test does not check if CKEDITOR.instances.destroyEditor.destroy(); remove editor from CKEDITOR.instances. This is test for adapters event and I want only check is destroy event is propagate properly.

TODO (for me):

  • check the difference between remove and destroy,
  • modify fort to run jquery tests with numerous versions of jQuery,
  • add information about promises to documentation.
Last edited 6 years ago by Piotr Jasiun (previous) (diff)

comment:19 Changed 6 years ago by Piotr Jasiun

Ok. A replaced remove with destroy in setUp and added information about promises into the documentation.

Now I'll work on running jquery tests with multiple versions of jQuery, but this will be fort modification so rest of code can be put in review.

comment:20 Changed 6 years ago by Piotr Jasiun

About remove and destroy: destroy method emit destroy signal what is the reason of some errors if it is emitted in setUp. So I switched back to remove.

comment:21 Changed 6 years ago by Piotr Jasiun

I've finished Fort modifications. Now if you add jQuerySupportedVersions to your config file:

jQuerySupportedVersions : {
	'1.7.1': '/cktester/lib/jquery-1.7.1.js',
	'1.7.2': '/cktester/lib/jquery-1.7.2.js',
	'1.8.3': '/cktester/lib/jquery-1.8.3.js',
	'1.9.1': '/cktester/lib/jquery-1.9.1.js',
	'1.10.1': '/cktester/lib/jquery-1.10.1.js'
}

all of tests with jquery tag:

<meta name="cktester-tags" content="editor,unit,jquery">

will be run with all of this versions. If you want to run a single test with a certain version of jquery you have to add jquery=[version] to url.

comment:22 Changed 6 years ago by Piotr Jasiun

And what do you, guys, think about promise and constructor? Now you can pass callback as a parameter to constructor:

$( 'textarea' ).ckeditor( function () {
   //some callback code in editor scope.
} );

But I realized that this callback will be called for each created editor, what is not nice if I want execute some code when all of editors will be created.

On the other hand we can return promise (like for .val) but then we will lose chaining (actually I don't believe that many people use it with editor) and current access to editor will be weird (editor could be member of returned promise, but it won't be very elegant):

$( 'textarea' ).ckeditor().editor;

comment:23 Changed 6 years ago by Piotrek Koszuliński

But I realized that this callback will be called for each created editor, what is not nice if I want execute some code when all of editors will be created.

Hm... Hard to tell. Promise returned from ckeditor() would need to group all callbacks, when callback passed to ckeditor() in my opinion should be executed for every editor. But I can't find any objective argument supporting this.

On the other hand we can return promise (like for .val) but then we will lose chaining (actually I don't believe that many people use it with editor) and current access to editor will be weird (editor could be member of returned promise, but it won't be very elegant):

So there are three options:

  1. ckeditor() returns a promise and editor is a property of this promise,
  2. ckeditor() does not return a promise and we keep the old API,
  3. new, separate method for retrieving editor instance is introduced.

To be honest - all 3 options are equally uncool, although for the consistency with val() I'd choose 1 or 3. We need more opinions and some examples from other libraries.

comment:24 Changed 6 years ago by Piotr Jasiun

Ad. 3 - it was the previous solution (in adapter for version 3 we had .ckeditorGet() method), but in my opinion this solution is very ugly.

Ad. 1 - when I checked my tests I realised that there are unfortunately more methods using chaining:

   $( 'error' ).ckeditor().get( 0 )l //to get javascript dom object
   $( '#instanceReadyEditor' ).ckeditor().on( 'instanceReady.ckeditor', function( event, editor ) { //to handle event

comment:25 Changed 6 years ago by Piotr Jasiun

And about examples: I've investigated how it works in jQuery, and i.e.: $.ajax returns promise what is not fn.jQuery object, also jQuery Deferred is not fn.jQuery child. Moreover Deferrer/promise support it's own chaining.

So if ckeditor() will returns promise then .ckeditor().get( 0 ), .ckeditor().on and .ckeditor().editor wouldn't have worked.

So in my opinion 2 is the best option. But I really don't like it.

comment:26 Changed 6 years ago by Piotrek Koszuliński

There's one thing that came to my mind... I wonder what you'll say :)

What if .ckeditor() returns a promise when it is used to instantiate editor (for the first time), but then the old, chainable object?

So:

$( '.editor' ).ckeditor(); -> promise
$( '.editor' ).ckeditor().editor; -> CKEDITOR.editor instance

comment:27 in reply to:  26 Changed 6 years ago by Olek Nowodziński

Replying to Reinmar:

There's one thing that came to my mind... I wonder what you'll say :)

What if .ckeditor() returns a promise when it is used to instantiate editor (for the first time), but then the old, chainable object?

So:

$( '.editor' ).ckeditor(); -> promise
$( '.editor' ).ckeditor().editor; -> CKEDITOR.editor instance

Feels good to me. If documented.

comment:28 Changed 6 years ago by Piotr Jasiun

Status: review_failedreview

comment:29 in reply to:  26 Changed 6 years ago by Piotr Jasiun

Replying to Reinmar:

There's one thing that came to my mind... I wonder what you'll say :)

What if .ckeditor() returns a promise when it is used to instantiate editor (for the first time), but then the old, chainable object?

So:

$( '.editor' ).ckeditor(); -> promise
$( '.editor' ).ckeditor().editor; -> CKEDITOR.editor instance

I think it would be very misleading. Especially when one use chaining. I.e.

$( 'textarea' ).ckeditor();
//... some lines of code
$( 'textarea' ).ckeditor().on( //... add event to editor

In this case if you remove first line you will get syntax error in different line.

In my opinion in jQuery: $( 'textarea' ).ckeditor().on( /* ... */ ); should means: create editor and add add events to it. Like in jQuery UI:

$( "#tags" )
      .autocomplete({
      //...
      })
      .bind( "keydown", function( event ) {
      //...
      })

In my opinion jQuery chaining is its core feature.

comment:30 Changed 6 years ago by Piotrek Koszuliński

I pushed commit to every repository (dev, tests, docs).

  1. I haven't seen any example of how to use forwarded events. Also, how are they fired for collections? This thing should be clarified.
  1. Are these a real return values for val()?

String|Number|Array|jQuery.fn|function(jQuery promise)

  1. Is Adapter tested with jQuery 2.x?

comment:31 in reply to:  30 Changed 6 years ago by Piotr Jasiun

Replying to Reinmar:

  1. Is Adapter tested with jQuery 2.x?

Nope. We don't support jQuery 2.x.

Or after short discussion, yes. We support jQuery 2.x :)

Last edited 6 years ago by Piotr Jasiun (previous) (diff)

comment:32 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_passed

I feel that jQuery adapter is just OK and ready. Mostly because:

  • It's just OK and ready :)
  • We've been working for too long on this feature (really).
  • It's time we focused on other issues and tasks.

In my opinion, this is a successful port of v3 feature, fixing all the bugs and annoyances. Except #8261 which I really don't like and I find it a way too intrusive to have it at the moment.

The overall quality of code and tests is good enough to have it in major. We're gonna fix problems as they emerge, which I believe is unlikely to happen.

I believe pjasiun deserves kudos for putting things together and bringing this much awaited feature to v4 line.

So R+, however...

We got to hold on for a while and improve the guide before "majorizing". There's a bunch of good things we got to reveal and be proud of. IMO what is missing:

  • Clear example: $( '#element' ).ckeditor( callback ) vs $( '.collection' ).ckeditor( callback(s) ).
    • What happens in callback when collection is used?
    • How many callbacks are executed? (good place to say that they correspond to instanceReady)
    • What are promises ( ckeditor().promise ) in both cases?
    • How to use promises and what is the order of resolving things (when does it happen?)?
  • What happens if val() is called with collection (both as setter and getter)?
    • Setter: How many promises?
    • Setter: Explain when promises are resolved.
    • Getter: which value is returned (why?)?
  • Some examples for forms. This is what people really use.
    • Inside of form.
    • Outside of form.

Of course: example-per-problem ;)

Additionally: jQuery 2.0.2 is to be added to libs and config.tpl.

Once we finish with docs and jQ 2.0.2, we'll merge things, starting off with squashing/fixuping commits if possible (branches seem way too long).

comment:33 Changed 6 years ago by Piotr Jasiun

Resolution: fixed
Status: review_passedclosed

dev: git:2999484d992206875277493057245927830cd4a9
test: bfed3524402bfd3c711ab185a5c0de14057977e8

Guide moved to #10578.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy