Opened 8 years ago

Closed 8 years ago

#3730 closed Bug (fixed)

Indent and BR selections

Reported by: Damian 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 8 years ago.
3730_2.patch (8.2 KB) - added by Garry Yao 8 years ago.
3730_3.patch (8.8 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 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 8 years ago by Martin Kou

Resolution: invalid
Status: newclosed

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 Changed 8 years ago by Damian

Resolution: invalid
Status: closedreopened

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 8 years ago by Damian

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 8 years ago by Frederico Caldeira Knabben

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 8 years ago by Martin Kou

Owner: set to Martin Kou
Status: reopenednew

Changed 8 years ago by Garry Yao

Attachment: 3730.patch added

comment:7 Changed 8 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 8 years ago by Garry Yao

Keywords: HasPatch removed
Owner: changed from Martin Kou to Garry Yao
Status: newassigned

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

Attachment: 3730_2.patch added

comment:9 in reply to:  7 Changed 8 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 8 years ago by Garry Yao

Attachment: 3730_3.patch added

comment:10 Changed 8 years ago by Garry Yao

Appending changelog.

comment:11 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:12 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

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

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