Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#4729 closed New Feature (fixed)

Support fakeobjects for comments

Reported by: fredck Owned by: fredck
Priority: Normal Milestone: CKEditor 3.1
Component: General Version:
Keywords: Confirmed Review+ Cc: pawel@…

Description

Currently, it's not possible to replace HTML comments with fake elements. We should provide support for it.

Attachments (5)

4729.patch (11.6 KB) - added by fredck 7 years ago.
drupalbreaks.zip (2.7 KB) - added by fredck 7 years ago.
Drupal plugin using this feature. Updated to support tables, lists, blockquotes, etc.
4729_2.patch (12.2 KB) - added by fredck 7 years ago.
4729_3.patch (11.4 KB) - added by fredck 7 years ago.
4729_4.patch (11.3 KB) - added by fredck 7 years ago.

Download all attachments as: .zip

Change History (18)

Changed 7 years ago by fredck

comment:1 Changed 7 years ago by fredck

  • Keywords Review? added
  • Status changed from new to assigned

comment:2 Changed 7 years ago by fredck

The motivation to code this feature right now is the support for break and pagebreak comments in Drupal. I've attached a proposal plugin for a Drupal plugin for it, which uses the features introduced in the proposed patch.

To activate it for testing, after applying the patch, simply unzip the file into the plugins folder and use the following config.js contents:

CKEDITOR.editorConfig = function( config )
{
	config.extraPlugins = 'drupalbreaks';
	
	this.on( 'pluginsLoaded', function()
	{
		CKEDITOR.config.toolbar_Full.push( [ 'DrupalBreak', 'DrupalPageBreak' ] );
	});
};

The basic idea is that we must always work to make plugins development a simply and exciting task. For that, our API must be flexible and all complexity must be handled by the core code.

Changed 7 years ago by fredck

Drupal plugin using this feature. Updated to support tables, lists, blockquotes, etc.

comment:3 Changed 7 years ago by pwilk

  • Cc pawel@… added
  • Keywords Confirmed Review? removed

comment:4 Changed 7 years ago by pwilk

  • Keywords Confirmed Review? added

comment:5 Changed 7 years ago by garry.yao

  • Keywords Review- added; Review? removed

Nice patch, kudo!
And here come some small problems that I found:

L18-L42 of fakeobject/plugin.js

There should be some guard since we should also considering non-element fake now. ( Identified by resize the fake comment and switch to source you'll get a JavaScript error);

L114 of the same plugin

if ( fakeElement.getAttribute( '_cke_real_node_type' ) == CKEDITOR.NODE_COMMENT )
	return null;

How about

if ( fakeElement.getAttribute( '_cke_real_node_type' ) != CKEDITOR.NODE_ELEMENT )
	return null;

Changed 7 years ago by fredck

comment:6 Changed 7 years ago by fredck

  • Keywords Review? added; Review- removed

comment:7 Changed 7 years ago by garry.yao

Note that the priority changes to the default rules may have impact on existing plugins which may assume the default rules are applied in prior, if it's not a necessary to have it, we can hold this change.

comment:8 Changed 7 years ago by garry.yao

  • Keywords Review- added; Review? removed

Changed 7 years ago by fredck

comment:9 Changed 7 years ago by fredck

  • Keywords Review? added; Review- removed

Changed 7 years ago by fredck

comment:10 Changed 7 years ago by fredck

Added a new patch based on a chat with Garry, where he stated that it's not a good idea to have the getText method into CKEDITOR.dom.comment, as this method is been (improperly) used in the code to detect node types (the type property should be used instead).

comment:11 Changed 7 years ago by garry.yao

  • Keywords Review+ added; Review? removed

comment:12 Changed 7 years ago by fredck

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed with [4556] (3.1.x branch).

comment:13 Changed 7 years ago by wwalc

Added missing entry in ckeditor.pack with [4558].

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