Issue 5137457368530944: Issue 1668 - Fixed direction on add-on pages for right-to-left scripts (Closed)
|
Created:
Dec. 8, 2014, 7:04 p.m. by Sebastian Noack Modified:
Dec. 10, 2014, 8:24 a.m. Visibility:
Public. |
Patch Set 1 #
Total comments: 4
Patch Set 2 : Used -(webkit|moz)-border-start #
Total comments: 12
Patch Set 3 : Also consider Persian #Patch Set 4 : Use browser APIs do determine direction #
MessagesTotal messages: 11
http://codereview.adblockplus.org/5137457368530944/diff/5629499534213120/chro... File chrome/skin/firstRun.css (right): http://codereview.adblockplus.org/5137457368530944/diff/5629499534213120/chro... chrome/skin/firstRun.css:255: direction: ltr; /* don't mess with left/right border */ Apparently, Chrome and Safari have a -webkit-border-start CSS property. So the proper solution would be using -moz-border-start/-webkit-border-start instead of border-left and -moz-border-end/-webkit-border-end instead of border-right.
http://codereview.adblockplus.org/5137457368530944/diff/5629499534213120/chro... File chrome/skin/firstRun.css (right): http://codereview.adblockplus.org/5137457368530944/diff/5629499534213120/chro... chrome/skin/firstRun.css:255: direction: ltr; /* don't mess with left/right border */ On 2014/12/08 20:35:09, Wladimir Palant wrote: > Apparently, Chrome and Safari have a -webkit-border-start CSS property. So the > proper solution would be using -moz-border-start/-webkit-border-start instead of > border-left and -moz-border-end/-webkit-border-end instead of border-right. Nice. Note that I used it the other way around (i.e. "-*-border-start-style: none"), for two reasons: 1. In case -*-border-start isn't supported we just get double borders in the middle instead omitting the order at the nd, which is still symmetrical. 2. That way I don't have to hardcode the border settings twice.
http://codereview.adblockplus.org/5137457368530944/diff/5629499534213120/chro... File chrome/skin/firstRun.css (right): http://codereview.adblockplus.org/5137457368530944/diff/5629499534213120/chro... chrome/skin/firstRun.css:255: direction: ltr; /* don't mess with left/right border */ On 2014/12/08 20:54:58, Sebastian Noack wrote: > On 2014/12/08 20:35:09, Wladimir Palant wrote: > > Apparently, Chrome and Safari have a -webkit-border-start CSS property. So the > > proper solution would be using -moz-border-start/-webkit-border-start instead > of > > border-left and -moz-border-end/-webkit-border-end instead of border-right. > > Nice. Note that I used it the other way around (i.e. "-*-border-start-style: > none"), for two reasons: > > 1. In case -*-border-start isn't supported we just get double borders in the > middle instead omitting the order at the nd, which is still symmetrical. > > 2. That way I don't have to hardcode the border settings twice. s/order at the nd/border at the end/
LGTM http://codereview.adblockplus.org/5137457368530944/diff/5629499534213120/chro... File chrome/skin/firstRun.css (right): http://codereview.adblockplus.org/5137457368530944/diff/5629499534213120/chro... chrome/skin/firstRun.css:255: direction: ltr; /* don't mess with left/right border */ On 2014/12/08 20:54:58, Sebastian Noack wrote: > Nice. Note that I used it the other way around (i.e. "-*-border-start-style: > none"), for two reasons: Yes, this is indeed better.
http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/content/ui/i18n.js:24: locale = ext.i18n.getMessage("@@ui_locale").replace(/_/g, "-"); What about Safari? We don't handle those predefined messages there yet so we would need to implement those first in safari/ext/common.js http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/content/ui/i18n.js:154: if (locale == "ar" || locale == "he") Similarly to above we could use `ext.i18n.getMessage("@@bidi_dir")` to get the text direction. See https://developer.chrome.com/extensions/i18n#overview-predefined http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... File chrome/skin/firstRun.css (right): http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/skin/firstRun.css:313: -moz-border-start-style: none; If you use prefixed properties, please also add the unprefixed version below.
http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/content/ui/i18n.js:24: locale = ext.i18n.getMessage("@@ui_locale").replace(/_/g, "-"); On 2014/12/09 10:29:04, Thomas Greiner wrote: > What about Safari? We don't handle those predefined messages there yet so we > would need to implement those first in safari/ext/common.js We do handle @@ui_locale on Safari. http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/content/ui/i18n.js:154: if (locale == "ar" || locale == "he") On 2014/12/09 10:29:04, Thomas Greiner wrote: > Similarly to above we could use `ext.i18n.getMessage("@@bidi_dir")` to get the > text direction. See > https://developer.chrome.com/extensions/i18n#overview-predefined Is there a way to get the direction on Firefox? Otherwise I'd prefer to keep the check here, instead duplicating it for Firefox and Safari, in the receptive abstraction code. http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... File chrome/skin/firstRun.css (right): http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/skin/firstRun.css:313: -moz-border-start-style: none; On 2014/12/09 10:29:04, Thomas Greiner wrote: > If you use prefixed properties, please also add the unprefixed version below. There is no unprefixed version. I couldn't even find a draft to get this into the standard. So if it will ever make it into the standard, chances are that the semantics will differ anyway.
http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/content/ui/i18n.js:24: locale = ext.i18n.getMessage("@@ui_locale").replace(/_/g, "-"); On 2014/12/09 10:41:13, Sebastian Noack wrote: > On 2014/12/09 10:29:04, Thomas Greiner wrote: > > What about Safari? We don't handle those predefined messages there yet so we > > would need to implement those first in safari/ext/common.js > > We do handle @@ui_locale on Safari. Sorry, missed that before. But why not simply use `Utils.appLocale`? Because that's exactly the same code as used here and you are already using it in the Firefox-specific code below anyway. http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/content/ui/i18n.js:154: if (locale == "ar" || locale == "he") On 2014/12/09 10:41:13, Sebastian Noack wrote: > On 2014/12/09 10:29:04, Thomas Greiner wrote: > > Similarly to above we could use `ext.i18n.getMessage("@@bidi_dir")` to get the > > text direction. See > > https://developer.chrome.com/extensions/i18n#overview-predefined > > Is there a way to get the direction on Firefox? Otherwise I'd prefer to keep the > check here, instead duplicating it for Firefox and Safari, in the receptive > abstraction code. It could be handled the same way as "@@ui_locale" but I guess for now this should do. BTW in Firefox there are preferences of the form `intl.uidirection.<locale>` (as used in https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-locale-dir(rtl)). They are only set for the locales "ar", "fa", "he", "ug" and "ur". A Firefox-specific version would then look like this: var dir = "ltr"; if (Services.prefs.getPrefType("intl.uidirection." + locale)) dir = Services.prefs.getCharPref("intl.uidirection." + locale; http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... File chrome/skin/firstRun.css (right): http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/skin/firstRun.css:313: -moz-border-start-style: none; On 2014/12/09 10:41:13, Sebastian Noack wrote: > On 2014/12/09 10:29:04, Thomas Greiner wrote: > > If you use prefixed properties, please also add the unprefixed version below. > > There is no unprefixed version. I couldn't even find a draft to get this into > the standard. So if it will ever make it into the standard, chances are that the > semantics will differ anyway. Ok, sounds reasonable.
http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/content/ui/i18n.js:154: if (locale == "ar" || locale == "he") On 2014/12/09 16:51:56, Thomas Greiner wrote: > A Firefox-specific version would then look like this: > > var dir = "ltr"; > if (Services.prefs.getPrefType("intl.uidirection." + locale)) > dir = Services.prefs.getCharPref("intl.uidirection." + locale; Utils.chromeRegistry.isLocaleRTL("adblockplus") should be better. Internally it will do exactly the same thing but we don't get to rely on Firefox implementation details.
http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/content/ui/i18n.js:24: locale = ext.i18n.getMessage("@@ui_locale").replace(/_/g, "-"); On 2014/12/09 16:51:56, Thomas Greiner wrote: > On 2014/12/09 10:41:13, Sebastian Noack wrote: > > On 2014/12/09 10:29:04, Thomas Greiner wrote: > > > What about Safari? We don't handle those predefined messages there yet so we > > > would need to implement those first in safari/ext/common.js > > > > We do handle @@ui_locale on Safari. > > Sorry, missed that before. But why not simply use `Utils.appLocale`? Because > that's exactly the same code as used here and you are already using it in the > Firefox-specific code below anyway. Because in block.html on Chrome/Opera/Safari there is no require(), and therefore no Utils. http://codereview.adblockplus.org/5137457368530944/diff/5724160613416960/chro... chrome/content/ui/i18n.js:154: if (locale == "ar" || locale == "he") On 2014/12/09 16:59:08, Wladimir Palant wrote: > On 2014/12/09 16:51:56, Thomas Greiner wrote: > > A Firefox-specific version would then look like this: > > > > var dir = "ltr"; > > if (Services.prefs.getPrefType("intl.uidirection." + locale)) > > dir = Services.prefs.getCharPref("intl.uidirection." + locale; > > Utils.chromeRegistry.isLocaleRTL("adblockplus") should be better. Internally it > will do exactly the same thing but we don't get to rely on Firefox > implementation details. Done. I'm going to implement @@bidi_dir for Safari.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
