Closed Bug 1111903 Opened 5 years ago Closed 5 years ago

[Settings] Refactor Dialog Service with l10n best practices

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: eragonj, Assigned: eragonj)

References

Details

Attachments

(2 files)

Currently DialogService would directly render strings on dialogs instead of following l10n best practices. For this bug, we have to update the API interface to follow the document - https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices
(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #19)
> In our API, we can pass `message` and `title` to make sure DialogService to
> render these strings on UI. But by following our l10n best practices, in
> worst case, we have to pass 4 extra parameters like this : 
> 
> ```
> DialogService.alert({
>   titleL10nId: 'ThisIsTitleId',
>   titleL10nArgs: {},
>   messageL10nId: 'ThisIsMessageId',
>   messageL10nArgs: {}
> })
> ```
> 
> To be honest, I don't think this makes sense for API design, any better
> idea, guys ?

Please, take a look at what we did with NotificationHelper recently: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/notification_helper.js

The API in the "worst" (most complex" case will look like this:

DialogService.alert({
  title: {id: "ThisIsTitleId", args: {}},
  message: {id: "ThisIsMessageId", args: {}}
});

Which is not really that different from the current:

DialogService.alert({
  title: navigator.mozL10n.get("ThisIsTitleId", {}),
  message: navigator.mozL10n.get("ThisIsmessageId", {})
});

Keep in mind that that's the "worst case". In most cases it will be:

DialogService.alert({
  title: "ThisIsTitleId",
  message: "ThisIsMessageId"
});

> Can we just leave this logic on caller ? For example, caller has to create a
> pseudo element and make sure these information would be rendered correctly
> on that element and then, we can directly retrieve `textContent` from the
> DOM and pass it to API ? (Just an idea, for me, this is not a good idea
> either hmmm)

The whole idea behind passing l10n-id/args to the API is so that internally you don't do:

domNode.textContent = title;

but:

domNode.setAttribute('data-l10n-id', title);

or in complex case:

navigator.mozL10n.setAttributes(domNode, title.id, title.args);

That part is different from Notification because we're not bound by W3C api and we can totally remove use of mozL10n.get here and be declarative.

That makes your whole code not depend on mozL10n resources being loaded (you can remove the call to mozL10n.once!) and retranslatable which enables dynamic language changes, adaptive l10n etc.
(In reply to Zibi Braniecki [:gandalf] from comment #1)
> (In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #19)
> > In our API, we can pass `message` and `title` to make sure DialogService to
> > render these strings on UI. But by following our l10n best practices, in
> > worst case, we have to pass 4 extra parameters like this : 
> > 
> > ```
> > DialogService.alert({
> >   titleL10nId: 'ThisIsTitleId',
> >   titleL10nArgs: {},
> >   messageL10nId: 'ThisIsMessageId',
> >   messageL10nArgs: {}
> > })
> > ```
> > 
> > To be honest, I don't think this makes sense for API design, any better
> > idea, guys ?
> 
> Please, take a look at what we did with NotificationHelper recently:
> https://github.com/mozilla-b2g/gaia/blob/master/shared/js/
> notification_helper.js
> 
> The API in the "worst" (most complex" case will look like this:
> 
> DialogService.alert({
>   title: {id: "ThisIsTitleId", args: {}},
>   message: {id: "ThisIsMessageId", args: {}}
> });
> 
> Which is not really that different from the current:
> 
> DialogService.alert({
>   title: navigator.mozL10n.get("ThisIsTitleId", {}),
>   message: navigator.mozL10n.get("ThisIsmessageId", {})
> });
> 
> Keep in mind that that's the "worst case". In most cases it will be:
> 
> DialogService.alert({
>   title: "ThisIsTitleId",
>   message: "ThisIsMessageId"
> });

Yeah, this is the same with our final conclusion !

> 
> > Can we just leave this logic on caller ? For example, caller has to create a
> > pseudo element and make sure these information would be rendered correctly
> > on that element and then, we can directly retrieve `textContent` from the
> > DOM and pass it to API ? (Just an idea, for me, this is not a good idea
> > either hmmm)
> 
> The whole idea behind passing l10n-id/args to the API is so that internally
> you don't do:
> 
> domNode.textContent = title;
> 
> but:
> 
> domNode.setAttribute('data-l10n-id', title);
> 
> or in complex case:
> 
> navigator.mozL10n.setAttributes(domNode, title.id, title.args);
> 

Yes, you are right !

> That part is different from Notification because we're not bound by W3C api
> and we can totally remove use of mozL10n.get here and be declarative.
> 
> That makes your whole code not depend on mozL10n resources being loaded (you
> can remove the call to mozL10n.once!) and retranslatable which enables
> dynamic language changes, adaptive l10n etc.

Yeah, for us, mozL10n can be transparent and we don't have to be aware its existence anymore. As what you said, we can decouple more with mozL10n in our codebases.

Zibi ++
Attached file patch on master
Arthur, based on our offline discussion, I updated the API for dialogService with l10n best practices. Please help me review this patch when you have time, thanks !
Attachment #8536985 - Flags: review?(arthur.chen)
Comment on attachment 8536985 [details] [review]
patch on master

The API is good. I left some comments regarding the updates to the dom elements, please check them in github. And we should also modify the code that use DialogService in this patch, right?
Attachment #8536985 - Flags: review?(arthur.chen)
Comment on attachment 8536985 [details] [review]
patch on master

Yeah, we should fix places using DialogService.XXX. It's my fault that I use `dialogService` to grep and can't find anything to change, what a stupid grep ! We name them in `DialogService` instead of `dialogService` !

I already addressed your comments on GitHub and did some manual tests already, please feel free to ping me back if you got any problem, Arthur.

Thanks !
Attachment #8536985 - Flags: review?(arthur.chen)
Comment on attachment 8536985 [details] [review]
patch on master

r=me, thanks!
Attachment #8536985 - Flags: review?(arthur.chen) → review+
As bug 1103894 just landed and it also uses the original API, please remember to update the patch correspondingly, thanks.
Flags: needinfo?(ejchen)
OK! I almost forgot you already landed your patch, so let me update the patch first !
Flags: needinfo?(ejchen)
Thanks all, merged at Gaia/master : https://github.com/mozilla-b2g/gaia/commit/6ef786085a7a40b9197d5ffee3646a09afc29d59
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Found a bug that would cause other dialogs broken, so reverted first : https://github.com/mozilla-b2g/gaia/commit/c7ee7755e0fd14c74595b2ae02f148e17be69777
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Two issues to be addressed:

- Check the existence of the message element.
- Only restore the content for the dialogs of the default types.
Arthur, can you help me check this patch ?

In this updated patch, we would check messageDOM before accessing it and we would only restore contents if the panel is not system-wise. In addition to these, I added one unit test for `base_dialog` and I think this can make our API more robust.

Thanks.
Attachment #8538930 - Flags: review?(arthur.chen)
Comment on attachment 8538930 [details] [review]
updated patch on master

r=me with the comment addressed, thanks!
Attachment #8538930 - Flags: review?(arthur.chen) → review+
Thanks Arthur, just re-land this patch here : https://github.com/mozilla-b2g/gaia/commit/ec14f58889f1ed52ba0c74ef375ab88565c31067
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.