Opened 16 years ago
Closed 16 years ago
#3189 closed Bug (fixed)
V3 : Padding block support
| Reported by: | Frederico Caldeira Knabben | Owned by: | Garry Yao |
|---|---|---|---|
| Priority: | Normal | Milestone: | CKEditor 3.0 |
| Component: | General | Version: | |
| Keywords: | Confirmed 3.0RC Review+ | Cc: |
Description
In V2, we implemented some checks to ensure that there is always a way to escape from special blocks, like <table> and <pre>.
For that we guarantee that there is always an element where to move at the very end of the document. We need it in V3.
Attachments (3)
Change History (14)
comment:1 Changed 16 years ago by
comment:2 Changed 16 years ago by
| Priority: | High → Normal |
|---|
comment:3 Changed 16 years ago by
| Keywords: | 3.0RC added |
|---|
comment:4 Changed 16 years ago by
We need to have #3190 committed first, because this ticket will work on the same code that is being introduced by that ticket.
comment:5 Changed 16 years ago by
| Owner: | changed from Frederico Caldeira Knabben to Garry Yao |
|---|---|
| Status: | new → assigned |
Changed 16 years ago by
| Attachment: | 3189.patch added |
|---|
comment:6 Changed 16 years ago by
| Keywords: | Review? added |
|---|
The patch is containing the latest related codes from #3190 for easy reviewing.
comment:7 Changed 16 years ago by
| Keywords: | Review- added; Review? removed |
|---|
The proposed fix function is to be executed for every selection change. So, it should be fast. We should avoid creating and selecting bookmarks, as well as manipulating ranges that much if not needed.
Also, in a call, me an Garry have agreed that a simpler solution could be available for the padding br, to have it for the end of body only.
Changed 16 years ago by
| Attachment: | 3189_2.patch added |
|---|
comment:8 Changed 16 years ago by
| Keywords: | Review? added; Review- removed |
|---|
A more simplified patch attached.
comment:9 Changed 16 years ago by
| Keywords: | Review- added; Review? removed |
|---|
- In line 144, it would be better to use getElementsByTag. It would make the loop faster, and the code simpler.
- In the for loop at line 145, there could be some coding style fixes.
- In line 153, instead of forcing the selection to be at the beginning of the paragraph, it would be safer to have a bookmark to be selected. In this way we avoid troubles in a theoretical case where the caret goes in the middle of text.
- For getPrevious and getNext, it would be better to have a while loop to check the nodes, instead of having recursive calls. Other than that, it looks like the current implementation would not work if you have more than one empty/spaces only nodes together.
- There is no really need to always extend the CKEDITOR.dtd for element names tables. You can safely use a local variable for that, as this is quite a specific case.
- I've noted that when inserting a table at the end of a text paragraph, the padding node is not getting added. That's because the table is actually splitting the paragraph, leaving an empty paragraph after it, which is wrong.
Changed 16 years ago by
| Attachment: | 3189_3.patch added |
|---|
comment:10 Changed 16 years ago by
| Keywords: | Review+ added; Review- removed |
|---|
After some research, it looks like its enough to have a <br> as the last element in <body> for Firefox. In IE instead, we just need an empty element, even a fake one like <cke:temp>. All that would be then fixed by #3190.
This approach would be different than what we have in V2, where we force it to have a <p> at the end. In this way, it's easier for us to identify whether or not and empty <p> is intentional, and also avoid having it displaying it with "show blocks".