Opened 15 years ago

Closed 15 years ago

#3188 closed Bug (fixed)

V3 : Implement the <pre> support

Reported by: Frederico Caldeira Knabben Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed Review+ Cc:

Description

In V2, we have special support for <pre> blocks, making it possible to switch them forth and back to <p>, for example. We need it in V3.

Attachments (5)

3188.patch (16.5 KB) - added by Garry Yao 15 years ago.
3188_2.patch (9.9 KB) - added by Garry Yao 15 years ago.
3188_3.patch (13.2 KB) - added by Garry Yao 15 years ago.
3188_4.patch (13.9 KB) - added by Garry Yao 15 years ago.
3188_5.patch (13.3 KB) - added by Garry Yao 15 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 15 years ago by Tobiasz Cudnik

Could you please describe, using some example, how this switching should work ?

comment:2 Changed 15 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

comment:3 Changed 15 years ago by Garry Yao

Keywords: Review? added

Proposing the patch with the following changeset:

  1. Dedicate the '<pre>' handling logic inside format plugin by introducing sort of 'processor' function to CKEDITOR.style for customizable style processing, which is different from v2, where specific logics were expanding the core style system codes, we could benefit from it when '<pre>' is not appeared within the format options at all;
  2. Adding a default rule for writer which preserve the indentation of contents within '<pre>';
  3. Enhance the html parser not stripping empty spaces in '<pre>'.
  4. Update the onElement filter, enable it to transform 'element' into 'text node' within the filter.

Note that part of the changes is already proposed at #2886 but with different implementations.

Changed 15 years ago by Garry Yao

Attachment: 3188.patch added

comment:4 Changed 15 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

I understand your intentions on having less code into the styles plugin, but the problem is that we need all the <pre> processing there, because we could easily have <pre> used into the styles combo.

Changed 15 years ago by Garry Yao

Attachment: 3188_2.patch added

comment:5 Changed 15 years ago by Garry Yao

Keywords: Review? added; Review- removed

The new patch comes with the following changes:

  1. Now 'pre' processing lives in styles plugin;
  2. Disable the spaces replacing logics at L22:text.js coz it doesn't make sense.

comment:6 Changed 15 years ago by Garry Yao

Keywords: Review- added; Review? removed

The empty spaces shrinking logic should be kept, but should be moved to somewhere to sensitive to <pre>.

comment:7 Changed 15 years ago by Garry Yao

This will be waiting for #3715 to complete first.

Changed 15 years ago by Garry Yao

Attachment: 3188_3.patch added

comment:8 Changed 15 years ago by Garry Yao

Keywords: Review? added; Review- removed

Besides things directly related to this feature, I'm introducing two other changes which it depends:

  1. Adding a function - 'CKEDITOR.tools.multiple' to replicate string;
  2. Adding a param to 'CKEDITOR.test.getInnerHtml' to preserve link breaks for a <pre> element, specially.

comment:9 Changed 15 years ago by Garry Yao

Updates to codes back in v2:

  1. '_CheckAndSplitPre'(v2) has been changed to RegExp based 'splitIntoPres'(v3);
  2. '_FromPre'(v2) has been changed to 'fromPres'(v3) which to be applied on every splitted sub pre, result in more accurate block header spaces;
  3. All the replacing functions now ignore bookmark nodes at start/end of block;

comment:10 Changed 15 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

At a first glance the changes looked good, but then a simple test with other format options showed that things got broken.

With the patch applied:

  1. Load the replacebyclass sample.
  2. Put the focus in the fist paragraph.
  3. Select the "Heading 1" format option.

The paragraph will disappear.

The changelog entry may be added at this point. This will be the first (and maybe only) new feature to be there.

Changed 15 years ago by Garry Yao

Attachment: 3188_4.patch added

comment:11 Changed 15 years ago by Garry Yao

Keywords: Review? added; Review- removed

Updates within the new patch:

  1. Fix an obvious logic error when apply block style other than <pre>;
  2. Fix a typo which cause a regression of [3854];

Changed 15 years ago by Garry Yao

Attachment: 3188_5.patch added

comment:12 Changed 15 years ago by Garry Yao

Tidied up the patch due to recent changesets.

comment:13 Changed 15 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Ok... I'm not going hard on the code review, but the functionality is ok.

Regarding the "multiple" function, please rename it to "repeat" before committing, as this is the common name it has out there.

comment:14 Changed 15 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3889]. Click here for more info about our SVN system.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy