1096120 - Context menu labels aren't invalidated after switching locales

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Locale switching and selection, defect)

Description

[Richard Newman [:rnewman]]

* Set browser to Spanish.
* Relaunch.
* Set browser to system locale.
* Observe that context menu still shows "Abrir en una pestaña nueva".

Comment 1

[Richard Newman [:rnewman]]

This might be a regression, by the way -- I seem to recall some work being done on context menus in the past few releases, and I'm pretty sure we would have noticed during initial testing.

Unlikely to affect the outcome, but worth knowing if/how far we should uplift.

Comment 2

[Richard Newman [:rnewman]]

I have what *should* be a fix for this: whenever we add a menu item, do it dynamically.

E.g.,

-    NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.openInPrivateTab"),
+    NativeWindow.contextmenus.add(stringGetter("contextmenu.openInPrivateTab"),

where stringGetter is:

+    function stringGetter (name) {
+      return function () {
+        console.log("XXX calling stringGetter: " + name + " = " +
+        Strings.browser.GetStringFromName(name));
+        return Strings.browser.GetStringFromName(name);
+      };
+    }

However, this alone doesn't work, because the StringBundle (Strings.browser) is returning stale values *even after the bundles have been flushed*:

I/GeckoConsole(23399): Locale:Changed: fr
I/GeckoConsole(23399): XXX flushing strings
I/GeckoConsole(23399): Default intl.accept_languages = fr, fr-fr, en-us, en
I/GeckoConsole(23399): Setting intl.accept_languages to fr,en-us,fr-fr,en
...
I/GeckoConsole(23399): XXX calling stringGetter: contextmenu.openInNewTab = Abrir en una pestaña nueva
I/GeckoConsole(23399): XXX reformatMenuItems. (e.g., Abrir en una pestaña nueva)
I/GeckoConsole(23399): XXX calling stringGetter: contextmenu.openInNewTab = Abrir en una pestaña nueva

Comment 3

[Richard Newman [:rnewman]]

Thanks to Axel for pointing out a flawed assumption in Bug 1020502.

Comment 4

[Richard Newman [:rnewman]]

Attachment: Invalidate context menu on locale change. v1

The fix in Bug 1020502 was incomplete; flushing the bundle service does not
(always?) invalidate existing bundles. This patch makes `Strings`
re-initializable, and does so when the locale changes.

Margaret gets review, 'cos she reviewed that fix.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8521113 [details] [diff] [review]
Invalidate context menu on locale change. v1

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

::: mobile/android/base/GeckoApp.java
@@ +391,4 @@
>                  onPreparePanel(featureId, mMenuPanel, mMenu);
>              }
>  
>              return mMenuPanel; 

While you're removing trailing whitespace, there's also some here.

::: mobile/android/chrome/content/browser.js
@@ +554,5 @@
>      WebappRT.init(status, url, callback);
>    },
>  
> +  initContextMenu: function () {
> +    let stringGetter = name => () => Strings.browser.GetStringFromName(name);

o_O Why do you need two arrows here?

@@ +1831,5 @@
>          Services.prefs.savePrefFile(null);
>  
>          // Blow away the string cache so that future lookups get the
>          // correct locale.
> +        Strings.flush();

Will this do the job? initContextMenu is called in BrowserApp.startup, so all these menu items will have already been added.

Or is this just a "part 1" patch, and there will be more?

Comment 6

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #5)

> > +    let stringGetter = name => () => Strings.browser.GetStringFromName(name);
> 
> o_O Why do you need two arrows here?

That's half of the fix.

We already allow our menu components to be functions as well as simple values. If they're functions, they'll be evaluated each time they're used, not only once.

That line is "let stringGetter be a function that takes a name, returning a function that looks up the string when called". It returns a thunk rather than a string.


> Will this do the job? initContextMenu is called in BrowserApp.startup, so
> all these menu items will have already been added.

This is the other half of the fix.

By making the labels dynamic, we don't need to reconstruct the context menu; we just need to ensure that Strings.browser is invalidated, so the next time we build the UI from the ContextMenuItems, we'll get the right strings.

Strings.flush takes care of that by throwing away the StringBundles and flushing the StringBundleService.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8521113 [details] [diff] [review]
Invalidate context menu on locale change. v1

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

Okay, that makes sense. I didn't realized you could pass a function to contextmenus.add. I think you should add a comment above that!

Comment 8

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/7a6800ce9153

Comment 9

[Richard Newman [:rnewman]]

I'd like to uplift this to Aurora.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8521113 [details] [diff] [review]
Invalidate context menu on locale change. v1

Approval Request Comment
[Feature/regressing bug #]:
  Locale switching edge case. Not a regression.

[User impact if declined]:
  Context menus will show the incorrect language after switching locales.

[Describe test coverage new/current, TBPL]:
  Test coverage not possible. Manually tested.

[Risks and why]: 
  Quite a simple patch; low risk.

[String/UUID change made/needed]:
  None.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7a6800ce9153

Comment 12

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

[Tracking Requested - why for this release]:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/95f5b36e1c44

Comment 14

[Cristina Madaras, QA [:CristinaM]]

After performing steps from comment 0, context menu options are displayed in system default language.
Verified as fixed in builds:
Firefox for Android 36.0a2 (2014-12-04);
Firefox for Android 35 Beta 2;

Device:
Asus Transformer TF101 (Android 4.0.3)

Comment 15

[Cristina Madaras, QA [:CristinaM]]

(In reply to Cristina Madaras, QA [:CristinaM] from comment #14)
> After performing steps from comment 0, context menu options are displayed in
> system default language.
> Verified as fixed in builds:
> Firefox for Android 36.0a2 (2014-12-04);
> Firefox for Android 35 Beta 2;
> 
> Device:
> Asus Transformer TF101 (Android 4.0.3)

Firefox for Android 35 Beta 1 build 2;
Sorry for that!