Bug 83668

Created attachment 136635 [details] Patch

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.

Can anyone try the patch and check RTL behavior?

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.

Can you provide some screenshots with this change?

(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?

Created attachment 137285 [details] Screenshot with Chromium + WebKit ToT

(In reply to comment #7) > Created an attachment (id=137285) [details] > Screenshot with Chromium + WebKit ToT This is on OS X + Arabic.

Created attachment 137295 [details] Patch 2

Created attachment 137296 [details] Screenshot with Patch 2

(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()

(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 on attachment 137295 [details] Patch 2 rs=me.

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.

Ok, sounds good to me.

Comment on attachment 137295 [details] Patch 2 Clearing flags on attachment: 137295 Committed r114356: <http://trac.webkit.org/changeset/114356>

All reviewed patches have been landed. Closing bug.