597296 - [RTL] Punctuation placed at the end in the awesome bar will show as the first character

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b7pre)Gecko/20100916 Firefox/4.0b7pre Fennec/2.0b1pre

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100916 Firefox/Fennec/2.0b1pre

1. Use the Arabic plugin and set the browser to arabic
2. type in http://www.yahoo.com/

Expected: 
1. The url will remain http://www.yahoo.com/
2. The awesomebar will change to Yahoo!

Actual:
1. The url will change to /http://www.yahoo.com
2. The awesomebar will change to !Yahoo

Note:
1. happens both one Mac Fennec and Maemo 5 GTK

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: bug in control panel screenshot

Also occurs in the Restart to complete changes when switching from arabic to english language.  see attached screenshot.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Vivien - is this a front-end bug or a platfrom bug?

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

(In reply to comment #2)
> Vivien - is this a front-end bug or a platfrom bug?

This sounds like a platform bug because the case in comment #1 does not depend on a user input and is normally a non-dynamic string. There is no reason for it to display differently.

The attached patch add a uri-element class to the urlbar to force it in ltr mode (which fix this bug) because this is the expected behavior for a textbox containing uri element in RTL (see bug 477842)

The css rule add in browser.css for the preferences panel is a workaround, I think the relevant platform bug is bug 582181 or bug 505685.

I wonder if they are duplicate or not?

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I think we should fix at least the urlbar uri-element class here since this is pretty easy and url's fields have to be ltr even in RTL

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 479355 [details] [diff] [review]
Patch

>diff -r 87fecae832b3 chrome/content/browser.xul

>                     <textbox id="urlbar-edit"
>                              type="autocomplete"
>+                             class="uri-element"

Yes, let's do this

>diff -r 87fecae832b3 themes/core/browser.css

>+/* preferences panel UI   -------------------------------------------------- */
>+
>+/* bug 597296 */
>+#prefs-languages * {
>+  direction: ltr !important;
>+}

No, let's skip this for now

r+ with the CSS dropped

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/5724d8d90ef6

I assume I should keep this bug open while the rest is not fix (by any means)?

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.1 (m-c)

Platform part of the bug that prevent a newly selected locale to have an effect on the strings until the browser is restarted or a call to nsIXULRegistry.reloadChrome() is done.

This resolve the part of the bug where the restart notification is in the newly selected language instead of staying into the old one

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.1 (Front-End) - Menulist

This patch resolve the "(English(US"  string inside the menulist, I think we should force LTR here since all the langs name are in a file not translated (http://mxr.mozilla.org/mobile-browser/source/chrome/content/languages.properties) and displaying some LTR names looks bad into a RTL oriented menulist

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2 (m-c)

The patch does not change the chrome registry selected locale until the user restart Firefox or a call to reloadChrome is done.

This is useful in Fennec because we're shipping multi-locales build and it is possible to switch between locales in the main preferences panel, without this patch the restart notification displayed when the user switch from one language to another is displayed in the newly selected locale if the strings have never been cache before (see screenshot in #c1)

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 488684 [details] [diff] [review]
Patch v0.1 (Front-End) - Menulist

Mark, as I've said before these strings are in a LTR file which should not be localized so I think it makes sense to force the display of the menulist label to ltr.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 488684 [details] [diff] [review]
Patch v0.1 (Front-End) - Menulist

You don't want to make _all_ menulist labels be LTR, do you? The previous patch, the one I r+, only sets the language list in the prefs panel to LTR.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #11)
> Comment on attachment 488684 [details] [diff] [review]
> Patch v0.1 (Front-End) - Menulist
> 
> You don't want to make _all_ menulist labels be LTR, do you? The previous
> patch, the one I r+, only sets the language list in the prefs panel to LTR.

Oh man, why I've done that! Obviously I don't want it, I'll upload an other patch...

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I also guess my platform patch will make tests from test_bug519468.js fails for some case since the getSelectedLocale will update late (http://mxr.mozilla.org/mozilla-central/source/chrome/test/unit/test_bug519468.js).

I would like to see if my patch is acceptable before updating the tests.

Comment 14

[Benjamin Smedberg]

Comment on attachment 488741 [details] [diff] [review]
Patch v0.2 (m-c)

There's a lot of logic here and hardly any comments. Who is producing and consuming the selected-locale-update topic, and why? It appears that the chrome registry is both the consume and the producer. But if that's the case, why bother with an observer-service notification at all? That interaction should definitely be documented.

The if (safeMode) check before using prefs is surprising. Does the pref service not obey safe-mode natively?

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3 (m-c)

This version use virtual methods instead of using an observer to communicate between the ChromeRegistry root class and the ChromeRegistryChrome instance, but there is still a notifyObserver call since I think this can be useful for detecting a language change during runtime (for updating the position of the left/right sidebars in Fennec for example)

I've also updated the tests to reflect the changes into the chrome registry.

Comment 16

[Benjamin Smedberg]

Comment on attachment 490637 [details] [diff] [review]
Patch v0.3 (m-c)

>diff --git a/chrome/src/nsChromeRegistry.h b/chrome/src/nsChromeRegistry.h

>+  virtual void UpdateSelectedLocale() = 0;

Needs doccomment!

>diff --git a/chrome/src/nsChromeRegistryChrome.cpp b/chrome/src/nsChromeRegistryChrome.cpp

>+      else {
>+        rv = SelectLocaleFromPref(prefs);
>+        if (NS_FAILED(rv)) {
>+          NS_ERROR("Couldn't select locale from pref!");
>+          return rv;
>+        }

Here and below you warn on failure of SelectLocaleFromPref, and here you're returning failure from nsIObserver.observe, which is meaningless. Why not just add the warning to SelectLocaleFromPref and make it return void?

>+void nsChromeRegistryChrome::UpdateSelectedLocale() {

Please file the style guide:

returntype
ClassName::MethodName(args)
{ // brace in column 0

Please attach a new patch with nits fixed, and re-request review from myself and mike hommey, since he wrote/knows the test better than I.

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.4 (m-c)

Addressed comments.

I'm think we still need the return value of SelectLocaleFromPref to avoid cleaning up the cache if this is not necessary.

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 491516 [details] [diff] [review]
Patch v0.4 (m-c)

The test looks good to me. I'd suggest to move the testsLocale definition, though. Since you made it global, there's no compelling reason to separate its definition and its value.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch (Front-End) for checkin

The updated front-end patch

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

m-c: http://hg.mozilla.org/mozilla-central/rev/a5d48a5d46cd
front-end: http://hg.mozilla.org/mobile-browser/rev/bfdc6dc130b6

Comment 21

[(no longer active)]

This broke builds with --disable-ipc.  I landed a followup to fix this issue:

http://hg.mozilla.org/mozilla-central/rev/0ebeeb2fae57

Comment 22

[Ioana Chiorean]

https://litmus.mozilla.org/show_test.cgi?id=15201

Comment 23

[Andreea Pod]

VERIFIED FIXED on:
Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110315
Firefox/4.0b13pre Fennec /4.0b6pre
Device: HTC Desire