Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#5909 closed New Feature (fixed)

BIDI: Support for switching base language is required

Reported by: damo Owned by: tobiasz.cudnik
Priority: Normal Milestone: CKEditor 3.4
Component: General Version:
Keywords: IBM Review+ Cc: satya, 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 6 years ago.
5909_2.patch (5.1 KB) - added by tobiasz.cudnik 6 years ago.
5909_3.patch (7.7 KB) - added by tobiasz.cudnik 6 years ago.
5909_4.patch (8.0 KB) - added by tobiasz.cudnik 6 years ago.
5909_5.patch (10.0 KB) - added by tobiasz.cudnik 6 years ago.
5909_6.patch (10.7 KB) - added by tobiasz.cudnik 6 years ago.
5909_7.patch (10.6 KB) - added by tobiasz.cudnik 6 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 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:2 Changed 6 years ago by tobiasz.cudnik

  • Keywords Review? added

Changed 6 years ago by tobiasz.cudnik

comment:3 Changed 6 years ago by tobiasz.cudnik

Second patch makes the new plugin a default one.

comment:4 Changed 6 years ago by fredck

  • 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 6 years ago by tobiasz.cudnik

comment:5 Changed 6 years ago by tobiasz.cudnik

  • Keywords Review? added; Review- removed

comment:6 Changed 6 years ago by fredck

  • 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 6 years ago by tobiasz.cudnik

comment:7 Changed 6 years ago by tobiasz.cudnik

  • Keywords Review? added; Review- removed

Fourth patch addresses the cases mentioned above.

comment:8 Changed 6 years ago by fredck

  • 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 6 years ago by tobiasz.cudnik

comment:9 Changed 6 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 6 years ago by fredck

  • 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 6 years ago by fredck

This patch is also broken with IE.

Changed 6 years ago by tobiasz.cudnik

comment:12 Changed 6 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 6 years ago by fredck

  • 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 6 years ago by tobiasz.cudnik

comment:14 Changed 6 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 6 years ago by fredck

  • 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 6 years ago by fredck

Please commit it into the 3.4.x branch.

comment:17 Changed 6 years ago by tobiasz.cudnik

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

Fixed with [5678].

comment:18 Changed 6 years ago by Saare

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 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy