Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#7907 closed Bug (fixed)

BIDI: Pressing decrease indent on a RTL list item causes a change in language direction

Reported by: James Cunningham Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.3
Component: Core : Lists Version: 3.4
Keywords: IBM Cc: Damian, Teresa Monahan, Satya Minnekanti

Description

Steps to reproduce the defect:

  1. Open the Ajax sample.
  1. Click on the RTL button in the toolbar.
  1. Create a bulleted/numbered list.
  1. Apply a paragraph format (e.g. Heading 1) to one of the list items.
  1. Place the cursor in the list item that the paragraph format was applied to & click on the decrease indent button in the toolbar.

Expected: The number/bullet is removed from the list item and it remains RTL.

Actual: The number/bullet is removed from the list item but the langauge direction changes to LTR.

Attachments (4)

7907.patch (3.3 KB) - added by Garry Yao 12 years ago.
7907_2.patch (6.0 KB) - added by Garry Yao 12 years ago.
7907_3.patch (5.8 KB) - added by Garry Yao 12 years ago.
7907_4.patch (4.2 KB) - added by Garry Yao 12 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 13 years ago by Jakub Ś

Status: newconfirmed
Version: 3.6.1 (SVN - trunk)3.4

Confirmed from CKEditor 3.4

comment:2 Changed 13 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedassigned

Changed 12 years ago by Garry Yao

Attachment: 7907.patch added

comment:3 Changed 12 years ago by Garry Yao

Status: assignedreview

comment:4 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

The following tests are failing with this patch:
http://ckeditor.t/tt/6461/1.html
http://ckeditor.t/tt/7645/3.html

comment:5 Changed 12 years ago by Frederico Caldeira Knabben

Btw, it seems to be a wrong commit on t/7907.

comment:6 Changed 12 years ago by Garry Yao

Status: review_failedreview

I would like to propose a re-fix to #6461, otherwise it would be hard to live with the fix here, in the sense that, when the paragraph to convert is already styled, then the block should be preserved in list item, this makes makes no visual difference from previous, while it eliminate any further troubles to inherit styles from the list item later, thus when a list item is removed, its owning styles just gone, but not the content styles.

Changed 12 years ago by Garry Yao

Attachment: 7907_2.patch added

comment:7 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

This one is now falling:
http://ckeditor.t/tt/4450/1.html

Would you mind doing a full test run before sending the next patch?

comment:8 Changed 12 years ago by Garry Yao

Status: review_failedreview

Ok, a full run shows that #4450 tc is the only that need to be updated to reflect the new behavior.

comment:9 in reply to:  8 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Replying to garry.yao:

Ok, a full run shows that #4450 tc is the only that need to be updated to reflect the new behavior.

I don't see this change on t/7907. Has it been committed somewhere else?

comment:10 Changed 12 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.3

comment:11 Changed 12 years ago by Garry Yao

Status: review_failedreview

comment:12 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Unfortunately enforcing <p> blocks on alignment operations is something to avoid. It brings important behavioral changes to the editor in both layout (e.g. margins for <p>) and semantics aspects.

Because of the above, the changes proposed for the #4450 tt are to be reverted.

Changed 12 years ago by Garry Yao

Attachment: 7907_3.patch added

comment:13 Changed 12 years ago by Garry Yao

Status: review_failedreview

New patch now copy/merge list item styles to content blocks when removed.

New ticket tcs are added to cover these cases.

comment:14 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

The following new test is not passing:
http://ckeditor.t/tt/7657/1.html

Changed 12 years ago by Garry Yao

Attachment: 7907_4.patch added

comment:15 Changed 12 years ago by Garry Yao

Status: review_failedreview

comment:16 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:17 Changed 12 years ago by Garry Yao

Component: Core : BiDiCore : Lists
Resolution: fixed
Status: review_passedclosed

Fixed with [7419].

comment:18 Changed 12 years ago by Garry Yao

Ticket test fails IE8, post fixing with [7425].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy