Opened 10 years ago

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

Download all attachments as: .zip

Change History (15)

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

Owner: set to Martin Kou
Status: reopenednew

Changed 10 years ago by Garry Yao

Attachment: 3730.patch added

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

Attachment: 3730_2.patch added

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

Attachment: 3730_3.patch added

comment:10 Changed 10 years ago by Garry Yao

Appending changelog.

comment:11 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:12 Changed 10 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 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy