Opened 10 years ago

Last modified 9 years ago

#2767 review_failed Bug

plug-in:basicstyle AND elementpath incorrect style range detection

Reported by: Garry Yao Owned by: Garry Yao
Priority: Normal Milestone:
Component: Core : Styles Version: 3.0 Beta 2
Keywords: Confirmed Cc:

Description (last modified by Martin Kou)

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.

Attachments (2)

2767.patch (7.4 KB) - added by Garry Yao 10 years ago.
2767_2.patch (7.3 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by Artur Formella

Keywords: Confirmed added

comment:2 Changed 10 years ago by 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

comment:3 in reply to:  2 Changed 10 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

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

comment:4 Changed 10 years ago by 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

comment:5 Changed 10 years ago by Garry Yao

Description: modified (diff)
Summary: plug-in:basicstyle incorrect style range detectionplug-in:basicstyle AND elementpath incorrect style range detection

Changed 10 years ago by Garry Yao

Attachment: 2767.patch added

comment:6 Changed 10 years ago by Garry Yao

Keywords: Review? added

comment:7 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

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.

comment:8 Changed 10 years ago by 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.

Changed 10 years ago by Garry Yao

Attachment: 2767_2.patch added

comment:9 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

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

comment:10 Changed 10 years ago by Martin Kou

Description: modified (diff)

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

comment:11 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
Milestone: CKEditor 3.0CKEditor 3.x

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.

comment:12 Changed 10 years ago by Garry Yao

Keywords: Oracle added

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

comment:13 in reply to:  11 Changed 10 years ago by 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.

comment:14 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Oracle removed

comment:15 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Version: SVN (FCKeditor)CKEditor 3.0 Beta 2

comment:16 Changed 9 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.x

Milestone CKEditor 3.x deleted

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