Implement the div container command.

[Martin Kou]

Need to implement the div container command and add it to the toolbar.

[Garry Yao]

This patch file requires 2871_4.patch

[Garry Yao]

The patch depends on #2871, this patch and future updates will assume the latest patches to #2871 must be applied first.

[Martin Kou]

Ok... The review took quite some time because I needed to sift through the ticket system and previous code for precedences. Feeling like being a lawyer here.

_source/plugins/newcontainer/plugin.js:

1. Line 32-48 - Plugin config entries must not use sub-namespaces. Refer to #2822.
2. Line 11-30 - Now that you aren't declaring anything in the local namespace, there's no need for the outermost closure.
3. Except for special cases like...

   function abc()
   {
     // This return does not work in Firefox - it returns undefined.
     return
     {
        attr : value
     };
   }
   

It is recommended that you follow the style conventions in other ​existing ​source ​files for literal object "blocks" - i.e. The opening bracket should occupy a new line instead of staying at the same line as the preview token.

_source/core/dom/element.js:

1. Line 378 - The regular expression is wrong. It matches non-pure-whitespace texts like "Hello world" or "There are some spaces in me".

_source/core/tools.js:

1. Line 320-323 - isEmpty() is not really needed for parsing form field values. Because when the user fills in 0, the value is the string "0" which evaluates to true. Empty form fields automatically evaluates to false because empty strings are false.
2. Line 329-331 - cloneArray() isn't really needed nor does it simplify anything. Writing someArray.concat( [] ) is much shorter than CKEDITOR.tools.cloneArray( someArray ), and is just as easy to understand.

_source/lang/en.js:

1. Lines 12-13 - Please don't remove the indenting whitespaces. If you want to fix the styles in these two lines, there's ​something else you can fix here.
2. Lines 375-387 - These entries are already defined in the common strings above, please delete them from the lang file and reuse the common strings in your dialog, so we can have less translation entries to take care of.
3. Lines 384-387 - Please don't add unnecessary sub-namespaces inside your lang namespace, it's only serving to increase code size without any benefits to readability.

_source/plugins/newcontainer/dialogs/newcontainer.js:

1. Lines 214-323 - Please follow the ​current convention for writing the arrays and object literals in dialog files. Instead of...

    children : [{ ...
   

You should write

 children : 
 [
   {
      ...
   }
 ]

Unless you've encountered some errors like the function example above.

2. Lines 183-202: Now that we have ​CKEDITOR.dom.node::getCommonAncestor(), you don't really need to define this function again in your dialog. To get the common ancestor of multiple nodes, use a loop.
3. The dialog's setup and commit phases should use the setup/commit system introduced in [2990]. This is done in order to facilitate third party developers to add and remove dialog fields and even tabs. Refer to the table or the checkbox dialog in [2990] for how it is done.

[Martin Kou]

_source/plugins/newcontainer/dialogs/newcontainer.js:

1. Lines 77-86, 104-129 - The indexOf() operation in these loops are unnecessarily increasing these loops' run time complexity to O(n²). Recommend to use the marker functions under CKEDITOR.dom.element for marking processed nodes.

[Garry Yao]

Fine grained review, the updated patch is fixing all the issues mentioned in Martin's above comments, it also BTW fix a minor bug inside CKEDITOR.dom.element.clearMarkers

[Martin Kou]

Still a few bugs:

1. I think I've mentioned this to you on Skype already. The div container command is supposedly used for laying out documents, so there's no limit on the number of div containers you can nest. Thus, the create div command should already create a new div to enclose all selected blocks, including previously created div containers.

To let users edit existing div containers, there should be a separate edit div command, which edits the div nearest to the selection range. The edit div command is expected to be accessible via context menu. We don't have a context menu yet, but that command still needs to be implemented and tested (via a toolbar button for testing purpose).

2. Line 98 should be

   ancestor = ancestor.getCommonAncestor( blockGroups[ i ][ j ], true ); 
   

   Missing the true flag means that if multiple blocks are selected and the create div command is executed, then you get a JavaScript error.
3. The problems mentioned in _source/lang.en.js are still not fixed.
4. Now that the toolbar button layout is changed, please place your create div toolbar button after the blockquote button in your new patch. Just like how it was in v2.

[Garry Yao]

The updated introduce the following changes after discuss with Martin on Skype:

• Separate edit with create mode which depend on two different commands;
• Make newly created 'div' inserted at correct position;
• Adding a config option CKEDITOR.config.div_wrapTable to distinguish whether newly created 'div' should wrap table or reside in table cell.
• Fixing language file issues.

[Garry Yao]

The patch also introduce a minor change to CKEDITOR.dom.elementpath which make the block limit elements definitoin list public visitable by putting them inside corresponding namespace.

[Martin Kou]

Taking over the ticket to add context menu and the TODO features.

[Martin Kou]

Proposing a new patch, which fixes the following issues:

1. Added edit div container and remove div container context menu options.
2. Fixed the style input in div container dialog.
3. Fixed a bug with the div container dialog which causes it to not work for the following case:

   <p>
   	paragr^aph 1</p>
   <div>
   	<p>
   		paragraph 2</p>
   	<p>
   		paragraph 3</p>
   </div>
   <p>
   	paragr^aph 4<br />
   </p>
   

4. Fixed JavaScript error in IE.

[Frederico Caldeira Knabben]

• Why has the "cke_button_creatediv" button been defined in the toolbar.css file, while others have been properly defined in icons.css?

• It misses the skin changes (icons) for the office2003 skin.

• As already defined before, there should not be "default" values in the settings.

• The "div" plugins requires "elementspath", but I can't understand the correlation between them.

• The dialog is too big, with a lot of blank space at the bottom. Has it been tested at all?!

• The getNonEmptyChildren doesn't look like a good addition to the API, and it's used by the div dialog only. It should be then moved to that dialog code as a private, instead on enlarging the CKEDITOR.dom.element class.

• Instead of having CKEDITOR.dom.elementPath.pathBlockLimitElements, we should instead have CKEDITOR.dtd.$blockLimit at this point.

[Garry Yao]

Align latest patch to trunk for easy reviewing.

[Garry Yao]

1. Debug statement need to be removed on L260 of _source\plugins\div\dialogs\div.js
2. 'compareNodes' function on L272 is idle;
3. Dialog 'minHeight' config could be reduced, notice that when switch to 'Advanced' tab, it's able to see unnecessary space after the last field.
4. Div detection logic on L100 of _source\plugins\div\plugin.js is not right, reproduce with this procedure:
   

Reproducing Procedures

1. Open the replace by class example page;
2. Make the following content with selection:

   	<table>
   		<tr><td><div>te^xt</div></td></tr>
   	</table>
   

3. Right click on the selection position to open context menu;
   • Expected Result: There's div editing items on menu;
   • Actual Result: The div editing items were not on menu;

[Martin Kou]

I've fixed point 1 to point 3.

Point 4 is actually the intended result. The div container is a layout element, div nodes that are used as paragraphs do not count.

[Garry Yao]

Replying to martinkou:

Point 4 is actually the intended result. The div container is a layout element, div nodes that are used as paragraphs do not count.

The input of this TC is actually the out come of apply 'div' with a table cell containing only test, so it's a bug.

[Martin Kou]

Then the lone div added to the table cell is wrong - the div container should never become a paragraph.

The new patch guarantees a div container would always contain paragraphs. It also fixes a bug where a collapsed <p> is seen in Firefox after performing the following procedure:

1. Open replacebyclass.html in Firefox.
2. Press Ctrl-A and Delete to delete all contents.
3. Open Create Div Container dialog.
4. Press OK.
5. A div container with a collapsed <p> is seen.

[Garry Yao]

L232 - L248 of div.js has caused another bug reproduced by :

1. Apply div on the following document with selection:

   <table>
   			<tr><td>te^xt</td></tr>
   </table>
   

• Expected Result :

  <table>
  			<tr><td><div><p>te^xt<p></div></td></tr>
  </table>
  

• Actual Result:

  <table>
  			<tr><td><div><p><br /><p></div></td></tr>
  </table>
  

[Garry Yao]

Replying to martinkou:

The new patch guarantees a div container would always contain paragraphs. It also fixes a bug where a collapsed <p> is seen in Firefox after performing the following procedure:

I've opened the problem you described at #3422, please exclude it from this ticket.

[Garry Yao]

1. The auto paragraph fixing logic make the div container creation more feasible, which should be a good move, but not sure in which case will we use L255-L261 of _source\plugins\div\dialogs\div.js codes?
2. Another problem here for IE: No way to create div within empty document in IE, we already have the same require from list plugin,so probably codes at L366-L379 of _source\plugins\list\plugin.js could be reused to archive this.

[Martin Kou]

Point 1... I've thought about that a bit in the previous patch as well. But I was worried whether it is still possible for empty div containers to exist at that point. I need to prove that it's impossible for empty div container to be there before deleting that block.

I've deleted that block of code in the new patch. My prove is as follows: For each block group...

1. If the created div container had nothing, then L213 - L219 was never executed for the current block group.
2. If L213 - L219 was never executed, then the current block group in L205 contains no block.
3. But L291 guarantees that each block group will have at least one block.

So L255 - L261 can be deleted.

Point 2... There's no need to copy the block of code from the list plugin. With the current logic, as long as there's something in the document to create div containers from, then at least one div container will be created. I've noticed the applyDiv() function does not save the current selection with bookmarks... So adding bookmarks for the current selection position will fix both problems - it keeps the previous selection intact after adding div containers, and it guarantees that at least one div will be created even in an empty document.

[Frederico Caldeira Knabben]

We should be totally focused on the stabilization process for the 3.0 release. This ticket is taking our time from it, so I'm moving to the next release.

[Garry Yao]

Beside update the changes to current trunk, the new patch also targeting the following issues:

1. Fix a bug where CKEDITOR.dom.element.setMarker failed on text node in IE ( doesn't allow expand property );
2. Simplified the logic for remove/edit div.

Ticket Tests added at :
​http://ckeditor.t/tt/2885/1.html. ​http://ckeditor.t/tt/2885/2.html.

[Frederico Caldeira Knabben]

• I'm still able to reproduce the issue mentioned in Garry's comment.

• Just for Kama, there are no icons to Edit and Remove.

[Garry Yao]

Replying to fredck:

I'm still able to reproduce the issue mentioned in Garry's comment.

The result is by purpose, which could somehow distinguish a 'div' been created from this command and the one created as a block-level style, but it's also cumbersome, I've simply removed it.
TCs updated.

[Frederico Caldeira Knabben]

Before committing:

• There is an extra cke_skin_office2003 style being defined in the Kama (_source/skins/kama/icons.css) files.

• Please move the configuration definition comments from the dialog file to the plugin.js file.

[Garry Yao]

Fixed with [4564] and [4565].

[Garry Yao]

The changes should not get into trunk, move it into 3.1.x branch with [4578].