Opened 10 years ago

Closed 10 years ago

#6253 closed Bug (fixed)

BIDI: creating a Numbered/Bulleted list causing improper behavior on bidi

Reported by: Satya Minnekanti Owned by: Tobiasz Cudnik
Priority: Normal Milestone: CKEditor 3.4.2
Component: Core : Lists Version: 3.4
Keywords: IBM Cc: Damian, joek, jamcunni@…

Description

To reproduce the defect

  1. Open Language sample and select Language as Hebrew or Arabic.
  1. Type first line of text and press Enter.
  1. Click LTR icon and enter text in the second line.
  1. Select 1st & 2nd line of data and click on Numbers/Bullets icon.

Error: These 2 lines data all become "LTR" text.

Expected result:

The 1st line data is keeping "RTL" with bullet on the right, & 2nd line data is keeping "LTR" with bullet on the left.

Attachments (6)

6253.patch (2.1 KB) - added by Tobiasz Cudnik 10 years ago.
6253_2.patch (752 bytes) - added by Garry Yao 10 years ago.
6253_3.patch (3.5 KB) - added by Tobiasz Cudnik 10 years ago.
6253_4.patch (3.7 KB) - added by Tobiasz Cudnik 10 years ago.
6253_5.patch (3.4 KB) - added by Tobiasz Cudnik 10 years ago.
6253_6.patch (3.4 KB) - added by Tobiasz Cudnik 10 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 10 years ago by Sa'ar Zac Elias

Component: GeneralCore : Lists
Milestone: CKEditor 3.4.1
Status: newconfirmed

comment:2 Changed 10 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: confirmedassigned

Changed 10 years ago by Tobiasz Cudnik

Attachment: 6253.patch added

comment:3 Changed 10 years ago by Tobiasz Cudnik

Status: assignedreview

comment:4 Changed 10 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

The direction CSS property must also be taken into consideration.

Changed 10 years ago by Garry Yao

Attachment: 6253_2.patch added

comment:5 Changed 10 years ago by Garry Yao

Maybe I'm missing one case that's targeted before but why don't we leave the list creation in ignorance of the BIDI feature?

Changed 10 years ago by Tobiasz Cudnik

Attachment: 6253_3.patch added

comment:6 Changed 10 years ago by Tobiasz Cudnik

Status: review_failedreview

I've added direction support into core/dom/element class, which handles both CSS and attribute directions.

@Garry: we need lists to smartly inherit direction from elements they are created.

comment:7 Changed 10 years ago by Garry Yao

Status: reviewreview_failed

Basically, I don't think it's necessary to pull up direction style from list items up onto the root node, even supposing that, the following logic is incorrect in a case where there's a second parent block with a different dir that wrap the 'contentBlock'.

var itemDir = contentBlock.getDirection() || editor.config.contentsLangDirection;

comment:8 Changed 10 years ago by Tobiasz Cudnik

  • Direction is read from contentBlock, not it's parent (there's no traversal to the root)
  • I don't see any problems with a 'second parent' with a different direction, can you provide a TC for this ?

comment:9 Changed 10 years ago by James

Cc: jamcunni@… added

Changed 10 years ago by Tobiasz Cudnik

Attachment: 6253_4.patch added

comment:10 Changed 10 years ago by Tobiasz Cudnik

Status: review_failedreview

This one should fix the doubts about the element being nested inside an explicit direction. Also it uses more compact dom.element extensions to save space.

comment:11 Changed 10 years ago by Garry Yao

Status: reviewreview_failed

Still the 'getDirection' method is problematic, by mixing computed style and inline style/attribute doesn't really reflects the browser's mind (in case we have !important rule). In this case it's enough to follow the same logic of BIDI plugin that depends on 'config.useComputedStyle'.

return this.getAttribute( 'dir' ) || this.getStyle( 'direction' ) || ( includeComputedDirection && this.getComputedStyle( 'direction' ) );

comment:12 Changed 10 years ago by Garry Yao

Another thing for performance, we don't need a separate routine to check explicitDirection, it could be instead checked along line L281 when content blocks are collected.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 6253_5.patch added

comment:13 Changed 10 years ago by Tobiasz Cudnik

Status: review_failedreview
  • Computed state is used according to the config setting.
  • Determining direction moved to the previous, existing loop.

comment:14 Changed 10 years ago by Garry Yao

Status: reviewreview_failed

The 'getDirection' should be something like below?

getDirection : function( useComputed )
{
	return useComputed? this.getComputedStyle( 'direction' ) : this.getStyle( 'direction' ) || this.getAttribute( 'dir' );
}

Changed 10 years ago by Tobiasz Cudnik

Attachment: 6253_6.patch added

comment:15 Changed 10 years ago by James

Does this ticket cover the following test case or should I create a new ticket?

Steps:

  1. Open the Ajax sample.
  2. Create a Bulleted list containing 4 items.
  3. Select any 2 of the list items & click on the RTL icon.

Expected: Only the selected list items should have RTL language direction.

Actual: The whole list has RTL language direction.

comment:16 in reply to:  15 Changed 10 years ago by Tobiasz Cudnik

Status: review_failedreview

Replying to james c:

Does this ticket cover the following test case or should I create a new ticket?

Steps:

  1. Open the Ajax sample.
  2. Create a Bulleted list containing 4 items.
  3. Select any 2 of the list items & click on the RTL icon.

Expected: Only the selected list items should have RTL language direction.

Actual: The whole list has RTL language direction.

It's related to #6235, where we have a patch for it, but in my opinion it should have a separated ticket.

comment:17 Changed 10 years ago by Garry Yao

@james, your TC should addressed by #6235.

comment:18 Changed 10 years ago by Garry Yao

Status: reviewreview_passed

comment:19 Changed 10 years ago by Tobiasz Cudnik

Resolution: fixed
Status: review_passedclosed

Fixed with [5963].

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