Opened 6 years ago

Closed 5 years ago

Last modified 5 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 5 years ago.
7907_2.patch (6.0 KB) - added by Garry Yao 5 years ago.
7907_3.patch (5.8 KB) - added by Garry Yao 5 years ago.
7907_4.patch (4.2 KB) - added by Garry Yao 5 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 6 years ago by Jakub Ś

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

Confirmed from CKEditor 3.4

comment:2 Changed 6 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedassigned

Changed 5 years ago by Garry Yao

Attachment: 7907.patch added

comment:3 Changed 5 years ago by Garry Yao

Status: assignedreview

comment:4 Changed 5 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 5 years ago by Frederico Caldeira Knabben

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

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

Attachment: 7907_2.patch added

comment:7 Changed 5 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 5 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 5 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 5 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.3

comment:11 Changed 5 years ago by Garry Yao

Status: review_failedreview

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

Attachment: 7907_3.patch added

comment:13 Changed 5 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 5 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

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

Changed 5 years ago by Garry Yao

Attachment: 7907_4.patch added

comment:15 Changed 5 years ago by Garry Yao

Status: review_failedreview

comment:16 Changed 5 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:17 Changed 5 years ago by Garry Yao

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

Fixed with [7419].

comment:18 Changed 5 years ago by Garry Yao

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

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