Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#5909 closed New Feature (fixed)

BIDI: Support for switching base language is required

Reported by: Damian Owned by: Tobiasz Cudnik
Priority: Normal Milestone: CKEditor 3.4
Component: General Version:
Keywords: IBM Review+ Cc: Satya Minnekanti, joek

Description

The editor should provide an easy method for a user to switch the base language direction of a paragraph or other block level element.

Switching a paragraph should behave like the block level styles and set the dir attribute on the enclosing <p>.

Two new buttons in the toolbar could be used to switch or set base language direction.

Attachments (7)

5909.patch (3.2 KB) - added by Tobiasz Cudnik 14 years ago.
5909_2.patch (5.1 KB) - added by Tobiasz Cudnik 14 years ago.
5909_3.patch (7.7 KB) - added by Tobiasz Cudnik 14 years ago.
5909_4.patch (8.0 KB) - added by Tobiasz Cudnik 14 years ago.
5909_5.patch (10.0 KB) - added by Tobiasz Cudnik 14 years ago.
5909_6.patch (10.7 KB) - added by Tobiasz Cudnik 14 years ago.
5909_7.patch (10.6 KB) - added by Tobiasz Cudnik 14 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 14 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

Changed 14 years ago by Tobiasz Cudnik

Attachment: 5909.patch added

comment:2 Changed 14 years ago by Tobiasz Cudnik

Keywords: Review? added

Changed 14 years ago by Tobiasz Cudnik

Attachment: 5909_2.patch added

comment:3 Changed 14 years ago by Tobiasz Cudnik

Second patch makes the new plugin a default one.

comment:4 Changed 14 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

First, the small details:

  • Let's be simpler, and just call the plugin "bidi", simply. Note that "switch" is not a valid thing for this feature (other than being too generic), as you can set and remove the direction by using the same button.
  • Yes, the plugin list in the configuration file is alphabetically ordered. So, please place the plugin name in the right place.
  • The language entries can be more understandable: "Text direction from left to right".
  • The icons are not showing up in the toolbar. Please note that the images are already on the strip. It's just a matter of setting the CSS files properly.
  • Just like the toolbar items, let's call the commands: bidiltr and bidirtl.

Then, the feature itself:

  • The implementation has been too simplistic. It doesn't handle properly the selection of some elements, like <table>, <ul>, <ol>, <blockquote> and <div>. If those elements are fully selected, the direction must be applied on them direction, not on each of there children.
  • On onSelectionChange, the command.setState() function must be used, which handles the even firing properly.
  • Inside the bidiCommand function, the bookmarks must be created only if ranges is defined.

Changed 14 years ago by Tobiasz Cudnik

Attachment: 5909_3.patch added

comment:5 Changed 14 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

comment:6 Changed 14 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The language file entries are still under the "bidiswitch" object. It should be named "bidi".

The following TCs are not working:

  1. Create a list.
  2. Click inside the list.
  3. Click the "ul" element in the elements path to select the entire list.
  4. Click the RTL button.

Nothing happens. It happens for almost every block element, including p, table, blockquote, etc. It does nothing if the selection is not "inside" the element.

The above tc with a table also doesn't work as expected when clicking the "tr" element of the table. All table cells get changed instead of the selected row only,

Another tc:

  1. Load this HTML:
<p>
	Para</p>
<ul>
	<li>
		Item 1</li>
	<li>
		Item 2</li>
	<li>
		Item 3</li>
</ul>
<blockquote>
	<p>
		Quote para 1.</p>
	<p>
		Quote para 2.</p>
	<p>
		Quote para 3.</p>
</blockquote>
<table border="1" cellpadding="1" cellspacing="1" style="width: 200px;">
	<tbody>
		<tr>
			<td>
				a</td>
			<td>
				b</td>
		</tr>
		<tr>
			<td>
				c</td>
			<td>
				d</td>
		</tr>
		<tr>
			<td>
				e</td>
			<td>
				f</td>
		</tr>
	</tbody>
</table>
  1. Hit CTRL+A to select all.
  2. Click the RTL button.

The dir attribute is applied to all internal elements of <ul>, <blockquote> and <table>, instead of being applied to those elements only.

Changed 14 years ago by Tobiasz Cudnik

Attachment: 5909_4.patch added

comment:7 Changed 14 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

Fourth patch addresses the cases mentioned above.

comment:8 Changed 14 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

This patch is missing the toolbar entries. I've taken it from the previous patch.

Ragarding my previous comment, while the first TC now works perfectly, the second one is still reproducible.

Changed 14 years ago by Tobiasz Cudnik

Attachment: 5909_5.patch added

comment:9 Changed 14 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

I know it may be not the most efficient, but neither snow solution which fixes the second TC.

comment:10 Changed 14 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The feature is becoming better and better on each new patch ;)

There is stil la tc we may consider, as it may be related to an intrinsic logic bug in the code:

  1. Load this HTML:
<blockquote>
	<p>
		Quote para 1.</p>
	<p>
		Quote para 2.</p>
	<p>
		Quote para 3.</p>
</blockquote>
  1. Click inside the second paragraph.
  2. Click on the <p> element in the elements path.
  3. Click the RTL button.

Note that dir is applied to the entire blockquote, instead of that single paragraph only.

The same happens on lists, when clicking the <li> element in the elements path.

comment:11 Changed 14 years ago by Frederico Caldeira Knabben

This patch is also broken with IE.

Changed 14 years ago by Tobiasz Cudnik

Attachment: 5909_6.patch added

comment:12 Changed 14 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

I had to do some additional changes to the link plugin to get this working. The last TC works for me in IE now.

comment:13 Changed 14 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

Things worked very well now. I was about to give a R+ to this, but I decided to make a final test. So, I've copied the contents of the HTML specs main page at W3C, pasted it into the editor, selected all and finally set the direction to RTL. I've noticed that some paragraphs didn't get the direction.

So, here is a reduced tc for the above issue:

  1. Load this HTML:
<div>
	<p>
		Para 1</p>
</div>
<p>
	Para 2</p>
  1. Hit CTRL+A to select all.
  2. Click the RTL button.

Note that the last paragraph doesn't get the direction applied.

Changed 14 years ago by Tobiasz Cudnik

Attachment: 5909_7.patch added

comment:14 Changed 14 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

There were some logical mistakes in the sixth patch.

The new one additionally groups language switching logic in one place, making it easier for adding new functions in the future.

comment:15 Changed 14 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

There is a "dirSwitch" variable being assigned at line 47, but it's not used at all. Please remove it before committing.

comment:16 Changed 14 years ago by Frederico Caldeira Knabben

Please commit it into the 3.4.x branch.

comment:17 Changed 14 years ago by Tobiasz Cudnik

Resolution: fixed
Status: assignedclosed

Fixed with [5678].

comment:18 Changed 14 years ago by Sa'ar Zac Elias

The plugin file commiting went wrong, and the language files were not updated as well, fixed with [5688] and [5689].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy