Closed Bug 1375883 Opened 7 years ago Closed 7 years ago

Replace "sites" with "websites" in strings for the preferences

Categories

(Firefox :: Settings UI, enhancement, P3)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: jaws, Assigned: jaws, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [photon-preference])

Attachments

(1 file, 1 obsolete file)

Bug 1375883 - Replace 'sites' with 'websites' in strings for preferences since the Mozilla language guide prefers the full 'websites' term.
59 bytes, text/x-review-board-request
mconley
: review+
flod
: feedback+
Details 
Various places within the preferences use the string "sites" when they should be using "websites".

See the following search for affected strings:
http://searchfox.org/mozilla-central/search?q=%5Cssites&case=false&regexp=true&path=browser%2Flocales%2Fen-US%2Fchrome%2Fbrowser%2Fpreferences%2F**.%7Bdtd%2Cproperties%7D%24

@Cindy, can we get this prioritized for photon-preferences? We should be consistent about our strings. I talked to Michelle Heubusch who gave the OK on the string changes.
Flags: needinfo?(chsiang)
Hey Jalen, would you be able to work on this?
Mentor: jaws
Flags: needinfo?(leftysolara)
Whiteboard: [good first bug]
Jared I would like to work on this...

Might require some help though Im new to bugzilla
I want to work on this bug but need some help on how to get started with it. Mentor Jared Wein, could you please guide me through the process ?
@jared, sorry for the late response. looks like you've got help to fix this bug so i'm clearing the need info. ni me again if i'm needed. thanks!
Flags: needinfo?(chsiang)
To fix this bug you'll need to clone mozilla-central and set up your local build environment [1]. I recommend using an Artifact Build[2] for this bug since you don't need to change any C++. Then you will need to change the files listed in the link in comment 0 of this bug. When you change a string, you will also need to change the string entity name, incrementing it by 1 is the standard practice that we use. When you change the string entity name, you will also need to update the code that references it. You can use the searchfox.org website to find the references to the old string.

After you make your changes you should build Firefox and check the preferences to make sure that you see what you had expected.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds
Flags: needinfo?(leftysolara)
Keywords: good-first-bug
Whiteboard: [good first bug]
Attached patch bug-1375883-fix.patch (obsolete) — — Splinter Review 
I am submitting the patch. Kindly review it and notify me if there is a mistake.
Attachment #8886146 - Flags: review?(jaws)
Comment on attachment 8886146 [details] [diff] [review]
bug-1375883-fix.patch

Review of attachment 8886146 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch! Please see my notes below. Note also that when you change an entity name, as I have asked for below, you will need to update the reference to it within the XUL and JS files. You can use http://searchfox.org/ to search for where strings are referenced in the codebase.

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd
@@ -59,5 @@
>  <!ENTITY httpCache.label                 "Cached Web Content">
>  
>  <!ENTITY offlineStorage2.label           "Offline Web Content and User Data">
>  
> -<!--  Site Data section manages sites using Storage API and is under Network -->

We can undo this change since users won't see it.

::: browser/locales/en-US/chrome/browser/preferences/content.dtd
@@ +7,5 @@
>  <!ENTITY  blockPopups.accesskey       "B">
>  
>  <!ENTITY  notificationsPolicy.label            "Notifications">
>  <!ENTITY  notificationsPolicyLearnMore.label   "Learn more">
> +<!ENTITY  notificationsPolicyDesc3.label       "Choose which websites are allowed to send you notifications">

Whenever we change a string, we need to update the entity name too so that localizers will know that the string has changed. Can you please change this to notificationsPolicyDesc4.label ?

See https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings for more information.

::: browser/locales/en-US/chrome/browser/preferences/permissions.dtd
@@ +8,5 @@
>  <!ENTITY treehead.sitename.label      "Site">
>  <!ENTITY treehead.status.label        "Status">
>  <!ENTITY removepermission.label       "Remove Site">
>  <!ENTITY removepermission.accesskey   "R">
> +<!ENTITY removeallpermissions.label   "Remove All websites">

removeallpermissions2.label

@@ +13,1 @@
>  <!ENTITY removeallpermissions.accesskey "e">

This one also needs to be updated so it stays in sync with the label, so change this to removeallpermissions2.accesskey

@@ -24,5 @@
>  <!ENTITY button.cancel.label          "Cancel">
>  <!ENTITY button.cancel.accesskey      "C">
>  <!ENTITY button.ok.label              "Save Changes">
>  <!ENTITY button.ok.accesskey          "S">
> -

Please undo this change.

::: browser/locales/en-US/chrome/browser/preferences/translation.dtd
@@ +17,4 @@
>  <!ENTITY treehead.siteName.label          "Sites">
>  <!ENTITY removeSite.label                 "Remove Site">
>  <!ENTITY removeSite.accesskey             "S">
> +<!ENTITY removeAllSites.label             "Remove All websites">

Please maintain the same capitalization as before, so this should be "Remove All Websites"
Attachment #8886146 - Flags: review?(jaws) → review-
Assignee: nobody → srivatsav1998
Status: NEW → ASSIGNED
Hi Srivatsav, are you able to keep working on this bug?
Flags: needinfo?(srivatsav1998)
Whiteboard: [photon-preference][triage]
Flags: qe-verify+
Priority: -- → P3
QA Contact: hani.yacoub
Whiteboard: [photon-preference][triage] → [photon-preference]
I'm going to put a patch up here so we can get this landed in time for 56 if possible.

@Srivatsav, please let me know if you are still looking to contribute and I will find another bug that you can help out with.
Assignee: srivatsav1998 → jaws
Flags: needinfo?(srivatsav1998)
Attachment #8886146 - Attachment is obsolete: true
Comment on attachment 8892157 [details]
Bug 1375883 - Replace 'sites' with 'websites' in strings for preferences since the Mozilla language guide prefers the full 'websites' term.

flod, can you look over these changes?
Attachment #8892157 - Flags: feedback?(francesco.lodolo)
Attachment #8892157 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8892157 [details]
Bug 1375883 - Replace 'sites' with 'websites' in strings for preferences since the Mozilla language guide prefers the full 'websites' term.

https://reviewboard.mozilla.org/r/163126/#review168490

Seems okay, but one string needs a new key.

Probably a good idea to locally test with both the new and old Preferences orgs to ensure there's no Yellow Screen of Death.

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:54
(Diff revision 2)
>  # string. Note that in the future these two strings (name, desc) could be
>  # displayed on two different lines.
>  mozstdName=Disconnect.me basic protection (Recommended).
>  mozstdDesc=Allows some trackers so websites function properly.
>  mozfullName=Disconnect.me strict protection.
> -mozfullDesc=Blocks known trackers. Some sites may not function properly.
> +mozfullDesc=Blocks known trackers. Some websites may not function properly.

Needs a new key.
Attachment #8892157 - Flags: review+
(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #14)
> Comment on attachment 8892157 [details]
> Bug 1375883 - Replace 'sites' with 'websites' in strings for preferences
> since the Mozilla language guide prefers the full 'websites' term.
> 
> https://reviewboard.mozilla.org/r/163126/#review168490
> 
> Seems okay, but one string needs a new key.
> 
> Probably a good idea to locally test with both the new and old Preferences
> orgs to ensure there's no Yellow Screen of Death.

Thanks, yes I tested with both old and new preferences.
 
> :::
> browser/locales/en-US/chrome/browser/preferences/preferences.properties:54
> (Diff revision 2)
> >  # string. Note that in the future these two strings (name, desc) could be
> >  # displayed on two different lines.
> >  mozstdName=Disconnect.me basic protection (Recommended).
> >  mozstdDesc=Allows some trackers so websites function properly.
> >  mozfullName=Disconnect.me strict protection.
> > -mozfullDesc=Blocks known trackers. Some sites may not function properly.
> > +mozfullDesc=Blocks known trackers. Some websites may not function properly.
> 
> Needs a new key.

I have fixed this now. This was non-fatal due to the unconventional way that the Tracking Protection blocklist dialog builds its strings from about:config preference names.
Attachment #8892157 - Flags: feedback?(francesco.lodolo)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0f6f2d7421e6
Replace 'sites' with 'websites' in strings for preferences since the Mozilla language guide prefers the full 'websites' term. r=mconley
Comment on attachment 8892157 [details]
Bug 1375883 - Replace 'sites' with 'websites' in strings for preferences since the Mozilla language guide prefers the full 'websites' term.

String changes look good, but there's one huge concern, and it's timing.

Tomorrow is August 2, merge day from central to beta. Here's what's going to happen:
* You land this, you might or might not catch the merge. If you miss, it becomes an uplift request (approval-mozilla-beta?).
* All these strings will fall back to English in the first betas, because nobody will have time to translate them right on. And these are not hidden strings, they're in the face of the user. 
* Churn: localizers will have to work on them twice, one in beta and one in central.

So, is there a good reason to land this now instead of fixing only the new preferences when 57 is in central? We have had this issue for years, it's hard for me to understand why we would need to land this right now.
Attachment #8892157 - Flags: feedback?(francesco.lodolo) → feedback+
https://hg.mozilla.org/mozilla-central/rev/0f6f2d7421e6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #19)
> https://hg.mozilla.org/mozilla-central/rev/0f6f2d7421e6

So much for all my comment above :-\

What's the point of asking for feedback if the patch is already in autoland? This is really frustrating, considering it took me only 9h to get to this f?
(In reply to Francesco Lodolo [:flod] from comment #18)
> Comment on attachment 8892157 [details]
> Bug 1375883 - Replace 'sites' with 'websites' in strings for preferences
> since the Mozilla language guide prefers the full 'websites' term.
> 
> String changes look good, but there's one huge concern, and it's timing.
> 
> Tomorrow is August 2, merge day from central to beta. Here's what's going to
> happen:
> * You land this, you might or might not catch the merge. If you miss, it
> becomes an uplift request (approval-mozilla-beta?).
> * All these strings will fall back to English in the first betas, because
> nobody will have time to translate them right on. And these are not hidden
> strings, they're in the face of the user. 
> * Churn: localizers will have to work on them twice, one in beta and one in
> central.
> 
> So, is there a good reason to land this now instead of fixing only the new
> preferences when 57 is in central? We have had this issue for years, it's
> hard for me to understand why we would need to land this right now.

The new preferences are shipping in 56. There will be some visual changes that will ship with 57 but 56 will be where users will see the reorganized preferences for the first time.

I wanted to land this ASAP so as to make sure that it made it to m-c before the merge because I didn't want to uplift strings since that is always frowned upon. I didn't know that Nightly is considered as partially string-frozen during the week before the merge.

As to the reason, it's important that the first release of the new preferences look and function as good as we can make it. Fixing consistency issues within the preferences was one of the large parts of the re-org work.

I agree that shipping the first beta with untranslated strings is bad. Is there anything we can do here to mitigate this? Would an email to the l10n mailing list help?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> The new preferences are shipping in 56. There will be some visual changes
> that will ship with 57 but 56 will be where users will see the reorganized
> preferences for the first time.

So, why are we touching preferences-old then? Was it really necessary? 

> I agree that shipping the first beta with untranslated strings is bad. Is
> there anything we can do here to mitigate this? Would an email to the l10n
> mailing list help?

There's really no mitigation for this, we need better planning and visibility.
(In reply to Francesco Lodolo [:flod] from comment #22)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> > The new preferences are shipping in 56. There will be some visual changes
> > that will ship with 57 but 56 will be where users will see the reorganized
> > preferences for the first time.
> 
> So, why are we touching preferences-old then? Was it really necessary? 

Some dialogs did not get their string files duplicated (permissions.dtd). I made the changes to all string files to maintain consistency since I didn't see a strong reason against consistency.
 
> > I agree that shipping the first beta with untranslated strings is bad. Is
> > there anything we can do here to mitigate this? Would an email to the l10n
> > mailing list help?
> 
> There's really no mitigation for this, we need better planning and
> visibility.

I'm sorry for landing these strings so late in the cycle. I'll flag you sooner next time as well as try to land them not so close to the merge.
I have reproduced this bug with Nightly 56.0a1 (2017-06-23) on Windows 8, 64-bit!

The bug's fix is now verified on Latest Beta 56.0b4.

Build ID 	20170818035007
User Agent 	Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

[testday-20170818]
Marking this as verified fixed, based on comment 24. 
Thank you Anika for your testing efforts.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: