Opened 14 years ago
Closed 14 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:
- Open Ajax sample.
- Type 6 paragraphs and apply LTR to Paragraphs(1,3&5) & RTL to paragraphs(2,4&6).
- 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)
Change History (19)
comment:1 Changed 14 years ago by
Milestone: | → CKEditor 3.4.1 |
---|---|
Status: | new → confirmed |
comment:2 Changed 14 years ago by
Owner: | set to Paweł Horzela |
---|---|
Status: | confirmed → assigned |
comment:3 Changed 14 years ago by
Status: | assigned → review |
---|
comment:4 Changed 14 years ago by
Status: | review → review_failed |
---|
comment:5 Changed 14 years ago by
Status: | review_failed → review |
---|
comment:6 Changed 14 years ago by
Status: | review → review_passed |
---|
comment:7 Changed 14 years ago by
Status: | review_passed → review_failed |
---|
I've just found a tc that fails with this patch:
- Load the following:
<p dir="ltr"> Line 1</p> <p dir="ltr"> Line 2</p> <p dir="ltr"> Para 3</p>
- CTRL+A to select all.
- 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.
comment:8 Changed 14 years ago by
Status: | review_failed → review |
---|
comment:9 Changed 14 years ago by
Status: | review → 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 ) );
comment:10 Changed 14 years ago by
Status: | review_failed → review |
---|
comment:11 Changed 14 years ago by
Status: | review → 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 14 years ago by
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 14 years ago by
Status: | review_failed → review |
---|
comment:14 Changed 14 years ago by
Status: | review → review_passed |
---|
You're right, it's enough to keep 6237_4.patch
comment:15 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed with [5912]
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.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.