Opened 11 years ago

Closed 10 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)

3189.patch (4.8 KB) - added by Garry Yao 11 years ago.
3189_2.patch (7.0 KB) - added by Garry Yao 10 years ago.
3189_3.patch (9.2 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 11 years ago by Frederico Caldeira Knabben

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".

comment:2 Changed 11 years ago by Frederico Caldeira Knabben

Priority: HighNormal

comment:3 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: 3.0RC added

comment:4 Changed 11 years ago by Frederico Caldeira Knabben

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 11 years ago by Garry Yao

Owner: changed from Frederico Caldeira Knabben to Garry Yao
Status: newassigned

Changed 11 years ago by Garry Yao

Attachment: 3189.patch added

comment:6 Changed 11 years ago by Garry Yao

Keywords: Review? added

The patch is containing the latest related codes from #3190 for easy reviewing.

comment:7 Changed 10 years ago by Frederico Caldeira Knabben

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 10 years ago by Garry Yao

Attachment: 3189_2.patch added

comment:8 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

A more simplified patch attached.

comment:9 Changed 10 years ago by Frederico Caldeira Knabben

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 10 years ago by Garry Yao

Attachment: 3189_3.patch added

comment:10 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review- removed

comment:11 Changed 10 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3549].

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