Opened 7 years ago

Closed 7 years ago

#6237 closed Bug (fixed)

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

Reported by: Satya Minnekanti Owned by: Paweł Horzela
Priority: Normal Milestone: CKEditor 3.4.2
Component: General Version: 3.4
Keywords: IBM Cc: Damian, 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 Paweł Horzela 7 years ago.
Patch added before code review
6237_2.patch (2.0 KB) - added by Paweł Horzela 7 years ago.
Patch added after code review
6237_3.patch (2.0 KB) - added by Paweł Horzela 7 years ago.
Patch
6237_4.patch (1.6 KB) - added by Paweł Horzela 7 years ago.
Patch

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by Sa'ar Zac Elias

Milestone: CKEditor 3.4.1
Status: newconfirmed

comment:2 Changed 7 years ago by Paweł Horzela

Owner: set to Paweł Horzela
Status: confirmedassigned

comment:3 Changed 7 years ago by Paweł Horzela

Status: assignedreview

comment:4 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 7 years ago by Paweł Horzela

Status: review_failedreview

Changed 7 years ago by Paweł Horzela

Attachment: #6237.patch added

Patch added before code review

Changed 7 years ago by Paweł Horzela

Attachment: 6237_2.patch added

Patch added after code review

comment:6 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

comment:7 Changed 7 years ago by Frederico Caldeira Knabben

Status: review_passedreview_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 7 years ago by Paweł Horzela

Attachment: 6237_3.patch added

Patch

comment:8 Changed 7 years ago by Paweł Horzela

Status: review_failedreview

comment:9 Changed 7 years ago by Garry Yao

Status: reviewreview_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 7 years ago by Paweł Horzela

Attachment: 6237_4.patch added

Patch

comment:10 Changed 7 years ago by Paweł Horzela

Status: review_failedreview

comment:11 Changed 7 years ago by Garry Yao

Status: reviewreview_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 7 years ago by Paweł Horzela

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 7 years ago by Paweł Horzela

Status: review_failedreview

comment:14 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

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

comment:15 Changed 7 years ago by Paweł Horzela

Resolution: fixed
Status: review_passedclosed

Fixed with [5912]

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