Opened 6 years ago

Closed 6 years ago

#6376 closed Bug (fixed)

BIDI: BIDI buttons should not toggle the base language direction

Reported by: damo Owned by: tobiasz.cudnik
Priority: Normal Milestone: CKEditor 3.4.2
Component: General Version: 3.4
Keywords: IBM Cc: satya_minnekanti@…, jamcunni@…

Description

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.

Attachments (8)

6376.patch (3.1 KB) - added by tobiasz.cudnik 6 years ago.
6376_2.patch (2.0 KB) - added by tobiasz.cudnik 6 years ago.
6376_3.patch (2.2 KB) - added by tobiasz.cudnik 6 years ago.
6376_4.patch (2.4 KB) - added by tobiasz.cudnik 6 years ago.
6376_5.patch (9.8 KB) - added by garry.yao 6 years ago.
6376_6.patch (6.0 KB) - added by tobiasz.cudnik 6 years ago.
6376_7.patch (5.8 KB) - added by tobiasz.cudnik 6 years ago.
6376_8.patch (6.2 KB) - added by tobiasz.cudnik 6 years ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 6 years ago by tobiasz.cudnik

  • Status changed from new to pending

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 ?

comment:2 Changed 6 years ago by tobiasz.cudnik

  • Milestone set to CKEditor 3.4.2

comment:3 Changed 6 years ago by damo

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.

comment:4 Changed 6 years ago by tobiasz.cudnik

  • Status changed from pending to confirmed

comment:5 Changed 6 years ago by satya

  • Cc satya_minnekanti@… added

comment:6 Changed 6 years ago by tobiasz.cudnik

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

Changed 6 years ago by tobiasz.cudnik

comment:7 Changed 6 years ago by tobiasz.cudnik

  • Status changed from assigned to review

comment:8 Changed 6 years ago by fredck

  • Status changed from review to review_failed

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!

comment:9 Changed 6 years ago by fredck

This one should fix #6440 as well.

comment:10 Changed 6 years ago by james c

  • Cc jamcunni@… added

Changed 6 years ago by tobiasz.cudnik

comment:11 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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.

comment:12 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

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.

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

Changed 6 years ago by tobiasz.cudnik

comment:14 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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

comment:15 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

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

Changed 6 years ago by tobiasz.cudnik

comment:16 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review
  • Optimized fetching the getComputedState config
  • Reduced the getComputedState usages.
  • Direction is again applied using an attribute.
  • dirAfter var is not used anymore.

comment:17 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

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>]

Changed 6 years ago by garry.yao

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

Changed 6 years ago by tobiasz.cudnik

comment:19 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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.

Changed 6 years ago by tobiasz.cudnik

comment:20 Changed 6 years ago by 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).

Changed 6 years ago by tobiasz.cudnik

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

comment:22 Changed 6 years ago by garry.yao

  • Status changed from review to review_passed

comment:23 Changed 6 years ago by tobiasz.cudnik

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

Fixed with [5970].

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