BIDI: BIDI buttons should not toggle the base language direction

[Damian]

The BIDI buttons should not toggle the direction of a selected block or blocks but should enforce (or reconfirm) the user's desire to set the selected direction on content.

This ability should work in conjunction with useComputedState="true".

Example: if I have this paragraph selected:

  <P dir="RTL">Test</P>

hitting the RTL button should not remove the dir attribute.

[Tobiasz Cudnik]

What should be the way to remove the direction from an element ? Should LTR button remove RTL direction and only on a second invocation set the direction to LTR ?

[Damian]

The recommendation is to use explicit directions. So, you effectively cannot remove the language direction once it has been set, only change from RTL to LTR and back.

The reasoning behind this is to effectively preserve WYSIWYG behaviour. Actually, each block should have an explicit direction set when doing editor.getData(), but this will be covered in another ticket.

[Frederico Caldeira Knabben]

Come on guys! We're not here to compete with js libraries.

The removeDirection is not used at all, and hasDirection seems not needed, having everything handled by getDirection.

KISS please!

[Frederico Caldeira Knabben]

This one should fix #6440 as well.

[Tobiasz Cudnik]

Second patch honors config.useComputedStyle property and changes direction only when it differs from the present one. It changes it always when useComputedStyle is disabled.

[Garry Yao]

Note that now the processing order is on the opposite of browser's priority which prefers inline style:

element.hasAttribute( 'dir' ) || element.getStyle( 'direction' )

which cause the following TC fails:

1. Load editor with config.contentsLangDirection = 'rtl';
2. With the following content and selection:

   <p style="direction:ltr">
   	[first paragraph
   </p>
   <p>
   	second paragraph
   </p>
   <p style="direction:ltr">
   	last paragraph]
   </p>
   

3. Click on 'RTL' button
   • Actual: 1st and 3rd remain unchanged.

[Garry Yao]

It changes it always when useComputedStyle is disabled.

It's not featured in the patch, check the following TC:

1. Load editor with config.contentsLangDirection = 'rtl';
2. With the following content and selection:

   <p>
    by computed style
   </p>
   

3. Click on 'RTL' button;
   • Expected: 'dir="rtl"' is applied on paragraph;
   • Actual: No explicit attribute/style applied.

[Tobiasz Cudnik]

This patch should target those 2 TCs. Additionally, style is always used to set the direction.

[Garry Yao]

Some small notes:

1. How about

   var useComputedState = ( 'useComputedState' in editor.config ) ? editor.config.useComputedState : 1;
   

2. The two calls at L60 and L67 of 'getComputedStyle' could be reused;
3. When do we decide to switch to inline style?

   element.removeAttribute( 'dir' );
   element.setStyle( 'direction', dir );
   

[Tobiasz Cudnik]

• Optimized fetching the getComputedState config
• Reduced the getComputedState usages.
• Direction is again applied using an attribute.
• dirAfter var is not used anymore.

[Garry Yao]

Sorry, after found the below case, I think the nested block problem we've discussed today should still be addressed, it's kind of irritated on this case for the user.

// Apply "LTR" on entire list.
[<ul>
	<li dir="rtl">item1</li>
	<li>item2</li>
</ul>]

[Garry Yao]

@Tobias, considering the emergency of this ticket, I've made necessary updates to 'switchDir' logic based on your latest patch that allowing this happen, feel free to give your fresh review.

The time complexity should remains the same with the original codes, which is optimistically still linear with a flat DOM tree and log(k)*n in worst case.

[Tobiasz Cudnik]

Your changes are looking good, reusing existing walker seems like a good idea, i would only be afraid of ranges iterator's performance, which is now used in all cases.

Anyway, i don't think now is the time to do refactorings like in 6376_5.patch. I also don't think we should create new switchDir method for each editor's instance. Instead, we should introduce a class with decent prototype inheritance. But first all related ticket should be closed.

[Tobiasz Cudnik]

I've updated the patch to the newest revision with one change, targeting TC from the 17th comment, which was failing inside an LTR document (it worked well for RTL documents).

[Tobiasz Cudnik]

This patch introduces consistency in result of applied direction, which is now the same for element without direction and with an opposite direction when computed state is ON.

[Tobiasz Cudnik]

Fixed with [5970].