Opened 12 years ago
Closed 12 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:
- Open the Ajax sample.
- 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>
- Select the first paragraph and the first 2 list items.
- 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)
Change History (25)
comment:1 Changed 12 years ago by
Status: | new → confirmed |
---|
comment:2 Changed 12 years ago by
Resolution: | → duplicate |
---|---|
Status: | confirmed → closed |
comment:3 Changed 12 years ago by
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 12 years ago by
Milestone: | → CKEditor 3.4.2 |
---|---|
Resolution: | duplicate |
Status: | closed → reopened |
@damo, you're right, let's keep this in 3.4.2 still.
Changed 12 years ago by
Attachment: | 6484.patch added |
---|
comment:5 Changed 12 years ago by
Owner: | set to Garry Yao |
---|---|
Status: | reopened → review |
comment:6 Changed 12 years ago by
Status: | review → 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 12 years ago by
Attachment: | 6484_2.patch added |
---|
comment:7 Changed 12 years ago by
Status: | review_failed → 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 12 years ago by
Status: | review → 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 12 years ago by
Attachment: | 6485_3.patch added |
---|
comment:9 Changed 12 years ago by
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 12 years ago by
My previous comment was incorrect, the LI with "cc" content is RTL by the document structure, so everything is fine here.
Changed 12 years ago by
Attachment: | 6485_4.patch added |
---|
comment:11 Changed 12 years ago by
Status: | review_failed → 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 12 years ago by
Status: | review → 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 12 years ago by
Attachment: | 6485_5.patch added |
---|
comment:13 Changed 12 years ago by
Status: | review_failed → review |
---|
New patch instead is always greedy on grabbing further ancestor.
comment:14 Changed 12 years ago by
Status: | review → review_passed |
---|
comment:15 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed with [6003].
comment:16 Changed 12 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Changed 12 years ago by
Attachment: | 6485_6.patch added |
---|
comment:17 Changed 12 years ago by
Status: | reopened → review |
---|
It's quite a pity that the changeset is reverted because of one bookmark node. :(
comment:18 Changed 12 years ago by
Status: | review → review_passed |
---|
comment:19 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed with [6031].
DUP of #6235.