Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#7907 closed Bug (fixed)

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

Reported by: jamescun Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.3
Component: Core : Lists Version: 3.4
Keywords: IBM Cc: damo, tmonahan, satya

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 4 years ago.
7907_2.patch (6.0 KB) - added by garry.yao 4 years ago.
7907_3.patch (5.8 KB) - added by garry.yao 4 years ago.
7907_4.patch (4.2 KB) - added by garry.yao 4 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by j.swiderski

  • Status changed from new to confirmed
  • Version changed from 3.6.1 (SVN - trunk) to 3.4

Confirmed from CKEditor 3.4

comment:2 Changed 5 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from confirmed to assigned

Changed 4 years ago by garry.yao

comment:3 Changed 4 years ago by garry.yao

  • Status changed from assigned to review

comment:4 Changed 4 years ago by fredck

  • Status changed from review to review_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 4 years ago by fredck

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

comment:6 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

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

comment:7 Changed 4 years ago by fredck

  • Status changed from review to review_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 follow-up: Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

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 4 years ago by fredck

  • Status changed from review to review_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 4 years ago by fredck

  • Milestone set to CKEditor 3.6.3

comment:11 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

comment:12 Changed 4 years ago by fredck

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

comment:13 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

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 4 years ago by fredck

  • Status changed from review to review_failed

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

Changed 4 years ago by garry.yao

comment:15 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

comment:16 Changed 4 years ago by fredck

  • Status changed from review to review_passed

comment:17 Changed 4 years ago by garry.yao

  • Component changed from Core : BiDi to Core : Lists
  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [7419].

comment:18 Changed 4 years ago by garry.yao

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

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