Opened 6 years ago

Closed 6 years ago

#6099 closed Bug (fixed)

BIDI: when we apply explicit language direction to Numbered/Bulleted List the corresponding BIDI Tool bar icon is not highlighted in the Toolbar

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

Description

To reproduce the defect:

  1. Open Ajax sample.
  1. Type few lines of text, select all the lines,click on Numbered/Bulleted list icon.
  1. See that a Numbered/Bulleted list appers with the selected lines of text.
  1. Now select the Numbered/Bulleted list and Click on RTL icon in the Tool bar.

Expected Result:

Numbered/Bulleted list is moved to right and it appears as a mirror image of the list in previous step and RTL icon is selected in the Tool bar.

Actual Result:

Numbered/Bulleted list is moved to right and it appears as a mirror image of the list in previous step but RTL icon is not selected in the Tool bar.

same behavior happens when we apply LTR direction to Numbered/Bulleted list.

Attachments (8)

6099.patch (621 bytes) - added by tobiasz.cudnik 6 years ago.
6099_2.patch (1.8 KB) - added by tobiasz.cudnik 6 years ago.
6099_3.patch (1.3 KB) - added by tobiasz.cudnik 6 years ago.
6099_4.patch (1.4 KB) - added by tobiasz.cudnik 6 years ago.
6099_5.patch (2.6 KB) - added by tobiasz.cudnik 6 years ago.
6099_6.patch (2.6 KB) - added by tobiasz.cudnik 6 years ago.
6099_7.patch (2.6 KB) - added by tobiasz.cudnik 6 years ago.
6099_8.patch (3.0 KB) - added by tobiasz.cudnik 6 years ago.

Download all attachments as: .zip

Change History (33)

comment:1 follow-up: Changed 6 years ago by fredck

  • Status changed from new to pending

WFM with both FF and IE. What browser version do you have there?

comment:2 in reply to: ↑ 1 Changed 6 years ago by satya

Replying to fredck:

WFM with both FF and IE. What browser version do you have there?

It's still not working for us we have used FF3.5, FF3.6, IE6 & IE7

comment:3 Changed 6 years ago by tobiasz.cudnik

  • Status changed from pending to confirmed

I can confirm this on 3.4 beta branch, although it works on trunk.

comment:4 Changed 6 years ago by tobiasz.cudnik

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

comment:5 Changed 6 years ago by tobiasz.cudnik

After deeper checking the issue it seems that my successful reproduction was due to Firebug exception handling. After turning off Firebug's script tab it WFM. Although this doesn't explain your experience on IE.

We will try to fix those exceptions to make it work with Firebug, maybe it will fix also IE issue.

Changed 6 years ago by tobiasz.cudnik

comment:6 Changed 6 years ago by tobiasz.cudnik

  • Status changed from assigned to pending

The patch fixes the case which allowed me to reproduce similar issue using FF with Firebug, although this part of code was never executed in IE, so probably it will not fix reported problem.

We need more informations on this - does it happens on the demo site ? Maybe you can provide some error msgs from any debugger ?

comment:7 follow-up: Changed 6 years ago by damo

It might be useful to mention that this issue occurs when config.useComputedState = false;

comment:8 in reply to: ↑ 7 Changed 6 years ago by fredck

  • Status changed from pending to confirmed

Replying to damo:

It might be useful to mention that this issue occurs when config.useComputedState = false;

Well, well... this definitely changes things, and I could confirm it immediately.

Changed 6 years ago by tobiasz.cudnik

comment:9 Changed 6 years ago by tobiasz.cudnik

  • Status changed from confirmed to review

comment:10 Changed 6 years ago by fredck

  • Milestone set to CKEditor 3.4.1

comment:11 follow-up: Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

The patch looks good except for the following line:

selectedElement = editor.getSelection().getCommonAncestor();

no matter whether useComputedState is set or not, the state should always be figured out from the first block instead of the entire selection, which keeps the selection change event fast enough and keep it simple for users to understand, other similar plugins just apply to this convention, so the logics here would actually matches L12~L48 of the justify plugin.

comment:12 Changed 6 years ago by garry.yao

Another thing, the toolbar icon direction (cke_mixed_dir_content), as a symbol of comforting user on text direction, should not depends on useComputedState.

Changed 6 years ago by tobiasz.cudnik

comment:13 in reply to: ↑ 11 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

Ive updated the patch to the newest revision. Replying to garry.yao:

The patch looks good except for the following line:

selectedElement = editor.getSelection().getCommonAncestor();

Fixed. Replying to garry.yao:

Another thing, the toolbar icon direction (cke_mixed_dir_content), as a symbol of comforting user on text direction, should not depends on useComputedState.

It depends on a config and it's already needed for setState, so we're reusing it when we can.

comment:14 follow-up: Changed 6 years ago by garry.yao

  • Status changed from review to review_failed
selectedElement = path.elements[ 0 ];

The above line in the patch could catch text dir on inline style also, am I wrong by saying BIDI in editor is a block-only feature?

It depends on a config and it's already needed for setState, so we're reusing it when we can.

I'm not talking about the BIDI command state, but the direction of indent/outdent, it should always changes along the actual text block direction IMO.

Changed 6 years ago by tobiasz.cudnik

comment:15 in reply to: ↑ 14 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

Replying to garry.yao:

selectedElement = path.elements[ 0 ];

The above line in the patch could catch text dir on inline style also, am I wrong by saying BIDI in editor is a block-only feature?

Right, BIDI is block only, so ive added some element tagname check to conform with those elements which receive direction from the editor.

I'm not talking about the BIDI command state, but the direction of indent/outdent, it should always changes along the actual text block direction IMO.

  • Direction of already indented content inside element which direction is changed, changes by flipping the margin side. (#5910)
  • Both indent icon and command are changed according to the selection. (#6041, #5910)

Anyway, how it's related to this ticket which concerns lists ?

comment:16 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

Ok, thanks for the explanation, I just think we could make it right at this ticket also.

Beside, with the latest patch, the element checking is not well controlled and walked up to the 'html' element, which cause the icon always lighted even with 'useComputedState' turned false.

Changed 6 years ago by tobiasz.cudnik

comment:17 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

Changes in this patch:

  • When useComputedState == false, checked is first element that would gain direction from one of direction buttons, BUT it stops on body and this element doesn't have to have applied direction (then no toolbar button will be highlighted).
  • Logic for cke_mixed_dir_content now always uses computedState and is not affected by toolbar states function (which was an issue before).

comment:18 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

Almost there, still that line could cause problem, e.g. command state should be affected by 'dir' applied on inline element, the first element to check should start from a block-level one.

selectedElement = path.elements[ 0 ];

Besides a small note, pls keep 'onSelectionChange' instead of 'setToolbarStates', it's a convention that already followed by other plugins.

comment:19 Changed 6 years ago by james c

  • Cc jamcunni@… added

comment:20 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review
  • Changed the selectedElement for disabled computed state so it's always an element.
  • Included a TD into the guard elements (only for state).
  • Aligned to the 'onSelectionChange' name convention.

Changed 6 years ago by tobiasz.cudnik

Changed 6 years ago by tobiasz.cudnik

comment:21 follow-up: Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

The patch fails for the following case with 'config.useComputedState = false' where state of 'RTL' command should be 'on':

<div dir="rtl">
	<p>Para^graph 1</p>
	<p>Paragraph 2</p>
</div>

comment:22 in reply to: ↑ 21 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

Replying to garry.yao:

The patch fails for the following case with 'config.useComputedState = false' where state of 'RTL' command should be 'on':

<div dir="rtl">
	<p>Para^graph 1</p>
	<p>Paragraph 2</p>
</div>

Result of this TC should be an OFF state for both direction buttons when the computed state is not used, as P is the element which will gain the direction, so it's the element to be checked for toolbar states.

List of such elements is inside the stateGuardElements.

RTL icon should on ON for this TC when the computed state is turned on.

Changed 6 years ago by tobiasz.cudnik

comment:23 Changed 6 years ago by tobiasz.cudnik

Ive moved some common logic into getElementForDirection method, it's now also used inside handleMixedDirContent, as discussed with Garry.

comment:24 Changed 6 years ago by garry.yao

  • Status changed from review to review_passed

comment:25 Changed 6 years ago by tobiasz.cudnik

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

Fixed with [5967].

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