Opened 7 years ago

Closed 7 years ago

#3730 closed Bug (fixed)

Indent and BR selections

Reported by: damo Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: IBM Confirmed Review+ Cc:

Description

When selecting text that uses <BR> for line separation, the indent feature bleeds the selection to the containing <P>. So that this:

<div>
	test 1<br />
	test 2<br />
	test 3<br />
	test 4<br />
	test 5<br />
</div>

becomes this;

<div style="margin-left: 40px;">
	test 1<br />
	test 2<br />
	test 3<br />
	test 4<br />
	test 5<br />
</div>

irrespective of how many lines of text were selected. The expected behavior would be to indent only the selected text. E.g. if line 3 & 4 were selected the result could be:

<div>
	test 1<br />
	test 2<br />
	<div style="margin-left: 40px;">
		test 3<br />
		test 4<br />
	</div>
	test 5<br />
</div>

Attachments (3)

3730.patch (2.3 KB) - added by garry.yao 7 years ago.
3730_2.patch (8.2 KB) - added by garry.yao 7 years ago.
3730_3.patch (8.8 KB) - added by garry.yao 7 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by garry.yao

  • Keywords Discussion added

This's been designed by purpose, but maybe your expected result will a better result for enterMode=BR.

comment:2 Changed 7 years ago by martinkou

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

This actually does not make sense when we look at the HTML specification - paragraphs are not supposed to contain other paragraphs. Let's say we feed this XHTML code to W3C's XHTML validator.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
    <head>
        <meta http-equiv="Content-type" content="text/html;charset=UTF-8" /> 
        <title>Test XHTML validity</title>
    </head>
    <body>
        <p>
            test 1<br />
            test 2<br />
            <p style="margin-left: 40px;">
                test 3<br />
                test 4<br />
            </p>
            test 5<br />
        </p>
    </body>
</html>

W3C will tell you that this isn't valid XHTML. And in fact, even if you force that HTML into the browser via Firebug or a raw HTML file on your local harddisk - the paragraph will look wrong.

Changing the inner block to a <div> would make it look "right" but it's still invalid XHTML.

comment:3 follow-up: Changed 7 years ago by damo

  • Resolution invalid deleted
  • Status changed from closed to reopened

I agree that nested <P> tags would cause invalid XHTML. On the other hand, if my document does not contain <P> tags but <DIV> tags and I used enterMode=BR, then I would expect that indent would work on my selected lines of text. The example provided seems like a valid use case to me. Am I missing something?

Reopened to continue discussion.

comment:4 in reply to: ↑ 3 Changed 7 years ago by damo

Replying to damo:

I agree that nested <P> tags would cause invalid XHTML. On the other hand, if my document does not contain <P> tags but <DIV> tags and I used enterMode=BR, then I would expect that indent would work on my selected lines of text. The example provided seems like a valid use case to me. Am I missing something?

Reopened to continue discussion.

Just to clarify, the example I am referring to is the code snippet with <DIV> tags.

comment:5 Changed 7 years ago by fredck

  • Keywords Confirmed added; Discussion removed

The missing information in the report is that this issue happens with enterMode=br. In this case, we should really have it working as the reporter described.

comment:6 Changed 7 years ago by martinkou

  • Owner set to martinkou
  • Status changed from reopened to new

Changed 7 years ago by garry.yao

comment:7 follow-up: Changed 7 years ago by garry.yao

  • Keywords HasPatch added

After discuss with Martin, we could provide a very simply way to go:

  1. If content is directly inside body(blockLimit), indent the selected lines separated by <br> as a whole, with a new <div> block;
  2. If content is otherwise inside a block, indent the existed block.

comment:8 Changed 7 years ago by garry.yao

  • Keywords HasPatch removed
  • Owner changed from martinkou to garry.yao
  • Status changed from new to assigned

After a group discussion, we need some changes for the second case, it should work as same with the ticket described, I'm taking over the ticket.

Changed 7 years ago by garry.yao

comment:9 in reply to: ↑ 7 Changed 7 years ago by garry.yao

  • Keywords Review? added

Replying to garry.yao:

After discuss with Martin, we could provide a very simply way to go:

  1. If content is directly inside body(blockLimit), indent the selected lines separated by <br> as a whole, with a new <div> block;

Remain unchanged.

  1. If content is otherwise inside a block, indent the existed block.

Change to : If indent the selected lines group as 1. and break the block.

Changed 7 years ago by garry.yao

comment:10 Changed 7 years ago by garry.yao

Appending changelog.

comment:11 Changed 7 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:12 Changed 7 years ago by garry.yao

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

Fixed with [3783]. Click here for more info about our SVN system.

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