Context menu labels aren't invalidated after switching locales

VERIFIED FIXED in Firefox 35

Status

()

Firefox for Android
Locale switching and selection
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: rnewman, Assigned: rnewman, Mentored)

Tracking

Trunk
Firefox 36
All
Android
Points:
---

Firefox Tracking Flags

(firefox34 wontfix, firefox35+ verified, firefox36 verified, fennec35+)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

3 years ago
tracking-fennec: --- → ?
(Assignee)

Comment 1

3 years ago
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.
(Assignee)

Comment 2

3 years ago
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
(Assignee)

Comment 3

3 years ago
Thanks to Axel for pointing out a flawed assumption in Bug 1020502.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Depends on: 1020502
(Assignee)

Comment 4

3 years ago
Created attachment 8521113 [details] [diff] [review]
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.
Attachment #8521113 - Flags: review?(margaret.leibovic)

Comment 5

3 years ago
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?
(Assignee)

Comment 6

3 years ago
(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

3 years ago
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!
Attachment #8521113 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/7a6800ce9153
(Assignee)

Comment 9

3 years ago
I'd like to uplift this to Aurora.
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → fixed
Flags: qe-verify?
(Assignee)

Comment 10

3 years ago
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.
Attachment #8521113 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7a6800ce9153
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
[Tracking Requested - why for this release]:
tracking-fennec: ? → 35+
status-firefox34: affected → wontfix
tracking-firefox35: --- → ?
Attachment #8521113 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox35: ? → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/95f5b36e1c44
status-firefox35: affected → fixed
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)
Status: RESOLVED → VERIFIED
status-firefox35: fixed → verified
status-firefox36: fixed → verified
(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!
(Assignee)

Updated

3 years ago
Flags: qe-verify?
You need to log in before you can comment on or make changes to this bug.