Opened 6 years ago

Closed 6 years ago

#6237 closed Bug (fixed)

BIDI: Applying same language direction to all paragraphs not working.

Reported by: satya Owned by: paho
Priority: Normal Milestone: CKEditor 3.4.2
Component: General Version: 3.4
Keywords: IBM Cc: damo, joek

Description

To reproduce the defect:

  1. Open Ajax sample.
  1. Type 6 paragraphs and apply LTR to Paragraphs(1,3&5) & RTL to paragraphs(2,4&6).
  1. Select all the paragraphs using Ctrl + A and click on RTL icon.

Expected Result

RTL Language direction is applied to all the paragraphs.

Actual Result

RTL language direction is not applied to all the paragraphs. It is applied only to paragraphs(1,3&5) and LTR is applied to Paragraphs(2,4&6).

Attachments (4)

#6237.patch (1012 bytes) - added by paho 6 years ago.
Patch added before code review
6237_2.patch (2.0 KB) - added by paho 6 years ago.
Patch added after code review
6237_3.patch (2.0 KB) - added by paho 6 years ago.
Patch
6237_4.patch (1.6 KB) - added by paho 6 years ago.
Patch

Download all attachments as: .zip

Change History (19)

comment:1 Changed 6 years ago by Saare

  • Milestone set to CKEditor 3.4.1
  • Status changed from new to confirmed

comment:2 Changed 6 years ago by paho

  • Owner set to paho
  • Status changed from confirmed to assigned

comment:3 Changed 6 years ago by paho

  • Status changed from assigned to review

comment:4 Changed 6 years ago by fredck

  • Status changed from review to review_failed

First the basics... minor details:

Now, the logic:

  • dir == editor.lang.dir: editor.lang is exclusively related to the editor UI language. It has nothing to do with the contents language. In that specific case, I think you were talking about the contentsLangDirection setting.
  • In any case, checking contentsLangDirection may not be enough, because the blocks may be inside a container (a div or a td) with direction different from contentsLangDirection.

Basically what we need is a way to be sure that the direction is always applied (not removed nor switched) for subsequent blocks if their current direction differs from the direction of the first block. For that, an additional parameter could be passed to the switchDir function.

comment:5 Changed 6 years ago by paho

  • Status changed from review_failed to review

Changed 6 years ago by paho

Patch added before code review

Changed 6 years ago by paho

Patch added after code review

comment:6 Changed 6 years ago by Saare

  • Status changed from review to review_passed

comment:7 Changed 6 years ago by fredck

  • Status changed from review_passed to review_failed

I've just found a tc that fails with this patch:

  1. Load the following:
<p dir="ltr">
	Line 1</p>
<p dir="ltr">
	Line 2</p>
<p dir="ltr">
	Para 3</p>
  1. CTRL+A to select all.
  2. Click the LTR button to remove the "dir" attribute.

Expected: The attribute is removed from all paragraphs.

After patch: The attribute is removed from the first paragraph only.

Changed 6 years ago by paho

Patch

comment:8 Changed 6 years ago by paho

  • Status changed from review_failed to review

comment:9 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

The bidi command actually have the same operation model with justify, where the command state is decided by the first block's style and in return gives the correct state to each block iteration, we could simply follow the logics how justify command dealt with this instead of introducing this additional parameter:

var apply =
	( this.state == CKEDITOR.TRISTATE_OFF ) &&
	( !useComputedState || ( getAlignment( block, true ) != this.value ) );

Changed 6 years ago by paho

Patch

comment:10 Changed 6 years ago by paho

  • Status changed from review_failed to review

comment:11 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

Nice done, as the final mile, would you mind simplify the following lines, to make it perform a little bit better, if you agree, we save a couple of style manipulation by placing them into a separate 'else'.

element.removeStyle( 'direction' );
element.removeAttribute( 'dir' );

if ( state == CKEDITOR.TRISTATE_OFF && element.getComputedStyle( 'direction' ).toLowerCase() != dir )
	element.setAttribute( 'dir', dir );

comment:12 Changed 6 years ago by paho

Do you mean something like this:

element.removeStyle( 'direction' );		

if ( state == CKEDITOR.TRISTATE_OFF && element.getComputedStyle( 'direction' ).toLowerCase() != dir )
	element.setAttribute( 'dir', dir );
else
	element.removeAttribute( 'dir' );

It won't work, beacuse element.removeAttribute method changes ComputedStyle of the element, so it must be called before 'if' clause. Otherwise, you have to change the other logic in attached patch.

But look here, in justify plugin, it's more-less the same:

block.removeAttribute( 'align' );
block.removeStyle( 'text-align' );

var apply =
	( this.state == CKEDITOR.TRISTATE_OFF ) &&
	( !useComputedState || ( getAlignment( block, true ) != this.value ) );

if ( apply )
	block.setStyle( 'text-align', this.value );

and it's used only 'if' clause, without 'else'.

comment:13 Changed 6 years ago by paho

  • Status changed from review_failed to review

comment:14 Changed 6 years ago by garry.yao

  • Status changed from review to review_passed

You're right, it's enough to keep 6237_4.patch

comment:15 Changed 6 years ago by paho

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

Fixed with [5912]

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