Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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.

Description

Issue 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -1 line) Patch
M safari/ext/common.js View 1 2 3 1 chunk +12 lines, -1 line 2 comments Download

Messages

Total messages: 12
Sebastian Noack
Dec. 9, 2014, 6:01 p.m. (2014-12-09 18:01:44 UTC) #1
Wladimir Palant
LGTM, addressing the nit below is optional. http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safari/ext/common.js#newcode120 safari/ext/common.js:120: }; Nit: ...
Dec. 9, 2014, 6:59 p.m. (2014-12-09 18:59:09 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safari/ext/common.js#newcode120 safari/ext/common.js:120: }; On 2014/12/09 18:59:09, Wladimir Palant wrote: > Nit: ...
Dec. 9, 2014, 7:30 p.m. (2014-12-09 19:30:16 UTC) #3
Wladimir Palant
LGTM http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5629499534213120/safari/ext/common.js#newcode120 safari/ext/common.js:120: }; On 2014/12/09 19:30:16, Sebastian Noack wrote: > ...
Dec. 9, 2014, 7:55 p.m. (2014-12-09 19:55:37 UTC) #4
Sebastian Noack
I uploaded a new patch set, ignoring the region when checking for RTL languages. Can ...
Dec. 10, 2014, 8:17 a.m. (2014-12-10 08:17:38 UTC) #5
Thomas Greiner
http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safari/ext/common.js#newcode107 safari/ext/common.js:107: var initCatalog = function(ui_locale) Style nit: I do see ...
Dec. 10, 2014, 10:29 a.m. (2014-12-10 10:29:42 UTC) #6
Sebastian Noack
http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safari/ext/common.js#newcode107 safari/ext/common.js:107: var initCatalog = function(ui_locale) On 2014/12/10 10:29:42, Thomas Greiner ...
Dec. 10, 2014, 10:37 a.m. (2014-12-10 10:37:29 UTC) #7
Thomas Greiner
http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safari/ext/common.js#newcode109 safari/ext/common.js:109: var bidi_dir = /^(ar|fa|he|ug|ur)_|$/.test(ui_locale) ? "rtl" : "ltr"; On ...
Dec. 10, 2014, 11:12 a.m. (2014-12-10 11:12:11 UTC) #8
Sebastian Noack
http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safari/ext/common.js#newcode109 safari/ext/common.js:109: var bidi_dir = /^(ar|fa|he|ug|ur)_|$/.test(ui_locale) ? "rtl" : "ltr"; On ...
Dec. 10, 2014, 11:32 a.m. (2014-12-10 11:32:06 UTC) #9
Thomas Greiner
LGTM http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5676830073815040/safari/ext/common.js#newcode109 safari/ext/common.js:109: var bidi_dir = /^(ar|fa|he|ug|ur)_|$/.test(ui_locale) ? "rtl" : "ltr"; ...
Dec. 10, 2014, 12:06 p.m. (2014-12-10 12:06:02 UTC) #10
Wladimir Palant
LGTM http://codereview.adblockplus.org/5404244093960192/diff/5161210659995648/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/5404244093960192/diff/5161210659995648/safari/ext/common.js#newcode109 safari/ext/common.js:109: var bidiDir = /^(ar|fa|he|ug|ur)(_|$)/.test(uiLocale) ? "rtl" : "ltr"; ...
Dec. 11, 2014, 4:52 p.m. (2014-12-11 16:52:15 UTC) #11
Sebastian Noack
Dec. 11, 2014, 5:03 p.m. (2014-12-11 17:03:05 UTC) #12
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.

Powered by Google App Engine
This is Rietveld