[Stingray] Card name does not change when locale changes

RESOLVED FIXED

Status

Firefox OS
Gaia::TV
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dwi2, Assigned: dwi2)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ft:conndevices][Stingray-Branch])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
We should fetch app name from manifest every time we need them.
(Assignee)

Updated

3 years ago
Summary: [Stingray] [Home] App name does not change when locale changes → [Stingray] Card name does not change when locale changes
(Assignee)

Comment 1

3 years ago
We should take all type of cards into consideration too. 
This includes: Application, AppBookmark, Folder, Deck.
(Assignee)

Updated

3 years ago
See Also: → bug 1121816
(Assignee)

Comment 2

3 years ago
Zibi provided guidance to fix it in https://bugzilla.mozilla.org/show_bug.cgi?id=1121816#c2
(Assignee)

Updated

3 years ago
Assignee: nobody → tzhuang
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
We have two cases for card name localization:
1. Original card names, which are fetched from manifest or default names of new folders. We should localize these names.
2. User changed name via rename function. In this case, we should respect the name user give, leave it as is and save it as {raw: 'given name'} in data structure. 

And we still have one issue here. We put 'name' label in smart-button element in Home app. However, there are other DOM elements reside in smart-button, which restrict us from using l10n pattern in [1]. If we set textContent of smart-button, it will throw exceptions like: 
  setTextContent is deprecated (https://bugzil.la/1053629). Setting text content of elements with 
  child elements is no longer supported by l10n.js.

I think we need some guidance from Zibi. Given restriction above, how do we localize content for case 1?


[1]: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs
Flags: needinfo?(gandalf)
Thanks for reaching out guys! I appreciate your attention to get this right :)

(In reply to Tzu-Lin Huang [:dwi2][:tzhuang] from comment #3)
> We have two cases for card name localization:
> 1. Original card names, which are fetched from manifest or default names of
> new folders. We should localize these names.
> 2. User changed name via rename function. In this case, we should respect
> the name user give, leave it as is and save it as {raw: 'given name'} in
> data structure.

That sounds right. I imagine you storing sth like:

card1.name = {'raw': "My custom name"}
card2.name = 'homeCardName'

that later gets into UI via:

if (typeof(name) === 'object') {
  if ('raw' in name) {
    node.textContent = name.raw;
  }
} else {
  node.setAttribute('data-l10n-id', name);
}

> And we still have one issue here. We put 'name' label in smart-button
> element in Home app. However, there are other DOM elements reside in
> smart-button, which restrict us from using l10n pattern in [1]. If we set
> textContent of smart-button, it will throw exceptions like: 
>   setTextContent is deprecated (https://bugzil.la/1053629). Setting text
> content of elements with 
>   child elements is no longer supported by l10n.js.
> 
> I think we need some guidance from Zibi. Given restriction above, how do we
> localize content for case 1?

Right, so because of the design of mozL10n, we want to think of node localization as a singular unit. You assign l10n-id and the engine localizes the node. For that reason, we cannot have mixed nodes with content from l10n and the developer.

Although I don't see any example of rich content inside the smart-button here [0] I assume that you want to something like:

<smart-button>
  <span data-l10n-id="myButton">Localizable button text</span>
  <img src="./icon.png"/>
</smart-button>

and then your span is properly localized and you can pull it into shadow dom.

That's what we do in shared/elements/*.

Does it help? If you need more guidance, I'll be happy to answer any questions!

Does it help?

[0] https://github.com/mozilla-b2g/gaia/blob/master/tv_apps/tv_shared/components/smart-button/examples/index.html#L37-L43
Flags: needinfo?(gandalf)
(Assignee)

Comment 5

3 years ago
It helps a lot. 
I know how to fix this one now, thanks

Comment 6

3 years ago
Created attachment 8564906 [details] [review]
[gaia] dwi2:bug1115642 > mozilla-b2g:master
(Assignee)

Comment 7

3 years ago
Comment on attachment 8564906 [details] [review]
[gaia] dwi2:bug1115642 > mozilla-b2g:master

Card name could come from two cases: 
1. If it is a application or deck (which is actually also an app), the localized name is come from its manifest.
2. If it has l10nId (usually in case of folder), we should use form like {id: 'l10nId'} and set its data-l10n-id to let l10n works.

We encapsulate the logic in cardManager.resolveCardName. Also, in order to let l10n works as comment 4 suggested, we separate `name` label from smart-button itself and put it as a child element under smart-button.
Attachment #8564906 - Flags: review?(im)
Attachment #8564906 - Flags: review?(gandalf)
Attachment #8564906 - Flags: review?(gandalf) → review+
Comment on attachment 8564906 [details] [review]
[gaia] dwi2:bug1115642 > mozilla-b2g:master

Looks good to me. But adding an extra element to smart-button may affect the animation. We should ask Sung about this, too.
Attachment #8564906 - Flags: review?(im)
Attachment #8564906 - Flags: review+
Attachment #8564906 - Flags: feedback?(suchiu)

Comment 9

3 years ago
Comment on attachment 8564906 [details] [review]
[gaia] dwi2:bug1115642 > mozilla-b2g:master

It looks good to me! The animation transform is applied on smart-button, so adding extra element on smart-button should not affect the animation.
Attachment #8564906 - Flags: feedback?(suchiu) → feedback+
(Assignee)

Comment 10

3 years ago
Thanks everyone.

I've addressed comments and rebased it to latest master.

Gaia-Try result:
https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=af33ff46e544
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed

Comment 11

3 years ago
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
(Assignee)

Updated

3 years ago
Component: Gaia → Gaia::TV
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed

Comment 12

3 years ago
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/857be0a240a9be649f40f651359a8cf7c4831db6

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

3 years ago
Daisuke-san,

This patch makes default apps or decks localizable in Smart-Home. I'll suggest you to pick this patch. Thanks.
Flags: needinfo?(938.daisuke)
Whiteboard: [ft:conndevices] → [ft:conndevices][Stingray-Branch]

Comment 14

3 years ago
Hi Tzu-Lin

Stingray has no UI to change locale on b2g(can change by module out of b2g) and
when locale changed, restart b2g.
So currently no need to introduce your patch.

Thanks
Flags: needinfo?(938.daisuke)
You need to log in before you can comment on or make changes to this bug.