Ticket #6253 (closed Bug: fixed)

Opened 4 years ago

Last modified 4 years ago

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

Reported by: satya Owned by: tobiasz.cudnik
Priority: Normal Milestone: CKEditor 3.4.2
Component: Core : Lists Version: 3.4
Keywords: IBM Cc: damo, 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

6253.patch (2.1 KB) - added by tobiasz.cudnik 4 years ago.
6253_2.patch (752 bytes) - added by garry.yao 4 years ago.
6253_3.patch (3.5 KB) - added by tobiasz.cudnik 4 years ago.
6253_4.patch (3.7 KB) - added by tobiasz.cudnik 4 years ago.
6253_5.patch (3.4 KB) - added by tobiasz.cudnik 4 years ago.
6253_6.patch (3.4 KB) - added by tobiasz.cudnik 4 years ago.

Change History

comment:1 Changed 4 years ago by Saare

  • Status changed from new to confirmed
  • Component changed from General to Core : Lists
  • Milestone set to CKEditor 3.4.1

comment:2 Changed 4 years ago by tobiasz.cudnik

  • Status changed from confirmed to assigned
  • Owner set to tobiasz.cudnik

Changed 4 years ago by tobiasz.cudnik

comment:3 Changed 4 years ago by tobiasz.cudnik

  • Status changed from assigned to review

comment:4 Changed 4 years ago by Saare

  • Status changed from review to review_failed

The direction CSS property must also be taken into consideration.

Changed 4 years ago by garry.yao

comment:5 Changed 4 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 4 years ago by tobiasz.cudnik

comment:6 Changed 4 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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

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

  • Cc jamcunni@… added

Changed 4 years ago by tobiasz.cudnik

comment:10 Changed 4 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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

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

comment:13 Changed 4 years ago by tobiasz.cudnik

  • Status changed from review_failed to review
  • Computed state is used according to the config setting.
  • Determining direction moved to the previous, existing loop.

comment:14 Changed 4 years ago by garry.yao

  • Status changed from review to review_failed

The 'getDirection' should be something like below?

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

Changed 4 years ago by tobiasz.cudnik

comment:15 follow-up: ↓ 16 Changed 4 years ago by 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.

comment:16 in reply to: ↑ 15 Changed 4 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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

@james, your TC should addressed by #6235.

comment:18 Changed 4 years ago by garry.yao

  • Status changed from review to review_passed

comment:19 Changed 4 years ago by tobiasz.cudnik

  • Status changed from review_passed to closed
  • Resolution set to fixed

Fixed with [5963].

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