plug-in:basicstyle AND elementpath incorrect style range detection

[Garry Yao]

Description

Style and element path detection on certain selection are incorrect.

Procedures

1. Open an editor instance;
2. Make selection as follow:
   

   <p> This is some <strong>sample ^text</strong>. You are using^ <a href="http://www.fckeditor.net/">FCKeditor</a>.</p>
   
   

3. Check the Bold style status

• Expected: style status should be off;
• Actual: style status is on;

Test Cases

Test cases created with :

1. Test select start partial in a bold style and end in non-style.
   
   • Original document:
     

     <p> level1<strong>le^vel2</strong>lev^el1</strong></p>
     
     

   • Reference results
     • FCKEditor2.6.3
       Bold style command status in on.
     • TinyMCE3.2.1
       Bold style command status in off, element path is body -> p.
   • Expected Result
     Bold style command status in off, element path is body -> p.
2. Test select start partial in a bold style, select through non-style texts and end in bold style.
   
   • Original document:
     

     <p> level1<strong>le^vel2</strong>level1<strong>le^vel2</strong></strong></p>
     
     

   • Reference results
     • FCKEditor2.6.3
       Bold style command status in on.
     • TinyMCE3.2.1
       Bold style command status in off, element path is body -> p.
   • Expected Result
     Bold style command status in off, element path is body -> p.
3. Test select start partial in a bold style which is in a td, selection goes through some other non-style text in cells, and end partial in a bold style in a td again.
   • Original document:
     

     <p>
     <table>
     	<tbody>
     		<tr>
     			<td><strong>c^ell1</strong></td>
     			<td>cell2</td>
     		</tr>
     		<tr>
     			<td>cell3</td>
     			<td><strong>cell^4</strong></td>
     		</tr>
     	</tbody>
     </table>
     </p>
     
     

• Reference results
  • FCKEditor2.6.3
    Bold style command status in on.
  • TinyMCE3.2.1
    Bold style command status in off, element path is body -> table -> tbody.
• Expected Result
  Bold style command status in off, element path is body -> table -> tbody.

[Garry Yao]

Detailed range boundaries and elementpath dumped as below:

1. Range boundaries:

   <p>This is some <strong>sample ^text</strong>. You are using^ <a _cke_saved_href="http://www.fckeditor.net/" href="http://www.fckeditor.net/">FCKeditor</a>.</p>
   

2. Element path:

• Expected: body -> p
• Actual: body -> p -> strong

[Garry Yao]

Replying to garry.yao:

Detailed range boundaries and elementpath dumped as below:

1. Range boundaries:

   <p>This is some <strong>sample ^text</strong>. You are using^ <a _cke_saved_href="http://www.fckeditor.net/" href="http://www.fckeditor.net/">FCKeditor</a>.</p>
   

2. Element path:

• Expected: body -> p
• Actual: body -> p -> strong

[Garry Yao]

Test cases created with :

1. Test select start partial in a bold style and end in non-style.
   • Original document:

     <p> level1<strong>le^vel2</strong>lev^el1</strong></p>
     
     

• Reference results
  • FCKEditor2.6.3
    Bold style command status in on, element path is p -> strong
  • TinyMCE3.2.1
    Bold style command status in off, element path is p
• Expected Result
  Bold style command status in off, element path is p

1. Test select start partial in a bold style, select through non-style texts and end in bold style.
   • Original document:

     <p> level1<strong>le^vel2</strong>level1<strong>le^vel2</strong></strong></p>
     
     

• Reference results
  • FCKEditor2.6.3
    Bold style command status in on, element path is p -> strong
  • TinyMCE3.2.1
    Bold style command status in off, element path is p
• Expected Result
  Bold style command status in off, element path is p

[Martin Kou]

Actually this has been discuss before (on how to determine the state of styles - starting point of selection or something else), when we first implemented the style system back in v2.5. Unfortunately, neither I nor Fred could find any record of that discussion now.

The basic premise of the patch makes sense - our style system's behavior should get as close to the usual word processors' as possible (i.e. MS Word, OpenOffice.org Writer, etc.). However, the style system is known to be a very delicate piece of code - very easy to get it wrong in some obscure practical cases. So we'll need quite some test to make sure it's right.

The getDCAOfNodes() function should be moved to somewhere else or (if possible), eliminated. The function's name also needs to be more descriptive. We already have something similar in our current codebase right now - CKEDITOR.dom.range::getCommonAncestor(). It may not be the best answer for all cases. But now that we have the CKEDITOR.dom namespace, this kind of DOM operation should be put inside of that instead of putting to CKEDITOR.tools. e.g. It may be good to put it as an instance method for CKEDITOR.dom.node objects.

Review- for the getDCAOfNodes() function. The rest of the patch is good in general, but needs to be tested rigorously.

[Garry Yao]

Actually this ticket could be split into two - one for plugin:elementpath AND one for plugin:styles. Yes, Martin, we need more boundary cases and discussions for these features, especially for the second one, currently I just provide basic test cases for this special beta.

The best place for introducing the getDCAOfNodes method is some objects represent a collection of nodes, since we haven't establish this sort of class, CKEDITOR.dom.range is a better place to go as you suggested.

[Garry Yao]

Considering the getDCAOfNodes function's exclusive usage by plugin:selection, revised it into a private function temporarily.

[Martin Kou]

Tidied up the description such that the italics don't leak into the comments.

[Frederico Caldeira Knabben]

I don't see much chances to have this patch included in the code. The fact is that style check is quite an expensive task in the editor. For every keyboard move, click or selection change, all buttons in the toolbar, as well as all entries in the styles, format, font and size combos get checked to update the current state. So, doing that by checking everything between the start and the end position of the selection would be overkill.

My editor reference has never been TinyMCE, but Word. It's not a case that it works just like Word, because TinyMCE uses the browser features to check whether the text is bold (queryCommandState), which is not useful in our case as we have a custom (and much more powerful) style system.

Anyway, let's postpone any decision about it, so maybe some ideas could come in the future.

[Garry Yao]

An concrete example of this was reported at #3231, so inherit the keyword.

[Garry Yao]

Replying to fredck:

I don't see much chances to have this patch included in the code. The fact is that style check is quite an expensive task in the editor...

Yes, I've saw a significant PF degrading when holding the arrow key to navigate through content at this time when I finished the patch, however, we could have a simplified approach for checking styles based on user behavior statistic: Just perform the check on both the range start node and end node, then we'll have a bigger chance to have the correct style detected if we can't resolve the PF issue of checking whole range.

[Frederico Caldeira Knabben]

Milestone CKEditor 3.x deleted