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

[Garry Yao]

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

[Frederico Caldeira Knabben]

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.

[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>
  

[Garry Yao]

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

[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>
  

2. 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>
  

3. 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>
  

4. 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>
  

[Martin Kou]

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

4. 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.

[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>
     

2. 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>
     

[Martin Kou]

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.

[Garry Yao]

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

[Martin Kou]

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.

[Martin Kou]

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

4. 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>
     

5. 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>
     

[Garry Yao]

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.

[Martin Kou]

Fixed with [3095].