Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#6041 closed Bug (fixed)

BIDI: Direction of Increase Indent & Decrease Indent icons are not reversed after changing Lang direction to RTL

Reported by: satya Owned by: tobiasz.cudnik
Priority: Normal Milestone: CKEditor 3.4.1
Component: General Version: 3.3
Keywords: IBM Cc: damo, joek

Description

To reproduce the defect:

  1. Open Ajax sample.
  1. Type some text, Select the text and click on RTL icon in the Tool bar.
  1. keep the cursor in the Paragraph and look for the direction of Increase Indent & Decrease Indent Icons.

Expected Result:

Direction of the Arrow in Increase Indent icon should be pointing from right to left and direction of Arrow in Decrease Indent icons should be pointing from right to left, since we have changed the direction of Paragraph from left to right.

Actual Result:

Direction of Increase Indent & Decrease Indent icons are not reversed, they are sill showing the same direction of Indentation for a Left to Right paragraph.

Attachments (6)

6041.patch (2.9 KB) - added by tobiasz.cudnik 6 years ago.
icons.png (4.8 KB) - added by tobiasz.cudnik 6 years ago.
/_source/skins/kama/
icons_rtl.png (4.8 KB) - added by tobiasz.cudnik 6 years ago.
/_source/skins/kama/
6041_2.patch (4.0 KB) - added by tobiasz.cudnik 6 years ago.
6041_3.patch (7.5 KB) - added by tobiasz.cudnik 6 years ago.
skins_3.zip (30.5 KB) - added by tobiasz.cudnik 6 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 6 years ago by Saare

The problem here is that the icon set is only suitable for LTR. A RTL version of the icon set will be handled in #3840.

comment:2 Changed 6 years ago by fredck

  • Milestone set to CKEditor 3.5

comment:3 Changed 6 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik
  • Status changed from new to assigned

Changed 6 years ago by tobiasz.cudnik

comment:4 Changed 6 years ago by tobiasz.cudnik

  • Status changed from assigned to review

Attached patch fixes Kama skin. Just replace image.png in main skin folder and place second one for RTL in same dir.

comment:5 Changed 6 years ago by fredck

  • Status changed from review to review_failed

The effect is pretty nice. There are just a few minor issues:

  • At lines 44 and 45, there is no need to use evt.editor, because we already have the editor variable.
  • Being this a UI feature that touches the toolbar, editor.lang.dir should be checked, not contentsLangDirection.
  • In both images, the last <div> container icons have been mistakenly mixed with the list icons, breaking them.
  • The following icons should be also reflected in the rtl strip: Source, Anchor, Find and Page Break for Printing.
  • It's ok to have the box at the left side in the Past from Word icon, but the small Word logo (W) must not be reflected.

I would also ask Saar to give us his opinion, as he's our native rtl man.

comment:6 Changed 6 years ago by Saare

  • The rtl icons should also be present on the context menu.
  • The textarea icon should also be changed (the scrollbar should go on the left), and so does the selectbox icon.
  • A few more icons that could be reflected (not an issue though, we can leave them as they are): Text color, Preview and Show blocks.

I'm closing #3840 as a DUP for now.

Changed 6 years ago by tobiasz.cudnik

/_source/skins/kama/

Changed 6 years ago by tobiasz.cudnik

/_source/skins/kama/

comment:7 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

Second patch address mentioned issues, including mirroring context menu icons depending on the selected element (like lists).

Personally i think we can also change all icons containing letters, like "search & replace".

Changed 6 years ago by tobiasz.cudnik

comment:8 Changed 6 years ago by Saare

  • Status changed from review to review_failed

The text in the textarea icon is now flipped in the rtl strip. Other than that I think you can now spread it to the other skins.

comment:9 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to assigned

Changed 6 years ago by tobiasz.cudnik

Changed 6 years ago by tobiasz.cudnik

comment:10 Changed 6 years ago by tobiasz.cudnik

  • Status changed from assigned to review

comment:11 Changed 6 years ago by Saare

  • Status changed from review to review_passed

Just to be safe of any DOM modification by the user, let's use editor.container instead of editor.element.getNext() (L47 in the bidi plugin).

comment:12 Changed 6 years ago by tobiasz.cudnik

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

Fixed with [5838].

comment:13 Changed 6 years ago by Saare

For reference, [5839] applies the change descibed in comment:11 also for the context menu.

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