1434427 - Recommend replacing "Revert" with "Cancel" for "Always Use Private Browsing Mode" selection

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P5)

Description

[Meridel [:meridel]]

Attachment: Revert.png

Location: Preferences > Privacy & Security > History > Use Custom Settings for History > "Always use private browsing mode"

When you select "Always use private browsing mode" you receive a pop-up with the button options: Revert or Restart Firefox Now. "Revert" is a confusing word choice as no changes have been made yet, and the term in of itself is more technical than necessary -- I would expect this button to say "Cancel," instead.

Comment 1

[Sam Foster [:sfoster] (he/him)]

This seems like a reasonable change to make in this context. We'll just need to bump the string name and value here: https://searchfox.org/mozilla-central/rev/f41017f5cd7b5f3835d282fbe011d2899574fc2b/browser/locales/en-US/chrome/browser/preferences/preferences.properties#228

Comment 2

[gregorywlodarek]

I'd like to work on this!

Comment 3

[Sam Foster [:sfoster] (he/him)]

(In reply to gregorywlodarek from comment #2)
> I'd like to work on this!

That's great. You'll need to go through the steps to get a local build of firefox if you've not already done this. I'd suggest starting here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Note you wont need to compile any C++ or touch platform code so a *manifest* build will be adequate here: much faster to set up and faster to build. If you get stuck, I can help with this (sfoster in #fx-team on irc), or ask anyone in that channel if our timezones and work-times don't line up. 

Then, read up on localization. This change is pretty simple, but its good to know some context. I'd suggest maybe starting here: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices

Your patch should change the string name as well as the value, this makes it clear to localizers that the wording changed and they'll need to re-translate it.

You should use searchfox.org or grep through your local checkout of the hg repo to find all the places where this string is used, and double-check the change makes sense in each context. 

When you have a patch ready, attach it here and flag me as the reviewer (simplest). Or, if you can set up mozreview and push there to trigger review. 
 
I'll assign this bug to you. If you find you aren't able to work on this in the next week or two, please let me know so we can reassign it to someone else.

Comment 4

[gregorywlodarek]

Attachment: 1434427.patch

Comment 5

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8950133 [details] [diff] [review]
1434427.patch

Thanks - this patch looks good. It will need a commit message before I can land it. Something along the lines of: 

Bug 1434427 - Replace string for preferences confirmRestartPrompt to use "cancel" instead of "revert" 

If you set up mozreview you that will make this and future patches easier to review and gives us a good workflow for iteration, testing and landing a patch: https://mozilla-l10n.github.io/documentation/tools/mercurial/setting_mercurial_environment.html

Comment 6

[gregorywlodarek]

Attachment: Bug 1434427 - Replace string for preferences confirmRestartPrompt to use "cancel" instead of "revert"

Review commit: https://reviewboard.mozilla.org/r/220356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220356/

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8951098 [details]
Bug 1434427 - Replace string for preferences confirmRestartPrompt to use "cancel" instead of "revert"

https://reviewboard.mozilla.org/r/220356/#review226316

That'll do it. Thanks!
I got a verbal ok from :jaws to review the patch on his behalf, so I'll push this to autoland.

Comment 8

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 26b088a4e60ebf98f99edd6d0f6a1ef2849a302a -d 6d4e050bc406: rebasing 447463:26b088a4e60e "Bug 1434427 - Replace string for preferences confirmRestartPrompt to use "cancel" instead of "revert" r=sfoster" (tip)
merging browser/components/preferences/in-content/preferences.js
merging browser/locales/en-US/chrome/browser/preferences/preferences.properties
warning: conflicts while merging browser/components/preferences/in-content/preferences.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/locales/en-US/chrome/browser/preferences/preferences.properties! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 9

[Sam Foster [:sfoster] (he/him)]

:gregorywlodarek, this is rotten luck. Your patch tried to land just after a major change to the way the preferences UI gets localized, and the string name you were updating has been moved and changed. You can see the conflicting changeset here: https://reviewboard.mozilla.org/r/208354/diff/15/

Comment #8 says to rebase, but I just had a look at its going to be easier to start over. You'll need to update your repo and return to the central branch. 

The new mechanism for defining, loading and injecting localized strings is called Fluent. You can read up about that at http://projectfluent.org/  The block of the preferences.properties file you were editing has been moved to a preferences.ftl in the same directory. The string that had the id "revertNoRestartButton" is now known as "revert-no-restart-button" in that file. 

I hope this make sense, and my apologies I didn't see this coming, I didn't know this change was so close to landing.

Comment 10

[gregorywlodarek]

:sfoster, no problem, will do it by tonight!

Comment 11

[gregorywlodarek]

Comment on attachment 8951098 [details]
Bug 1434427 - Replace string for preferences confirmRestartPrompt to use "cancel" instead of "revert"

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220356/diff/1-2/

Comment 12

[Sam Foster [:sfoster] (he/him)]

Sorry for the delay on this. I got sick. The updated patch looks good. I've pushed it to autoland, which should mean that it'll get merged and show up in Nightly tomorrow or so.

Comment 13

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 3 changes to 3 files
remote: (ftl_check check enabled per config override)
remote: 
remote: ************************ ERROR *************************
remote: You are trying to commit a change to an FTL file.
remote: At the moment modifying FTL files requires a review from
remote: one of the L10n Drivers.
remote: Please, request review from either:
remote:     - Francesco Lodolo (:flod)
remote:     - Zibi Braniecki (:gandalf)
remote:     - Axel Hecht (:pike)
remote:     - Stas Malolepszy (:stas)
remote: ********************************************************
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.mozhooks hook failed
abort: push failed on remote

Comment 14

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1434427 - Replace string for preferences confirmRestartPrompt to use "cancel" instead of "revert".

* Removed the revertNoRestartButton entry from the migration script as this no longer exists after this change

Review commit: https://reviewboard.mozilla.org/r/223270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223270/

Comment 15

[Francesco Lodolo [:flod]]

Comment on attachment 8954125 [details]
Bug 1434427 - Replace string for preferences confirmRestartPrompt to use "cancel" instead of "revert".

https://reviewboard.mozilla.org/r/223270/#review229310

Thanks, looks good.

Comment 16

[Sam Foster [:sfoster] (he/him)]

:gregorywlodarek, you'll see in #comment 13, when I tried to land this, we have a rule currently set up to ensure these kind of patches get review from a localization peer (which I was unaware of.) After consulting on the #fx-team channel, I went ahead an implemented their suggestion to remove the revertNoRestart entry from the migration script rather than update it. 

Your patch has had r+ and is pushed again to autoland.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f285b62e035
Replace string for preferences confirmRestartPrompt to use "cancel" instead of "revert". r=flod

Comment 18

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/8f285b62e035