Issue 5404244093960192: Issue 1678 - Added bidi support for Safari (Closed)
|
Created:
Dec. 9, 2014, 6 p.m. by Sebastian Noack Modified:
Dec. 11, 2014, 5:03 p.m. CC:
kzar Visibility:
Public. |
DescriptionIssue 1678 - Added bidi support for Safari
Patch Set 1 #
Total comments: 3
Patch Set 2 : Used Object.create(null) #Patch Set 3 : Ignore region #
Total comments: 7
Patch Set 4 : Addressed comments #
Total comments: 2
MessagesTotal messages: 12
LGTM, addressing the nit below is optional. http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safa... safari/ext/common.js:120: }; Nit: Please consider using Object.create(null) instead of {__proto__:null}, the latter being non-standard.
http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safa... safari/ext/common.js:120: }; On 2014/12/09 18:59:09, Wladimir Palant wrote: > Nit: Please consider using Object.create(null) instead of {__proto__:null}, the > latter being non-standard. I haven't seen through your policy for unrelated changes yet. ;) Done.
LGTM http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safa... safari/ext/common.js:120: }; On 2014/12/09 19:30:16, Sebastian Noack wrote: > I haven't seen through your policy for unrelated changes yet. ;) The policy is very simple - fix the code when you touch it, unless it will complicate the review significantly. :)
I uploaded a new patch set, ignoring the region when checking for RTL languages. Can you review again?
http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... safari/ext/common.js:107: var initCatalog = function(ui_locale) Style nit: I do see why you used underscores here but JavaScript variables shouldn't contain any underscores to keep it consistent with the rest of our code. Same applies to `bidi_dir` below. http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... safari/ext/common.js:109: var bidi_dir = /^(ar|fa|he|ug|ur)_|$/.test(ui_locale) ? "rtl" : "ltr"; This regular expression will match any string due to the `|` at the end. I'd assume you wanted to make the underscore optional. In this case just replace `|` with `?`.
http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... safari/ext/common.js:107: var initCatalog = function(ui_locale) On 2014/12/10 10:29:42, Thomas Greiner wrote: > Style nit: I do see why you used underscores here but JavaScript variables > shouldn't contain any underscores to keep it consistent with the rest of our > code. > > Same applies to `bidi_dir` below. That wasn't an aware decision. It happened rather accidentally when I copied the variable names from the message names. Sure, that should be camel-case. http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... safari/ext/common.js:109: var bidi_dir = /^(ar|fa|he|ug|ur)_|$/.test(ui_locale) ? "rtl" : "ltr"; On 2014/12/10 10:29:42, Thomas Greiner wrote: > This regular expression will match any string due to the `|` at the end. Ouch, I forgot to set parentheses. > assume you wanted to make the underscore optional. In this case just replace `|` > with `?`. Then "ar_" would match while "ar_SA" wouldn't.
http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... safari/ext/common.js:109: var bidi_dir = /^(ar|fa|he|ug|ur)_|$/.test(ui_locale) ? "rtl" : "ltr"; On 2014/12/10 10:37:29, Sebastian Noack wrote: > > assume you wanted to make the underscore optional. In this case just replace > `|` > > with `?`. > > Then "ar_" would match while "ar_SA" wouldn't. The corrected version still matches "ar_". By adding `.` after `_` you could ensure that it won't match. Apart from that it looks good.
http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... safari/ext/common.js:109: var bidi_dir = /^(ar|fa|he|ug|ur)_|$/.test(ui_locale) ? "rtl" : "ltr"; On 2014/12/10 11:12:12, Thomas Greiner wrote: > On 2014/12/10 10:37:29, Sebastian Noack wrote: > > > assume you wanted to make the underscore optional. In this case just replace > > `|` > > > with `?`. > > > > Then "ar_" would match while "ar_SA" wouldn't. > > The corrected version still matches "ar_". By adding `.` after `_` you could > ensure that it won't match. Apart from that it looks good. Why should we care? We want to match "ar" and "ar_SA". "ar_" is a theoretical case we can ignore. And even if it occurs, it can still be considered a variant of Arabic, where we have to apply right-to-left.
LGTM http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safa... safari/ext/common.js:109: var bidi_dir = /^(ar|fa|he|ug|ur)_|$/.test(ui_locale) ? "rtl" : "ltr"; On 2014/12/10 11:32:06, Sebastian Noack wrote: > On 2014/12/10 11:12:12, Thomas Greiner wrote: > > On 2014/12/10 10:37:29, Sebastian Noack wrote: > > > > assume you wanted to make the underscore optional. In this case just > replace > > > `|` > > > > with `?`. > > > > > > Then "ar_" would match while "ar_SA" wouldn't. > > > > The corrected version still matches "ar_". By adding `.` after `_` you could > > ensure that it won't match. Apart from that it looks good. > > Why should we care? We want to match "ar" and "ar_SA". "ar_" is a theoretical > case we can ignore. And even if it occurs, it can still be considered a variant > of Arabic, where we have to apply right-to-left. I can agree with that line of argument.
LGTM http://codereview.adblockplus.org/5404244093960192/diff/5161210659995648/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5161210659995648/safa... safari/ext/common.js:109: var bidiDir = /^(ar|fa|he|ug|ur)(_|$)/.test(uiLocale) ? "rtl" : "ltr"; I only realized now that we are looking at UI locale here rather than our locale. What will happen if the UI locale is "ur" and we don't have that translation? I guess that the user will see the fallback to English yet rendered as RTL. Given that his browser UI is RTL, this might even make sense... But I guess that this is a purely theoretical discussion. Sebastian mentioned that Safari only supports ar and he, we have both of them covered.
http://codereview.adblockplus.org/5404244093960192/diff/5161210659995648/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5161210659995648/safa... safari/ext/common.js:109: var bidiDir = /^(ar|fa|he|ug|ur)(_|$)/.test(uiLocale) ? "rtl" : "ltr"; On 2014/12/11 16:52:15, Wladimir Palant wrote: > I only realized now that we are looking at UI locale here rather than our > locale. What will happen if the UI locale is "ur" and we don't have that > translation? I guess that the user will see the fallback to English yet rendered > as RTL. Given that his browser UI is RTL, this might even make sense... > > But I guess that this is a purely theoretical discussion. Sebastian mentioned > that Safari only supports ar and he, we have both of them covered. To be honest I didn't consider that case and agree that this code is theoretically wrong, in terms that it considers the language of the browser, instead the language of the translation used. However, as you say, currently the only right-to-left languages supported by OS X are Arabic and Hebrew. So this case won't happen in practice. Also note that considering the language of the translation would require some additional complexity since translations are loaded lazily on first use. 
|
