Closed Bug 1087819 Opened 10 years ago Closed 6 years ago

[email][2.1] Variable {{ subject }} displayed in notification for emails without subject

Categories

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: xshen, Unassigned)

References

Details

(Whiteboard: LocRun2.1-1)

Attachments

(1 file)

Steps:
1)Choose language as "Simplified Chinese"
2)Config a email account on Email APP, and send a email without subject to this account
3)Wait for a moment, there is a notification like "a new Email" -- see screenshot
4)Go to Email APP,it displays correctly as "无主题"

Expected Result:
subject should be translated to simplified Chinese.

Actual Result:
It displays as "{{subject}}"

Build version:
20141021161205
Device:
Flame kk v2.1(v180 base)
Whiteboard: [l10n] LocRun2.1
No problem found for new-emails-notify-multiple-accounts-body={{ from }} “{{ subject }}” on http://mxr.mozilla.org/gaia/search?string=new-emails-notify-multiple-accounts-body.
I think the "{{subject}}" is non-visible for end user, it's better to show"无主题" in the notification.
I don't understand why the problem happen, the variable placeholder should be the normal conversion.
Whiteboard: [l10n] LocRun2.1 → LocRun2.1-1
Hi Delphine, 
  Could you please check this one? it seems not a l10n bug but a i18n one.
Br.
Holy
I don't see anything wrong with the current strings, so I assume it's an issue with the email app. 
I'm trying to test it on master, but it's proving hard to get a notification (doesn't seem to sync every 5 minutes as set).
Assignee: shaohua.wen → nobody
Component: zh-CN / Chinese (Simplified) → Gaia::E-Mail
Product: Mozilla Localizations → Firefox OS
QA Contact: shaohua.wen
Finally got a notification in master: still broken but in a different way (I see 'null' as subject, only the sender under it).
Summary: [b2g][l10n][2.1][zh-CN][Notification]Subject is not translated to simplified Chinese → [email][2.1] Variable {{ subject }} displayed in notification for emails without subject
Blocks: 1103882
No longer blocks: 1103882
It looks like we may end up doing mozL10n.get('subject-using-id', { subject: null }).  This causes l10n.js to freak out.  subPlaceable handles numbers and strings okay, but it just returns "'{{ ' + id + ' }}'" otherwise.  (Note that resolveIdentifier will end up in its failure case too and provide it with a value of "undefined".)  There's no comment providing rationale, but it seems like given the potential for null/undefined leakage that it is the wrong call.

Why: Looking at the screenshot, it looks like this only happens to the ActiveSync-based hotmail account and not the IMAP based Yahoo account.  Looking at activesync/folder.js I see that if the WBXML lacks a text child node (very likely for an empty subject), we will assign null to the value.  mail_rep.js recognizes null as a valid value, and I think that's a mistake.  We allow null for the snippet, but that has a semantic meaning (we didn't compute a snippet yet).  There's no useful semantic meaning to a null subject.

This gets to the front-end in sync.js where we can use null without doing "|| ''", which we should probably be doing.

This probably wants 3 fixes:

- l10n.js probably should never return its own key when it doesn't find data it likes.  We should spin a bug off for this.

- email's front-end should defensively modify sync.js to do the || '' thing.  There's no reason for us not to be providing a string there.  That should be done on this bug.

- email's back-end should have the subject always be a string.  We should correct ActiveSync to itself generate a header with a string, but we should also have mail_rep normalize null/undefined to ''.  This also wants to be a new bug.

I'm punting on the spin-offs/etc. for now.
CCing stas and gandalf for the l10n part
(In reply to Andrew Sutherland [:asuth] from comment #7)

> - l10n.js probably should never return its own key when it doesn't find data
> it likes.  We should spin a bug off for this.

I actually differ here. Our strategy moving forward is to build a solid fallback mechanism so that we never show an empty string to the user. We don't have a strategy finalized yet, but we do have a state for when an entity errors (because of missing variable or unresolved macro etc.)

In the future we may want to fallback to get such entity from a fallback language but if that fails as well, we will want to show the unresolved entity to the user. We believe it to be more meaningful (hint: meaningful l10n-ids for the win!) then an empty string and safer than partial string.

My suggestion for this bug is that we should never, ever break the social contract between the localizer and the developer. It means, if the entity is supposed to receive a subject, it should always receive a subject. If there's a case where there is no subject, it should spawn another entity:

entity1 = Foo {{subject}}
entity2 = Foo

Once we start moving to l20n file format, we will have additional option of string variants, which may enable us to do things like:

<entity[subject ? "withsubject" : "withoutsubject"] {
  withsubject: "Foo {{subject}}",
  withoutsubject: "Foo"
}>

but that's more complex than it sounds because in L20n we so far avoid implicit type casting (string to bool) and we don't have Null type (and we really would love not to have to add it). But if enough cases that need them surface, we may have to reconsider.

We may also try to resolve it differently with some global macro:
<entity[@length(subject) === 0 ? 'withoutsubject': 'withsubject'] {
  withsubject: "Foo {{subject}}",
  withoutsubject: "Foo"
}>

But we will still error on passing null valued variable.
I agree that given the domain it makes sense to have a separate localizable string for the case where there is no subject.  But as it relates to programmers screwing up the variables...

(In reply to Zibi Braniecki [:gandalf] from comment #9)
> (In reply to Andrew Sutherland [:asuth] from comment #7)
> > - l10n.js probably should never return its own key when it doesn't find data
> > it likes.  We should spin a bug off for this.
> 
> I actually differ here. Our strategy moving forward is to build a solid
> fallback mechanism so that we never show an empty string to the user. We
> don't have a strategy finalized yet, but we do have a state for when an
> entity errors (because of missing variable or unresolved macro etc.)
> 
> In the future we may want to fallback to get such entity from a fallback
> language but if that fails as well, we will want to show the unresolved
> entity to the user. We believe it to be more meaningful (hint: meaningful
> l10n-ids for the win!) then an empty string and safer than partial string.

I think it's worth differentiating between a broken entity and a broken placeable (the lib's lingo for "{{ interpolatedName }}") in an entity's translation.

A locale missing an entity/l10n-id translation or the code asking for an entity/l10n-id that doesn't exist is clearly a bug in one of the two places.  And there's not enough context for the app to minimize damage; the whole string is a loss.

A placeable that has no corresponding entry in l10n-args is either a typo in a localization or a bug/poorly handled edge-case in the app generating the l10n-args.  If the app is being used in a development/QA/localizer context, having "{{ subject }}" displayed is way handier than returning a space or just stringifying "null".  But in the context of shipped Firefox OS devices where we are effectively unable to update the email app, I think it's less embarrassing to us and less annoying to the user if we can fail in a somewhat more subtle fashion.

If there is a way for the app to register a handler hook for this case where we're given the opportunity to fill in the blanks, I think that would be most useful.  Specifically, the email app could vary things based on the build type/branch status:
- In Automated tests we could make sure to generate a failure.
- In engineering builds or on the active development trunk, we could do the helpful {{ key }} thing and also log context to the console and our structured logging.
- In production builds / the week before a branch gets cut, we could return an empty string.  We would still do the logging stuff so if we get a bug report and they give us a logcat we have more to go on.

(This would also make sense for the NaN case.)

What makes sense I think hinges on examples, but I don't have a lot.  The only other place the email app has faced assumed-invariant violations that might be surfaced as strings is where we think we have an email address but instead we have a group.  (We expect { name, address }, we have { name, group: [...] }).  But in that case if displaying "address" we directly assign to a DOM node's textContent and textContent treats assignment of null/undefined specially and just clears everything out.  (spec link per MDN http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#Node3-textContent)
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: