Opened 7 years ago

Closed 7 years ago

#6376 closed Bug (fixed)

BIDI: BIDI buttons should not toggle the base language direction

Reported by: Damian 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 7 years ago.
6376_2.patch (2.0 KB) - added by Tobiasz Cudnik 7 years ago.
6376_3.patch (2.2 KB) - added by Tobiasz Cudnik 7 years ago.
6376_4.patch (2.4 KB) - added by Tobiasz Cudnik 7 years ago.
6376_5.patch (9.8 KB) - added by Garry Yao 7 years ago.
6376_6.patch (6.0 KB) - added by Tobiasz Cudnik 7 years ago.
6376_7.patch (5.8 KB) - added by Tobiasz Cudnik 7 years ago.
6376_8.patch (6.2 KB) - added by Tobiasz Cudnik 7 years ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 7 years ago by Tobiasz Cudnik

Status: newpending

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 7 years ago by Tobiasz Cudnik

Milestone: CKEditor 3.4.2

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

comment:4 Changed 7 years ago by Tobiasz Cudnik

Status: pendingconfirmed

comment:5 Changed 7 years ago by Satya Minnekanti

Cc: satya_minnekanti@… added

comment:6 Changed 7 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: confirmedassigned

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6376.patch added

comment:7 Changed 7 years ago by Tobiasz Cudnik

Status: assignedreview

comment:8 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 7 years ago by Frederico Caldeira Knabben

This one should fix #6440 as well.

comment:10 Changed 7 years ago by James

Cc: jamcunni@… added

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6376_2.patch added

comment:11 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

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 7 years ago by Garry Yao

Status: reviewreview_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 7 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 7 years ago by Tobiasz Cudnik

Attachment: 6376_3.patch added

comment:14 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

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

comment:15 Changed 7 years ago by Garry Yao

Status: reviewreview_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 7 years ago by Tobiasz Cudnik

Attachment: 6376_4.patch added

comment:16 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview
  • 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 7 years ago by Garry Yao

Status: reviewreview_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 7 years ago by Garry Yao

Attachment: 6376_5.patch added

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

Attachment: 6376_6.patch added

comment:19 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

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 7 years ago by Tobiasz Cudnik

Attachment: 6376_7.patch added

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

Attachment: 6376_8.patch added

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

Status: reviewreview_passed

comment:23 Changed 7 years ago by Tobiasz Cudnik

Resolution: fixed
Status: review_passedclosed

Fixed with [5970].

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