Closed Bug 1100995 Opened 7 years ago Closed 6 years ago

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

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S10 (17apr)

People

(Reporter: gsvelto, Assigned: gsvelto)

References

Details

(Whiteboard: [planned-sprint c=1])

Attachments

(1 file)

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.
Duplicate of this bug: 1127194
Duplicate of this bug: 1150911
Whiteboard: [planned-sprint c=1]
Target Milestone: --- → 2.2 S10 (17apr)
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
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.
Attachment #8589865 - Flags: review?(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
Flags: needinfo?(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.
Flags: needinfo?(gsvelto)
Attachment #8589865 - Flags: review?(thills)
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.
Attachment #8589865 - Flags: review?(thills)
Blocks: 1020138
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.
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
Flags: needinfo?(gsvelto)
Yeah, I'm seeing the same issue now; I'll try to figure out what's wrong with it.
Flags: needinfo?(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().
Attachment #8589865 - Flags: review?(thills)
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.
Attachment #8589865 - Flags: review?(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
Attachment #8589865 - Flags: review?(thills) → review+
Thanks for the review, squashed the patches, this is ready for landing.
Keywords: checkin-needed
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.
Rebased, second try.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.