Opened 14 years ago

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

Download all attachments as: .zip

Change History (31)

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

Milestone: CKEditor 3.4.2

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

Status: pendingconfirmed

comment:5 Changed 13 years ago by Satya Minnekanti

Cc: satya_minnekanti@… added

comment:6 Changed 13 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: confirmedassigned

Changed 13 years ago by Tobiasz Cudnik

Attachment: 6376.patch added

comment:7 Changed 13 years ago by Tobiasz Cudnik

Status: assignedreview

comment:8 Changed 13 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 13 years ago by Frederico Caldeira Knabben

This one should fix #6440 as well.

comment:10 Changed 13 years ago by James

Cc: jamcunni@… added

Changed 13 years ago by Tobiasz Cudnik

Attachment: 6376_2.patch added

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

Attachment: 6376_3.patch added

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

Attachment: 6376_4.patch added

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

Attachment: 6376_5.patch added

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

Attachment: 6376_6.patch added

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

Attachment: 6376_7.patch added

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

Attachment: 6376_8.patch added

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

Status: reviewreview_passed

comment:23 Changed 13 years ago by Tobiasz Cudnik

Resolution: fixed
Status: review_passedclosed

Fixed with [5970].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy