Closed Bug 597296 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
All
defect
Not set

Tracking

(fennec2.0b3+)

VERIFIED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: nhirata, Assigned: vingtetun)

Details

Attachments

(3 files, 5 obsolete files)

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
Also occurs in the Restart to complete changes when switching from arabic to english language.  see attached screenshot.
tracking-fennec: --- → ?
Flags: in-testsuite?
Flags: in-litmus?
tracking-fennec: ? → 2.0+
Vivien - is this a front-end bug or a platfrom bug?
Assignee: nobody → 21
Attached patch Patch (obsolete) — Splinter Review
(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?
Attachment #479355 - Flags: review?(mark.finkle)
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 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
Attachment #479355 - Flags: review?(mark.finkle) → review+
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)?
tracking-fennec: 2.0+ → 2.0b3+
Whiteboard: [fennec-checkin-postb2][has-patch]
Attached patch Patch v0.1 (m-c) (obsolete) — Splinter Review
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
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
Attached patch Patch v0.2 (m-c) (obsolete) — Splinter Review
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)
Attachment #488683 - Attachment is obsolete: true
Attachment #488741 - Flags: review?
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.
Attachment #488684 - Flags: review?(mark.finkle)
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.
Attachment #488684 - Flags: review?(mark.finkle) → review-
(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...
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 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?
Attachment #488741 - Flags: review?(benjamin) → review-
Attached patch Patch v0.3 (m-c) (obsolete) — Splinter Review
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.
Attachment #490637 - Flags: review?
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.
Attachment #490637 - Flags: review?(benjamin) → review-
Attached patch Patch v0.4 (m-c)Splinter Review
Addressed comments.

I'm think we still need the return value of SelectLocaleFromPref to avoid cleaning up the cache if this is not necessary.
Attachment #488741 - Attachment is obsolete: true
Attachment #490637 - Attachment is obsolete: true
Attachment #491516 - Flags: review?(mh+mozilla)
Attachment #491516 - Flags: review?(benjamin)
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.
Attachment #491516 - Flags: review?(mh+mozilla) → review+
Whiteboard: [fennec-checkin-postb2][has-patch] → [has-patch]
Attachment #488684 - Attachment is obsolete: true
tracking-fennec: 2.0b3+ → 2.0+
Attachment #491516 - Flags: review?(benjamin) → review+
The updated front-end patch
Attachment #479355 - Attachment is obsolete: true
Attachment #493969 - Flags: review?(mark.finkle)
Attachment #493969 - Flags: review?(mark.finkle) → review+
tracking-fennec: 2.0+ → 2.0b3+
Whiteboard: [has-patch]
This broke builds with --disable-ipc.  I landed a followup to fix this issue:

http://hg.mozilla.org/mozilla-central/rev/0ebeeb2fae57
https://litmus.mozilla.org/show_test.cgi?id=15201
Flags: in-litmus? → in-litmus+
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.