1196949 - Use Cancel string provided by Firefox language file

Status: RESOLVED FIXED

(Firefox for iOS :: Browser, defect)

Description

[Ton [:Tonnes]]

When testing by Onno and looking at the screenshot (see URL), it turns out the Cancel string in one particular string/screen is probably obtained from iOS itself, resulting in conflicting wording style.

Instead, it should use all Firefox strings by default like the other Cancel occurrences in Firefox. There may also just be a need for an additional entry for this screen only, similar to the Add string.

Comment 1

[Stephan Leroux [:sleroux]]

Attachment: https://github.com/mozilla/firefox-ios/pull/1008

Comment 2

[Stephan Leroux [:sleroux]]

Attachment: https://github.com/mozilla/firefox-ios/pull/1009

Comment 3

[Stephan Leroux [:sleroux]]

I also updated the system buttons that used the Done action instead of our localized copy.

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8655444 [details] [review]
https://github.com/mozilla/firefox-ios/pull/1008

LGTM

Comment 5

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8655446 [details] [review]
https://github.com/mozilla/firefox-ios/pull/1009

LGTM

Comment 6

[Stephan Leroux [:sleroux]]

Merged.

Comment 7

[Stefan Plewako [:stef]]

(In reply to Stephan Leroux [:sleroux] from comment #3)
> I also updated the system buttons that used the Done action instead of our
> localized copy.

Either l10n comment is misleading (all the title occurrences instead of button label) or this effectively reuses one string for button labels and other elements which will not work well.

https://github.com/mozilla/firefox-ios/commit/6eb78d27624fbcd116a03cc9ca5a25650fd11246

Comment 8

[Stephan Leroux [:sleroux]]

Sorry about that - ya 'button label' is probably better than 'title'. I can submit a patch that will fix that. Thanks!

Comment 9

[Stephan Leroux [:sleroux]]

Attachment: https://github.com/mozilla/firefox-ios/pull/1017

I've updated the comment for the done buttons. Does this work?

Comment 10

[Stefan Plewako [:stef]]

Comment on attachment 8655976 [details] [review]
https://github.com/mozilla/firefox-ios/pull/1017

Yes, this makes comments for "Done" more understandable, thx.

In the same time I wonder why "Cancel title from search engine picker" and "Cancel title for selecting client screen" comments from 6eb78d27624fbcd116a03cc9ca5a25650fd11246 commit do not show up in exported xliff.

Comment 11

[Stephan Leroux [:sleroux]]

Looks like there was no tableName specified for the strings I added. Also looks like this might be the cause for a bunch of other strings. I've filed a follow bug for to investigate these warnings here: https://bugzilla.mozilla.org/show_bug.cgi?id=1201102

Comment 13

[Stefan Plewako [:stef]]

This should be reopened as the patch doesn't seem to work as expected, https://bugzilla.mozilla.org/attachment.cgi?id=8663926

Comment 14

[Stephan Leroux [:sleroux]]

I wonder if the export conflict is causing this string to not be shown

https://bugzilla.mozilla.org/show_bug.cgi?id=1201102

Will reopen and investigate

Comment 15

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I doubt it has anything to do with bug 1201102

I think the problem here is that the Cancel string is missing the tableName: "SendTo". Without that, the string ends up in the main strings table and I'm pretty sure the extension cannot access strings from the main bundle.

Comment 16

[Stephan Leroux [:sleroux]]

Ah that makes more sense. How would that impact our string freeze? Simple fix code wise.

Comment 17

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

We cannot fix this because we are past string freeze.

What I would like to suggest is that we revert the change. Instead of the NLocalizedString() strings we go back to using UIBarButtonSystemItem.Cancel and UIBarButtonSystemItem.Done

We will fix this as soon as possible in a followup release.

I understand that this means that we will use language that :stef prefers not to use, but it will be 'iOS standard' so I think it is a good short term compromise.

Comment 18

[Francesco Lodolo [:flod]]

(In reply to Stefan Arentz [:st3fan] from comment #17)
> I understand that this means that we will use language that :stef prefers
> not to use, but it will be 'iOS standard' so I think it is a good short term
> compromise.

Sounds good for me, and I think the issue was more for Ton (and Dutch) who reported the bug originally.

Comment 19

[Stefan Plewako [:stef]]

I wouldn't care about string freeze but exactly what Flod said, in Polish there is no difference - unless the sting is in English of course…

Comment 20

[Ton [:Tonnes]]

(In reply to Francesco Lodolo [:flod] from comment #18)
> Sounds good for me, and I think the issue was more for Ton (and Dutch) who
> reported the bug originally.

As well as other locales suffering this. Just to be sure: it couldn’t be (temporarily) fixed just by adding a Cancel string for that screen only, as mentioned in comment 0?

Comment 21

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

As a compromise we have just landed bug 1214676

This will use a system provided Cancel button (UIBarButtonSystemItem.Cancel) in the Send To and Share To dialogs.

When the string windows opens again for the next release (either bugfix or big release) we can work on this bug again.

(This bug will then be about reverting bug 1214676 and making sure the strings have the correct table name specified)

Comment 22

[Ton [:Tonnes]]

Attachment: Screenshot using the Cancel string

Attaching screenshot to the bug just in case.

Comment 23

[Stephan Leroux [:sleroux]]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1677

Comment 24

[Stephan Leroux [:sleroux]]

master f998f960111e14c8c88f622c1c0056a558d24180

Comment 25

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: PR https://github.com/mozilla/firefox-ios/pull/1768

Comment 26

[Brian Nicholson (:bnicholson)]

Comment on attachment 8747743 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/1768

If you say it works, I'll take your word for it!

I don't like the fact that this requires another argument to NSLocalizedString -- that's just one more thing for us to get wrong. I'll keep playing around with strings to see if we can simplify this process more. I'm still not happy with it :(