Opened 7 years ago

Closed 7 years ago

#5910 closed Bug (fixed)

BIDI: Indentation should work with mixed BIDI documents

Reported by: Damian Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.4
Component: General Version:
Keywords: IBM Cc: Satya Minnekanti, joek

Description

Direction sensitive operations like indentation and alignment should work properly according to the base language direction of the relevant part of the document. The toolbar icons for these functions should also render correctly when moving between RTL and LTR content.

Example problem 1
Set this content in source and switch back to rich text.

<p dir="rtl">RTL content</p>

Notice that it isn't possible to use the indentation buttons in the editor (left margin is applied instead of right).

Example problem 2
Using the same content as above, notice that the justify icons shows left aligned, when it should show right aligned.

Attachments (4)

5910.patch (4.8 KB) - added by Tobiasz Cudnik 7 years ago.
5910_2.patch (5.7 KB) - added by Tobiasz Cudnik 7 years ago.
5910_3.patch (4.4 KB) - added by Tobiasz Cudnik 7 years ago.
5910_4.patch (3.8 KB) - added by Frederico Caldeira Knabben 7 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

comment:2 Changed 7 years ago by Frederico Caldeira Knabben

Related to this, one o the feature of the BiDi tools is that, when switching the direction, the left and right margin styles will be also switched, effectively mirroring the block.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5910.patch added

comment:3 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added

Maybe we need some event about switching the direction to notify other plugins (like this one) ? Or maybe we will implement margin flipping feature (and possibly others) of inner content, inside plugin which switched the direction ?

comment:4 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

KISS, switching the margins in the bidi plugin directly.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5910_2.patch added

comment:5 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

This patch should include all features, along with better indentation from multiple selection.

comment:6 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

Things are pretty broken after the patch:

  • The indentation button stopped working.
  • The direction buttons get stuck once applied. It's not possible to switch to another direction. We must first remove the applied direction and then set the other.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5910_3.patch added

comment:7 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

This is simplified patch using getComputedStyle instead of DOM traversing to determine text direction.

Additionally i've fixed dir switch buttons.

comment:8 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed
  • KISS, we can get rid of indentCssProperty and make things work properly. Insisting with it is a bad idea.
  • getComputedStyle should be used in the bidi plugin also, instead of hacks to find out the inherited direction.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 5910_4.patch added

comment:9 Changed 7 years ago by Frederico Caldeira Knabben

Owner: changed from Tobiasz Cudnik to Frederico Caldeira Knabben
Status: review_failedreview

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

Status: reviewreview_passed

comment:11 Changed 7 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [5746].

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