Ticket #5079 (closed Bug: fixed)

Opened 4 years ago

Last modified 4 years ago

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

Reported by: JoeK Owned by: fredck
Priority: Normal Milestone: CKEditor 3.4
Component: General Version: SVN (CKEditor) - OLD
Keywords: IBM Cc: dchojna@…, fredck

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

5079.patch (3.7 KB) - added by garry.yao 4 years ago.
5079_2.patch (1.2 KB) - added by garry.yao 4 years ago.
5079_3.patch (2.1 KB) - added by tobiasz.cudnik 4 years ago.
5079_4.patch (533 bytes) - added by fredck 4 years ago.

Change History

comment:1 Changed 4 years ago by fredck

  • Keywords Confirmed added
  • Milestone set to CKEditor 3.3

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

Changed 4 years ago by garry.yao

comment:2 Changed 4 years ago by garry.yao

  • Keywords Review? added
  • Status changed from new to assigned
  • Owner set to garry.yao

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

comment:3 Changed 4 years ago by alfonsoml

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

comment:4 Changed 4 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 4 years ago by alfonsoml

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

  • Keywords Discussion added
  • Cc fredck 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 4 years ago by garry.yao

  • Keywords Review- removed

comment:8 follow-up: ↓ 10 Changed 4 years ago by fredck

  • Milestone changed from CKEditor 3.3 to CKEditor 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 4 years ago by tobiasz.cudnik

  • Status changed from assigned to new
  • Owner changed from garry.yao to tobiasz.cudnik

Changed 4 years ago by tobiasz.cudnik

comment:10 in reply to: ↑ 8 Changed 4 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 follow-up: ↓ 14 Changed 4 years ago by alfonsoml

  • 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 4 years ago by tobiasz.cudnik

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

Fixed with [5674].

comment:13 Changed 4 years ago by tobiasz.cudnik

Committed into correct branch with [5697].

comment:14 in reply to: ↑ 11 Changed 4 years ago by fredck

  • Keywords Confirmed Review+ removed
  • Status changed from closed to reopened
  • Resolution fixed deleted

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 4 years ago by fredck

comment:15 Changed 4 years ago by fredck

  • Status changed from reopened to review
  • Owner changed from tobiasz.cudnik to fredck

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

comment:16 Changed 4 years ago by tobiasz.cudnik

  • Status changed from review to review_passed

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

comment:17 Changed 4 years ago by fredck

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

Fixed with [5791].

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