Bug 83668

Summary: Calendar Picker: Support RTL layout
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: aharon, morrita, playmobil, rniwa, webkit.review.bot, xji, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 53961    
Attachments:
Description Flags
Patch
none
Screenshot with Chromium + WebKit ToT
none
Patch 2
none
Screenshot with Patch 2 none

Description Kent Tamura 2012-04-10 23:54:35 PDT
Calendar Picker: Support RTL layout
Comment 1 Kent Tamura 2012-04-11 00:00:43 PDT
Created attachment 136635 [details]
Patch
Comment 2 Kent Tamura 2012-04-11 00:05:53 PDT
Comment on attachment 136635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136635&action=review

> Source/WebCore/Resources/calendarPicker.js:132
> +function isRTLLanguage() {
> +    var lang = getLanguage();
> +    return lang == "ar" || lang == "fa" || lang == "he" || lang == "iw" || lang == "ur" || lang == "yi";
> +}

I'm not sure if this is correct.
I referred to http://code.google.com/p/closure-library/source/browse/trunk/closure/goog/i18n/bidi.js#54

> ManualTests/forms/calendar-picker.html:77
> +};
>  setTimeout(function() {
>      frame.contentWindow.postMessage(JSON.stringify(englishArguments), "*");

You can try the RTL layout by
 1. Replace 'englishArguments' with 'arabicArguments'
 2. Open calendar-picker.html with a WebKit browser.
Comment 3 Kent Tamura 2012-04-12 18:39:39 PDT
Can anyone try the patch and check RTL behavior?
Comment 4 Aharon (Vladimir) Lanin 2012-04-15 00:50:55 PDT
1. When/where is calendarPicker.js used? Can you give a URL where I can see it in action (before the change)?

2. Re the following:

>> Source/WebCore/Resources/calendarPicker.js:132
>> +function isRTLLanguage() {
>> +    var lang = getLanguage();
>> +    return lang == "ar" || lang == "fa" || lang == "he" || lang == "iw" || lang == "ur" || lang == "yi";
>> +}

> I'm not sure if this is correct.
> I referred to http://code.google.com/p/closure-library/source/browse/trunk/closure/goog/i18n/bidi.js#54

This code is extremely limited. It only supports the most widely used RTL language codes, and does not support script codes (e.g. he-Latn, which means Hebrew transliterated into the Latin script, which is obviously LTR) or region codes. Script and/or region code support is essential for some languages that, depending on the country, are usually written in either an LTR or an RTL script. The canonical example is Azerbaijani, which, depending on region, is written in either the Cyrillic or the Latin or the Perso-Arabic script (http://en.wikipedia.org/wiki/Azerbaijani_language#Alphabets), of which only the last is RTL. But there are other languages with the same problem, e.g. Kurdish.

BTW, the goog.i18n.bidi.IS_RTL definition is necessarily limited (and stilted) because for the sake of efficiency it needs to compile down to a constant when goog.LOCALE is specified on the compilation command line. This precludes the use of regular expressions.

For a somewhat more robust locale-to-direction function based on regular expressions see goog.i18n.bidi.isRtlLanguage(), which at least recognizes a few more language codes that are usually written in an RTL script (e.g. Pashto, Sindhi and Maldivian) as well as some script codes. (Unfortunately, however, it is case sensitive - about to fix that.) Nevertheless, this too is only a simplification.
Comment 5 Jeremy Moskovich 2012-04-15 03:48:23 PDT
Can you provide some screenshots with this change?
Comment 6 Kent Tamura 2012-04-15 22:17:15 PDT
(In reply to comment #4)

Thank you for the comments.

> 1. When/where is calendarPicker.js used? Can you give a URL where I can see it in action (before the change)?

This code is used in WebKit implementation of <input type=date>.
You can try this code without building WebKit if you have WebKit source code. (See Comment #2).

> This code is extremely limited. It only supports the most widely used RTL language codes, and does not support script codes (e.g. he-Latn, which means Hebrew transliterated into the Latin script, which is obviously LTR) or region codes. Script and/or region code support is essential for some languages that, depending on the country, are usually written in either an LTR or an RTL script. The canonical example is Azerbaijani, which, depending on region, is written in either the Cyrillic or the Latin or the Perso-Arabic script (http://en.wikipedia.org/wiki/Azerbaijani_language#Alphabets), of which only the last is RTL. But there are other languages with the same problem, e.g. Kurdish.
> 
> BTW, the goog.i18n.bidi.IS_RTL definition is necessarily limited (and stilted) because for the sake of efficiency it needs to compile down to a constant when goog.LOCALE is specified on the compilation command line. This precludes the use of regular expressions.
> 
> For a somewhat more robust locale-to-direction function based on regular expressions see goog.i18n.bidi.isRtlLanguage(), which at least recognizes a few more language codes that are usually written in an RTL script (e.g. Pashto, Sindhi and Maldivian) as well as some script codes. (Unfortunately, however, it is case sensitive - about to fix that.) Nevertheless, this too is only a simplification.

What calendarPicker.js knows is:
 - The default locale code of the browser
 - Some localized text labels for month names and day-of-week names.

The locale code might lack country and variant. It might be just a language.
Should we detect RTL from the script of month/day names?
Comment 7 Kent Tamura 2012-04-15 23:53:17 PDT
Created attachment 137285 [details]
Screenshot with Chromium + WebKit ToT
Comment 8 Kent Tamura 2012-04-15 23:54:50 PDT
(In reply to comment #7)
> Created an attachment (id=137285) [details]
> Screenshot with Chromium + WebKit ToT

This is on OS X + Arabic.
Comment 9 Kent Tamura 2012-04-16 00:50:49 PDT
Created attachment 137295 [details]
Patch 2
Comment 10 Kent Tamura 2012-04-16 00:52:55 PDT
Created attachment 137296 [details]
Screenshot with Patch 2
Comment 11 Aharon (Vladimir) Lanin 2012-04-16 08:01:10 PDT
(In reply to comment #6)
> What calendarPicker.js knows is:
>  - The default locale code of the browser

You are talking about the locale of the browser chrome? If so:

- I presume that the locales supported by Chromium are a known set that caqn not be extended except by a change to Chromium. Am I correct? If so, the set of locales is quite limited and a quick-and-dirty approach would work fine. Or does your use of the term "default" mean that this is not the case? If so, the set of locales is unlimited, and to get 100% correct behavior we need to deal with the region and script codes.

- Are we sure that it makes sense to do the <input type=date> control in the locale of the browser chrome? Would it not make more sense to do it in the locale of the (computed) lang attribute of the <input>? If so, we have an unlimited set of locales again.

>  - Some localized text labels for month names and day-of-week names.

Where do you get this? 

> The locale code might lack country and variant. It might be just a language.

Sure. There has to be a default.

> Should we detect RTL from the script of month/day names?

Nice idea, if the set of locales is in fact unlimited. You can use something like the goog.i18n.bidi.hasAnyRtl()
Comment 12 Kent Tamura 2012-04-16 17:06:53 PDT
(In reply to comment #11)
> (In reply to comment #6)
> > What calendarPicker.js knows is:
> >  - The default locale code of the browser
> 
> You are talking about the locale of the browser chrome? If so:
> 
> - I presume that the locales supported by Chromium are a known set that caqn not be extended except by a change to Chromium. Am I correct? If so, the set of locales is quite limited and a quick-and-dirty approach would work fine. Or does your use of the term "default" mean that this is not the case? If so, the set of locales is unlimited, and to get 100% correct behavior we need to deal with the region and script codes.

Yes, it's the browser locale such as Google Chrome's locale.
However, we can't assume it's limited because the calendar picker might be used by any WebKit browsers.


> - Are we sure that it makes sense to do the <input type=date> control in the locale of the browser chrome? Would it not make more sense to do it in the locale of the (computed) lang attribute of the <input>? If so, we have an unlimited set of locales again.

I'm sure applying the browser locale, not the lang attribute value.  Every form control uses the browser locale at this moment though we might switch to the lang attribute in the future.

> >  - Some localized text labels for month names and day-of-week names.
> 
> Where do you get this? 

From ICU, or OS API.

> > The locale code might lack country and variant. It might be just a language.
> 
> Sure. There has to be a default.
> 
> > Should we detect RTL from the script of month/day names?
> 
> Nice idea, if the set of locales is in fact unlimited. You can use something like the goog.i18n.bidi.hasAnyRtl()

"Patch 2" applied this way.  Using u_charDirection() of ICU.
Comment 13 Hajime Morrita 2012-04-16 23:28:46 PDT
Comment on attachment 137295 [details]
Patch 2

rs=me.
Comment 14 Kent Tamura 2012-04-16 23:42:51 PDT
Comment on attachment 137295 [details]
Patch 2

Let's check in this change and check the behavior of Google Chrome Canary.  Please file a bug if something is wrong.
Comment 15 Aharon (Vladimir) Lanin 2012-04-17 00:15:50 PDT
Ok, sounds good to me.
Comment 16 WebKit Review Bot 2012-04-17 00:31:04 PDT
Comment on attachment 137295 [details]
Patch 2

Clearing flags on attachment: 137295

Committed r114356: <http://trac.webkit.org/changeset/114356>
Comment 17 WebKit Review Bot 2012-04-17 00:31:10 PDT
All reviewed patches have been landed.  Closing bug.