Hey there,

I uploaded to the Captcha fonts folder an Hebrew ttf file, the module show the letters correctly and actually recognize them as it should, the only problem though is that the validation is being run from Left to Right instead of Right to Left.

So for example, if It's written 'חתול' at the image it expect me to write 'לותח' (if it was English it would expect me to type 'tac' for an image which display 'cat').

What can be done to fix this issue?

Thanks!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

soxofaan’s picture

Version: 6.x-1.0-rc2 » 6.x-2.x-dev
Category: bug » feature
Status: Active » Postponed

Interesting use case.

This should be possible to solve in a clean way when #473002: Provide a way to define custom CAPTCHA validation is fixed

soxofaan’s picture

Status: Postponed » Needs review
FileSize
2.33 KB

does this patch fix it for you? I can't really test it because I don't speak right to left ;)

The patch makes the rendering of the code language direction dependent (using the current language direction of the current user), instead of doing the validation language direction dependent.

soxofaan’s picture

Issue tags: +RTL, +hebrew
FileSize
1.8 KB

is there any interest in this issue/patch?
I can't really test this, as I am not very familiar with RTL support.

tagging and rerolling too

soxofaan’s picture

FileSize
2.41 KB

reroll

soxofaan’s picture

reroll

ahwebd’s picture

Title: Right to Left fonts validation » image_captcha RTL support
Issue tags: -hebrew
FileSize
1.63 KB

Here's a patch to add rtl support, I'm using it in my sites and it is working fine, it will add an option in admin page to make characters in captcha image start from the right if the current language is RTL.
(note: this patch was made against version: 6.x-2.2)

Status: Needs review » Needs work

The last submitted patch, image_captcha_rtl_support.patch, failed testing.

soxofaan’s picture

Status: Needs work » Needs review
FileSize
1.8 KB

hi Ahmed,
Why making it an option?
Does it make sense to not do RTL when the current language is RTL?

anyway, reroll of patch from #5

ahwebd’s picture

Hi soxofaan,

yes it should be an option, mainly because rtl image captcha won't work out of the box for rtl sites, the site admin needs to add a font that supports his language, and for multilingual sites he needs to set 'image_captcha_image_allowed_chars' as a multilingual variable in settings.php to allow for different character sets for each language. You can't use latin characters for an rtl language because typing latin characters in a text box will result in them being ordered from left to right (will confuse the user) as opposed to arabic characters for example.

I'll check your patch using character position in the image, but I think using array_reverse is cleaner and more straight.

soxofaan’s picture

#6: image_captcha_rtl_support.patch queued for re-testing.

soxofaan’s picture

Hi Ahmed,

Thanks for your feedback. I'm not familiar with RTL languages, so this is a bit unknown territory for me.

If I read your comment, it appears that there is much more to consider than just rendering the characters from right to left. Is the patch enough or should there be more change than just the rendering, e.g. language dependent font selection?

ahwebd’s picture

Hi soxofaan,

Is the patch enough or should there be more change than just the rendering

The patch is all what is needed from the module for rtl support, we can't bypass some steps to be done by the site admin (or more accurately, not practical) by adding more changes to the module.

Well, for single-language site the site admin just needs to ensure the font used supports the site language then in the allowed characters field he changes it with characters from site language (in my case the supplied fonts don't support arabic, so I need to add another font, otherwise arabic characters will appear as rectangular boxes in the image) .
We can't add fonts to the module to cater for all languages as this will make the module size too big.

for a multilingual site the site admin needs also to set 'image_captcha_image_allowed_chars' as a multilingual variable in settings.php , see here , this will allow him to select for example latin characters for "english" and arabic characters for arabic in field :"Characters to use in the code".

language dependent font selection?

No need, the site admin can choose one (or more) font that supports all languages of the site. (generally rtl fonts support also latin characters)

Here's a patch for version 6.x-2.3 , I removed the css that makes font previews appear in multi-columns because it includes LTR specific style ( float: left; ) and would make previews appear wrong in an RTL site, also I prefer them in one column, if you prefer the other way we can add another css file for rtl and include it.

For people using Arabic I'm attaching a free font that supports arabic (it might support other languages)
(uncompress and rename to FreeSerif.ttf then put in "sites/all/libraries/fonts")

soxofaan’s picture

hi ahwebd,

What is the licencing of that FreeSerif font? We can't just package any font with the CAPTCA module. For more info see: #497822: Drupal.org CVS policy regarding fonts and #259219: provide a better default font for image CAPTCHA.

About the CSS-tweak you did: is there in case of RTL mode a special "rtl" related class on the body element, so we could switch to right floating in that case (instead of disabling floating everywhere)?

ahwebd’s picture

What is the licencing of that FreeSerif font?

GPLv3 (see here) , However I did not mean to include it in the module (its size is a little big (1.2M))
Attached below two small size fonts that support both arabic and latin characters, they are licensed under GPLv2 ( they were downloaded and extracted from here (the package includes licensing info) )
these fonts were tested and behaved nicely with image_captcha as opposed to some other fonts where sometimes parts of the letters go outside the image (can you increase the space between the letters and the image borders to resolve this issue with some fonts)

About the CSS-tweak you did: is there in case of RTL mode a special "rtl" related class on the body element, so we could switch to right floating in that case (instead of disabling floating everywhere)?

in patch below there is an added css file for RTL

soxofaan’s picture

Status: Needs review » Needs work
Issue tags: +low-hanging fruit

Hi ahweb,

Thanks for the update,
Apparently, we don't have to do

global $language;
  if ($language->direction) {
    drupal_add_css(drupal_get_path('module', 'image_captcha') .'/css/image_captcha_rtl.css');
  }

because Drupal does this automatically: see http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ad...

I would also keep the CSS files where there are instead of moving them to a new directory. It makes the patch review harder. If you look to all modules of Drupal core, you'll see that the all css files are at the same level of the .module files and not in a dedicated directory.

ahwebd’s picture

Status: Needs work » Needs review
FileSize
2.1 KB

Thanks soxofaan,
new patch

soxofaan’s picture

Hi Ahmed,

thanks for the update.

I was thinking about user interface optimization/streamlining: maybe we should only show the option for RTL mode when at least one of the languages used on the site is an RTL language? What do you think?

also, I noticed some inconsistency in your patch:

// in image_captcha.admin.inc
variable_get('image_captcha_direction', 0);
// in image_captcha.user.inc
variable_get('image_captcha_direction', FALSE);

Apart form that, I would also call the variable "image_captcha_rtl_support" or something instead of "image_captcha_direction", because LTR language, not unlike RTL ones, have a direction too :)

ahwebd’s picture

Hello soxofaan,

maybe we should only show the option for RTL mode when at least one of the languages used on the site is an RTL language? What do you think?

feature included in the new patch

also, I noticed some inconsistency in your patch

the use context is different:
assuming the 'image_captcha_direction' variable is not set;
in "image_captcha.admin.inc" I wanted a return value of "0" to put in '#default_value' of the form while in "image_captcha.user.inc" I wanted a boolean return value to use in the "if" statement, anyway I modified the code so they look the same (end result is the same, php treats 0 as false)

I would also call the variable "image_captcha_rtl_support" or something instead of "image_captcha_direction"

This variable defines the direction of the letters in the image (from right to left or from left to right) thus its name.
The name "image_captcha_direction" describes the variable purpose exactly in the same way as "$language->direction" does, (if the value is 1 then rtl, if 0 then ltr)

soxofaan’s picture

FileSize
3.69 KB

Hi Ahmed,

I tried to merge the patch from #8 with your work, see attachment.

Nice trick to detect if there are RTL languages, by the way.

I still disagree about "direction" being a good name for a variable with boolean interpretation (it's represented as a checkbox). (Note that the values for $language->direction in Drupal core come from named constants LANGUAGE_LTR and LANGUAGE_RTL).
I decided to use the longer but more clear "image_captcha_rtl_support" instead.

Anyway, if you have no further remarks on this patch (and the test bot is happy with it), I'd like to commit it.

ahwebd’s picture

Status: Needs review » Reviewed & tested by the community

patch tested, working nicely

soxofaan’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks for testing, committed to Drupal 6 version: http://drupal.org/cvs?commit=457086

To be ported to Drupal 7 version

ahwebd’s picture

Issue tags: -low-hanging fruit

Hello soxofaan,
any news about port to D7 version ?

soxofaan’s picture

Issue tags: +low-hanging fruit

hi Ahmed,

I don't have time to do it myself, but this would be a nice task for the co-maintainers I'm looking for, so setting back the issue tag.

hejazee’s picture

Status: Patch (to be ported) » Needs review
FileSize
3.31 KB

This is #19 but using git. (works well)

Status: Needs review » Needs work

The last submitted patch, image_captcha-RTL-472450-24.patch, failed testing.

hejazee’s picture

Status: Needs work » Reviewed & tested by the community
soxofaan’s picture

Issue tags: -RTL, -low-hanging fruit

#24: image_captcha-RTL-472450-24.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +RTL, +low-hanging fruit

The last submitted patch, image_captcha-RTL-472450-24.patch, failed testing.

hejazee’s picture

Status: Needs work » Patch (to be ported)

I have created this patch using git
git diff > output-file.patch

So I don't know why it fails testing.
Anyway the patch works and solves the RTL problem.

I'm changing the issue status to "patch to be ported" as it was before my patch.

soxofaan’s picture

Status: Patch (to be ported) » Closed (fixed)

committed to d7