Opened 8 years ago
Closed 8 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)
Change History (19)
comment:1 Changed 8 years ago by tobiasz.cudnik
comment:2 Changed 8 years ago by garry.yao
- Owner set to garry.yao
- Status changed from new to assigned
comment:3 Changed 8 years ago by garry.yao
- Keywords Review? added
Proposing the patch with the following changeset:
- 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;
- Adding a default rule for writer which preserve the indentation of contents within '<pre>';
- Enhance the html parser not stripping empty spaces in '<pre>'.
- 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 8 years ago by garry.yao
comment:4 Changed 8 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 8 years ago by garry.yao
comment:5 Changed 8 years ago by garry.yao
- Keywords Review? added; Review- removed
The new patch comes with the following changes:
- Now 'pre' processing lives in styles plugin;
- Disable the spaces replacing logics at L22:text.js coz it doesn't make sense.
comment:6 Changed 8 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 8 years ago by garry.yao
This will be waiting for #3715 to complete first.
Changed 8 years ago by garry.yao
comment:8 Changed 8 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:
- Adding a function - 'CKEDITOR.tools.multiple' to replicate string;
- Adding a param to 'CKEDITOR.test.getInnerHtml' to preserve link breaks for a <pre> element, specially.
comment:9 Changed 8 years ago by garry.yao
Updates to codes back in v2:
- '_CheckAndSplitPre'(v2) has been changed to RegExp based 'splitIntoPres'(v3);
- '_FromPre'(v2) has been changed to 'fromPres'(v3) which to be applied on every splitted sub pre, result in more accurate block header spaces;
- All the replacing functions now ignore bookmark nodes at start/end of block;
comment:10 Changed 8 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:
- Load the replacebyclass sample.
- Put the focus in the fist paragraph.
- 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 8 years ago by garry.yao
comment:11 Changed 8 years ago by garry.yao
- Keywords Review? added; Review- removed
Updates within the new patch:
- Fix an obvious logic error when apply block style other than <pre>;
- Fix a typo which cause a regression of [3854];
Changed 8 years ago by garry.yao
comment:12 Changed 8 years ago by garry.yao
Tidied up the patch due to recent changesets.
comment:13 Changed 8 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 8 years ago by garry.yao
- Resolution set to fixed
- Status changed from assigned to closed

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