1100995 - Tapping the # key causes l10n errors to be printed in the log

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Gabriele Svelto [:gsvelto]]

I noticed two of this messages are printed to the error console whenever we tap the hash key:

Content JS WARN: mozL10n: A non-existing entity requested: undefined
     at getWithFallback (app://communications.gaiamobile.org/shared/js/l10n.js:1278:0)

Printing out the stacks of both instances yields this:

getWithFallback@app://communications.gaiamobile.org/shared/js/l10n.js:1278:85
Context.prototype.get@app://communications.gaiamobile.org/shared/js/l10n.js:1284:17
get@app://communications.gaiamobile.org/shared/js/l10n.js:1440:14
localized@app://communications.gaiamobile.org/dialer/js/suggestion_bar.js:262:29
ll10n_get@app://communications.gaiamobile.org/shared/js/lazy_l10n.js:19:9
sb_setItem@app://communications.gaiamobile.org/dialer/js/suggestion_bar.js:261:5
sb_clear@app://communications.gaiamobile.org/dialer/js/suggestion_bar.js:275:5
sb_searchCallback@app://communications.gaiamobile.org/dialer/js/suggestion_bar.js:184:9
onsuccess@app://communications.gaiamobile.org/shared/js/dialer/contacts.js:202:9

... and this:

getWithFallback@app://communications.gaiamobile.org/shared/js/l10n.js:1278:85
Context.prototype.get@app://communications.gaiamobile.org/shared/js/l10n.js:1284:17
get@app://communications.gaiamobile.org/shared/js/l10n.js:1440:14
localized@app://communications.gaiamobile.org/dialer/js/suggestion_bar.js:262:29
ll10n_get@app://communications.gaiamobile.org/shared/js/lazy_l10n.js:19:9
sb_setItem@app://communications.gaiamobile.org/dialer/js/suggestion_bar.js:261:5
sb_clear@app://communications.gaiamobile.org/dialer/js/suggestion_bar.js:275:5
sb_searchCallbackFb@app://communications.gaiamobile.org/dialer/js/suggestion_bar.js:219:9
sb_searchCallback/req.onsuccess@app://communications.gaiamobile.org/dialer/js/suggestion_bar.js:202:9
fb.utils.Request/this.done/<@app://communications.gaiamobile.org/shared/js/fb/fb_request.js:23:11

This might be worth investigating even only for leaving the log clean.

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] gabrielesvelto:bug-1100995-suggestion-bar-l10n-fix > mozilla-b2g:master

Comment 4

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8589865 [details] [review]
[gaia] gabrielesvelto:bug-1100995-suggestion-bar-l10n-fix > mozilla-b2g:master

The fix ended up being slightly more difficult than I had anticipated because we need to correctly detect when a localized phone type is not available and use the user-provided string instead. This was previously done while letting a stray exception be thrown in the process which was what we also saw in the log. This patch fixes both the original case (when mozL10n.get() was being called without an argument) and the custom phone type case.

Comment 5

[Tamara Hills [:thills]]

Hi Gabriele,

It looks good.  Couple of things:

1. I'm not able to check that the unit tests work locally due to bug 1146077.  So, I won't be able to check that for you.

2. Without your patch, I wasn't able to reproduce the exact error mentioned in this bug.  But it did produce the following:  W/Communications( 5337): Content JS WARN: L10nError: "undefined" not found in en-US in app://communications.gaiamobile.org/dialer/index.html#keyboard-view 
W/Communications( 5337):     at reportMissingEntity (app://communications.gaiamobile.org/shared/js/l10n.js:1629:9).  After applying your patch, this went away.  It seems like a very similar error to what you reported, but not sure what the difference is.  So, just wanted to check this with you.

Thanks,

-tamara

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Tamara Hills [:thills] from comment #5)
> 1. I'm not able to check that the unit tests work locally due to bug
> 1146077.  So, I won't be able to check that for you.

OK, I've been able to run the tests locally fortunately and they all seem to pass.

> 2. Without your patch, I wasn't able to reproduce the exact error mentioned
> in this bug.  But it did produce the following:  W/Communications( 5337):
> Content JS WARN: L10nError: "undefined" not found in en-US in
> app://communications.gaiamobile.org/dialer/index.html#keyboard-view 
> W/Communications( 5337):     at reportMissingEntity
> (app://communications.gaiamobile.org/shared/js/l10n.js:1629:9).  After
> applying your patch, this went away.  It seems like a very similar error to
> what you reported, but not sure what the difference is.  So, just wanted to
> check this with you.

Yes, that's the error. In comment 0 I've added some stack traces which do not appear in the normal log. BTW I've noticed a small issue with my patch: when we can't find a localized string for the telephony type we're supposed to use the user-provided string as-is. In that case I set the textContent field to the said string. This has a problem however: the data-l10n-id field on that element is still set so if we switch language the user-provided string will be replaced with whatever localized string was present in the data-l10n-id field before we overwrote textContent. I'll address this issue and post an updated patch.

Comment 7

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8589865 [details] [review]
[gaia] gabrielesvelto:bug-1100995-suggestion-bar-l10n-fix > mozilla-b2g:master

As it turns out there were more problems with my patch: firstly mozL10n.get is going to be deprecated soon so it's better to get rid of it, and secondly with my change if the user provides a custom telephony type that matches another unrelated localized string the custom type will be translated where it should not. I've updated the patch to whitelist the telephone types we recognize and use the custom user-provided type for anything else. I'm also properly removing the data-l10n-id attribute when using a custom type. All tests have been updated accordingly and I've pushed an additional patch on top of the previous one in the PR.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

this patch looks great from l10n API standpoint. That's exactly what we want to move toward in such cases - whitelist and data-l10n-id.

Comment 9

[Tamara Hills [:thills]]

Hi Gabriele,

I'm seeing a problem when I type in a number in the suggestion bar.  It does not seem like the phoneType is getting localized when I'm in arabic.  When I debugged it, this phoneTypes.includes(type) was returning false when type = 'mobile'.  https://github.com/gabrielesvelto/gaia/commit/f3403ec5f758ce9861a80ec740e928b2a1d1222c#diff-fbedf9c5945c76df46f13be0f8b33f50R274

I don't *think* it's a localization issue because it localizes 'mobile' correctly without the change.

I was looking at the Array.prototype.includes and it says it's experimental??? Maybe that can be the issue?

Thanks,

-tamara

Comment 10

[Gabriele Svelto [:gsvelto]]

Yeah, I'm seeing the same issue now; I'll try to figure out what's wrong with it.

Comment 11

[Gabriele Svelto [:gsvelto]]

OK, figured it out. That's JavaScript equality checks being obnoxious as usual. Apparently if I do this:

var array = [ 'mobile' ];
var string = 'mobile';

string == array[0]; // true, 'mobile' is equal to 'mobile'
string === array[0]; // false, 'mobile' is not strictly equal to 'mobile' because they're not the same object, I suppose

Array.prototype.includes, Array.prototype.indexOf and friends use the second form (understandably I might say considering that == returns whatever it wants depending on the types). The solution is to use some().

Comment 12

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8589865 [details] [review]
[gaia] gabrielesvelto:bug-1100995-suggestion-bar-l10n-fix > mozilla-b2g:master

Pushed yet another patch on top of the PR to address the issue with finding the string in the array.

Comment 13

[Tamara Hills [:thills]]

Comment on attachment 8589865 [details] [review]
[gaia] gabrielesvelto:bug-1100995-suggestion-bar-l10n-fix > mozilla-b2g:master

Hi Gabriele,

Looks good.  Issue is fixed.

Thanks,

-tamara

Comment 14

[Gabriele Svelto [:gsvelto]]

Thanks for the review, squashed the patches, this is ready for landing.

Comment 15

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/29403

The pull request could not be applied to the integration branch. Please try again after current integration is complete. You may need to rebase your branch against the target branch.

Comment 16

[Gabriele Svelto [:gsvelto]]

Rebased, second try.

Comment 17

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/3365ee1787bff84e056250e0764d095011e780dc