Opened 11 years ago

Closed 11 years ago

#2768 closed Bug (fixed)

plug-in:basicstyle all basic styles unable to apply reversely

Reported by: Garry Yao Owned by: Martin Kou
Priority: Normal Milestone: CKEditor 3.0
Component: Core : Styles Version: SVN (FCKeditor) - Retired
Keywords: Confirmed Review+ Cc:

Description (last modified by Frederico Caldeira Knabben)

  1. Open an editor instance and select some unformatted text;
  2. Apply any basic style;
  3. Cancel any basic style;
  • Expected: Style applied has been canceled
  • Actual: Style applied still there

Attachments (5)

2768.patch (22.9 KB) - added by Garry Yao 11 years ago.
2768_2.patch (22.8 KB) - added by Garry Yao 11 years ago.
2768_3.patch (50.0 KB) - added by Garry Yao 11 years ago.
2768_4.patch (6.5 KB) - added by Martin Kou 11 years ago.
2768_5.patch (4.7 KB) - added by Martin Kou 11 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 11 years ago by Artur Formella

Keywords: Confirmed added
Priority: NormalHigh

comment:2 Changed 11 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

comment:3 Changed 11 years ago by Frederico Caldeira Knabben

Description: modified (diff)
Priority: HighNormal

I think the problem here is related to the selection position after applying the style. If the start boundary of the selection is placed right before the applied tag, the editor will understand that the style is to be applied again, instead of being removed.

I'm just guessing though.

comment:4 Changed 11 years ago by Garry Yao

From my observing, this is not a bug, but a missing feature of remove style from selecton , which required to be ported from v2. Toward the command status after the style is applied , IMO the following selections, e.g. after apply a bold style, are the same for either plugin:elementpath or plugin:styles, since they both relay on CKEditor.dom.selection.getStartElement:

  • Anchor beforeStart/afterEnd the <strong> tag ( which v3 is using now)
    <p>This is some ^<strong>sample text</strong>^. You are using <a
    	href="http://www.fckeditor.net/">FCKeditor</a>.</p>
    
  • Anchor afterEnd/beforeStart the <strong> tag, by fix the boundaries( which v2 is using)
    <p>This is some <strong>^sample text^</strong>. You are using <a
    	href="http://www.fckeditor.net/">FCKeditor</a>.</p>
    

comment:5 Changed 11 years ago by Garry Yao

Related v2 tickets: #1318, #1270,#1318
Related v3 tickets: #2767

comment:6 Changed 11 years ago by Garry Yao

Test cases created with following ones, not sure on some of them:

  1. Test reverse apply bold style across two partial selected nodes.
  • Original document:
    <p><strong> level1 <strong>le^vel2</strong><strong>le^vel2</strong> </strong></p>
    
  • Reference results
    • FCKEditor2.6.3
      <p>level1 level2le<strong><strong>vel2</strong> </strong></p>
      
    • TinyMCE3.2.1
      <p><strong> level1 <strong>le</strong></strong>vel2le<strong><strong>vel2</strong> </strong></p>
      
  • Expected Result
    <p><strong>level1 <strong>le</strong></strong>vel2le<strong><strong>vel2</strong> </strong></p>
    
  1. Test reverse apply bold style contain one partial selected node.
  • Original document:
    <p>level0^<strong>level1<strong>le^vel2</strong></strong></p>
    
  • Reference results
    • FCKEditor2.6.3
      <p>level level2le<strong><strong>vel2</strong></strong></p>
      
    • TinyMCE3.2.1
      <p>level0level1lev<strong><strong>el2</strong></strong></p>
      
  • Expected Result
    <p>level0level1le<strong>vel2</strong></p>
    
  1. Test reverse apply bold style in other nested style elements
  • Original document:
    <p><strong>level1<em>le^ve^l2</em></strong></p>
    
  • Reference results
    • FCKEditor2.6.3
      <p><strong>level1<em>le</em></strong><em>ve</em><strong><em>l2</em></strong></p>
      
    • TinyMCE3.2.1
      <p><strong>level1<em>le</em></strong><em>ve</em><strong><em>l2</em></strong></p>
      
  • Expected Result
    <p><strong>level1<em>le</em></strong><em>ve</em><strong><em>l2</em></strong></p>
    
  1. Test reverse apply bold style when selection is collapsed in other nested style elements
  • Original document:
    <p><strong>level1<em>le^vel2</em></strong></p>
    
  • Reference results
    • FCKEditor2.6.3
      <p><strong>level1<em>le</em></strong><em>ve</em><strong><em>l2</em></strong></p>
      
    • TinyMCE3.2.1
      <p><strong>level1<em>level2</em></strong></p>
      
  • Expected Result
    <p><strong>level1<em>le</em></strong>^<strong><em>vel2</em></strong></p>
    

Changed 11 years ago by Garry Yao

Attachment: 2768.patch added

Changed 11 years ago by Garry Yao

Attachment: 2768_2.patch added

comment:7 Changed 11 years ago by Garry Yao

Keywords: Review? added

comment:8 Changed 11 years ago by Martin Kou

Keywords: Review- added; Review? removed

There's still one case where the patch does not work:

  1. Test removal of a style across two inline tags.
  • Original document
    <p>
    	This is some <strong>sample <em>text</em></strong><em>. You</em> are using <a href="http://www.fckeditor.net/">FCKeditor</a>.</p>
    
  • Press the I button to remove the italics.
  • Expected result
    <p>
    	This is some <strong>sample text</strong>. You are using <a href="http://www.fckeditor.net/">FCKeditor</a>.</p>
    
  • Actual result
    <p>
    	This is some <strong>sample </strong><strong>text</strong>. You are using <a href="http://www.fckeditor.net/">FCKeditor</a>.</p>
    

The patch also has some coding style problems. I've talked to Garry on Skype about them.

comment:9 in reply to:  6 Changed 11 years ago by Garry Yao

Two test cases added according to Martin's reviewing:

  1. Test merge siblings after reverse apply styles. ( in case of removed tags )
    • Original document:
      <p><b><em>level2</em></b><b><em>level2</em></b></p>
      
    • Reference results
      • FCKEditor2.6.3
        <p><b>level2</b><b>level2</b></p>
        
      • TinyMCE3.2.1
        <p><strong>level2</strong><strong>level2</strong></p>
        
    • Expected Result
      <p><b>level2level2</b></p>
      
  1. Test merge siblings after reverse apply styles. ( in case of normalized text nodes)
    • Original document:
      <p><strong>level1<em>level2</em></strong><em>level1</em></p>
      
    • Reference results
      • FCKEditor2.6.3
        <p><strong>level1</strong><strong>level2</strong>level1</p>
        
      • TinyMCE3.2.1
        <p><strong> level1<strong>le</strong></strong>vel2le<strong><strong>vel2</strong> </strong></p>
        
    • Expected Result
      <p><strong>level1level2</strong>level1</p>
      

Changed 11 years ago by Garry Yao

Attachment: 2768_3.patch added

comment:10 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:11 Changed 11 years ago by Martin Kou

Owner: changed from Garry Yao to Martin Kou
Status: assignednew

Garry's patch is already too old to be applied to the trunk - there're tons of conflicts with the trunk code that patching it is very difficult. Moreover, many of the proposed changes the Garry's patch is already in the trunk (e.g. CKEDITOR.tools.bind()) in some other form.

I'm proposing a much simpler patch that captures the logic in Garry's patch.

Changed 11 years ago by Martin Kou

Attachment: 2768_4.patch added

comment:12 Changed 11 years ago by Garry Yao

Keywords: Review- added; Review? removed

Please check test case 1, 4 in comment:6 which failed,other two passed.

comment:13 Changed 11 years ago by Martin Kou

Keywords: Review? added; Review- removed

Ok. The last patch is already committed due to #2905 having a higher priority. But the bugs mentioned by Garry still needs to be fixed.

Test case 1 is fixed in the patch. For test case 4, however, I think the expected result should look something like this:

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

Why? It's because for the user, it's meaningless to have the <strong> tag broken along a collapsed selection. In fact, from a word processor's standpoint, it's meaningless and wasteful for the user to have two adjacent <strong> tags - they look like they're just one in the WYSIWYG area, and they should be one in the source. So, the correct action for removing a bold style at a collapsed selection, is to remove the whole bold text effect *visually*, instead of breaking the <strong> or <b> tag into two siblings.

comment:14 Changed 11 years ago by Martin Kou

So, one additional test case, and one that's changed...

  1. Remove bold style at collapsed selection.
    • Original document
      <p><strong>level1<em>le^vel2</em></strong></p>
      
    • Action: Remove bold
    • Expected result:
      <p>
      	level1<em>le^vel2</em></p>
      
  1. Remove bold style at collapsed selection with siblings.
    • Original document
      <p><strong> level1 <strong>level2</strong><strong>le^vel2</strong> </strong></p>
      
    • Action: Remove bold
    • Expected result
      <p>
      	level1 level2le^vel2 </p>
      

Changed 11 years ago by Martin Kou

Attachment: 2768_5.patch added

comment:15 in reply to:  13 Changed 11 years ago by Garry Yao

Keywords: Review+ added; Review? removed

Replying to martinkou:

Why? It's because for the user, it's meaningless to have the <strong> tag broken along a collapsed selection.

The orginial case was designed to be able to insert a unbold text among bold chunk, after apply unbold in a collapsed selection, user could begin to type normal text, check MS-Word for a reference, we could discuss this feature later though.

All successful result received from above test cases.

comment:16 Changed 11 years ago by Martin Kou

Fixed with [3095].

comment:17 Changed 11 years ago by Martin Kou

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