Opened 7 years ago

Closed 7 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 7 years ago.
6253_2.patch (752 bytes) - added by Garry Yao 7 years ago.
6253_3.patch (3.5 KB) - added by Tobiasz Cudnik 7 years ago.
6253_4.patch (3.7 KB) - added by Tobiasz Cudnik 7 years ago.
6253_5.patch (3.4 KB) - added by Tobiasz Cudnik 7 years ago.
6253_6.patch (3.4 KB) - added by Tobiasz Cudnik 7 years ago.

Download all attachments as: .zip

Change History (25)

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

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

comment:2 Changed 7 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: confirmedassigned

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6253.patch added

comment:3 Changed 7 years ago by Tobiasz Cudnik

Status: assignedreview

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

Status: reviewreview_failed

The direction CSS property must also be taken into consideration.

Changed 7 years ago by Garry Yao

Attachment: 6253_2.patch added

comment:5 Changed 7 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 7 years ago by Tobiasz Cudnik

Attachment: 6253_3.patch added

comment:6 Changed 7 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 7 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 7 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 7 years ago by James

Cc: jamcunni@… added

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6253_4.patch added

comment:10 Changed 7 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 7 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 7 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 7 years ago by Tobiasz Cudnik

Attachment: 6253_5.patch added

comment:13 Changed 7 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 7 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 7 years ago by Tobiasz Cudnik

Attachment: 6253_6.patch added

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

@james, your TC should addressed by #6235.

comment:18 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

comment:19 Changed 7 years ago by Tobiasz Cudnik

Resolution: fixed
Status: review_passedclosed

Fixed with [5963].

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