Use Cancel string provided by Firefox language file

RESOLVED FIXED in 4.0

Status

()

Firefox for iOS
Browser
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ton, Assigned: st3fan)

Tracking

unspecified
All
iOS
Dependency tree / graph

Firefox Tracking Flags

(fxios4.0+, fxios-v3.0 affected, fxios-v4.0 fixed)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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.
tracking-fxios: --- → ?
tracking-fxios: ? → +
Assignee: nobody → sleroux
tracking-fxios: + → 1.0.5+
Created attachment 8655444 [details] [review]
https://github.com/mozilla/firefox-ios/pull/1008
Attachment #8655444 - Flags: review?(sarentz)
Created attachment 8655446 [details] [review]
https://github.com/mozilla/firefox-ios/pull/1009
Attachment #8655446 - Flags: review?(sarentz)
I also updated the system buttons that used the Done action instead of our localized copy.
(Assignee)

Comment 4

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

LGTM
Attachment #8655444 - Flags: review?(sarentz) → review+
(Assignee)

Comment 5

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

LGTM
Attachment #8655446 - Flags: review?(sarentz) → review+
Merged.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(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
Flags: needinfo?(sleroux)
Sorry about that - ya 'button label' is probably better than 'title'. I can submit a patch that will fix that. Thanks!
Flags: needinfo?(sleroux)
Created attachment 8655976 [details] [review]
https://github.com/mozilla/firefox-ios/pull/1017

I've updated the comment for the done buttons. Does this work?
Attachment #8655976 - Flags: feedback?(splewako)
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.
Attachment #8655976 - Flags: feedback?(splewako) → feedback+
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
Duplicate of this bug: 1206939
This should be reopened as the patch doesn't seem to work as expected, https://bugzilla.mozilla.org/attachment.cgi?id=8663926
Flags: needinfo?(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
Status: RESOLVED → REOPENED
Flags: needinfo?(sleroux)
Resolution: FIXED → ---
(Assignee)

Comment 15

2 years ago
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.
Ah that makes more sense. How would that impact our string freeze? Simple fix code wise.
Flags: needinfo?(sarentz)
tracking-fxios: 1.0.5+ → 2.0+
OS: iOS 8 → iOS
Hardware: Other → All

Updated

2 years ago
Blocks: 1092387
(Assignee)

Comment 17

2 years ago
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.
Flags: needinfo?(splewako)
Flags: needinfo?(sarentz)
Flags: needinfo?(francesco.lodolo)
(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.
Flags: needinfo?(francesco.lodolo)
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…
Flags: needinfo?(splewako)
(Reporter)

Comment 20

2 years ago
(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?

Updated

2 years ago
Blocks: 1212574
(Assignee)

Comment 21

2 years ago
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)
(Assignee)

Updated

2 years ago
Whiteboard: [needstrings]
tracking-fxios: 2.0+ → +
(Assignee)

Updated

2 years ago
status-fxios-v3.0: --- → affected
tracking-fxios: + → 3.0+
(Reporter)

Comment 22

2 years ago
Created attachment 8728454 [details]
Screenshot using the Cancel string

Attaching screenshot to the bug just in case.
(Assignee)

Updated

2 years ago
Rank: 3
tracking-fxios: 3.0+ → 4.0+
Created attachment 8736002 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1677
Attachment #8655444 - Attachment is obsolete: true
Attachment #8655446 - Attachment is obsolete: true
Attachment #8655976 - Attachment is obsolete: true
Attachment #8736002 - Flags: review?(sarentz)
(Assignee)

Updated

2 years ago
Attachment #8736002 - Flags: review?(sarentz) → review?(bnicholson)
Attachment #8736002 - Flags: review?(bnicholson) → review+
master f998f960111e14c8c88f622c1c0056a558d24180
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-fxios-v4.0: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 4.0
(Assignee)

Comment 25

2 years ago
Created attachment 8747743 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/1768
Attachment #8747743 - Flags: review?(bnicholson)
(Assignee)

Updated

2 years ago
Whiteboard: [needstrings] → [needsuplift]
(Assignee)

Updated

2 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 years ago
Attachment #8747743 - Flags: review?(sleroux)
(Assignee)

Updated

2 years ago
Assignee: sleroux → sarentz
Attachment #8747743 - Flags: review?(sleroux) → review+
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 :(
Attachment #8747743 - Flags: review?(bnicholson) → review+
(Assignee)

Updated

2 years ago
Whiteboard: [needsuplift]
(Assignee)

Updated

2 years ago
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.