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

[Satya Minnekanti]

To reproduce the defect

1. Open Language sample and select Language as Hebrew or Arabic.

2. Type first line of text and press Enter.

3. Click LTR icon and enter text in the second line.

4. 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.

[Sa'ar Zac Elias]

The direction CSS property must also be taken into consideration.

[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?

[Tobiasz Cudnik]

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.

[Garry Yao]

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;

[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 ?

[Tobiasz Cudnik]

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.

[Garry Yao]

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' ) );

[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.

[Tobiasz Cudnik]

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

[Garry Yao]

The 'getDirection' should be something like below?

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

[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.

[Tobiasz Cudnik]

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.

[Garry Yao]

@james, your TC should addressed by #6235.

[Tobiasz Cudnik]

Fixed with [5963].