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

[Satya Minnekanti]

To reproduce the defect:

1. Open Ajax sample.

2. Type few lines of text, select all the lines,click on Numbered/Bulleted list icon.

3. See that a Numbered/Bulleted list appers with the selected lines of text.

4. 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.

[Frederico Caldeira Knabben]

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

[Satya Minnekanti]

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

[Tobiasz Cudnik]

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

[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.

[Tobiasz Cudnik]

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 ?

[Damian]

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

[Frederico Caldeira Knabben]

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.

[Garry Yao]

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.

[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.

[Tobiasz Cudnik]

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.

[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?

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.

[Tobiasz Cudnik]

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 ?

[Garry Yao]

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.

[Tobiasz Cudnik]

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).

[Garry Yao]

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.

[Tobiasz Cudnik]

• 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.

[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>

[Tobiasz Cudnik]

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.

[Tobiasz Cudnik]

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

[Tobiasz Cudnik]

Fixed with [5967].