987966 - [contacts] placeholder for date fields in add/edit mode is always in English: 'Date'

Status: RESOLVED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Boot2Gecko 1.5.0.0-prerelease 20140325024149 on Keon

The placeholder for date fields in add and edit mode is always in English and reads as 'Date'.

Comment 1

[Francesco Lodolo [:flod]]

I might be wrong, but having chosen data.placeholder as entity name, it gets used to localize the "placeholder" attribute of the element with data-l10n-id="date", not the element with data-l10n-id="date.placeholder".
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/elements/form.html#L77

Comment 2

[Francesco Lodolo [:flod]]

I think this is also on 1.4 :-\

@Jose
Can you please take a look?

From what I understand "date" is actually unused, so it should be possible to just assign it to the other control and get rid of the .placeholder string

Comment 3

[Jose Manuel Cantera]

Attachment: 17632.html

simple patch that fixes the issue. After r+ I will ask for approval to uplift to 1.4

Comment 4

[Francesco Lodolo [:flod]]

Comment on attachment 8396980 [details]
17632.html

You should ask a proper a reviewer, but I have 2 notes:
* you can't have two elements with the same data-l10n-id ("date" is still used below)
* remove the data.placeholder string if it's unused

Comment 5

[Jose Manuel Cantera]

(In reply to Francesco Lodolo [:flod] from comment #4)
> Comment on attachment 8396980 [details]
> 17632.html
> 
> You should ask a proper a reviewer, but I have 2 notes:
> * you can't have two elements with the same data-l10n-id ("date" is still
> used below)

oh yes

> * remove the data.placeholder string if it's unused

I will keep it as it is used by the <input type=date>, although this element is never shown to the user. But for consistency I'm keeping it

thanks for the quick feedback

Comment 6

[Francesco Lodolo [:flod]]

Comment on attachment 8396980 [details]
17632.html

My suggestion of using "date" for the first element, was to avoid a new string, especially since this has to land in 1.4

If we actually need the date.placeholder string in place, I guess this patch is correct.

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8396980 [details]
17632.html

Nice!

Thanks, merge once travis is green.

Comment 8

[Jose Manuel Cantera]

https://github.com/mozilla-b2g/gaia/commit/783ebe5beeb824ebff5cffaaa66e2bb8c9587203

Comment 11

[helloarvin]

Hi Francesco,

we eagerly need merge this modification to the branch v1.4, please help land it.

Thanks a lot.

Comment 12

[Francesco Lodolo [:flod]]

Unfortunately you're talking with the wrong person, not a gaia developer here ;-)

I suggested a string-free approach for this bug, that would have let us land on 1.4.
At this point, introducing new strings in 1.4 is not an option.

Comment 13

[helloarvin]

Hi Jose,

As mentioned in comment3, could you please help land the patch to v1.4?

Thanks a lot.

Comment 14

[Jose Manuel Cantera]

We cannot land this to 1.4 until it is 1.4+ed. If not we would need to ask for an approval-1.4, but let's wait to 1.4+ .

Comment 15

[helloarvin]

Thank you.
Waiting...

Comment 16

[Preeti Raghunath(:Preeti)]

Do we know if this one will become a cert blocker?

Comment 17

[Preeti Raghunath(:Preeti)]

https://github.com/mozilla-b2g/gaia/commit/783ebe5beeb824ebff5cffaaa66e2bb8c9587203#diff-a8e39eba575534c9d35a2b692b6550e1R47

Dylan

Can you please help here?

Comment 18

[Francesco Lodolo [:flod]]

Preeti, Sebastian (archaeopteryx) is the German localizer, definitely not the right person to answer about certification ;-)

Comment 19

[Axel Hecht [:Pike]]

PS: if this turns out to be a cert blocker, I suggest to:

Clone this bug, and request a patch that picks up one of the existing "Date" strings, and make this bug non-block.

I'm pretty sure there has to be some way to hack around this, event listeners or whatnot.

Comment 20

[Dylan Oliver [:doliver]]

Preeti, I'm not sure what help you're asking for. If we need a different patch for 1.4, then Francisco or Jose would be the right people to ask. With the blocking decision earlier today, I expect this patch will be uplifted soon unless it gets marked to avoid uplift.

Comment 21

[Preeti Raghunath(:Preeti)]

(In reply to Francesco Lodolo [:flod] from comment #18)
> Preeti, Sebastian (archaeopteryx) is the German localizer, definitely not
> the right person to answer about certification ;-)

Oops Sorry!! 
Maria,
Please help answer the cert blocker question?
Also would you be able to do the re-base?

Comment 22

[Maria Angeles Oteo (:oteo)]

Beatriz, could you confirm if this bug is a certification blocker?
Thanks a lot!

Comment 23

[Beatriz Rodríguez [:brg]]

yes it is a cert blocker! Thanks for asking.

Comment 24

[Francesco Lodolo [:flod]]

Can we discuss on how to create a patch that doesn't add new strings? As it is, a sheriff will pass by and commit the patch on 1.4

Adding a new string now means that the string is localizable, but it won't necessarily be localized, especially by smaller locales.

https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/communications/contacts/elements/form.html#L77-L80

As I asked at the beginning of this bug, can't we just move the "date" data-l10n-id from input to span? Is the data-l10n-id on the input element actually used?

Comment 25

[Francesco Lodolo [:flod]]

To add more context, these are the strings we already have on 1.4
https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/communications/contacts/locales/contacts.en-US.properties#L47-L48

Comment 26

[Dylan Oliver [:doliver]]

Tagging this to avoid uplift until things get straightened out here.

Comment 27

[yaoyao.wu]

(In reply to Francesco Lodolo [:flod] from comment #24)
> Can we discuss on how to create a patch that doesn't add new strings? As it
> is, a sheriff will pass by and commit the patch on 1.4
> 
> Adding a new string now means that the string is localizable, but it won't
> necessarily be localized, especially by smaller locales.
> 
> https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/communications/contacts/
> elements/form.html#L77-L80
> 
> As I asked at the beginning of this bug, can't we just move the "date"
> data-l10n-id from input to span? Is the data-l10n-id on the input element
> actually used?
Hi Francesco,
change data-l10n-id="date.placeholder" to data-l10n-id="dateplaceholder" 
and the string can be translated into the appropriate language.
This method can avoid add a new string, just remove the dote is OK

Comment 28

[Francesco Lodolo [:flod]]

(In reply to yaoyao.wu from comment #27)
> Hi Francesco,
> change data-l10n-id="date.placeholder" to data-l10n-id="dateplaceholder" 
> and the string can be translated into the appropriate language.
> This method can avoid add a new string, just remove the dote is OK

I doubt that would work (we don't have a string ID called 'dateplaceholder' in .properties file).
Note that I'm the developer who fixed this bug too.

Comment 29

[yaoyao.wu]

(In reply to Francesco Lodolo [:flod] from comment #28)
> (In reply to yaoyao.wu from comment #27)
> > Hi Francesco,
> > change data-l10n-id="date.placeholder" to data-l10n-id="dateplaceholder" 
> > and the string can be translated into the appropriate language.
> > This method can avoid add a new string, just remove the dote is OK
> 
> I doubt that would work (we don't have a string ID called 'dateplaceholder'
> in .properties file).
> Note that I'm the developer who fixed this bug too.
Hi Francesco,
yes , this method need to change both the index.html and corresponding .properties files .
This is just for your reference.
Thanks.

Comment 30

[Francesco Lodolo [:flod]]

And then we're back to square one: we don't want to change the .properties file and introduce new strings months after string freeze. If that would be the case, we could just land the r+ patch in this bug.

This is the stringless approach I suggested a couple of times, if someone wants to test it or comment
https://github.com/flodolo/gaia/commit/a6df479be37be8a97adbb5daa337134a4648838d

Comment 31

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Dylan Oliver [:doliver] from comment #26)
> Tagging this to avoid uplift until things get straightened out here.

Reopening to provide a specific patch for 1.4 that could be applied without any new string.

Prefer to do this instead of backout the patch, since this is in 2.0 and 2.1 already.

Comment 32

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Patch for branch 1.4

Just got the commit by Francesco and tried on branch 1.4

Working for me perfectly but want r? from Jose to see that we are not missing anything.

Thanks Francesco!

Comment 33

[Jose Manuel Cantera]

Comment on attachment 8443516 [details]
Patch for branch 1.4

the proposed patch introduces a regression already fixed in master (bug 1027867)

Comment 34

[Francesco Lodolo [:flod]]

Have you tested the patch on 1.4? Because I don't see anything like bug 1027867 while testing the patch, and we're two revisions apart.

When editing I see "Date" if the date is empty, the actual date if there's a value, and the actual date when viewing the contact.

Comment 35

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: IMG_20140623_201855.jpg

This is a screenshot with the patch applied to branch 1.4.

I cannot see the effect of bug 1027867

Could you recheck?

Comment 36

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: PR to branch 1.4

Comment 37

[Jose Manuel Cantera]

Comment on attachment 8446498 [details] [review]
PR to branch 1.4

thanks Francisco, works perfectly

Comment 38

[Francisco Jordano [:arcturus] [:francisco]]

Dylan,

should I merge directly to 1.4 or a sheriff will take care of this?

Comment 39

[Dylan Oliver [:doliver]]

Francisco, you can either land it and resolve it, or mark it for landing by sheriff by removing the '[NO_UPLIFT]' whiteboard tag and adding the 'checkin-needed' keyword.

And then I'm assuming we can also remove the late-l10n keyword with this approach?

Comment 40

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Dylan Oliver [:doliver] from comment #39)
> Francisco, you can either land it and resolve it, or mark it for landing by
> sheriff by removing the '[NO_UPLIFT]' whiteboard tag and adding the
> 'checkin-needed' keyword.
> 
> And then I'm assuming we can also remove the late-l10n keyword with this
> approach?

We have a specific patch for 2.0 that will still need the late-l10n, so will leave it.

Marking as resolved and asking for checkin-needed

Comment 41

[Francisco Jordano [:arcturus] [:francisco]]

Asking for checkin-needed for attachment:

https://bugzilla.mozilla.org/attachment.cgi?id=8446498

in branch 1.4

Comment 42

[Francesco Lodolo [:flod]]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #40)
> We have a specific patch for 2.0 that will still need the late-l10n, so will
> leave it.

2.0 has been fixed several months ago with a new string, I don't think it needs further fixing. Am I missing something?
Also note that 2.0 is closed for strings, so late-l10n at this point is not relevant.

Comment 43

[Francisco Jordano [:arcturus] [:francisco]]

Thanks Francesco for the clarification, now removing the flag.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/76e669c6fa387f97bcaaaee5d179ee4a9dcca986