Opened 8 years ago

Closed 7 years ago

#3188 closed Bug (fixed)

V3 : Implement the <pre> support

Reported by: fredck 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 7 years ago.
3188_2.patch (9.9 KB) - added by garry.yao 7 years ago.
3188_3.patch (13.2 KB) - added by garry.yao 7 years ago.
3188_4.patch (13.9 KB) - added by garry.yao 7 years ago.
3188_5.patch (13.3 KB) - added by garry.yao 7 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by tobiasz.cudnik

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

comment:2 Changed 7 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from new to assigned

comment:3 Changed 7 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 7 years ago by garry.yao

comment:4 Changed 7 years ago by fredck

  • 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 7 years ago by garry.yao

comment:5 Changed 7 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 7 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 7 years ago by garry.yao

This will be waiting for #3715 to complete first.

Changed 7 years ago by garry.yao

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

  • 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 7 years ago by garry.yao

comment:11 Changed 7 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 7 years ago by garry.yao

comment:12 Changed 7 years ago by garry.yao

Tidied up the patch due to recent changesets.

comment:13 Changed 7 years ago by fredck

  • 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 7 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from assigned to closed

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

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