Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(94)

Issue 5404244093960192: Issue 1678 - Added bidi support for Safari (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 2 months ago by Sebastian Noack
Modified:
5 years, 2 months ago
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
5 years, 2 months ago (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: ...
5 years, 2 months ago (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: ...
5 years, 2 months ago (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: > ...
5 years, 2 months ago (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 ...
5 years, 2 months ago (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 ...
5 years, 2 months ago (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 ...
5 years, 2 months ago (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 ...
5 years, 2 months ago (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 ...
5 years, 2 months ago (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"; ...
5 years, 2 months ago (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"; ...
5 years, 2 months ago (2014-12-11 16:52:15 UTC) #11
Sebastian Noack
5 years, 2 months ago (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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5