Opened 8 years ago

Closed 7 years ago

#5079 closed Bug (fixed)

Page break in lists move to above the list when you switch from WYSIWYG to HTML mode and back.

Reported by: Joe Kavanagh Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.4
Component: General Version: SVN (CKEditor) - OLD
Keywords: IBM Cc: dchojna@…, Frederico Caldeira Knabben

Description

Enter a list with a few items. Add a page break in the middle of the list. Switch to HTML mode and back to WYSIWYG mode. Observe that the page break has moved to before the list

Attachments (4)

5079.patch (3.7 KB) - added by Garry Yao 8 years ago.
5079_2.patch (1.2 KB) - added by Garry Yao 8 years ago.
5079_3.patch (2.1 KB) - added by Tobiasz Cudnik 7 years ago.
5079_4.patch (533 bytes) - added by Frederico Caldeira Knabben 7 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Confirmed added
Milestone: CKEditor 3.3

In fact the page break element is inserted in between the <li> elements, not inside the current one.

Changed 8 years ago by Garry Yao

Attachment: 5079.patch added

comment:2 Changed 8 years ago by Garry Yao

Keywords: Review? added
Owner: set to Garry Yao
Status: newassigned

Proposing of insert page-break into <body>, which matches better with it's semantics.

comment:3 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

I've never used the page break command but I agree that it seems more logical to break everything until the body. But what will happen when putting the line break in the middle of an ordered list? it could restart the counter in the next page...

Anyway, the change in pathBlockLimitElements needs to include also "ol" at least, and maybe "li". Testing with an OL gets the editor in an infinite loop, so this can be risky, what other tags need to be considered? dl, dt, dd...?

Changed 8 years ago by Garry Yao

Attachment: 5079_2.patch added

comment:4 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

After some spec reading, it's more appropriate to void breaking the path block.

comment:5 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

Inserting a page break in the default sample text generates this code

<p>
	This is some <strong>sample text</strong>.
	<div style="page-break-after: always;">
		<span style="display: none;">&nbsp;</span></div>
	You are using <a href="http://ckeditor.com/">CKEditor</a>.</p>

after switching to design and source again that's changed to

<p>
	This is some <strong>sample text</strong>.</p>
<div style="page-break-after: always;">
	<span style="display: none;">&nbsp;</span></div>
<p>
	You are using <a href="http://ckeditor.com/">CKEditor</a>.</p>

comment:6 Changed 8 years ago by Garry Yao

Cc: Frederico Caldeira Knabben added
Keywords: Discussion added

Ok...I'm proposing here to use the following mark when output page break, with such we could have both the line breaking visual effect and valid HTML.

<span style="page-break-after: always;display:block">&nbsp;</span>

comment:7 Changed 7 years ago by Garry Yao

Keywords: Review- removed

comment:8 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.3CKEditor 3.4

I think a nice solution would be instead changing the current thing we have there. Instead of having a block element, which renders as a line in the editor, we could have it as an inline icon, just like we have for anchors. This will give the flexibility of placing the break at any place in the contents, avoiding it breaking the contents structure in any way.

We need more time to discuss and implement this feature, so I'm postponing it a bit.

comment:9 Changed 7 years ago by Tobiasz Cudnik

Owner: changed from Garry Yao to Tobiasz Cudnik
Status: assignednew

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5079_3.patch added

comment:10 in reply to:  8 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added

Replying to fredck:

I think a nice solution would be instead changing the current thing we have there. Instead of having a block element, which renders as a line in the editor, we could have it as an inline icon, just like we have for anchors. This will give the flexibility of placing the break at any place in the contents, avoiding it breaking the contents structure in any way.

Newest patch implements this ideas. For visual comfort, page break is much wider then anchor icon.

comment:11 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review+ added; Discussion Review? removed

It seems to address the current issues. I guess that some people will claim that using an inline element this way is wrong or that the way that it's shown isn't as clear as a whole line block element, but we can't make everyone happy.

comment:12 Changed 7 years ago by Tobiasz Cudnik

Resolution: fixed
Status: newclosed

Fixed with [5674].

comment:13 Changed 7 years ago by Tobiasz Cudnik

Committed into correct branch with [5697].

comment:14 in reply to:  11 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Confirmed Review+ removed
Resolution: fixed
Status: closedreopened

Replying to alfonsoml:

It seems to address the current issues. I guess that some people will claim that using an inline element this way is wrong or that the way that it's shown isn't as clear as a whole line block element, but we can't make everyone happy.

Well, as long as they don't blame on us because it's not working on Ie, I'm ok with it :)

That's not the case though: #6077.

I've so reverted [5697] with [5787].

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 5079_4.patch added

comment:15 Changed 7 years ago by Frederico Caldeira Knabben

Owner: changed from Tobiasz Cudnik to Frederico Caldeira Knabben
Status: reopenedreview

I'm proposing a less drastic solution, targeted to fix the reported issue.

comment:16 Changed 7 years ago by Tobiasz Cudnik

Status: reviewreview_passed

Very simple fix, but works very well for all cases i've tested (multiple page breaks, nested lists).

comment:17 Changed 7 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [5791].

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