Opened 6 years ago

Closed 6 years ago

#6485 closed Bug (fixed)

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

Reported by: james c Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.4.2
Component: Core : Lists Version: 3.4.1
Keywords: IBM Cc: damo, joek, satya

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 6 years ago.
6484_2.patch (2.7 KB) - added by garry.yao 6 years ago.
6485_3.patch (2.7 KB) - added by garry.yao 6 years ago.
6485_4.patch (9.7 KB) - added by garry.yao 6 years ago.
6485_5.patch (10.3 KB) - added by garry.yao 6 years ago.
6485_6.patch (10.4 KB) - added by garry.yao 6 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 6 years ago by krst

  • Status changed from new to confirmed

comment:2 Changed 6 years ago by garry.yao

  • Resolution set to duplicate
  • Status changed from confirmed to closed

DUP of #6235.

comment:3 Changed 6 years ago by damo

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

  • Milestone set to CKEditor 3.4.2
  • Resolution duplicate deleted
  • Status changed from closed to reopened

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

Changed 6 years ago by garry.yao

comment:5 Changed 6 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from reopened to review

comment:6 Changed 6 years ago by Saare

  • Status changed from review to review_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 6 years ago by garry.yao

comment:7 Changed 6 years ago by garry.yao

  • Status changed from review_failed to review

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 6 years ago by Saare

  • Status changed from review to review_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 6 years ago by garry.yao

comment:9 Changed 6 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 6 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 6 years ago by garry.yao

comment:11 Changed 6 years ago by garry.yao

  • Status changed from review_failed to review

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

  • Status changed from review to review_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 6 years ago by garry.yao

comment:13 Changed 6 years ago by garry.yao

  • Status changed from review_failed to review

New patch instead is always greedy on grabbing further ancestor.

comment:14 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review to review_passed

comment:15 Changed 6 years ago by garry.yao

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

Fixed with [6003].

comment:16 Changed 6 years ago by Saare

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Changed 6 years ago by garry.yao

comment:17 Changed 6 years ago by garry.yao

  • Status changed from reopened to review

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

comment:18 Changed 6 years ago by Saare

  • Status changed from review to review_passed

comment:19 Changed 6 years ago by garry.yao

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

Fixed with [6031].

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