Opened 10 years ago

Closed 10 years ago

#3715 closed Bug (fixed)

Pre output formatting is broken

Reported by: Garry Yao Owned by: Garry Yao
Priority: Must have (possibly next milestone) Milestone: CKEditor 3.0
Component: General Version:
Keywords: IBM Confirmed Review+ Cc: damian.chojna@…

Description (last modified by Garry Yao)

Note this's not a DUP of #3188, where this ticket should deal with the basic <pre> content foramtting.

Reproducing Procedures

  1. Open the replace by class example page in FF;
  2. Load the following content with 'source' mode;
    <pre>
    first line
    second line
    
    </pre>
    
  3. Switch to 'wysiwyg' mode and switch back.
    • Actual Result:
      <pre>
      	first line second line</pre>
      
    • Expected Result:
      <pre>first line
      second line
      
      </pre>
      

Attachments (3)

3715.patch (5.7 KB) - added by Garry Yao 10 years ago.
3715_2.patch (6.3 KB) - added by Garry Yao 10 years ago.
3715_3.patch (6.5 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (12)

Changed 10 years ago by Garry Yao

Attachment: 3715.patch added

comment:1 Changed 10 years ago by Garry Yao

Description: modified (diff)
Keywords: Review? added
Status: newassigned

Since browser will remove spaces at the front of block, update the expected result accordingly.

comment:2 Changed 10 years ago by Damian

Cc: damian.chojna@… added
Keywords: IBM added

comment:3 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

The patch seems to be more complicated than necessary to me.

Moving the compressSpaces logic to htmldataprocessor plugin should be a good idea. And because now we're going to have different compress logic for text nodes of different states, feeding the text node to the filter handler is also necessary.

But on the other hand, the isPre flag should be unnecessary - it has to be set two times in the code but only checked one time - so why not just check it one time by checking for element.name == 'pre'?

The other problem with the patch is lines 121 - 122 in fragment.js. So here we're checking specifically for the 'pre' element just to allow the compressSpaces logic in htmldataprocessor to do its work. This does feel right because now the core is doing part of the job of the htmldataprocessor plugin. Since the stuff in addElement() are postprocessing logic anyways, I think it would be a good idea to move move lines 121 - 134 to the htmldataprocessor plugin.

comment:4 Changed 10 years ago by Martin Kou

#3710 is having a similar problem to <pre> tags with <style> tags. I guess we can reuse this ticket's code to solve the problem for <style> tags as well.

Changed 10 years ago by Garry Yao

Attachment: 3715_2.patch added

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

Keywords: Review? added; Review- removed

Replying to martinkou:

The patch seems to be more complicated than necessary to me...

Actually I've abused the filter...The reason is I didn't take this case into consideration:

<pre><b>\ntext  text\n</b></pre> // Previous patch doesn't work

<pre> parsing is a context-sensitive task, such task could be easily done with parser, but hard to accomplish with filter or even has side effect. E.g. Transforming <br> node into '\n' requires a lot of change to the filter, but with much less changes comparing with changing parser.
As a whole, filter usages should be conservative and with cautious.

comment:6 in reply to:  4 Changed 10 years ago by Garry Yao

Replying to martinkou:

#3710 is having a similar problem to <pre> tags with <style> tags. I guess we can reuse this ticket's code to solve the problem for <style> tags as well.

Not sure if you're talking about #3737?

Changed 10 years ago by Garry Yao

Attachment: 3715_3.patch added

comment:7 Changed 10 years ago by Garry Yao

Inspired by Martin, move the indent rule to htmlwriter plugin.

comment:8 Changed 10 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:9 Changed 10 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3676].

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