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

RESOLVED FIXED in Firefox 60

Status

()

P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mwalkington, Assigned: gregorywlodarek, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 60
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Posted image 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.
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
Mentor: sfoster
Keywords: good-first-bug
Priority: -- → P5
(Assignee)

Comment 2

a year ago
I'd like to work on this!
(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.
Assignee: nobody → gregorywlodarek
Flags: needinfo?(gregorywlodarek)
(Assignee)

Comment 4

a year ago
Posted patch 1434427.patchSplinter Review
Attachment #8950133 - Flags: review?(sfoster)
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
Flags: needinfo?(gregorywlodarek)
Attachment #8950133 - Flags: review?(sfoster) → feedback+

Comment 7

a year ago
mozreview-review
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.
Attachment #8951098 - Flags: review+
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)
: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.
Flags: needinfo?(gregorywlodarek)
(Assignee)

Comment 10

a year ago
:sfoster, no problem, will do it by tonight!
Flags: needinfo?(gregorywlodarek)
Comment hidden (mozreview-request)
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.
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 hidden (mozreview-request)
Attachment #8951098 - Attachment is obsolete: true

Comment 15

a year ago
mozreview-review
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.
Attachment #8954125 - Flags: review?(francesco.lodolo) → review+
: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

a year ago
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f285b62e035
Replace string for preferences confirmRestartPrompt to use "cancel" instead of "revert". r=flod

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f285b62e035
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.