Closed Bug 853424 Opened 11 years ago Closed 11 years ago

Gecko weirdness in browser app when using non-English locales

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:tef+, b2g18 verified, b2g18-v1.0.1 verified)

VERIFIED FIXED
blocking-b2g tef+
Tracking Status
b2g18 --- verified
b2g18-v1.0.1 --- verified

People

(Reporter: benfrancis, Assigned: tkundu)

References

Details

(Whiteboard: [TD-10262])

Attachments

(1 file, 9 obsolete files)

189 bytes, text/html
vingtetun
: review+
Details
STR:
* Change language in settings to something other then English (I chose French)
* Open Browser app

From bug 835862
* Tap "1>" to open side tray
* Tap "<" to close side tray
Expected:
* Side tray closes
Actual:
* Screen flashes white, nothing happens

From bug 835776
* Load a web page
* Try to pinch to zoom or double tap to zoom
Expected:
* Zooms
Actual:
* Nothing happens 

Also sometimes, the whole browser app iframe just turns white. Then if you go to the task switcher and re-select the browser app it renders properly again.

The odd thing is that all of this works fine in English. I can't figure out what the difference is.

I can't see any JavaScript errors in the console and the flashing white makes me think this could be at least partly a gecko issue.

This effects both Gaia master and 1.0.1, I assume v1-train also.
I do experience the same behavior on my Nexus S. Especially, the white bug also happens sometimes only when just starting the Browser app. I haven't been able to reproduce it anywhere else, only browser.
Blocks: 853038
blocking-b2g: --- → tef?
blocking-b2g: tef? → tef+
I can't reproduce with b2g18 + gaia master.
blocking-b2g: tef+ → ---
I've tested with 3/22 gaia master, and bug 835776 835862 853038 also occurs in chinese version

my steps:

Start from en version

1. open yahoo in browser 
2. pinch/zoom, swith

change language to other language in settings

4. close the browser
5. open browser again, and Bug 835776 835862 853038 occurred
This is happening in v1.0.1 with the following tips: 

Gecko: 353867e
Gaia: 42de12a

This is the reason for being tef+, Fabrice, can you please check in v1.0.1?
blocking-b2g: --- → tef+
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #2)
> I can't reproduce with b2g18 + gaia master.

I'm reproducing it on b2g18 + gaia master (1c093ea)
The app name will change along with the locale, however, our OOP black list is hard coded to 'Browser'. So, browser app will be OOP if we change to non-English locales.
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L1112

Remove Browser from OOP black list will reproduce this bug in English locale.
This fixes problem from French language. Please verify it for French language
Oh... that's really bad, thanks for investigating. Tapas, we need to fix that without relying to hard-coded localized names.
Flags: needinfo?(fabrice)
Assignee: nobody → tkundu
Something like that should work:

diff --git a/apps/system/js/window_manager.js b/apps/system/js/window_manager.js
index cf471ef..0405217 100644
--- a/apps/system/js/window_manager.js
+++ b/apps/system/js/window_manager.js
@@ -1110,13 +1110,13 @@ var WindowManager = (function() {
     // run out of process. All other apps will be run OOP.
     //
     var outOfProcessBlackList = [
-      'Browser'
+      'app://browser.gaiamobile.org/manifest.webapp'
       // Requires nested content processes (bug 761935).  This is not
       // on the schedule for v1.
     ];
 
     if (!isOutOfProcessDisabled &&
-        outOfProcessBlackList.indexOf(name) === -1) {
+        outOfProcessBlackList.indexOf(manifestURL) === -1) {
       // FIXME: content shouldn't control this directly
       iframe.setAttribute('remote', 'true');
     }
It is working for all languages. We don't see any issues mentioned on https://bugzilla.mozilla.org/show_bug.cgi?id=853424#c0 with this patch.

Thanks to :fabrice. I just added your modification as patch. It worked for me.
It worked for me in 1.0.1 . I don't see issues mentioned on https://bugzilla.mozilla.org/show_bug.cgi?id=853424#c0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
Comment on attachment 728490 [details] [diff] [review]
This fixes problem from All language.

Review of attachment 728490 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/system/js/window_manager.js
@@ +1205,1 @@
>        'Dogfood',

Please change this one also to use the manifest URI: app://dogfood.gaiamobile.org/manifest.webapp
adding myself to cc
(In reply to Tapas Kumar Kundu from comment #10)
> Created attachment 728490 [details] [diff] [review]
> This fixes problem from All language.
> 
> It is working for all languages. We don't see any issues mentioned on
> https://bugzilla.mozilla.org/show_bug.cgi?id=853424#c0 with this patch.
> 
> Thanks to :fabrice. I just added your modification as patch. It worked for
> me.

This patch does not apply, probably because there is no "Navigateur" line in the branch.
After applying by hand, that's a nice fix! It fixed a lot of issues I had with Browser.
Fixed on MC, is still an issue for v1 train and v1.0.1

Alex, there must be a patch that resolved this issue somewhere in v1 train?  We need this pushed down the train because of bug 854044

This is with the 3/25 builds.
Flags: needinfo?(akeybl)
(In reply to Naoki Hirata :nhirata from comment #19)
> Fixed on MC, is still an issue for v1 train and v1.0.1
> 
> Alex, there must be a patch that resolved this issue somewhere in v1 train? 
> We need this pushed down the train because of bug 854044
> 
> This is with the 3/25 builds.

Tapas - did you land a patch to resolve this issue? If so, can you please link to the changeset on GitHub so that our tree sheriffs can uplift to v1.0.1/v1.1?
Flags: needinfo?(akeybl) → needinfo?(tkundu)
(In reply to Naoki Hirata :nhirata from comment #19)
> Fixed on MC, is still an issue for v1 train and v1.0.1
> 
> Alex, there must be a patch that resolved this issue somewhere in v1 train? 
> We need this pushed down the train because of bug 854044
> 
> This is with the 3/25 builds.

I'm not sure I understood your request, however I simply applied the patch attached to this bug on my gaia master branch, and it fixes ALL issues I had previously with Browser app (screen turning white, slowliness, bogus zoom, etc.).
Are we really sure this has landed in master? Cannot see any related change in https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js

qawanted to verify if this has been really fixed in master.
Keywords: qawanted
Tested in unagi with commit 06bbd6d0503c095ecfcddebbad1461465118613c from master, using fr as spanish is not included and still not working.
Keywords: qawanted
Tapas, can you update your patch to address my review comment and then ask for review to vingtetun?
Fabrice Desré [:fabrice] I created new patch as you requested.
Attachment #728442 - Attachment is obsolete: true
Attachment #728490 - Attachment is obsolete: true
Flags: needinfo?(tkundu)
Attachment #729715 - Flags: review?(21)
Comment on attachment 729715 [details] [diff] [review]
this fixes both browser and dogfood app for all languages

Clearing r, as this patch is not ready for review.  Tapas will upload a new one soon.
Attachment #729715 - Attachment is obsolete: true
Attachment #729715 - Flags: review?(21)
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Add patch for Browser app only . I didn't include patch for Dogfood app as we don't use it.
Attachment #730268 - Flags: review?(21)
Status: REOPENED → ASSIGNED
Comment on attachment 730268 [details] [diff] [review]
Patch for browser app which fixes for all language.

Can you build that dynamically? there is a lof of example in the code that use the location object to not tied Gaia to gaiamobile.org and to the app: protocol.

I will r+ with this.
Attachment #730268 - Flags: review?(21) → review-
I can dynamically produce browser manifer url using something like 

var browsermanifestUrl = 'app://browser'+homescreenManifestURL.substr(homescreenManifestURL.indexOf('.'));

Is this fine with you ? If not then please suggest. I saw app url is used directly in most of gaia app.
Flags: needinfo?(21)
(In reply to Tapas Kumar Kundu from comment #30)
> I can dynamically produce browser manifer url using something like 
> 
> var browsermanifestUrl =
> 'app://browser'+homescreenManifestURL.substr(homescreenManifestURL.indexOf('.
> '));
> 
> Is this fine with you ? If not then please suggest. I saw app url is used
> directly in most of gaia app.

You should use something similar to that:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/quick_settings.js#L186
Flags: needinfo?(21)
Attached patch Language fix for browser (obsolete) — Splinter Review
Attachment #730268 - Attachment is obsolete: true
Attached patch Language fix for browser (obsolete) — Splinter Review
Attachment #732526 - Attachment is obsolete: true
Attachment #732527 - Flags: review?(21)
Attached patch Language fix for browser (obsolete) — Splinter Review
Attachment #732527 - Attachment is obsolete: true
Attachment #732527 - Flags: review?(21)
Attachment #732528 - Flags: review?(21)
Attached patch Language fix for browser (obsolete) — Splinter Review
Added it again to fix html redirection to github.
Attachment #732528 - Attachment is obsolete: true
Attachment #732528 - Flags: review?(21)
Attachment #732921 - Flags: review?
Attached file Language fix for browser (obsolete) —
I added same patch again to fix html redirection to github commit. I also asked for review from 21@vingtetun.org.
Attachment #732921 - Attachment is obsolete: true
Attachment #732921 - Flags: review?
Attachment #732922 - Flags: review?(21)
I changed mime type of patch link to make sure that it redirects to github.
Attachment #733108 - Flags: review?(21)
Attachment #732922 - Attachment is obsolete: true
Attachment #732922 - Attachment is patch: false
Attachment #732922 - Flags: review?(21)
Comment on attachment 733108 [details]
Language fix for browser

Thanks!
Attachment #733108 - Flags: review?(21) → review+
Please checkin this patch .

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
How are we doing with landing this? Can anyone help tapas?
Flags: needinfo?(fabrice)
Pushed to master:
https://github.com/mozilla-b2g/gaia/commit/35238a361ff796fee597a7a152c9d8c5d2523e24
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Flags: needinfo?(fabrice)
Resolution: --- → FIXED
(In reply to Fabrice Desré [:fabrice] from comment #41)
> Pushed to master:
> https://github.com/mozilla-b2g/gaia/commit/
> 35238a361ff796fee597a7a152c9d8c5d2523e24

This commit introduces lint error which is fixed in the follow up here:
https://github.com/mozilla-b2g/gaia/commit/ffceeff41096100425a6f98a8188df9f2e324b32
Opps, thanks Tim.  Did you detect this via the "Travis build" or some other method?
Verfied fixed on Master:
Unagi Build ID: 20130409031002
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/mozilla-central/rev/b1fb34b07c17
Gaia: 65dd37c938b1cd38fce96fcfa7f4be09e259c255

Not fixed on 

Unagi Build ID: 20130409070202
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3b51ced33215
Gaia: f80afc35f8c63e713b88ec26a0ee7c5af50f4096

and on 

Unagi Build ID: 20130409070205
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/cfef36c0c8bc
Gaia: a80be95f553de517e5d8a159e04511cda5e38be4
Status: RESOLVED → VERIFIED
Can we uplift this soon so that we can fully test the browser in Spanish (key for certification).
Flags: needinfo?(jhford)
Verified fixed on 

Unagi Build ID: 20130411070205
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f83d169babee
Gaia: 81cab0e96c0064b03db46a5248047fed617c2f26

and on 

Unagi Build ID: 20130411070205
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/f671fa539473
Gaia: e7e338a765e22334b40ced41489a785941382c66

Screen no longer flashes white.  User is now able to zoom in or out and/or double tap screen.  Use is also now able to click on the > button and will bring user to a spot that user is able to either add or delete a page as well as go to internet settings.  Tested all in Spanish, French, Dutch, and Hrvatski
Whiteboard: [TD-10262]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: