Opened 11 years ago

Closed 11 years ago

#10280 closed New Feature (fixed)

Replace textarea with inline editor

Reported by: Frederico Caldeira Knabben Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.2
Component: General Version:
Keywords: Oracle Cc: Senthil, ramesh.ponnusamy@…

Description

The idea is introducing a new instance creation method which is similar to CKEDITOR.replace, but instead of having a themed-ui editor instance created, CKEditor should replace the <textarea> with a <div>, created on the fly, enabling inline editing on it.

The created <div> must have standard class names available, so implementors can customize it's appearance.

Then, when the form is posted, the original textarea is filled with the editor contents, just like it happens with CKEDITOR.replace.

CKEDITOR.destroy should revert it back to the textarea, just like CKEDITOR.replace as well.

Change History (14)

comment:1 Changed 11 years ago by Frederico Caldeira Knabben

Cc: Senthil added
Keywords: Oracle added
Milestone: CKEditor 4.2
Status: newconfirmed

We can tentatively include this in CKEditor 4.2.

comment:2 Changed 11 years ago by Ramesh Ponnusamy

Cc: ramesh.ponnusamy@… added

comment:3 Changed 11 years ago by Piotrek Koszuliński

The created <div> must have standard class names available, so implementors can customize it's appearance.

I think that these classes should be configurable, just like they should be configurable for a divarea, which we cannot style right now.

comment:4 Changed 11 years ago by Maciej Guzek

Owner: set to Maciej Guzek
Status: confirmedassigned

comment:5 Changed 11 years ago by Maciej Guzek

Status: assignedreview

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

Status: reviewreview_failed
  • Many tests are not passing.
  • Tests for this feature should be placed in dt/ (they are design tests).
  • Code style... huge number of issues in every file.
  • There are no tests for:
    • editor initialization with string (id),
    • what happens after destroy() (whether everything is cleaned up, whether element is updated, etc),
    • what happens on form submit,
    • for editor.attachToForm,
    • whether textarea is hidden,
    • "fix the readonly setting" for this comment from dev code,
  • From tests: this.editor1.instanceReady. What is it?
  • We don't need all these plugins in those tests.
  • There's no docs for attachToForm and it is not clear for which editor this method should be executed, for which can be. Perhaps this should be private.
  • Why do we need changes in core/editable.js?
  • Missing sample.

comment:7 Changed 11 years ago by Maciej Guzek

Status: review_failedreview

sample added to samples/inlinebycode.html. Additional tests added. Code style corrected. Ready for next review.

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

Status: reviewreview_failed

I pushed several commits both in dev and test branch. I fixed lots of issues, enhanced tests and refactored the code. For more details, please refer to commit descriptions. The most notable are:

  • Test in core/creators/creators.html
  • Tests in core/creators/inline-textarea.html
  • Test in core/editor/readonly.html. While extending the editor.readOnly discovery I noticed that element.getAttribute is used to determine disabled attribute value. It's incorrect since disabled doesn't require any value to make an element read-only like:
    // It's disabled but element.getAttribute( 'disabled' ) is empty string.
    <textarea disabled>Foo</textarea>
    
    I switched the condition to hasAttibute( 'disabled' ) and created tests for all available creators to make sure this is correct.

There are still several things that make this branch R-:

  • Two tests in inline-textarea.html fail. It's possible to create a new instance with CKEDITOR.inline() on the container used by another inline-textarea editor.
  • Missing tests for form submit.
  • Missing tests for editor.attachToForm.
    • Which editors can use it?
    • Why?
    • Does de-attachment work on editor destroy?
    • Most likely in core/editor/form.html
  • Missing docs for the new feature.
    • For CKEDITOR.inline
    • Some others?
    • We should remember to update guides as well.
  • Sample needs tuning.
    • It doesn't clearly say that it's possible to create editor from <textarea>.
    • There's no sample code in the description.
    • Two cases (contenteditable=true and textarea) must be clearly separated from each other. Example is samples/magicline.html where two editors have different descriptions (.description) and share the introduction. Each editor should have own sample code.

Other remarks (already fixed):

  • editor.setData is async (see docs)
  • We don't use editor.instanceReady to determine the status. Event callback is the right place.
  • document.getElementById is CKEDITOR.document.getById
  • We optimize the code
    assert.isTrue( document.getElementById( 'editor1' ).offsetWidth == 0 );
    assert.isTrue( document.getElementById( 'editor1' ).offsetHeight == 0 ); // another getElementById call
    
    editor.on( 'destroy', function() { // editor is in the outer scope
    	var editor = this, // Why?
            ...
    	editor.element.clearCustomData();
    	...
    	element.clearCustomData(); // Why again?
    	...
    });
    

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

Comment regarding this line: https://github.com/cksource/ckeditor-dev/commit/2675b477953058660386128669e8a0804fd6452c#L0R42

IMO it could be better to first create that div and then set its innerHTML. This way we would be secure in case of corrupted data. It won't be possible that data splits contenteditable div into more elements (e.g. when this is loaded '</div>').

On the other hand - only the moment between inline() call and the final #loaded (when processed&fixed data is set back to editable div) would be influenced by not fixing this.

comment:10 Changed 11 years ago by Piotr Jasiun

Owner: changed from Maciej Guzek to Piotr Jasiun
Status: review_failedassigned

comment:11 in reply to:  8 Changed 11 years ago by Piotr Jasiun

Replying to a.nowodzinski:

There are still several things that make this branch R-:

  • Two tests in inline-textarea.html fail. It's possible to create a new instance with CKEDITOR.inline() on the container used by another inline-textarea editor.

After talk with Fred and PK I agree that there is no reason to check this. Container is internal div created by JS method. We don't check if it is possible to create Editor on any other div created using replace/inline.

  • Missing tests for form submit.

I'm working on this.

  • Missing tests for editor.attachToForm.
    • Which editors can use it?
    • Why?
    • Does de-attachment work on editor destroy?
    • Most likely in core/editor/form.html

editor.attachToForm is now private. I also added docs to this method. tests for form submit will check if attachToForm works so we don't need separate tests for that.

  • Missing docs for the new feature.
    • For CKEDITOR.inline
    • Some others?
    • We should remember to update guides as well.
  • Sample needs tuning.
    • It doesn't clearly say that it's possible to create editor from <textarea>.
    • There's no sample code in the description.
    • Two cases (contenteditable=true and textarea) must be clearly separated from each other. Example is samples/magicline.html where two editors have different descriptions (.description) and share the introduction. Each editor should have own sample code.

I will do this.

I also added class cke_textarea_inline to div created inside inline.

comment:12 Changed 11 years ago by Piotr Jasiun

Status: assignedreview

Test for submit are done. I've also made small refactoring and added documetation to inline method.

I will work now on sample with is not ready yet, but I put it in review to make thinks faster.

comment:13 Changed 11 years ago by Piotrek Koszuliński

Status: reviewreview_passed

After some additional cleanup and fixes - R+.

comment:14 Changed 11 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Merged to major with git:d20c978 on dev and c00a6d5 on tests.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy