Opened 7 years ago

Closed 7 years ago

#6485 closed Bug (fixed)

BIDI: Language direction should be applied to individual list items and not the entire block

Reported by: James Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.4.2
Component: Core : Lists Version: 3.4.1
Keywords: IBM Cc: Damian, joek, Satya Minnekanti

Description

Steps to reproduce the defect:

  1. Open the Ajax sample.
  2. Enter the following content into the editor:
<p>This is a sample paragraph.</p>
<ul>
	<li>List item 1</li>
	<li>List item 2</li>
	<li>List item 3</li>
	<li>List item 4</li>
</ul>
  1. Select the first paragraph and the first 2 list items.
  2. Click on the RTL icon.

Expected: RTL language direction is applied to the first paragraph & the first 2 items in the list.

Actual: RTL language direction is applied to the first paragraph & the whole list.

Attachments (6)

6484.patch (1.4 KB) - added by Garry Yao 7 years ago.
6484_2.patch (2.7 KB) - added by Garry Yao 7 years ago.
6485_3.patch (2.7 KB) - added by Garry Yao 7 years ago.
6485_4.patch (9.7 KB) - added by Garry Yao 7 years ago.
6485_5.patch (10.3 KB) - added by Garry Yao 7 years ago.
6485_6.patch (10.4 KB) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 7 years ago by Krzysztof Studnik

Status: newconfirmed

comment:2 Changed 7 years ago by Garry Yao

Resolution: duplicate
Status: confirmedclosed

DUP of #6235.

comment:3 Changed 7 years ago by Damian

This is not actually a duplicate of #6235, which only works if the selection is confined to within the list.

The test case for this ticket is selecting a paragraph outside the list and some items from within the list.

Reproduced on the nightly build, the effect of making a selection like this causes the whole list to become RTL instead of just the selected paragraph and list items.

comment:4 Changed 7 years ago by Garry Yao

Milestone: CKEditor 3.4.2
Resolution: duplicate
Status: closedreopened

@damo, you're right, let's keep this in 3.4.2 still.

Changed 7 years ago by Garry Yao

Attachment: 6484.patch added

comment:5 Changed 7 years ago by Garry Yao

Owner: set to Garry Yao
Status: reopenedreview

comment:6 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

The patch failes for nested tabels:

<ol>
	<li>
		[aa]
		<ol>
			<li>
				bb</li>
			<li>
				[cc]</li>
		</ol>
	</li>
	<li>
		dd</li>
	<li>
		ee</li>
</ol>

Changed 7 years ago by Garry Yao

Attachment: 6484_2.patch added

comment:7 Changed 7 years ago by Garry Yao

Status: review_failedreview

It's related to the wrong way of iterating ranges, actually wouldn't taken into count at this ticket, but well, considering of multiple ranges is completely broken because of it, it's in!

comment:8 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

Selecting the first and last items converts the whole list:

<ol>
	<li>
		[a]</li>
	<li>
		b</li>
	<li>
		[c]</li>
</ol>

Also, it seems that the processedElements variable is never in use, you can remove it from the code.

Changed 7 years ago by Garry Yao

Attachment: 6485_3.patch added

comment:9 Changed 7 years ago by Tobiasz Cudnik

Following TC fails using 6485_3.patch:

<ol>
	<li>
		[aa
		<ol>
			<li>
				bb]</li>
			<li>
				cc</li>
		</ol>
	</li>
	<li>
		dd</li>
	<li>
		ee</li>
</ol>

I don't think we should bother about multi range selection at this point.

comment:10 Changed 7 years ago by Tobiasz Cudnik

My previous comment was incorrect, the LI with "cc" content is RTL by the document structure, so everything is fine here.

Changed 7 years ago by Garry Yao

Attachment: 6485_4.patch added

comment:11 Changed 7 years ago by Garry Yao

Status: review_failedreview

Well, things are become a lot complicated to support Firefox's multiple ranges, I'm proposing a patch here that should handles general situations, if the review of this fails, we'll be probably accepting the fail of last TC and open ticket to fix it later.

comment:12 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

The previous patch doesn't pass an edge case which Tobias pointed out, in the following TC, the entire table should hold the style instead of on the table row.

<table>
 <tbody>
  <tr>
   <td>
    ^TD</td>
  </tr>
 </tbody>
</table>

Changed 7 years ago by Garry Yao

Attachment: 6485_5.patch added

comment:13 Changed 7 years ago by Garry Yao

Status: review_failedreview

New patch instead is always greedy on grabbing further ancestor.

comment:14 Changed 7 years ago by Tobiasz Cudnik

Status: reviewreview_passed

comment:15 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [6003].

comment:16 Changed 7 years ago by Sa'ar Zac Elias

Resolution: fixed
Status: closedreopened

Reverted [6003] with [6007] as it brings some pretty bad regressions (#6536, #6530), block handling is pretty badly damaged.

Changed 7 years ago by Garry Yao

Attachment: 6485_6.patch added

comment:17 Changed 7 years ago by Garry Yao

Status: reopenedreview

It's quite a pity that the changeset is reverted because of one bookmark node. :(

comment:18 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

comment:19 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [6031].

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