Setting text direction to content within CKEditor dialogs

[Tomer Mahlin]

CKSource ticket ​http://dev.ckeditor.com/ticket/10633 covers the use case where users change the direction of content within a dialog and it is then inserted into the editor with the specified direction.

This should be available for all dialogs and not just paste one. Example with standard CKEditor dialog - table dialog and caption field in it:

1. Open the Insert Table dialog
2. Enter some text in the Caption field e.g. hello world!!!
3. Switch direction of the field using bidi shortcuts e.g. Shift+Ctrl+X in FF so that the Caption field now displays !!!hello world
4. Click OK

Problem: The Caption gets inserted into the editor as hello world!!! rather than !!!hello world

Dialogs should also preserve the direction displayed in the editor. For example, using the table inserted in the above scenario,

1. Select the caption text in the editor and click the 'Text Direction right to left' toolbar button. This will display

!!!hello world in the editor.

2. Then right click on the table and select 'Table Properties'.

Problem: The Caption field displays hello world!!! rather than !!!hello world.

Following the same reasoning as in ticket ​http://dev.ckeditor.com/ticket/10633 the suggestion is to see this ticket as new feature request. My team will suggest a sample patch for it.

[Jakub Ś]

Yes that is true. If you add dir attribute to caption in HTML and then open Table Properties Dialog you will not see text aligned to right.

On one hand this sounds nice but on the other elements in dialog is created in a static way and attributes from edited table element are not available when elements are created. Furthermore when dialog is created its code stays and is not changed.

IMHO this makes this feature a won't fix issue. I'm however setting it as pending and would like to head secondary opinion from someone from CKSource team.

[Tomer Mahlin]

Direction and alignment are two quite different things. Alignment is cosmetic, while direction affects text readibility. I would rephrase ".. you won't see text aligned to right ..." to "... you won't see text with RTL direction ...". This is just to emphasis the importance of text direction for bidirectional languages (such as Arabic / Hebrew).

I am not sure I fully understand the argument against adding this feature. Are you saying there is a technical difficulty to implement it or there is a conceptual problem ? CKEditor should provide WYSIWYG experience. The same text should be displayed the same way in all contexts. If you change direction of table caption in editable area, you should see this change reflected in Table Properties dialog and vice versa.

Please observe that Table Properties dialog is just one of the examples. There are additional dialogs for which control over text direction is applicable. For example:

1. Image properties dialog - Alternative text field

2. Anchor properties dialog - Anchor name field

3. Form properties dialog - Name field

4. Check box properties dialog - Name / Value fields

In all mentioned above cases, we see expected behavior. When you change direction in the field (in the dialog) using keyboard shortcuts, it is persisted. In other words, when you open this dialog again, the direction is preserved. From this I understand that conceptually CKEditor does support the expected behavior.

There are other contexts in which this support is not provided. In addition to Table properties dialog, here is another example: Selection Field Properties dialog - Text / Value fields . When you change the direction of text in the fields and press Add or Modify buttons, the direction remains as it is set in Text / Value fields (instead of being carried with data). Try to set LTR direction to one Text / Value pair and RTL to another one. This setting won't be reflected in the list boxes which appear just under Text / Value fields. The direction of text shown on the list boxes will be still according to UI direction.

[Jakub Ś]

Alignment is cosmetic, while direction affects text readibility. I would rephrase ".. you won't see text aligned to right ..." to "... you won't see text with RTL direction ..."

This is IMHO about alignment. At least this is only what editor can do.

Please have a look at below example:

1. Go to nanguages sample
2. Change language to e.g. Arabic
3. Type RTL container LTR language.
4. Select text

   LTR container LTR language.             .RTL container LTR language
   

Setting language in CKEritor to RTL (E.g. config.language='he') or using attribute dir="rtl" is only setting container to RTL. Language is still LTR and that is why selection looks so weird (First "." than "e"). To change the language to RTL, you would have to change it in your OS. After such changes and assuming that below text is in RTL language: .RTL container RTL language
The selection would look like: First "." than "R" than "T" and so on.

If this was the issue you meant then this part can't be fixed as it simply works that way.

[Tomer Mahlin]

When I talk about language I don't refer to language of UI but to language of content end user author. We can assume that UI language (language in which all menus and titles in dialogs appear) is English only. It is content which end user types that can include text in different languages. When you change direction of text you usually change its alignment as well. By the way, in the input fields in CKEditors dialogs this is not so. When you change direction by Ctrl-Left-Shift / Ctrl-Right-Shift, you do change the direction but alignment remains the same.This is not what I am complaining about :-)) even though this is also an issue.

I am talking about the fact that once you do change the direction via keyboard shortcuts it should remain the same for edited text. This already happens partially in following contexts:

1.Image properties dialog - Alternative text field

2.Anchor properties dialog - Anchor name field

3.Form properties dialog - Name field

4.Check box properties dialog - Name / Value fields

Open any of those dialogs. Type "hello !!!" in the specified field. Change direction to RTL by pressing <Ctrl>-<Right-Shift>. Observe that text is displayed as "!!!hello". Press OK (to assure object is created). Right click object and select appropriate properties. In the opened dilaog the same field in which you changed the direction will show "!!!hello". Namely the direction was preserved. Well... As I just realized the direction is preserved on the dialog level, rather than on the text level. If you create two entities and set different directions in the dialog, the latest one will show up when you open the dialog again. So, it does not fully work after all.

[Jakub Ś]

In the opened dilaog the same field in which you changed the direction will show "!!!hello".

Please refresh your cache as the way I see it it doesn't work at all.

[Tomer Mahlin]

It does work consistently. Let us observe how.

1. Just open any dialog (i.e. Anchor properties dialog )

2. Type some text (i.e. "hello !!!")

3. Change direction of the field by <Ctrl>-<Right-Shift> to RTL

Result: Text appears as "!!! hello" - expected However text is still alligned to the left (less expected)

Running the same text against any standard field (for example search fields on www.cnn.com or www.google.com) will result in similar text display AND right text alignment

From my perspective this attests to the fact that CKEditor do use a customized input field since otherwise alignment would be different.

4. Close the dialog by pressing Cancel button

5. Open the same dialog again (the input field will be empty)

6. Type the same text as before (i.e. "hello !!!")

Result: The text will be displayed as "!!! hello"). This means that dialog somehow remember what text direction was set in the input field last time it was opened.

7. Change text direction to LTR by pressing <Ctrl> <Left-Shift>

Result: text will be displayed as "hello !!!" - expected

8. Close the dialog by pressing on Cancel button

9. Open the dialog again

10. Type again the same text (i.e. "hello !!!")

Result: the text will be displayed with LTR direction as: "hello !!!". In other words, last text direction set in this field is preserved and restored each time dialog is loaded.

[Jakub Ś]

Ok, I was able to reproduce this is Blink and IE but not FF.

What you talk about is browser and not editor feature. Furthermore IMHO (although result was the same in IE and Blink) I think you should rather test this with OK and not Cancel button so that value could get updated/created/edited.

[Tomer Mahlin]

It does not matter much if I use OK or Cancel button the results are very similar.

Indeed, "persistence" of manually selected text direction (via keyboard shortcut) is working on IE (on the GUI control level) but does not work on FF.

However, the requested feature is associated with editor not browser since persistence (or association) of base text direction is required on the data not GUI control level. GUI control is coming from browser indeed. However, data belongs to editor. It is through GUI control I specify the text direction (and this works perfectly, Ctrl/Shift or Ctrl/Shift/X work like charm), but it is in editor responsibility to assure that this information is available and used when the same text is displayed again. Please observe that through the same instance of GUI control (i.e. the same input field in the same dialog) I can display different data with different directions (different instances of object type created through the dialog). Thus we need the text direction information to be associated with data and to be communicated to control when data is displayed in it.

There is no much difference between this feature (applicable for many dialogs) and the one discussed in ​http://dev.ckeditor.com/ticket/10633 ticket (associated with copy/paste dialog). In both cases:

1. Separate dialog is used to accept some input from user (either through copy / paste or via direct typing). The only difference that while in copy / paste case the text can be rich one (including markup), in current case, usually the text is plain one.

2. End user can manually specify/change direction of text through keyboard shortcuts (standard tools provided by all browsers).

3. It is expected that base text direction specification is carried together with data (to assure the data is displayed the same way next time it is shown).

[Frederico Caldeira Knabben]

I understand the issue clearly, but I'm worried about the technical limitations that HTML impose to us here.

The table caption field is a "fixable" case, because it creates a <caption> element, which can hold the "dir" attribute to enforce the direction. This attribute could be them retrieved on editing and applied to the field in the dialog.

We have very few cases like the above though (I still didn't check all cases) but the great majority of them are related to values set to HTML attributes. For example, the "Alternative Text" on images. This is output as the "alt" attribute of the <img> element. How are we supposed to save the direction information of that field now? I don't have an answer for it right now.

What we could instead is forcing these problematic dialog fields to NOT accept chancing the direction. Not sure if this is doable though and still it doesn't sound like a good thing.

[Tomer Mahlin]

The key question is: "How are we supposed to save the direction information of that field now?" It is clear that we should do that. Hopefully you agree with that. "...Forcing these problematic dialog fields to NOT accept chancing the direction..." is not a way to go. By doing so, you will deny from the customer the only way to properly see the text.

There are might be various ways of saving direction information including using Unicode Control Characters. However, those are already technical details which I though we can discuss during code review phase.

Do you agree that this issue should be fixed ? If so, we can provide a patch and continue this discussion using the coded solution.

[Frederico Caldeira Knabben]

Interesting point about the ​Unicode Control Characters. It looks like it would enable a fix for it. We would be happy to check an eventual patch you could provide for it. Thanks!

[Moshe Wajnberg]

Hello Fred,

I submitted a pull request for you. Can you please review ?

[Moshe Wajnberg]

Hello Fred,

Any updates ?

[Piotrek Koszuliński]

Hi wajnberg,

We remember about your pull request. This is a new feature and therefore it will be considered for CKEditor 4.4. But first we need to review it and this process takes time. To make sure that we won't forget about review, I set milestone to 4.4.

Thanks!

[Marek Lewandowski]

As a proof of concept, the proposed code looks good. You should try now bringing it to a production state.

This should be a generic feature in the dialog system, which should be easily enabled when developing dialogs. Therefore the logic should be moved to dialog plugin, especifically touching the text and textarea field types.

In terms of API, the dialog system should expose the bidi (bool, default: false) property, to be used in dialog definition. For instance consider following example for table plugin dialog:

{
	type: 'text',
	id: 'txtCaption',
	bidi: true,
	requiredContent: 'caption',
	label: editor.lang.table.caption,
	setup: function( selectedTable ) {
		// (...)
	},
	commit: function( data, table ) {
		// (...)
	}
},
{
	type: 'text',
	id: 'txtSummary',
	bidi: true,
	requiredContent: 'table[summary]',
	label: editor.lang.table.summary,
	setup: function( selectedTable ) {
		// (...)
	},
	commit: function( data, selectedTable ) {
		// (...)
	}
}

By setting the bidi property to true, the custom logic presented in your pull request will be applied and the ALT+SHIFT+HOME and ALT+SHIFT+END hotkeys enabled.

The final result is that the getValue() will automatically prepend the field value with the control characters if necessary.

[Tomer Mahlin]

Thanks a lot for review and advise. We will proceed along the lines suggested above.

[edithk]

Hello,

I submitted a pull request implementing your recommendation of moving the suggested code to dialog plugin - for a more generic solution.

This solution will serve all dialogs but Image, ImageButton and Anchor which do not activate the dialog's setup and commit events.

The pull request URL for your review: ​https://github.com/ckeditor/ckeditor-dev/pull/118

[Moshe Wajnberg]

Hello,

Do you have any updates ?

Thank you

[Piotrek Koszuliński]

Hi,

Sorry for the delay, but @m.lewandowski, who is supposed to review your ticket, was busy for the last couple of weeks and unfortunately will still be for the 3-4 next weeks, too.

I took a look on your code (without checking its correctness) and I can point out few things:

1. Code style - we always put spaces inside brackets; we keep opening curly braces in the same line as if, for, etc. And for the keyUpHandler we would rather use function definition and indentation is too deep inside it. Also, we use inline comments instead of block ones for code documentation.
2. This will be validated by @m.lewandowski, but I think that we cannot store input direction in the document, because it's possible that e.g. one table's summary direction will be ltr and second rtl.
3. Perhaps the code could be moved to the ​text input constructor (in dialogui plugin). It may also make sense for textarea, but not for other inputs. What's more - e.g. the part which registers keyup listener should be executed once, not on every dialog#show.

The most important is in fact the 2nd point - I'll ask @m.lewandowski for an opinion and let you know.

[Piotrek Koszuliński]

OK, I've got a feedback from @fredck and @m.lewandowski.

So I confirm the point 2 - storing direction value in the document is not correct. The ​previously proposed solution used a special character for that, so the direction was bound to the value itself what's a better solution. Thanks to that direction will be always correctly set, regardless of what was edited using the same dialog before. (In case it's not clear - a dialog instance isn't destroyed when it's closed - it's reused when opening the same dialog again.)

What I would propose is adding the code handling that special character to the textarea's and textInput's setValue and getValue methods. These both classes share the same prototype and e.g. the setValue function is defined here - ​https://github.com/ckeditor/ckeditor-dev/blob/ac30879060fad80545040a0e1b98a69c632264c9/plugins/dialogui/plugin.js#L988-L1000. getValue you'd need to add in that prototype, because it's inherited from CKEDITOR.ui.dialog.uiElement (which is defined in dialog plugin).

• setValue should strip the character and set the dir attribute accordingly,
• getValue should add the character.

These methods will then be used by setup and commit callbacks of each field.

[edithk]

Hi,

Thank you for your comments.

I have relocated the bidi support functionality to dialogui/plugin as you suggested:

• Direction is saved by attaching an lre/rle character to the value.
• This character is added in the getValue method in textInput class.
• This character is removed in the setValue method in textInput class.
• These two methods,setValue and getValue, apply also for textArea elements.
• The keyup event functionality has been added to textInput and textArea constructors.

The pull request URL for your review : ​https://github.com/ckeditor/ckeditor-dev/pull/126

[Moshe Wajnberg]

Hello,

Do you have any updates ?

Thank you

[Piotrek Koszuliński]

Hey,

I should be able to review your pull request in next two days - most likely tomorrow.

[Piotrek Koszuliński]

I reviewed and improved your pull request. You can find the code in ​branch:t/10986.

There were couple of problems in the pull request:

1. Code duplication.
2. Logic was hidden in the setValue and getValue methods.
3. The bidiSet wasn't the best attribute name - I renamed it to data-cke-dir-marker.
4. The getValue method of parent class wasn't correctly used.
5. The dir attribute wasn't cleaned upon opening dialog for a new element.
6. In a result of 5 marker was automatically added to the dialog opened for another object. This was also causing displaying the closing confirmation dialog even if I haven't modified anything.

I fixed all these issues in my branch.

Now the code needs tests.

• For all new methods.
• For the special behaviour of the get/setValue methods.
• For the field's 'required' flag - the validator should return false when direction is set, but value is empty.

You can read here about our testing environment: ​http://docs.ckeditor.com/#!/guide/dev_tests

[edithk]

Hello,

I set up the test environment, following the instructions in the link you sent. I was able to run the existing tests locally but I am having a hard time creating new test files due to lack of sufficient documentation online.

Based on existing test files I was able to create a dialog object and to set its field with a value. However, I wasn't able to fire the keyup event which is essential to the test. See example in the following test file:

/* bender-tags: editor,unit */
/* bender-ckeditor-plugins: dialogadvtab,entities */

	//<![CDATA[
( function() {
	bender.editor = true;
	bender.test(
	{
	  setUp : function() {
	    var ed = this.editor;
		var adv = ed.plugins.dialogui;
		ed.addCommand( 'testDialog', new CKEDITOR.dialogCommand( 'testDialog' ) )
		CKEDITOR.dialog.add( 'testDialog', function() {
		  return {
			title : 'Test Dialog',
			contents: 
			[
				{
			      id: 'info',
				  elements:
				  [
				    {
                                      id: 'value1',
				      type: 'text',
				      bidi: true, 
				      label: 'Value 1',
				      setup: function( widget ) {
					this.setValue( widget.data.value1 );
				      },
				      commit: function( widget ) {
					widget.setData( 'value1', this.getValue() );
				      }
				    }
				  ]
				}
			]
		  };
		} );
	  },

	  'test setting rtl direction using CTRL + ALT + END keys' : function() {
	     var bot = this.editorBot, editor = this.editor;
	     bot.dialog( 'testDialog', function( dialog ) {
             var field = dialog.getContentElement( 'info', 'value1' );
             field.setValue( 'hello!' );
	     field.focus();
				
	     // ??? this should activate setDirectionMarker() function 
	     //and add a 'dir' and 'data-cke-dir-marker' attributes
	     fireKeyup( CKEDITOR.SHIFT + CKEDITOR.ALT + 35 );
	     // test the existence of dir and data-cke-dir-marker 
	     //attributes after setting direction 
             assert.areSame( 'rtl', field.getInputElement().getAttribute( 'dir' ) || '', 'dir attribute is set to rtl' );
	     assert.areSame( 'rtl', field.getInputElement().getAttribute( 'data-cke-dir-marker' ) || '', 'marker attribute is set to rtl' );
			
	     // getValue() should save the rle marker within the value
	     var valueAndMarker = field.getValue();
	     var marker = valueAndMarker.charAt(0);
	     // test the value after getValue() is activated. 
	     //It should return the original value + rle character
	     assert.areSame('\u202B', marker, 'marker should be - rle');
				
	     // this should remove rle marker from value;
	     field.setValue(valueAndMarker);
	     var value = field.getValue();
	     // test field's value contains the original value without the rle marker
	     assert.areSame('hello!', value, 'value should be identical to original value');
	} );
			
	function fireKeyup( keyCode ) {
           var evt = new CKEDITOR.dom.event( { keyCode: keyCode } );
           evt.preventDefault = 0;
           bot.editor.editable().fire( 'keyup', evt );
	}
       }
    } );
} )();
	//]]>

Kindly assist with this problem or advice on a different approach to perform the test. Any tutorial on writing bender tests for ckeditor will be appreciated as well.

Thank you

[Piotrek Koszuliński]

Hey,

There's no more tutorials at the moment, because we publicised the tests suite recently. But actually, the way how you started is pretty good and I think that you don't need to know a lot more to finish the tests.

I hope this hints (starting from a micro review) will help you:

• Remember about the code style. Few weeks ago we introduced JSHint and JSCS that may help you. To install them (on a Linux or Mac machine; on Windows you don't need sudo I think):

  sudo npm install -g jscs
  sudo npm install -g jshint
  

And to run them for your test:

jscs tests/plugins/dialogui/yourtestfilename.js
jshint tests/plugins/dialogui/yourtestfilename.js

• I don't think you need the dialogadvtab and entities plugins.
• You definitely don't need the CDATA comments - they are for inlined scripts and make sense in XHTML only.
• You don't need the commit and setup callbacks.
• Instead of testing keystrokes it will be easier to test behaviour of the new methods and the modified ones.
• TC 1:
  • Open dialog.
  • Call setValue() with the RTL marker.
  • Check (you can have a helper method for that because this will be repeated in many tests):
    • Whether the RTL marker is not present in the real input's value.
    • Check if getValue() contains the RTL marker.
    • Check getDirectionMarker().
  • Call setValue() again, now with the LTR marker.
  • Check the same.
  • Call setValue() without any marker.
  • Close the dialog using dialog.getButton( 'ok' ).click().
• TC2:
  • Check above scenario on a field without the bidi property.
  • Nothing special should happen.
• TC3:
  • Like TC1 but check setting empty value.
  • Nothing should fail.
• TC4:
  • Like TC1 but check setting value consisting only of the marker.
  • getValue() should return empty string (no marker) so required validator works.
• TC5:
  • Check basic behaviour of the setDirectionMarker() and getDirectionMarker() methods.
• If you still want to check whether the keystroke listener works, you need to fire the keyup event on the input element: field.getInputElement().fire( 'keyup', evt )

[edithk]

Hi Reinmar,

Great tips. Thank you!

I submitted the new test file, called dir_marker.js, for your review.

The pull request URL is: ​https://github.com/ckeditor/ckeditor-dev/pull/141

[edithk]

Hello Reinmar,

Any updates on the submitted test file?

Thank you

[Piotrek Koszuliński]

Hey. We're sorry, but the entire team is now involved in other tasks. We'll get back to reviewing after releasing CKEditor 4.4.6, so hopefully next week.

[Piotrek Koszuliński]

I've finally find time to review this ticket. The tests were generally good, but I needed to fix their code style. Some notes that I made:

• Switched expected, actual order in assertions.
• Unnecessary intermediate variables (like msg) instead of formatting longer assertions (splitting into multiple lines).
• Insufficient vertical spacing in the code.
• Comments always start with a uppercased letter and end with a period. Also, there should be a space after ““.
• It isn’t required to declare variable with a value.

Also, I needed to add manual tests.

But finally everything in the code was ok, and the tests passed, so I merged this feature to major with ​git:daf04e4.

Huge thanks to everybody involved!

[edithk]

Great news. Thank you very much.