Re-arrange sections in about:preferences according to updated spec

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Preferences
RESOLVED FIXED
4 months ago
11 hours ago

People

(Reporter: mconley, Assigned: Zachary Herrick, Mentored, NeedInfo)

Tracking

(Depends on: 3 bugs, Blocks: 2 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

4 months ago
See slide 3 of this slide deck: https://docs.google.com/presentation/d/1R-HofYuN0bJcROlwUXStxr5CqcOF6WKooZgciikFJgw/edit
(Reporter)

Updated

4 months ago
Blocks: 1324168
Assignee: nobody → herrickz
Mentor: jaws@mozilla.com, mconley@mozilla.com
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8834653 [details]
Bug 1335907 - Pref Reorg. WORK IN PROGRESS.

https://reviewboard.mozilla.org/r/110514/#review112114

It looks like you've got a good start. You should also be moving the JS functions around that need to move.
Attachment #8834653 - Flags: review?(jaws)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8834653 [details]
Bug 1335907 - Pref Reorg. WORK IN PROGRESS.

https://reviewboard.mozilla.org/r/110514/#review112886

There are some issues here with how the patch file is generated. We can go over that in person tomorrow.
Attachment #8834653 - Flags: review?(jaws)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8834653 [details]
Bug 1335907 - Pref Reorg. WORK IN PROGRESS.

Clearing review on the older patch.
Attachment #8834653 - Flags: review?(mconley)
Attachment #8834653 - Flags: review?(jaws)
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111802/#review113388

Overall this is looking good. Please delete the old files that are mostly commented out (using `hg rm path/to/file`). The file will also need to be removed from the jar.mn file. 

After removing the files, please run the tests using `mach test browser/components/preferences` and also run eslint using `mach eslint browser/components/preferences`.
Attachment #8836376 - Flags: review?(jaws)

Comment 15

3 months ago
I mostly like the changes, I am always looking for the password manager in the privacy tab for some reason. I am not sure about some of the decisions, though:

1. What will happen when I click “Change Search settings” in the search entry? Will it open the “General” tab and scroll to the “Default search engine” section? I use the “Change Search settings” button quite often and having to find the relevant section in the tab every time would not be a very smooth experience.

2. Why are “Updates” so prominent to deserve their own tab? On some platforms the section is completely irrelevant as updates are often handled by system-wide package manager (on Arch Linux even for Nightly). Telemetry could also be considered falling into “Privacy” category.

3. The ellipses look quite out of place – they are too far from the centre of the line and too bold. Additionally, they still seem to be rather inconsistent – for example “Change Master Password”, unlike remaining actions (“Clear all data”) are followed by ellipsis, same for “Remove” button at the bottom of the page where it stands out even more. On the other hand “View certificates” could probably be changed to “Certificates” to mirror “Saved Logins”. https://blog.nightly.mozilla.org/files/2017/02/4-privacySecurity.png

4. Speaking of buttons, how do you decide if an action should be a link or a button? It looks to me like the links are used for less important actions. If it is that way, some buttons like password manager’s “Exceptions” could be made into links.

5. The headings use nouns, “Use Tracking Protection” is an exception here.

6. The learn more link is sometimes next to heading (“Container Tabs”, “Tracking Protection”) and sometimes next to the copy text (“DRM Content”, “Notifications”).

7. “Share additional data” copy text is cropped on smaller screens: https://blog.nightly.mozilla.org/files/2017/02/5-update.png

I used images from the Nightly blog as a reference, sorry if some of the points were fixed in the meanwhile. https://blog.nightly.mozilla.org/2017/02/14/these-weeks-in-firefox-issue-10/
To me it seems a bit unbalanced - General + Privacy & Security are very long pages (that exactly what I hate in Chromium, where all preferences are on one page), where you need to scroll to discover the options you can set.
(Reporter)

Comment 17

3 months ago
needinfo'ing Tina for comment 15 and comment 16 in case she wants to weigh in on this feedback.
Flags: needinfo?(thsieh)
(Reporter)

Comment 18

3 months ago
mozreview-review
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111802/#review114198

This is looking good, you two. Great work!

There's a lot of dead code to remove, and I've got some notes on strings below. Otherwise, it's mostly style nits. Thanks!

::: browser/components/preferences/in-content/advanced.xul:45
(Diff revision 2)
>  #endif
>  
>    <preference id="browser.search.update"
>                name="browser.search.update"
>                type="bool"/>
> -
> +               

Trailing whitespace

::: browser/components/preferences/in-content/privacy.js:12
(Diff revision 2)
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "ContextualIdentityService",
>                                    "resource://gre/modules/ContextualIdentityService.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
>                                    "resource://gre/modules/PluralForm.jsm");
>  

Let's get rid of this newline

::: browser/components/preferences/in-content/privacy.js:14
(Diff revision 2)
>                                    "resource://gre/modules/ContextualIdentityService.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
>                                    "resource://gre/modules/PluralForm.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "LoginHelper",
> + "resource://gre/modules/LoginHelper.jsm");

Please align this line such that `"resource://.."` is lined up under `(this, `, like lines 10 and 11. We try to remain consistent within files (even if we're bad at being truly consistent across files. :| )

::: browser/components/preferences/in-content/privacy.js:16
(Diff revision 2)
>                                    "resource://gre/modules/PluralForm.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "LoginHelper",
> + "resource://gre/modules/LoginHelper.jsm");
> +
> +Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");

We should load this lazily, like above.

::: browser/components/preferences/in-content/privacy.js:808
(Diff revision 2)
> +   toggleDoNotDisturbNotifications(event) {
> +    AlertsServiceDND.manualDoNotDisturb = event.target.checked;
> +  },

Busted alignment here. Line 808 looks okay, but line 809 needs to go in a space, and line 810 needs to be lined up with the "t" in "toggleDoNotDisturbNotifications".

::: browser/components/preferences/in-content/privacy.js:1124
(Diff revision 2)
> +  showSecurityDevices() {
> +    gSubDialog.open("chrome://pippki/content/device_manager.xul");
> +  },
> +
> +
> +  // NETWORK 

Trailing whitespace.

::: browser/components/preferences/in-content/privacy.xul:15
(Diff revision 2)
> +	      hidden="true"
>                type="bool"/>
>    <preference id="privacy.trackingprotection.pbmode.enabled"
>                name="privacy.trackingprotection.pbmode.enabled"
> +	      hidden="true"
>                type="bool"/>
> -
> +	      
>    <!-- XXX button prefs -->
>    <preference id="pref.privacy.disable_button.cookie_exceptions"
> +              hidden="true"
>                name="pref.privacy.disable_button.cookie_exceptions"
>                type="bool"/>
>    <preference id="pref.privacy.disable_button.view_cookies"
> +              hidden="true"
>                name="pref.privacy.disable_button.view_cookies"

I don't think <xul:preference> nodes know anything about being "hidden" - and that's okay; they're not visible by default anyways. I think you can get rid of any place where you set hidden="true" on these - I'm reasonably sure they're being ignored.

::: browser/components/preferences/in-content/privacy.xul:21
(Diff revision 2)
>                type="bool"/>
>    <preference id="privacy.trackingprotection.pbmode.enabled"
>                name="privacy.trackingprotection.pbmode.enabled"
> +	      hidden="true"
>                type="bool"/>
> -
> +	      

Trailing whitespace

::: browser/components/preferences/in-content/privacy.xul:478
(Diff revision 2)
>      &suggestionSettings.label;
>    </label>
>  </groupbox>
>  
> +
> +  <!-- Certificates -->

Indentation is busted here.

::: browser/components/preferences/in-content/privacy.xul:500
(Diff revision 2)
> +<separator/>
> +<checkbox id="enableOCSP"
> +          label="&enableOCSP.label;"
> +          accesskey="&enableOCSP.accesskey;"
> +          onsyncfrompreference="return gPrivacyPane.readEnableOCSP();"
> +          onsynctopreference="return gPrivacyPane.writeEnableOCSP();"
> +          preference="security.OCSP.enabled"/>
> +<separator/>
> +<hbox>
> +  <button id="viewCertificatesButton"
> +          flex="1"
> +          label="&viewCerts.label;"
> +          accesskey="&viewCerts.accesskey;"
> +          preference="security.disable_button.openCertManager"/>
> +  <button id="viewSecurityDevicesButton"
> +          flex="1"
> +          label="&viewSecurityDevices.label;"
> +          accesskey="&viewSecurityDevices.accesskey;"
> +          preference="security.disable_button.openDeviceManager"/>
> +  <hbox flex="10"/>
> + </hbox>

Indentation looks a bit busted here - this is still inside the groupbox introduced on line 479.

::: browser/components/preferences/in-content/search.xul:1
(Diff revision 2)
> -    <preferences id="searchPreferences" hidden="true" data-category="paneSearch">
> +    <!--<preferences id="searchPreferences" hidden="true" data-category="paneSearch">

Please remove dead code where possible.

::: browser/components/preferences/in-content/search.xul:22
(Diff revision 2)
> -    <hbox id="header-search"
> +    <!--<hbox id="header-search"
>            class="header"
>            hidden="true"
>            data-category="paneSearch">
>        <label class="header-name" flex="1">&paneSearch.title;</label>
>        <html:a class="help-button" target="_blank" aria-label="&helpButton.label;"></html:a>
> -    </hbox>
> +    </hbox>-->
>  
>      <!-- Default Search Engine -->
> -    <groupbox id="defaultEngineGroup" align="start" data-category="paneSearch">
> +    <!--<groupbox id="defaultEngineGroup" align="start" data-category="paneSearch">

Same as above.

::: browser/components/preferences/in-content/security.js:17
(Diff revision 2)
>  
>    /**
>     * Initializes master password UI.
>     */
>    init() {
> -    function setEventListener(aId, aEventType, aCallback) {
> +    // function setEventListener(aId, aEventType, aCallback) {

As above and elsewhere - please remove dead code.

::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:172
(Diff revision 2)
>  
>    yield BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_BASE_URL + "site_data_test.html");
>    yield waitForEvent(gBrowser.selectedBrowser.contentWindow, "test-indexedDB-done");
>    yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
>  
> -  yield openPreferencesViaOpenPreferencesAPI("advanced", "networkTab", { leaveOpen: true });
> +  yield openPreferencesViaOpenPreferencesAPI("privacy", undefined, { leaveOpen: true });

Other callers are using "null" instead of "undefined" for the second arg when not choosing a particular tab - might as well be consistent.

::: browser/components/preferences/in-content/tests/browser_bug1020245_openPreferences_to_paneContent.js:15
(Diff revision 2)
>  add_task(function*() {
> -  let prefs = yield openPreferencesViaOpenPreferencesAPI("paneContent");
> -  is(prefs.selectedPane, "paneContent", "Content pane was selected");
> -  prefs = yield openPreferencesViaOpenPreferencesAPI("advanced", "updateTab");
> +  let prefs = yield openPreferencesViaOpenPreferencesAPI("panePrivacy");
> +  is(prefs.selectedPane, "panePrivacy", "Privacy pane was selected");
> +  prefs = yield openPreferencesViaOpenPreferencesAPI("advanced", undefined);
>    is(prefs.selectedPane, "paneAdvanced", "Advanced pane was selected");
> -  is(prefs.selectedAdvancedTab, "updateTab", "The update tab within the advanced prefs should be selected");
> +  // is(prefs.selectedAdvancedTab, "updateTab", "The update tab within the advanced prefs should be selected");

More dead code.

::: browser/components/preferences/in-content/tests/browser_security.js:76
(Diff revision 2)
>      is(checked, val, "downloads preference is initialized correctly");
>      // should be disabled when val is false (= pref is turned off)
>      is(blockUncommon.hasAttribute("disabled"), !val, "block uncommon checkbox is set correctly");
>  
> -    // click the checkbox
> +    // scroll the checkbox into view and click it
> +    checkbox.scrollIntoView();

Hehehe - remember how much of a pain in the ass this was to figure out? :) Can you expand on the comment in line 75, and mention that it _needs_ to be scrolled into view, otherwise the synthesized click will be ignored?

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:44
(Diff revision 2)
>  <!ENTITY enableTelemetryData.label       "Share Additional Data (i.e., Telemetry)">
>  <!ENTITY enableTelemetryData.accesskey   "T">
>  <!ENTITY telemetryLearnMore.label        "Learn More">
>  
>  <!ENTITY crashReporterDesc2.label         "Crash reports help &vendorShortName; fix problems and make your browser more stable and secure">
> -<!ENTITY alwaysSubmitCrashReports.label   "Allow &brandShortName; to send backlogged crash reports on your behalf">
> +<!ENTITY alwaysSubmitCrashReports.label   "Enable Crash Reporter">

I _think_ the spec is out of date on this one. I'm 99% certain this string change should be reverted.

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd:20
(Diff revision 2)
> -<!ENTITY  paneApplications.title  "Applications">
> -<!ENTITY  panePrivacy.title       "Privacy">
> +<!ENTITY  paneApplications.title  "Downloads &amp; Links">
> +<!ENTITY  panePrivacy.title       "Privacy &amp; Security">
>  <!ENTITY  paneContainers.title    "Container Tabs">
>  <!ENTITY  paneSecurity.title      "Security">
> -<!ENTITY  paneAdvanced.title      "Advanced">
> +<!ENTITY  paneAdvanced.title      "Updates">

Any place where you've changed the meaning of the string, we should change the key to suit. For example, `paneApplications.title` doesn't really suit a section called `Downloads & Links`.

Also, in order for our localizers tools to notice when a string change occurs (excepting things like case changes), it's important that we change the key for them.
Attachment #8836376 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

3 months ago
Attachment #8838234 - Attachment is obsolete: true

Updated

3 months ago
Attachment #8838255 - Attachment is obsolete: true
(In reply to Jan Tojnar from comment #15)
Hi Jan, thanks for your feedback. Please see my replies inline.

> I mostly like the changes, I am always looking for the password manager in
> the privacy tab for some reason. I am not sure about some of the decisions,
> though:
> 
> 1. What will happen when I click “Change Search settings” in the search
> entry? Will it open the “General” tab and scroll to the “Default search
> engine” section? I use the “Change Search settings” button quite often and
> having to find the relevant section in the tab every time would not be a
> very smooth experience.
Yes, it suppose to take you to the section of search settings. 

> 
> 2. Why are “Updates” so prominent to deserve their own tab? On some
> platforms the section is completely irrelevant as updates are often handled
> by system-wide package manager (on Arch Linux even for Nightly). Telemetry
> could also be considered falling into “Privacy” category.
The telemetry section is WIP, and we're planning to move it to the category "Privacy & Security".
We did a serial user researches for the new grouping, and the results showed that users tend to sort update options together during the free card sorting test. And there will be more options coming to that category.

> 
> 3. The ellipses look quite out of place – they are too far from the centre
> of the line and too bold. Additionally, they still seem to be rather
> inconsistent – for example “Change Master Password”, unlike remaining
> actions (“Clear all data”) are followed by ellipsis, same for “Remove”
> button at the bottom of the page where it stands out even more. On the other
> hand “View certificates” could probably be changed to “Certificates” to
> mirror “Saved Logins”.
> https://blog.nightly.mozilla.org/files/2017/02/4-privacySecurity.png
We have a copy rule for ellipsis: if a button requires more user actions, it'll come out with an ellipsis. Apparently we have more copy strings that needs to follow this rule.

> 
> 4. Speaking of buttons, how do you decide if an action should be a link or a
> button? It looks to me like the links are used for less important actions.
> If it is that way, some buttons like password manager’s “Exceptions” could
> be made into links.
Links are used for opening a website in a new tab.
Buttons are used for opening a window or doing an action.

> 
> 5. The headings use nouns, “Use Tracking Protection” is an exception here.
Yes. It should be "Tracking Protection". We'll have students to update copy strings in Preferences in the next step.
> 
> 6. The learn more link is sometimes next to heading (“Container Tabs”,
> “Tracking Protection”) and sometimes next to the copy text (“DRM Content”,
> “Notifications”).
As far as I know, it's WIP. We'll have all the "Learn more" next to copy strings of the options.

> 
> 7. “Share additional data” copy text is cropped on smaller screens:
> https://blog.nightly.mozilla.org/files/2017/02/5-update.png
Yeah, that needs to be fixed.

> 
> I used images from the Nightly blog as a reference, sorry if some of the
> points were fixed in the meanwhile.
> https://blog.nightly.mozilla.org/2017/02/14/these-weeks-in-firefox-issue-10/
Thanks for spending time on Preferences! Every question is welcome :)
Flags: needinfo?(thsieh)
(In reply to Michal Stanke (Mozilla.cz) [:MikkCZ] (off until 05/2017, use needinfo) from comment #16)
> To me it seems a bit unbalanced - General + Privacy & Security are very long
> pages (that exactly what I hate in Chromium, where all preferences are on
> one page), where you need to scroll to discover the options you can set.

During our user test, we found that users were quite confused when they were trying to find options in seperated Privacy/ Security categories. And it helped them to find options a lot after we merged Privacy & Security together. 

Currently options are listed by the rank of commonly used options, so users won't need to scroll down too far to see options that need to be changed frequently. (e.g. History is on top of Privacy & Security page)

Additionally, we're going to have the new search function in Preferences that can quickly find options for you.
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111802/#review116456

::: browser/components/preferences/in-content/advanced.xul:44
(Diff revision 3)
>  #endif
>  #endif
>  
>    <preference id="browser.search.update"
>                name="browser.search.update"
> -              type="bool"/>
> +              type="bool"/>      

Please remove the extra trailing spaces that you added here. I confirmed that eslint isn't running on the .xul files so you'll have to be extra cautious here about making sure different style issues don't get introduced.

::: browser/components/preferences/in-content/advanced.xul:99
(Diff revision 3)
>  
> -    <!-- Certificates -->
> -    <tabpanel id="encryptionPanel" orient="vertical">
> -      <groupbox id="certSelection" align="start">
> +
> +
> +

Please keep this to one blank line between sections. Right now this shows that there are five blank lines.

::: browser/components/preferences/in-content/applications.js:912
(Diff revision 3)
>      setEventListener("handlersView", "select",
>        gApplicationsPane.onSelectionChanged);
>      setEventListener("typeColumn", "click", gApplicationsPane.sort);
>      setEventListener("actionColumn", "click", gApplicationsPane.sort);
>  
> +    // ZACK AND AVALON

Please remove this comment.

::: browser/components/preferences/in-content/applications.xul:113
(Diff revision 3)
> +              value="false"
> +              label="&alwaysAskWhere.label;"
> +              accesskey="&alwaysAskWhere.accesskey;"/>
> +      </hbox>
> +    </radiogroup>
> +  </groupbox> 

Please remove this trailing space.

::: browser/components/preferences/in-content/main.js:118
(Diff revision 3)
> -    setEventListener("browser.download.dir", "change",
> -                     gMainPane.displayDownloadDirPref);
> +    // setEventListener("browser.download.dir", "change",
> +    //                  gMainPane.displayDownloadDirPref);
>      if (AppConstants.HAVE_SHELL_SERVICE) {

These lines shouldn't be commented out. I'm assuming this will break setting the default download directory.

::: browser/components/preferences/in-content/main.js:130
(Diff revision 3)
> -    setEventListener("chooseFolder", "command",
> -                     gMainPane.chooseFolder);
> +    // setEventListener("chooseFolder", "command",
> +    //                 gMainPane.chooseFolder);
> +

These lines shouldn't be commented out. I'm assuming this will break choosing the default download folder.

::: browser/components/preferences/in-content/main.js:133
(Diff revision 3)
>      setEventListener("restoreDefaultHomePage", "command",
>                       gMainPane.restoreDefaultHomePage);
> -    setEventListener("chooseFolder", "command",
> -                     gMainPane.chooseFolder);
> +    // setEventListener("chooseFolder", "command",
> +    //                 gMainPane.chooseFolder);
> +
> +    // Content.js setEventListener language functions

This comment should be removed. It's confusing because there is no content.js file anymore.

::: browser/components/preferences/in-content/main.js:143
(Diff revision 3)
> +    setEventListener("translateButton", "command",
> +      gMainPane.showTranslationExceptions);
> +    setEventListener("font.language.group", "change",
> +      gMainPane._rebuildFonts);
> +
> +    // Content.js setEventListener font functions

This comment should be removed. It's confusing because there is no content.js file anymore.

::: browser/components/preferences/in-content/main.js:149
(Diff revision 3)
> +    setEventListener("advancedFonts", "command",
> +      gMainPane.configureFonts);
> +    setEventListener("colors", "command",
> +      gMainPane.configureColors);
> +
> +    // Content.js init function

This comment should be removed. It's fine to keep the one on the line below.

::: browser/components/preferences/in-content/main.js:467
(Diff revision 3)
> -
> -    // don't override the preference's value in UI
>      return undefined;
>    },
>  
> -  displayDownloadDirPrefTask: Task.async(function* () {
> +  // // DOWNLOADS

This should only have one set of double-slashes.

::: browser/components/preferences/in-content/main.js:503
(Diff revision 3)
> +  // readUseDownloadDir() {
> +  //   var downloadFolder = document.getElementById("downloadFolder");
> +  //   var chooseFolder = document.getElementById("chooseFolder");
> +  //   var preference = document.getElementById("browser.download.useDownloadDir");
> +  //   downloadFolder.disabled = !preference.value || preference.locked;
> +  //   chooseFolder.disabled = !preference.value || preference.locked;
> +
> +  //   // don't override the preference's value in UI
> +  //   return undefined;
> +  // },

Please delete this commented out code now that it lives in application.js.

::: browser/components/preferences/in-content/main.js:794
(Diff revision 3)
>        let selectedIndex = shellSvc.isDefaultBrowser(false, true) ? 1 : 0;
>        document.getElementById("setDefaultPane").selectedIndex = selectedIndex;
>      }
>    },
> +
> +  // CONTENT JS FUNCTIONS

This comment should be removed.

::: browser/components/preferences/in-content/main.xul:16
(Diff revision 3)
> +<!--<preferences id="searchPreferences" hidden="true" data-category="paneSearch">
> +
> +      <preference id="browser.search.suggest.enabled"
> +                  name="browser.search.suggest.enabled"
> +                  type="bool"/>
> +
> +      <preference id="browser.urlbar.suggest.searches"
> +                  name="browser.urlbar.suggest.searches"
> +                  type="bool"/>
> +
> +      <preference id="browser.search.hiddenOneOffs"
> +                  name="browser.search.hiddenOneOffs"
> +                  type="unichar"/>
> +
> +</preferences>-->

This "searchPreferences" section should be deleted since it is duplicated about 50 lines lower.

::: browser/components/preferences/in-content/main.xul:149
(Diff revision 3)
>  #endif
>      <preference id="browser.ctrlTab.previews"
>                  name="browser.ctrlTab.previews"
>                  type="bool"/>
> +
> +    <!-- Fonts -->

Please line up the comment so that it starts at the same column as the <preference> tag beneath it.

::: browser/components/preferences/in-content/main.xul:380
(Diff revision 3)
> +<!-- Downloads
>  <groupbox id="downloadsGroup"
>            data-category="paneGeneral"
>            hidden="true">
>    <caption><label>&downloads.label;</label></caption>

Please delete this section since it now appears in applications.xul

::: browser/components/preferences/in-content/preferences.js:30
(Diff revision 3)
>  var gLastHash = "";
>  
>  var gCategoryInits = new Map();
>  function init_category_if_required(category) {
>    let categoryInfo = gCategoryInits.get(category);
> +

Please remove this extra line that got added.

::: browser/components/preferences/in-content/privacy.js:201
(Diff revision 3)
>      setEventListener("browserContainersCheckbox", "command",
>                       gPrivacyPane._checkBrowserContainers);
>      setEventListener("browserContainersSettings", "command",
>                       gPrivacyPane.showContainerSettings);
> +
> +    // security.js eventListener

Please remove this comment as security.js doesn't exist anymore.

::: browser/components/preferences/in-content/privacy.js:213
(Diff revision 3)
> +    setEventListener("showPasswords", "command",
> +      gPrivacyPane.showPasswords);
> +    setEventListener("addonExceptions", "command",
> +      gPrivacyPane.showAddonExceptions);
> +
> +    // advanced.js certificates listeners

Please remove this comment as there is no need to mention advanced.js here.

::: browser/components/preferences/in-content/privacy.js:220
(Diff revision 3)
> +                     gPrivacyPane.showCertificates);
> +    setEventListener("viewSecurityDevicesButton", "command",
> +                     gPrivacyPane.showSecurityDevices);
> +
> +
> +    // advanced.js network listeners

Same for this line too.

::: browser/components/preferences/in-content/privacy.js:237
(Diff revision 3)
> +    this._pane = document.getElementById("panePrivacy");
> +    this._initMasterPasswordUI();
> +    this._initSafeBrowsing();
> +
> +
> +    // advanced.js network init

Ditto.

::: browser/components/preferences/in-content/privacy.js:249
(Diff revision 3)
> +    document.getElementById("offlineAppsList")
> +            .style.height = bundlePrefs.getString("offlineAppsList.height");
> +    setEventListener("offlineAppsListRemove", "command",
> +                     gPrivacyPane.removeOfflineApp);
> +
> +    // Content.js event listeners

Ditto.

::: browser/components/preferences/in-content/privacy.js:810
(Diff revision 3)
>     *
>     * privacy.userContext.enabled
>     * - true if containers is enabled
>     */
>  
> -   /**
> +    /**

Please undo this extra space that got added here.

::: browser/components/preferences/in-content/privacy.js:821
(Diff revision 3)
>  
>       settings.disabled = !pref.value;
> +   },
> +
> +   toggleDoNotDisturbNotifications(event) {
> +    AlertsServiceDND.manualDoNotDisturb = event.target.checked;

This needs one more space of indent.

::: browser/components/preferences/in-content/privacy.xul:21
(Diff revision 3)
>                type="bool"/>
>    <preference id="privacy.trackingprotection.pbmode.enabled"
>                name="privacy.trackingprotection.pbmode.enabled"
> +	      hidden="true"
>                type="bool"/>
> -
> +	      

Please remove the trailing whitespace that got added here.

::: browser/components/preferences/in-content/privacy.xul:455
(Diff revision 3)
> +    </rows>
> +  </grid>
> +</groupbox>
> +
> +
> -<!-- Location Bar -->
> +  <!-- Location Bar -->

Please undo the extra indentation that got added here.

::: browser/components/preferences/in-content/privacy.xul:475
(Diff revision 3)
>    <label class="text-link" onclick="gotoPref('search')">
>      &suggestionSettings.label;
>    </label>
>  </groupbox>
>  
> +  <!-- addons, forgery (phishing) UI Security -->

Please undo the extra indentation that got added here.

::: browser/components/preferences/in-content/privacy.xul:507
(Diff revision 3)
> +                accesskey="&blockUncommonAndUnwanted.accesskey;" />
> +    </vbox>
> +  </vbox>
> +</groupbox>
> +
> +  <!-- Certificates -->

Please undo the indentation that got added here.

::: browser/components/preferences/in-content/privacy.xul:513
(Diff revision 3)
> +<groupbox id="certSelection" align="start" data-category="panePrivacy" hidden="true">
> +  <caption><label>&certPersonal.label;</label></caption>
> +  <!--<description id="CertSelectionDesc" control="certSelection">&certPersonal.description;</description>-->
> +
> +  <!--
> +    The values on these radio buttons may look like l12y issues, but

Please change l12y to l10n while you're here. l12y isn't a term that is used, probably means "localizability" but it should be l10n which is short for "localization" ("l" followed by 10 characters followed by "n").

::: browser/components/preferences/in-content/privacy.xul:550
(Diff revision 3)
> +            preference="security.disable_button.openDeviceManager"/>
> +    <hbox flex="10"/>
> +  </hbox>
> +</groupbox>
> +
> +  <!-- DRM Content -->

Please undo the indentation that got added here.

::: browser/components/preferences/in-content/sync.xul:217
(Diff revision 3)
>  
>    <vbox id="hasFxaAccount">
>      <hbox>
>        <vbox id="fxaContentWrapper">
>          <groupbox id="fxaGroup">
> -          <caption><label>&syncBrand.fxAccount.label;</label></caption>
> +          <!--<caption><label>&syncBrand.fxAccount.label;</label></caption>-->

Why is this commented out? Is it supposed to be removed? If so, please just delete the line.

::: browser/components/preferences/in-content/tests/browser_bug1020245_openPreferences_to_paneContent.js:13
(Diff revision 3)
>  });
>  
>  add_task(function*() {
> -  let prefs = yield openPreferencesViaOpenPreferencesAPI("paneContent");
> -  is(prefs.selectedPane, "paneContent", "Content pane was selected");
> -  prefs = yield openPreferencesViaOpenPreferencesAPI("advanced", "updateTab");
> +  let prefs = yield openPreferencesViaOpenPreferencesAPI("panePrivacy");
> +  is(prefs.selectedPane, "panePrivacy", "Privacy pane was selected");
> +  prefs = yield openPreferencesViaOpenPreferencesAPI("advanced", undefined);

You don't need to explicitly pass in `undefined` if it's the last argument to a function. The function argument will be set to `undefined` automatically if one isn't provided.

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:118
(Diff revision 3)
>  <!ENTITY offlineAppsListRemove.label     "Remove…">
>  <!ENTITY offlineAppsListRemove.accesskey "R">
>  <!ENTITY offlineAppRemove.confirm        "Remove offline data">
>  
>  <!ENTITY certificateTab.label            "Certificates">
> -<!ENTITY certPersonal.label              "Requests">
> +<!ENTITY certPersonal.label              "Certificates">

Are we still using certificateTab.label? I don't see it being used anywhere in preferences now. Do we need another string that has the same value?

Please check to see if there are any other strings that are still defined but unused. Unfortunately I don't know of an automated way to do this, but you can look at what parts don't exist anymore in the spec and use that as a starting guide.

Also, side-topic, whenever a string value changes we need to change the key for that value.

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd:27
(Diff revision 3)
> -<!ENTITY  paneContainers.title    "Container Tabs">
> -<!ENTITY  paneSecurity.title      "Security">
> -<!ENTITY  paneAdvanced.title      "Advanced">
> +<!ENTITY  paneContainers.title          "Container Tabs">
> +<!ENTITY  paneSecurity.title            "Security">
> +<!ENTITY  paneUpdates.title             "Updates">
>  
>  <!-- LOCALIZATION NOTE (paneSync.title): This should match syncBrand.shortName.label in ../syncBrand.dtd -->
> -<!ENTITY  paneSync.title          "Sync">
> +<!ENTITY  paneSync.title          "Firefox Account">

Note, this string entity name will need to be updated since the value has been changed.

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd:17
(Diff revision 3)
>  <!ENTITY  trackingProtectionLearnMore.label    "Learn more">
>  <!ENTITY  trackingProtectionExceptions.label   "Exceptions…">
>  <!ENTITY  trackingProtectionExceptions.accesskey "x">
>  
>  <!ENTITY tracking.label                 "Tracking">
> -<!ENTITY trackingProtectionPBM5.label         "Use Tracking Protection in Private Windows">
> +<!ENTITY trackingProtectionPBM5.label         "Tracking Protection">

This string entity name will need to be updated since the value has changed.

::: browser/locales/en-US/chrome/browser/preferences/security.dtd:5
(Diff revision 3)
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> -<!ENTITY  general.label                 "General">
> +<!ENTITY  general.label                 "Security">

This string entity name will need to be updated since the value has changed.

::: browser/locales/en-US/chrome/browser/preferences/security.dtd:29
(Diff revision 3)
>  <!ENTITY  addonExceptions.accesskey     "E">
>  
>  
> -<!ENTITY  logins.label                  "Logins">
> +<!ENTITY  formsAndPasswords.label       "Forms &amp; Passwords">
>  
> -<!ENTITY  rememberLogins.label          "Remember logins for sites">
> +<!ENTITY  rememberLogins.label          "Remember logins and passwords for sites">

This string entity name will need to be updated since the value has changed.

::: browser/locales/en-US/chrome/browser/preferences/sync.dtd:42
(Diff revision 3)
>  <!ENTITY engine.addons.accesskey    "A">
>  
>  <!-- Device Settings -->
>  <!ENTITY syncDeviceName.label       "Device Name:">
>  <!ENTITY fxaSyncDeviceName.label       "Device Name">
> -<!ENTITY changeSyncDeviceName.label "Change Device Name…">
> +<!ENTITY changeSyncDeviceName.label "Change Device Name">

This string entity name will need to be updated since the value has changed.
Attachment #8836376 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 26

3 months ago
mozreview-review
Comment on attachment 8840630 [details]
fixed bug for change search engine pref

https://reviewboard.mozilla.org/r/115086/#review117540

::: browser/components/preferences/in-content/privacy.xul:470
(Diff revision 1)
>              accesskey="&locbar.bookmarks.accesskey;"
>              preference="browser.urlbar.suggest.bookmark"/>
>    <checkbox id="openpageSuggestion" label="&locbar.openpage.label;"
>              accesskey="&locbar.openpage.accesskey;"
>              preference="browser.urlbar.suggest.openpage"/>
> -  <label class="text-link" onclick="gotoPref('search')">
> +  <label class="text-link" onclick="gotoPref('paneGeneral')">

What's the story with this commit? Is this supposed to be folded into the previous patch?
(Reporter)

Comment 27

3 months ago
mozreview-review
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111802/#review117542

Clearing review flag - jaws' review is solid here.
Attachment #8836376 - Flags: review?(mconley)
Comment hidden (mozreview-request)

Updated

3 months ago
Attachment #8840630 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111800/#review117690

::: browser/base/content/utilityOverlay.js:676
(Diff revision 8)
>    function switchToAdvancedSubPane(doc) {
> -    if (extraArgs && extraArgs["advancedTab"]) {
> -      let advancedPaneTabs = doc.getElementById("advancedPrefs");
> -      advancedPaneTabs.selectedTab = doc.getElementById(extraArgs["advancedTab"]);
> -    }
> +    // if (extraArgs && extraArgs["advancedTab"]) {
> +    //   let advancedPaneTabs = doc.getElementById("advancedPrefs");
> +    //   advancedPaneTabs.selectedTab = doc.getElementById(extraArgs["advancedTab"]);
> +    // }

If the function isn't needed anymore it should just be deleted.

::: browser/components/preferences/in-content/main.xul:11
(Diff revision 8)
>  
>  <script type="application/javascript"
>          src="chrome://browser/content/preferences/in-content/main.js"/>
>  
> +<script type="application/javascript"
> +            src="chrome://browser/content/preferences/in-content/search.js"/>

I thought search.js was deleted? This line should be removed most likely.

::: browser/components/preferences/in-content/privacy.xul:15
(Diff revision 8)
> +	            hidden="true"
>                type="bool"/>
>    <preference id="privacy.trackingprotection.pbmode.enabled"
>                name="privacy.trackingprotection.pbmode.enabled"
> +	      hidden="true"

Why did both of these get hidden="true" added to them?

::: browser/components/preferences/in-content/privacy.xul:470
(Diff revision 8)
>              accesskey="&locbar.bookmarks.accesskey;"
>              preference="browser.urlbar.suggest.bookmark"/>
>    <checkbox id="openpageSuggestion" label="&locbar.openpage.label;"
>              accesskey="&locbar.openpage.accesskey;"
>              preference="browser.urlbar.suggest.openpage"/>
> -  <label class="text-link" onclick="gotoPref('search')">
> +  <label class="text-link" onclick="gotoPref('paneGeneral')">

Please use gotoPref('general') instead of 'paneGeneral'

::: browser/components/preferences/in-content/tests/browser_security.js:75
(Diff revision 8)
>      let checked = checkbox.checked;
>      is(checked, val, "downloads preference is initialized correctly");
>      // should be disabled when val is false (= pref is turned off)
>      is(blockUncommon.hasAttribute("disabled"), !val, "block uncommon checkbox is set correctly");
>  
> -    // click the checkbox
> +    // scroll the checkbox into view, otherwise the syntehsizeMouseAtCenter will be ignored, and click it

typo with synthesizeMouseAtCenter

::: browser/components/preferences/in-content/tests/browser_security.js:106
(Diff revision 8)
>      let doc = gBrowser.selectedBrowser.contentDocument;
>      let checkbox = doc.getElementById("blockUncommonUnwanted");
>      let checked = checkbox.checked;
>      is(checked, val1 && val2, "unwanted/uncommon preference is initialized correctly");
>  
> -    // click the checkbox
> +    // scroll the checkbox into view, otherwise the syntehsizeMouseAtCenter will be ignored, and click it

typo with synthesizeMouseAtCenter

::: browser/locales/en-US/chrome/browser/preferences/security.dtd:5
(Diff revision 8)
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> -<!ENTITY  general.label                 "General">
> +<!ENTITY  general1.label                 "Security">

I think we should change the name so it's not general1.label when the string says "Security". Can you think of a better name here?

::: browser/locales/en-US/chrome/browser/preferences/sync.dtd:42
(Diff revision 8)
> -<!ENTITY changeSyncDeviceName.label "Change Device Name…">
> +<!ENTITY changeSyncDeviceName1.label "Change Device Name">
>  <!ENTITY changeSyncDeviceName.accesskey "h">

If there is a label and associated accesskey, and you need to change the entity name of one of them, please change the other and keep them in sync.

::: browser/themes/shared/incontentprefs/icons.svg:39
(Diff revision 8)
>        <path d="M21.632,9.541c-0.083,1.403,0.246,3.079-1.597,5.498 c-1.965,2.578-3.914,2.594-4.284,2.575c-2.249-0.117-2.502-1.875-3.792-1.875c-1.13,0-2.012,1.745-3.711,1.836 c-0.37,0.02-2.319,0.042-4.284-2.536c-1.841-2.419-1.514-4.095-1.597-5.498C2.287,8.138,2,6.618,2,6.618s0.887,0.895,2.033,0.973 C5.179,7.67,5.394,7.191,7.811,6.501C10.424,5.752,12,8.814,12,8.814s1.776-3.016,4.189-2.313c2.414,0.7,2.515,1.169,3.661,1.091 C20.996,7.513,22,6.618,22,6.618S21.713,8.138,21.632,9.541z M8.117,10.129c-1.429-0.314-2.028,0.223-2.642,0.451 c-0.534,0.202-1.02,0.264-1.02,0.264s0.083,0.819,1.515,1.521c1.432,0.703,4.37,0.338,4.37,0.338S10.651,10.687,8.117,10.129z M18.525,10.58c-0.612-0.228-1.212-0.765-2.642-0.451c-2.534,0.558-2.223,2.573-2.223,2.573s2.938,0.365,4.37-0.338 c1.432-0.702,1.515-1.521,1.515-1.521S19.059,10.782,18.525,10.58z"/>
>      </g>
>      <g id="security-shape">
>        <path d="M18.909,9.783h-0.863V8.086C18.046,4.725,15.339,2,12,2 C8.661,2,5.954,4.725,5.954,8.086v1.697H5.091c-0.955,0-1.728,0.779-1.728,1.739v8.738c0,0.961,0.773,1.74,1.728,1.74h13.818 c0.954,0,1.728-0.779,1.728-1.74v-8.738C20.637,10.562,19.863,9.783,18.909,9.783z M8.545,8.086c0-1.92,1.547-3.478,3.455-3.478 c1.908,0,3.455,1.557,3.455,3.478v1.697h-6.91V8.086z M5.181,16.092l-0.909-1.2v-2.284l2.728,3.483H5.181z M8.818,16.092 l-2.773-3.657h1.727l2.864,3.657H8.818z M12,16.092l-2.773-3.657h1.727l2.864,3.657H12z M15.637,16.092l-2.773-3.657h1.727 l2.864,3.657H15.637z M19.728,16.092h-0.455l-2.773-3.657h1.727l1.501,1.916V16.092z"/>
>      </g>
>      <g id="sync-shape">

Why did sync-shape and advanced-shape get so much more complex? Were you given new icons?
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111802/#review117696
Attachment #8836376 - Flags: review?(jaws) → review-
(Reporter)

Updated

3 months ago
Depends on: 1343682
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 34

3 months ago
mozreview-review
Comment on attachment 8834653 [details]
Bug 1335907 - Pref Reorg. WORK IN PROGRESS.

https://reviewboard.mozilla.org/r/110514/#review118356

Hey avalon and zach,

Great work here - really happy to see it coming together. There's quite a bit of dead code in here to tear out, and some general alignment issues. I think we'll need to do another pass without the dead code to make sure nothing's gotten lost in the re-org. We'll probably also want to do a try build and hand it off to UX for testing.

::: browser/components/preferences/in-content/advanced.js
(Diff revision 8)
> -
> -      let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "storage-permissions";
> -      document.getElementById("siteDataLearnMoreLink").setAttribute("href", url);

Reading through the comments in bug 1335907, I don't see any feedback about removing the storage permission "learn more" stuff. Is that from an accidental overwrite during a rebase?

::: browser/components/preferences/in-content/advanced.xul:113
(Diff revision 8)
> -      <groupbox id="accessibilityGroup" align="start">
> +      <!--<groupbox id="accessibilityGroup" align="start">
>          <caption><label>&accessibility.label;</label></caption>
>  
>  #ifdef XP_WIN
>          <checkbox id="useOnScreenKeyboard"
>                    hidden="true"
>                    label="&useOnScreenKeyboard.label;"
>                    accesskey="&useOnScreenKeyboard.accesskey;"
>                    preference="ui.osk.enabled"/>
>  #endif
>          <checkbox id="useCursorNavigation"
>                    label="&useCursorNavigation.label;"
>                    accesskey="&useCursorNavigation.accesskey;"
>                    preference="accessibility.browsewithcaret"/>
>          <checkbox id="searchStartTyping"
>                    label="&searchOnStartTyping.label;"
>                    accesskey="&searchOnStartTyping.accesskey;"
>                    preference="accessibility.typeaheadfind"/>
>          <checkbox id="blockAutoRefresh"
>                    label="&blockAutoReload.label;"
>                    accesskey="&blockAutoReload.accesskey;"
>                    preference="accessibility.blockautorefresh"/>
> -      </groupbox>
> +      </groupbox>-->

Dead code...

::: browser/components/preferences/in-content/advanced.xul:137
(Diff revision 8)
> -      <groupbox id="browsingGroup" align="start">
> +      <!--<groupbox id="browsingGroup" align="start">
>          <caption><label>&browsing.label;</label></caption>
>  
>          <checkbox id="useAutoScroll"
>                    label="&useAutoScroll.label;"
>                    accesskey="&useAutoScroll.accesskey;"
>                    preference="general.autoScroll"/>
>          <checkbox id="useSmoothScrolling"
>                    label="&useSmoothScrolling.label;"
>                    accesskey="&useSmoothScrolling.accesskey;"
>                    preference="general.smoothScroll"/>
>          <checkbox id="allowHWAccel"
>                    label="&allowHWAccel.label;"
>                    accesskey="&allowHWAccel.accesskey;"
>                    preference="layers.acceleration.disabled"/>
>          <checkbox id="checkSpelling"
>                    label="&checkUserSpelling.label;"
>                    accesskey="&checkUserSpelling.accesskey;"
>                    onsyncfrompreference="return gAdvancedPane.readCheckSpelling();"
>                    onsynctopreference="return gAdvancedPane.writeCheckSpelling();"
>                    preference="layout.spellcheckDefault"/>
> -      </groupbox>
> +      </groupbox>-->

Dead code...

::: browser/components/preferences/in-content/advanced.xul
(Diff revision 8)
> -          <label id="siteDataLearnMoreLink" class="learnMore text-link" value="&siteDataLearnMoreLink.label;"></label>
> -          <spacer flex="1" />

Why was this removed?

::: browser/components/preferences/in-content/applications.js:9
(Diff revision 8)
> -Components.utils.import("resource://gre/modules/Services.jsm");
> -Components.utils.import("resource://gre/modules/AppConstants.jsm");
> +Components.utils.import('resource://gre/modules/Services.jsm');
> +Components.utils.import('resource://gre/modules/AppConstants.jsm');

Why the shift to single-quotes?

::: browser/components/preferences/in-content/applications.js:912
(Diff revision 8)
>      setEventListener("handlersView", "select",
>        gApplicationsPane.onSelectionChanged);
>      setEventListener("typeColumn", "click", gApplicationsPane.sort);
>      setEventListener("actionColumn", "click", gApplicationsPane.sort);
>  
> +    // ZACK AND AVALON

Please remove comments like this. :)

::: browser/components/preferences/in-content/applications.xul:114
(Diff revision 8)
> +              value="false"
> +              label="&alwaysAskWhere.label;"
> +              accesskey="&alwaysAskWhere.accesskey;"/>
> +      </hbox>
> +    </radiogroup>
> +  </groupbox> 

Trailing whitespace.

::: browser/components/preferences/in-content/content.js:26
(Diff revision 8)
> -    this._rebuildFonts();
> -    var menulist = document.getElementById("defaultFont");
> -    if (menulist.selectedIndex == -1) {
> -      menulist.value = FontBuilder.readFontSelection(menulist);
> -    }
> +    // this._rebuildFonts();
> +    // var menulist = document.getElementById("defaultFont");
> +    // if (menulist.selectedIndex == -1) {
> +    //   menulist.value = FontBuilder.readFontSelection(menulist);
> +    // }

More dead code to remove.

::: browser/components/preferences/in-content/content.js:61
(Diff revision 8)
> -    setEventListener("advancedFonts", "command",
> -      gContentPane.configureFonts);
> -    setEventListener("colors", "command",
> -      gContentPane.configureColors);
> -    setEventListener("chooseLanguage", "command",
> -      gContentPane.showLanguages);
> -    setEventListener("translationAttributionImage", "click",
> -      gContentPane.openTranslationProviderAttribution);
> -    setEventListener("translateButton", "command",
> -      gContentPane.showTranslationExceptions);
> +    // setEventListener("advancedFonts", "command",
> +    //   gContentPane.configureFonts);
> +    // setEventListener("colors", "command",
> +    //   gContentPane.configureColors);
> +    // setEventListener("chooseLanguage", "command",
> +    //   gContentPane.showLanguages);
> +    // setEventListener("translationAttributionImage", "click",
> +    //   gContentPane.openTranslationProviderAttribution);
> +    // setEventListener("translateButton", "command",
> +    //   gContentPane.showTranslationExceptions);

And more dead code. I'm going to stop mentioning it now. Any dead code should be removed.

::: browser/components/preferences/in-content/main.xul:13
(Diff revision 8)
> +<!--<preferences id="searchPreferences" hidden="true" data-category="paneSearch">
> +
> +      <preference id="browser.search.suggest.enabled"
> +                  name="browser.search.suggest.enabled"
> +                  type="bool"/>
> +
> +      <preference id="browser.urlbar.suggest.searches"
> +                  name="browser.urlbar.suggest.searches"
> +                  type="bool"/>
> +
> +      <preference id="browser.search.hiddenOneOffs"
> +                  name="browser.search.hiddenOneOffs"
> +                  type="unichar"/>
> +
> +</preferences>-->

More dead code to be removed.

::: browser/components/preferences/in-content/main.xul:147
(Diff revision 8)
> +  <preference id="font.language.group"
> +              name="font.language.group"
> +              type="wstring"/>

Nit: These, and the preferences below, all appear to be unindented by about 2 spaces. See the <preference> nodes above for reference.

::: browser/components/preferences/in-content/main.xul:151
(Diff revision 8)
> +    <!-- Languages -->
> +  <preference id="browser.translation.detectLanguage"
> +              name="browser.translation.detectLanguage"
> +              type="bool"/>
> +

In the spec linked in comment 0 of the bug, "Languages" appears below "Accessibility". Is the posted spec out of date?

::: browser/components/preferences/in-content/main.xul:156
(Diff revision 8)
> +    <!-- Languages -->
> +  <preference id="browser.translation.detectLanguage"
> +              name="browser.translation.detectLanguage"
> +              type="bool"/>
> +
> +    <!-- General tab -->

This section has kinda been removed, right? If so, we can probably get rid of this line.
Attachment #8834653 - Flags: review-
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111802/#review118762

Two changesets needed to be reapplied to your work here when I rebased it to latest mozilla-central.

https://hg.mozilla.org/mozilla-central/rev/c79f0867b4ec
and
https://hg.mozilla.org/mozilla-central/rev/8366261e6460

::: browser/components/preferences/in-content/advanced.xul:133
(Diff revision 7)
> -    </tabpanel>
> -  </tabpanels>
> -</tabbox>
> +        </vbox>
> +      </groupbox>
> +#endif
> +
> +#ifdef MOZ_DATA_REPORTING
> +

nit, you can remove the blank line between these two #ifdefs

::: browser/components/preferences/in-content/main.js:30
(Diff revision 7)
> +var gEngineView = null;
>  
>  var gMainPane = {
> +
> +
> +    /**

nit, please fix the indent of the start of this comma. i think in other places we have the last star line up with the one below it? i know this is super nit-picky but your patch so far otherwise looks good so i might as well point these things out :)

::: browser/components/preferences/in-content/main.js:447
(Diff revision 7)
> -  // DOWNLOADS
> -
> -  /*
> -   * Preferences:
> +  // /**
> +  //  * Utility function to enable/disable the button specified by aButtonID based
> +  //  * on the value of the Boolean preference specified by aPreferenceID.
> +  //  */

You should remove the //'s here since we don't need this double commented (slashes and slash-stars).

::: browser/components/preferences/in-content/preferences.js:130
(Diff revision 7)
>      case "advanced":
> -      let advancedPaneTabs = document.getElementById("advancedPrefs");
> -      switch (advancedPaneTabs.selectedTab.id) {
> -        case "generalTab":
> -          return "advancedGeneral";
> +      return "advancedGeneral";

Since we now only have an advancedGeneral, we should just call this "advanced", and move the "advanced" line higher up so it just returns `category`.

Now that we have different catgories, we'll need to remove the old telemetry probe and make a new one that uses the new category names.

Look in Histograms.json for FX_PREFERENCES_CATEGORY_OPENED. I guess you can rename that one to FX_PREFERENCES_CATEGORY_OPENED_V2

Also, you'll need to update the cases above to have the new category names, and update the places that try to update this histogram to use the new name and probably other details related to the histogram within Histograms.json such as "expires_in_version": "59".
Attachment #8836376 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
Attachment #8836376 - Flags: review?(mconley)
Attachment #8834653 - Attachment is obsolete: true
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111802/#review122804

::: browser/components/preferences/in-content/preferences.js:125
(Diff revisions 7 - 8)
>      case "applications":
>      case "privacy":
>      case "security":
> +    case "advanced":
> +      return "advanced";
>      case "sync":
>        return category;

This doesn't seem right at all. This code right now is returning "advanced" even when the category is "privacy". It looks like your previous version was more correct here.
Attachment #8836376 - Flags: review?(jaws) → review-
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111802/#review122976

::: browser/components/preferences/in-content/preferences.js:123
(Diff revisions 7 - 8)
>      case "search":
>      case "content":
>      case "applications":

As discussed in today's meeting, please add a category here for the search-results page too.
(Reporter)

Comment 39

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e6ece6c5117
Comment hidden (mozreview-request)
(Reporter)

Updated

2 months ago
Blocks: 1348828
Comment hidden (mozreview-request)
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111802/#review123976

::: toolkit/components/telemetry/Histograms.json:5327
(Diff revision 9)
> -  "FX_PREFERENCES_CATEGORY_OPENED": {
> +  "FX_PREFERENCES_CATEGORY_OPENED_V2": {
>      "bug_numbers": [1324167],
>      "alert_emails": ["jaws@mozilla.com"],
> -    "expires_in_version": "56",
> +    "expires_in_version": "59",
>      "kind": "categorical",
> -    "labels": ["unknown", "general", "search", "content", "applications", "privacy", "security", "sync", "advancedGeneral", "advancedDataChoices", "advancedNetwork", "advancedUpdates", "advancedCerts"],
> +    "labels": ["unknown", "general", "applications", "privacy", "sync", "advanced"],

This list is missing "search-results".
Attachment #8836376 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
Comment on attachment 8849116 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

Hey Benjamin, can we get a privacy review on the telemetry probe that is changing here. We're just changing the names of the categories that the probe will be using (and as a result we have changed the name of the probe). We are not collecting any extra information through this probe.
Attachment #8849116 - Flags: feedback?(benjamin)
Comment on attachment 8849116 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/121948/#review125100

::: browser/components/preferences/in-content/preferences.js:129
(Diff revision 1)
> +    case "advanced":
> +      return "advanced";

This needs to return `category` instead of "advanced". Also, you need to add "search-results" to this list. And you might as well make this alphabetical while you are here.

::: toolkit/components/telemetry/Histograms.json:5493
(Diff revision 1)
>      "kind": "boolean",
>      "releaseChannelCollection": "opt-out",
>      "description": "Whether the browser we migrated from was the browser with the most recent data. Keyed by that browser's identifier (e.g. 'ie', 'edge', 'safari', etc.)."
>    },
> -  "FX_PREFERENCES_CATEGORY_OPENED": {
> +  "FX_PREFERENCES_CATEGORY_OPENED_V2": {
>      "bug_numbers": [1324167],

This should be updated to 1335907.

::: toolkit/components/telemetry/Histograms.json:5497
(Diff revision 1)
> -  "FX_PREFERENCES_CATEGORY_OPENED": {
> +  "FX_PREFERENCES_CATEGORY_OPENED_V2": {
>      "bug_numbers": [1324167],
>      "alert_emails": ["jaws@mozilla.com"],
> -    "expires_in_version": "56",
> +    "expires_in_version": "59",
>      "kind": "categorical",
> -    "labels": ["unknown", "general", "search", "content", "applications", "privacy", "security", "sync", "advancedGeneral", "advancedDataChoices", "advancedNetwork", "advancedUpdates", "advancedCerts"],
> +    "labels": ["unknown", "general", "applications", "privacy", "sync", "advanced"],

This should have "search-results" in the list.
Comment hidden (mozreview-request)

Comment 47

2 months ago
mozreview-review
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111802/#review125420

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:117
(Diff revision 11)
>  <!ENTITY offlineAppsList.height          "7em">
>  <!ENTITY offlineAppsListRemove.label     "Remove…">
>  <!ENTITY offlineAppsListRemove.accesskey "R">
>  <!ENTITY offlineAppRemove.confirm        "Remove offline data">
>  
> -<!ENTITY certificateTab.label            "Certificates">
> +<!ENTITY certPersonal.label              "Certificates">

certPersonal.label already existed in the file, and was set to "Requests". Not sure if you really wanted to move away from certificateTab.label, in case you need a brand new string ID

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd:26
(Diff revision 11)
> -<!ENTITY  panePrivacy.title       "Privacy">
> -<!ENTITY  paneContainers.title    "Container Tabs">
> -<!ENTITY  paneSecurity.title      "Security">
> -<!ENTITY  paneAdvanced.title      "Advanced">
> +<!ENTITY  panePrivacySecurity.title     "Privacy &amp; Security">
> +<!ENTITY  paneContainers.title          "Container Tabs">
> +<!ENTITY  paneSecurity.title            "Security">
> +<!ENTITY  paneUpdates.title             "Updates">
>  
> -<!-- LOCALIZATION NOTE (paneSync.title): This should match syncBrand.shortName.label in ../syncBrand.dtd -->
> +<!-- LOCALIZATION NOTE (paneSync1.title): This should match syncBrand.shortName.label in ../syncBrand.dtd -->

This comment is not relevant anymore. You probably want "match syncBrand.fxAccount.label".

https://hg.mozilla.org/mozilla-central/file/default/browser/locales/en-US/chrome/browser/syncBrand.dtd

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd:18
(Diff revision 11)
>  <!ENTITY  trackingProtectionExceptions.label   "Exceptions…">
>  <!ENTITY  trackingProtectionExceptions.accesskey "x">
>  
>  <!ENTITY tracking.label                 "Tracking">
> -<!ENTITY trackingProtectionPBM5.label         "Use Tracking Protection in Private Windows">
> +<!ENTITY trackingProtectionPBM6.label         "Tracking Protection">
>  <!ENTITY trackingProtectionPBM5.accesskey     "v">

If you update the label ID, change also the accesskey.

'v' is not a valid option anymore, since the character is not available in the label

::: browser/locales/en-US/chrome/browser/preferences/security.dtd:29
(Diff revision 11)
>  <!ENTITY  addonExceptions.accesskey     "E">
>  
>  
> -<!ENTITY  logins.label                  "Logins">
> +<!ENTITY  formsAndPasswords.label       "Forms &amp; Passwords">
>  
> -<!ENTITY  rememberLogins.label          "Remember logins for sites">
> +<!ENTITY  rememberLogins1.label          "Remember logins and passwords for sites">

Question for copy and UX: doesn't 'logins' already include both user and passwords?

::: browser/locales/en-US/chrome/browser/preferences/security.dtd:30
(Diff revision 11)
>  
>  
> -<!ENTITY  logins.label                  "Logins">
> +<!ENTITY  formsAndPasswords.label       "Forms &amp; Passwords">
>  
> -<!ENTITY  rememberLogins.label          "Remember logins for sites">
> +<!ENTITY  rememberLogins1.label          "Remember logins and passwords for sites">
>  <!ENTITY  rememberLogins.accesskey      "R">

Please update the accesskey label accordingly (rememberLogins1.accesskey)
I have the feeling I commented the wrong patch, but issues seem to be present in both.
(Reporter)

Comment 49

2 months ago
Comment on attachment 8849116 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

This was a rebase commit that I don't think is required any longer, as ava1on and zack have picked it up.
Attachment #8849116 - Attachment is obsolete: true
Attachment #8849116 - Flags: feedback?(benjamin)
(Reporter)

Comment 50

2 months ago
Comment on attachment 8849116 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

Whoops, apparently we want this a little while longer. f? for bsmedberg (see comment 44).
Attachment #8849116 - Attachment is obsolete: false
Attachment #8849116 - Flags: feedback?(benjamin)

Comment 51

2 months ago
mozreview-review
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/111802/#review125642

::: toolkit/components/telemetry/Histograms.json:5497
(Diff revision 11)
> -  "FX_PREFERENCES_CATEGORY_OPENED": {
> -    "bug_numbers": [1324167],
> +  "FX_PREFERENCES_CATEGORY_OPENED_V2": {
> +    "bug_numbers": [1335907],
>      "alert_emails": ["jaws@mozilla.com"],
> -    "expires_in_version": "56",
> +    "expires_in_version": "59",
>      "kind": "categorical",
> -    "labels": ["unknown", "general", "search", "content", "applications", "privacy", "security", "sync", "advancedGeneral", "advancedDataChoices", "advancedNetwork", "advancedUpdates", "advancedCerts"],
> +    "labels": ["unknown", "search-results", "general", "applications", "privacy", "sync", "advanced"],

The label should follow rule of ^[a-z][a-z0-9_]+[a-z0-9]$, otherwise it can't get compiled.
Should change to "search_results"?
Comment on attachment 8836376 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

I'm fixing up the issues now so this can land. It will have to land on top of bug 1350087.
Attachment #8836376 - Flags: review?(jaws) → review+
(In reply to Ziyan Long from comment #51)
> The label should follow rule of ^[a-z][a-z0-9_]+[a-z0-9]$, otherwise it
> can't get compiled.
> Should change to "search_results"?

I've changed this to "searchresults" locally.
Depends on: 1350087
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8836376 - Attachment is obsolete: true
The patches that I have just uploaded are a WIP, there are lingering test issues that need to be fixed first. And we need to land bug 1350087 first anyways. I will fold the two patches together once I get review, since the second part doesn't really stand-alone.

Updated

2 months ago
Blocks: 1328561

Comment 57

2 months ago
Hi Jared,

Do you think we could finished the bug in near future? Looks like this is almost done. Ask the question becuase it blocks an bug (Bug 1328561) we would like to work on recently. Thanks. :)

Updated

2 months ago
Blocks: 1330552
No longer blocks: 1328561

Updated

2 months ago
Blocks: 1328561
No longer blocks: 1330552

Comment 58

2 months ago
Sorry for that change for many times. To clear, I would say Bug 1335907 block Bug 1330552, which is the telemetry plan for Preferences from Taipei team.
Blocks: 1330552
No longer blocks: 1328561
I didn't read the patches (maybe there is no issue, but even just in case) and don't really know if this is good place to report this but existing strings shouldn't be reused (in a new context).

This means that not only strings need new identifiers when the text has been changed but all the strings in the changed UI should be essentially new (copied maybe) and not shared with old UI.
(In reply to Stefan Plewako [:stef] from comment #59)
> I didn't read the patches (maybe there is no issue, but even just in case)
> and don't really know if this is good place to report this but existing
> strings shouldn't be reused (in a new context).

Strings are not going to be used in a different context, existing panels are going to be reorganized, and eventually new section titles added
https://docs.google.com/presentation/d/1R-HofYuN0bJcROlwUXStxr5CqcOF6WKooZgciikFJgw/edit#slide=id.g19a4c15f47_0_29

The only concern could be accesskeys, but from the mock-ups I've seen it won't be possible to have unique identifiers in the same panel.
(In reply to Evan Tseng [:evanxd] from comment #57)
> Hi Jared,
> 
> Do you think we could finished the bug in near future? Looks like this is
> almost done. Ask the question becuase it blocks an bug (Bug 1328561) we
> would like to work on recently. Thanks. :)

I am almost finished with this work here so I would rather not pass it along since someone new will have to learn all about this patch. I hope to have this done within the next few work days.

(In reply to Stefan Plewako [:stef] from comment #59)
> I didn't read the patches (maybe there is no issue, but even just in case)
> and don't really know if this is good place to report this but existing
> strings shouldn't be reused (in a new context).
> 
> This means that not only strings need new identifiers when the text has been
> changed but all the strings in the changed UI should be essentially new
> (copied maybe) and not shared with old UI.

flod is correct. We will not be sharing strings between the new and old UI. We have forked the strings and copied them to another directory for use by the old UI until we can remove it.

Comment 62

2 months ago
mozreview-review
Comment on attachment 8849116 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

https://reviewboard.mozilla.org/r/121948/#review126502

data-r=me (I only reviewed Histograms.json)
Attachment #8849116 - Flags: review+
Attachment #8849116 - Flags: feedback?(benjamin)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8850788 - Flags: review?(jaws) → review+
Comment on attachment 8849116 [details]
Bug 1335907 - Pref Reorg. Rebased on top of central (8d967436d696) and killswitch (bug 1343682).

Thanks for the data-review+ Benjamin. I'm going to obsolete this now since mconley had uploaded it and I've continued the rebase.
Attachment #8849116 - Attachment is obsolete: true
(Reporter)

Comment 66

2 months ago
mozreview-review
Comment on attachment 8850789 [details]
Bug 1335907 - Reorganize the preferences based on feedback from user research.

https://reviewboard.mozilla.org/r/123308/#review126560

These changes also look sane - I confirmed that these two patches, once folded, revert the string changes in the preferences-old folder.

**Note**: There is, however, a change to browser/themes/shared/incontentprefs-old/icons.svg that we should also revert, and that's missing here.
Attachment #8850789 - Flags: review?(mconley) → review+
(Reporter)

Comment 67

2 months ago
mozreview-review
Comment on attachment 8850788 [details]
Bug 1335907 - Reorganize the preferences based on feedback from user research.

https://reviewboard.mozilla.org/r/123306/#review126562

::: browser/themes/shared/incontentprefs-old/icons.svg:40
(Diff revision 2)
> -      <path d="M17.024,3.351 c-0.562,0.331 -1.311,0.879 -1.821,1.698 -0.367,0.592 -0.752,1.288 -1.08,1.914 0.987,0.413 1.862,1.095 2.476,2.029 0.614,0.957 0.929,2.122 0.83,3.351 -0.201,1.787 -1.359,3.433 -3.046,4.36 -0.696,-0.774 -1.951,-2.945 -1.951,-2.945 -0.007,0.007 -0.004,2.556 -0.871,4.334 -0.573,1.184 -1.24,2.202 -2.305,2.995 1.431,0.51 2.915,0.886 4.282,0.909 l 0.162,0.002 c 2.99,0.021 5.844,-0.48 5.844,-0.48 0,0 -1.236,-0.802 -1.808,-1.346 1.86,-1.072 3.111,-2.791 3.634,-4.708 0.283,-0.759 0.478,-1.566 0.57,-2.409 C 22.383,9.011 20.33,5.278 17.024,3.351 Z M 6.569,12.302 C 6.526,10.271 7.755,8.327 9.644,7.29 c 0.696,0.774 2.32,2.899 2.32,2.899 0,0 -0.064,-5.157 1.657,-7.973 -6.097,-0.668 -9.69,0.443 -9.69,0.443 0,0 1.763,0.607 2.333,1.136 C 6.122,3.891 5.984,3.992 5.85,4.096 4.4,5.064 3.368,6.449 2.825,7.994 2.436,8.892 2.171,9.863 2.06,10.887 1.622,14.886 3.629,18.572 6.871,20.515 7.39,20.124 7.975,19.631 8.61,18.983 9.189,18.389 9.647,17.682 10.021,16.967 8.082,16.208 6.714,14.404 6.569,12.302 Z"/>
> +      <path style="fill:#F1F1F1;" d="M0.9,19.9h3.3H15h3.3c0.5,0,0.9-0.4,0.9-0.9v-1.1c0-1-0.5-1.9-1.2-2.5c-2.3-1.8-4.6-2.9-5.1-3.1
> +      c-0.1,0-0.1-0.1-0.1-0.2v-1.6c0.3-0.5,0.4-1,0.5-1.5c0.2,0.1,0.6,0.1,1-1.3c0.3-1.1,0.1-1.5-0.2-1.6C15,1.7,13,1.6,13,1.6
> +      S12.7,1,11.8,0.5C11.3,0.2,10.5-0.1,9.5,0C9.1,0,8.8,0.1,8.5,0.2c-0.4,0.1-0.7,0.3-1,0.5C7.1,1,6.8,1.2,6.4,1.6
> +      c-0.5,0.5-1,1.2-1.1,2C5.1,4.3,5.1,5,5.4,5.8C5,5.7,4.6,5.9,5,7.4c0.3,1.1,0.6,1.4,0.8,1.4c0.1,0.6,0.3,1.3,0.7,1.9v1.4
> +      c0,0.1,0,0.1-0.1,0.2c-0.5,0.2-2.8,1.4-5.1,3.1C0.5,16,0,16.9,0,17.9V19C0,19.5,0.4,19.9,0.9,19.9"/>
>      </g>
>      <g id="advanced-shape">
> -      <path d="M19.173,16.163c0.004,0.04,0.007,0.08,0.007,0.121c0,1.748-3.197,3.165-7.14,3.165 c-3.943,0-7.14-1.417-7.14-3.165c0 -0.037,0.003-0.073,0.006-0.109C3.11,16.572,2,17.243,2,18.341C2,20.362,6.477,22,12,22 c5.523,0,10-1.638,10-3.659 C22,17.22,20.922,16.553,19.173,16.163z"/>
> -      <path d="M18.224,15.979c0.006-0.11-0.018-0.285-0.054-0.39c0,0-0.762-2.205-1.176-3.403 c-0.624-1.807-2.112-6.139-2.112-6.139c-0.036-0.104-0.031-0.273,0.01-0.376l0.497-1.234c0.041-0.102,0.116-0.266,0.166-0.364 l0.986-1.942c0.05-0.098,0.013-0.133-0.081-0.077L9.965,5.871c-0.095,0.056-0.203,0.186-0.24,0.29c0,0-0.252,0.7-0.412,1.144 C8.64,9.173,7.968,11.04,7.296,12.908c-0.26,0.723-0.52,1.446-0.78,2.168c-0.056,0.156-0.112,0.311-0.168,0.466 c-0.093,0.26-0.049,0.617,0.032,0.881c0.237,0.763,1.001,1.189,1.708,1.435c0.611,0.213,1.254,0.328,1.895,0.403 c0.895,0.105,1.805,0.14,2.706,0.112c1.356-0.041,2.767-0.261,4.004-0.846c0.429-0.203,0.854-0.459,1.174-0.816 c0.121-0.135,0.226-0.287,0.297-0.455C18.215,16.134,18.224,15.979,18.224,15.979z M14.063,16.131 c0.019,0.108-0.046,0.156-0.143,0.104l-1.466-0.772c-0.097-0.052-0.257-0.052-0.354,0l-1.466,0.773 c-0.097,0.052-0.162,0.004-0.143-0.104l0.28-1.636c0.019-0.109-0.031-0.261-0.109-0.338l-1.186-1.158 c-0.079-0.077-0.054-0.153,0.055-0.169l1.638-0.239c0.109-0.016,0.238-0.11,0.286-0.209l0.733-1.488 c0.049-0.099,0.128-0.099,0.177,0l0.733,1.488c0.049,0.099,0.178,0.193,0.286,0.209l1.639,0.239 c0.109,0.016,0.134,0.092,0.055,0.169l-1.186,1.158c-0.079,0.077-0.128,0.229-0.109,0.338L14.063,16.131z"/>
> +      <path style="fill:#F1F1F1;" d="M19.3,13.4C19.3,13.4,19.3,13.4,19.3,13.4L19.3,13.4C19.3,13.4,19.3,13.4,19.3,13.4L19.3,13.4z
> +      M19.3,13.4c-0.1,1-0.5,1.8-1.2,2.4c-0.5,0.5-1.1,0.9-1.7,1.2c-0.2,0.4-0.4,0.7-0.8,1c-0.5,0.4-1.3,0.8-2.1,1
> +      c-0.9,0.2-1.6,0.3-2.3,0.3h-0.9c0.1,0.2,0.2,0.2,0.4,0.2c-1.1-0.1-2.1-0.3-2.9-0.5c0.2,0.2,0.5,0.4,0.8,0.4c-1.5-0.3-2.9-1-4.1-1.9
> +      c-0.3-0.2-0.4-0.3-0.5-0.4c-1-0.8-1.7-1.6-2.3-2.6c-0.7-1.1-1.1-2.5-1.3-4.1c-0.1,0.5-0.1,0.9-0.1,1.1C0.1,10.2,0.1,9,0.4,7.9
> +      C0.3,8.2,0.1,8.6,0,9c0.1-0.8,0.4-1.8,0.9-2.8C1,5.8,1.2,5.5,1.3,5.4V4.3c0-0.1,0-0.3,0.1-0.6c0-0.2,0.1-0.4,0.2-0.6v0.1
> +      c0,0,0-0.1,0-0.1c0-0.1,0-0.1,0-0.1c0-0.1,0-0.2,0.1-0.2v0.1c0,0,0-0.1,0-0.1c0,0,0-0.1,0-0.1s0-0.1,0-0.2C2,2.2,2,2.1,2,2.1
> +      C2.2,2,2.2,1.9,2.3,1.9V2c0,0.2,0.1,0.5,0.3,1l0-0.1C2.8,3.1,3,3.4,3.4,3.7c0.8-0.2,1.5-0.3,2.4-0.1C5.9,3.5,5.9,3.4,6,3.3v0.1
> +      C6.1,3.2,6.3,3.1,6.6,3v0c0.2-0.1,0.5-0.3,1-0.4L7.5,2.6c0.1-0.1,0.2-0.1,0.3-0.1c0.1,0,0.2,0,0.3,0c0.1,0,0.3,0,0.4,0
> +      c-0.1,0-0.1,0.1-0.1,0.1c0,0-0.1,0.1-0.1,0.1s-0.1,0-0.1,0h0.1C7.8,3.2,7.4,3.7,7.2,4.4c-0.1,0-0.1,0.1-0.1,0.2l0.1,0.1
> +      c0.2,0.3,0.5,0.5,0.9,0.5h1.4c0.2,0.1,0.3,0.2,0.3,0.2c0,0.2-0.1,0.4-0.3,0.6c0,0.1-0.1,0.2-0.3,0.4C8.5,7,8,7.3,7.7,7.6v0.1
> +      c0,0,0,0,0,0.1c0,0,0,0,0,0.1c-0.1,0-0.1,0-0.1,0.1C7.7,7.9,7.8,8,7.8,8.2c0,0.2,0,0.4-0.1,0.6c0,0-0.1-0.1-0.1-0.1
> +      c-0.1,0-0.1,0-0.1,0c0.1,0.1,0.2,0.1,0.2,0.2V9c0,0-0.1,0-0.1,0c0,0-0.1-0.1-0.1-0.1l-0.1,0c-0.1,0-0.1,0-0.2-0.1
> +      c-0.1,0-0.1,0-0.1,0L7,8.7c-0.1,0-0.1,0-0.1,0L7,8.7c-0.1,0-0.1,0-0.2,0l-0.1,0C6.5,8.8,6.4,9,6.4,9.3c0,0.6,0.4,1.1,1.1,1.5
> +      C7.7,11,8,11.1,8.3,11.2c0.3,0,0.5,0,0.7,0c0.2-0.1,0.4-0.1,0.6-0.2s0.4-0.2,0.5-0.2c0.7-0.2,1.4,0,1.9,0.5c0.2,0.1,0.2,0.2,0.1,0.4
> +      c-0.1,0.2-0.2,0.3-0.4,0.2c-0.1,0-0.2,0-0.2,0c-0.1,0-0.1,0-0.3,0.1S11.1,12,11,12s-0.2,0.1-0.3,0.2c-0.1,0.1-0.3,0.2-0.4,0.3
> +      c-0.9,0.5-1.9,0.6-3,0.4c0.4,0.4,0.7,0.7,1.1,0.9c0.1,0,0.2,0.1,0.6,0.1c0.3,0,0.5,0.1,0.6,0.2C9.4,14,9.1,14,8.9,14.1
> +      c0.8,0.5,1.7,0.7,2.8,0.5c0.4-0.1,0.7-0.2,1-0.4c-0.1,0.1-0.1,0.3-0.2,0.4c0.1,0.1,0.4-0.1,0.6-0.5c0.1-0.1,0.3-0.3,0.6-0.4
> +      c0,0,0,0.1,0,0.2c0,0.1,0,0.1,0,0.2s0,0.1,0.1,0.1c0.4,0,0.7-0.4,1.1-1.3c0.3-0.6,0.5-1.3,0.6-2.2c0.1,0.2,0.2,0.5,0.2,0.8
> +      c0.2-0.5,0.3-1,0.3-1.3c0-0.3,0-1.3-0.1-2.9c0.3,0.4,0.5,0.7,0.7,1.1c0.1-1.2-0.1-2.2-0.5-3.1c-0.4-0.8-0.9-1.5-1.4-1.9
> +      c0.5,0.1,1,0.3,1.4,0.6l-0.3-0.2c-1.4-1.4-3.1-2.2-5.1-2.4c-2-0.2-3.8,0.3-5.4,1.4c-0.6,0-1.1,0-1.5,0.1C3.6,2.7,3.5,2.6,3.5,2.5
> +      C5.3,0.8,7.4,0,9.9,0s4.6,0.8,6.5,2.4L16.2,2c0.6,0.3,1,0.7,1.4,1.2l-0.1-0.3l0,0.1l0-0.1c0.9,0.7,1.4,1.6,1.7,2.7
> +      c0.2,0.9,0.2,1.6,0.1,2.3c0.1,0.1,0.1,0.1,0.1,0.3l0.3-1.1c0.1,0.3,0.2,0.7,0.2,1C20,8.5,20,8.9,20,9.2c0,0.4-0.1,0.7-0.1,1
> +      c-0.1,0.3-0.1,0.7-0.2,1s-0.2,0.6-0.3,0.8c-0.1,0.2-0.2,0.5-0.3,0.7C19.1,12.8,19.2,13,19.3,13.4L19.3,13.4z"/>

These changes need to be reverted in the next patch in this series, I believe.
Comment hidden (mozreview-request)
Attachment #8850788 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Hey Mike, would you be able to do a quick review of some changes I had to make on top of what you already reviewed? They're short, and you can see them at https://reviewboard.mozilla.org/r/123308/diff/3-4/. Thanks!
Flags: needinfo?(mconley)
Comment on attachment 8850789 [details]
Bug 1335907 - Reorganize the preferences based on feedback from user research.

https://reviewboard.mozilla.org/r/123308/#review126904
Attachment #8850789 - Flags: review?(jaws) → review+
(Reporter)

Comment 72

2 months ago
mozreview-review
Comment on attachment 8850789 [details]
Bug 1335907 - Reorganize the preferences based on feedback from user research.

https://reviewboard.mozilla.org/r/123308/#review126906

Interdiff (https://reviewboard.mozilla.org/r/123308/diff/3-4/) looks good to me!
Flags: needinfo?(mconley)

Comment 73

2 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ef667136bbe
Reorganize the preferences based on feedback from user research. r=mconley
Sorry, I had to back this out for failing browser_datachoices_notification.js:

https://hg.mozilla.org/integration/autoland/rev/5b5080b0f6209ae2dd835fc694b9aeae3c64eb4c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9ef667136bbe97e7e8e8606df7103f333b006ac2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=87072595&repo=autoland

[task 2017-03-28T21:29:24.192434Z] 21:29:24     INFO - TEST-START | browser/base/content/test/general/browser_storagePressure_notification.js
[task 2017-03-28T21:29:27.300789Z] 21:29:27     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-03-28T21:29:27.303601Z] 21:29:27     INFO - Buffered messages logged at 21:29:24
[task 2017-03-28T21:29:27.305549Z] 21:29:27     INFO - Entering test bound 
[task 2017-03-28T21:29:27.307523Z] 21:29:27     INFO - TEST-PASS | browser/base/content/test/general/browser_storagePressure_notification.js | Should display storage pressure notification - 
[task 2017-03-28T21:29:27.309729Z] 21:29:27     INFO - TEST-PASS | browser/base/content/test/general/browser_storagePressure_notification.js | Should not display storage pressure notification more than once within the given interval - 
[task 2017-03-28T21:29:27.311585Z] 21:29:27     INFO - Buffered messages logged at 21:29:26
[task 2017-03-28T21:29:27.313722Z] 21:29:27     INFO - TEST-PASS | browser/base/content/test/general/browser_storagePressure_notification.js | Should display storage pressure notification after the given interval - 
[task 2017-03-28T21:29:27.317392Z] 21:29:27     INFO - Leaving test bound 
[task 2017-03-28T21:29:27.319436Z] 21:29:27     INFO - Entering test bound 
[task 2017-03-28T21:29:27.321591Z] 21:29:27     INFO - TEST-PASS | browser/base/content/test/general/browser_storagePressure_notification.js | Should display storage pressure notification - 
[task 2017-03-28T21:29:27.324045Z] 21:29:27     INFO - Buffered messages finished
[task 2017-03-28T21:29:27.328292Z] 21:29:27     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_storagePressure_notification.js | uncaught exception - TypeError: doc.getElementById(...) is null at switchToAdvancedSubPane@chrome://browser/content/utilityOverlay.js:710:38
[task 2017-03-28T21:29:27.330180Z] 21:29:27     INFO - advancedPaneLoadedObs@chrome://browser/content/utilityOverlay.js:762:7
[task 2017-03-28T21:29:27.332068Z] 21:29:27     INFO - init_all@chrome://browser/content/preferences/in-content/preferences.js:96:3
[task 2017-03-28T21:29:27.334068Z] 21:29:27     INFO - EventListener.handleEvent*@chrome://browser/content/preferences/in-content/preferences.js:50:1
[task 2017-03-28T21:29:27.335947Z] 21:29:27     INFO - 
[task 2017-03-28T21:29:27.338080Z] 21:29:27     INFO - Stack trace:
[task 2017-03-28T21:29:27.341570Z] 21:29:27     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1648
[task 2017-03-28T21:29:27.343401Z] 21:29:27     INFO - chrome://browser/content/preferences/in-content/preferences.js:init_all:96
[task 2017-03-28T21:29:27.345615Z] 21:29:27     INFO - GECKO(1386) | JavaScript error: chrome://browser/content/utilityOverlay.js, line 710: TypeError: doc.getElementById(...) is null
[task 2017-03-28T21:29:27.375168Z] 21:29:27     INFO - GECKO(1386) | *************************
[task 2017-03-28T21:29:27.375534Z] 21:29:27     INFO - GECKO(1386) | A coding exception was thrown and uncaught in a Task.
[task 2017-03-28T21:29:27.375853Z] 21:29:27     INFO - GECKO(1386) | Full message: TypeError: advancedPrefs is null
[task 2017-03-28T21:29:27.377263Z] 21:29:27     INFO - GECKO(1386) | Full stack: @chrome://mochitests/content/browser/browser/base/content/test/general/browser_storagePressure_notification.js:61:3
[task 2017-03-28T21:29:27.378777Z] 21:29:27     INFO - GECKO(1386) | process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23

Please fix the issues, run it against the Try server because there are more failures, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=87072564&repo=autoland
Flags: needinfo?(jaws)
Flags: needinfo?(herrickz)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05fc26f6683dfde3a27b04b163922d497905d385
Flags: needinfo?(jaws)
Flags: needinfo?(herrickz)
Comment hidden (mozreview-request)
https://reviewboard.mozilla.org/r/123304/diff/4-6/ shows the changes since it was pushed to m-c. This is a rebase on top of bug 1312349 as well as more test fixes. @mconley, does this look OK to you?
Flags: needinfo?(mconley)
(Reporter)

Comment 78

2 months ago
mozreview-review
Comment on attachment 8850789 [details]
Bug 1335907 - Reorganize the preferences based on feedback from user research.

https://reviewboard.mozilla.org/r/123304/#review127274

Looks okay - but let's do a try run this time. :)

::: browser/components/search/test/browser_aboutSearchReset.js:109
(Diff revision 6)
>  {
>    desc: "Click the settings link.",
>    *run() {
>      let loadPromise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser,
>                                                       false,
> -                                                     "about:preferences#search")
> +                                                     "about:preferences")

Should this be about:preferences#general, since that's where the search preferences are?
(Reporter)

Updated

2 months ago
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) (High latency until March 31) from comment #78)
> >                                                       false,
> > -                                                     "about:preferences#search")
> > +                                                     "about:preferences")
> 
> Should this be about:preferences#general, since that's where the search
> preferences are?

about:preferences#general and about:preferences are synonyms of each other. In this case, the opening code is now just opening to about:preferences since that's where the search prefs are, so this test needs to just use about:preferences without the hash. I could dig through where this is getting opened from, but I didn't think it was important to worry about having the `#general` hash in the location bar.
(In reply to Mike Conley (:mconley) (High latency until March 31) from comment #78)
> Looks okay - but let's do a try run this time. :)

Yep, try run in comment 75.
(Reporter)

Comment 81

2 months ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #79)
> 
> about:preferences#general and about:preferences are synonyms of each other.
> In this case, the opening code is now just opening to about:preferences
> since that's where the search prefs are, so this test needs to just use
> about:preferences without the hash. I could dig through where this is
> getting opened from, but I didn't think it was important to worry about
> having the `#general` hash in the location bar.

Okay, fair enough - issue dropped.

Comment 82

2 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/069b6372cc15
Reorganize the preferences based on feedback from user research. r=mconley

Comment 83

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/069b6372cc15
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 months ago
Depends on: 1352374
(Reporter)

Comment 84

2 months ago
Release Note Request (optional, but appreciated)
[Why is this notable]:

We're changing the organization of about:preferences. Note that, while we've landed this in 55, we haven't gotten QA sign-off yet, and the change is pref-able. If we don't get sign-off to release in 55, we will have to turn the change off.

[Affects Firefox for Android]:

No.

[Suggested wording]:

about:preferences has been cleaned up and reorganized to make it easier for you to find what you need.


[Links (documentation, blog post, etc)]:

https://docs.google.com/presentation/d/1R-HofYuN0bJcROlwUXStxr5CqcOF6WKooZgciikFJgw/edit
relnote-firefox: --- → ?

Updated

2 months ago
Depends on: 1349051
I see this landed in Nightly now. But I see very confusing having the setting for the address bar behavior on two places - search engines in General and Addressbar itself in Privacy & Security. Could we merge them to a separate extra category? It would also reduce the length of both sections. The scrolling is also very uncomfortable compared to the old categories. But this might be solved if there is some settings search added, something like Chromium has.

And the same question as :flod mentioned above, isn't login meant like username+password? As the string "Remember logins and passwords for sites" is something confusing compared to the terminology used so far.
Flags: needinfo?(jaws)
The "settings search" is being added in bug 1335905.

Login may mean "username + password" to technical users, but users may also associate login with only the username. These names were arrived at through multiple rounds of user testing so I am confident that they are an improvement over what we were shipping previously. By the way, Firefox does offer the ability to remember logins without the associated password for a site, so this new wording is actually technically correct.
Flags: needinfo?(jaws)
No longer blocks: 1348828
Depends on: 1348828
Depends on: 1352498
Cool, I have been aware only of the option to save password without the username. Anyway we won't translate this word by word in our localization.
Depends on: 1352505
Depends on: 1353036

Updated

2 months ago
Depends on: 1353056

Updated

2 months ago
Depends on: 1267480
I've tested on Windows 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.12 using latest Nightly 55.0a1 (2017-04-05) and I have the following mentions: 

- based on slide 3 of this slide deck: https://docs.google.com/presentation/d/1R-HofYuN0bJcROlwUXStxr5CqcOF6WKooZgciikFJgw/edit , the first category from GENERAL is "Browser" -> we don't have this category; any thought? it is expected? 

- under "Downloads & Links" only Downloads category is displayed (which apparently contains the Application options) -> I think Application (File Type Handler, Link Type Handler) title is missing; we should differentiate downloads from applications -> What do you think?
Flags: needinfo?(mconley)
(In reply to Francesco Lodolo [:flod] from comment #60)
> Strings are not going to be used in a different context, existing panels are
> going to be reorganized, and eventually new section titles added

So they are used in a different contexts and you are completely wrong.

> The only concern could be accesskeys, but from the mock-ups I've seen it
> won't be possible to have unique identifiers in the same panel.

Maybe it is your only concern but unfortunately not for us.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #61) 
> flod is correct. We will not be sharing strings between the new and old UI.
> We have forked the strings and copied them to another directory for use by
> the old UI until we can remove it.

The problem is that you reuse strings in a context they were never meant for. Old UI translations in a new easily removable place doesn't fix any of the l10n issues. This is exactly opposite to how Mozilla l10n system should work.
(In reply to Stefan Plewako [:stef] from comment #89)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #61) 
> > flod is correct. We will not be sharing strings between the new and old UI.
> > We have forked the strings and copied them to another directory for use by
> > the old UI until we can remove it.
> 
> The problem is that you reuse strings in a context they were never meant
> for. Old UI translations in a new easily removable place doesn't fix any of
> the l10n issues. This is exactly opposite to how Mozilla l10n system should
> work.

Can you share a specific example where this has caused an issue for you?
Flags: needinfo?(splewako)
(In reply to Camelia Badau, QA [:cbadau] from comment #88)
> I've tested on Windows 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.12 using
> latest Nightly 55.0a1 (2017-04-05) and I have the following mentions: 
> 
> - based on slide 3 of this slide deck:
> https://docs.google.com/presentation/d/1R-
> HofYuN0bJcROlwUXStxr5CqcOF6WKooZgciikFJgw/edit , the first category from
> GENERAL is "Browser" -> we don't have this category; any thought? it is
> expected? 

Hi Camelia, looking at slide 4 it shows what would go in to the "Browser" section. Can you please file a bug for moving the "default browser" options to their own "Browser" section?
 
> - under "Downloads & Links" only Downloads category is displayed (which
> apparently contains the Application options) -> I think Application (File
> Type Handler, Link Type Handler) title is missing; we should differentiate
> downloads from applications -> What do you think?

This is a link to the spec that we should use to verify this bug: https://mozilla.invisionapp.com/share/5RA0R4HAE#/screens/214905995

I notice now that "Downloads & Links" has been renamed to "Files & Applications". Also, the applications part of that view should have the "Applications" header.

Could you file bugs for these issues, as well as any other issues you find when comparing the spec I linked to above? You should mark the new bugs as blocking this bug. Thanks!
Flags: needinfo?(mconley)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #90)
> (In reply to Stefan Plewako [:stef] from comment #89)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #61) 
> > > flod is correct. We will not be sharing strings between the new and old UI.
> > > We have forked the strings and copied them to another directory for use by
> > > the old UI until we can remove it.
> > 
> > The problem is that you reuse strings in a context they were never meant
> > for. Old UI translations in a new easily removable place doesn't fix any of
> > the l10n issues. This is exactly opposite to how Mozilla l10n system should
> > work.
> 
> Can you share a specific example where this has caused an issue for you?

What do you exactly mean by "specific example" and "an issue", what do you need that for? Right now I'm starring at changed nightly preferences and updated files trying to figure out what exactly has changed and how might change in the near future. Keep in mind that we are not about accesskeys but grammatical constructs and other ways that localization differs from simple translations. And for accesskeys I will need to find Windows machine and retest all access keys in preferences essentially wasting hours…
Flags: needinfo?(splewako)
(In reply to Stefan Plewako [:stef] from comment #92)
> What do you exactly mean by "specific example" and "an issue", what do you
> need that for? Right now I'm starring at changed nightly preferences and
> updated files trying to figure out what exactly has changed and how might
> change in the near future. Keep in mind that we are not about accesskeys but
> grammatical constructs and other ways that localization differs from simple
> translations. And for accesskeys I will need to find Windows machine and
> retest all access keys in preferences essentially wasting hours…

Entire blocks of preferences have moved into a different panel, it's not like one radio button moved under a different label. 
I don't see how that could affect the grammatical structure, if you have examples, that would be helpful and constructive criticism.

As for accesskeys, as explained you need to give up the idea of having unique accesskeys in a panel.
(In reply to Francesco Lodolo [:flod] from comment #93)
> Entire blocks of preferences have moved into a different panel, it's not
> like one radio button moved under a different label.

What do you exactly mean by "entire blocks"?

> I don't see how that could affect the grammatical structure

In Italian? I don't see either but in Polish I'm expecting that I will need to change things after reading comment 15 (esp points 3, 4, 5) and one place where whole section is translated quite differently.

Strings length, wrapping and truncation also contribute to anxiety.

> As for accesskeys, as explained you need to give up the idea of having
> unique accesskeys in a panel.

I don't know what exactly you mean by "you need to give up the idea of having unique accesskeys in a panel" but I'm pretty sure I don't agree with you about implications.
(In reply to Stefan Plewako [:stef] from comment #94)
> Strings length, wrapping and truncation also contribute to anxiety.

Either you can provide a practical example, or it's not useful to have this discussion. I'm not going to ask localizers to retranslate 600 strings without good data to support such a request.

> I don't know what exactly you mean by "you need to give up the idea of
> having unique accesskeys in a panel" but I'm pretty sure I don't agree with
> you about implications.

It's not "my implication", it's a design choice. There are too many strings to have enough characters in every single panel.

Have you tried opening the preferences on Nightly? That would answer most of your questions (blocks, accesskeys).
(In reply to Francesco Lodolo [:flod] from comment #95)
> (In reply to Stefan Plewako [:stef] from comment #94)
> > Strings length, wrapping and truncation also contribute to anxiety.
> 
> Either you can provide a practical example, or it's not useful to have this
> discussion. I'm not going to ask localizers to retranslate 600 strings
> without good data to support such a request.

I thought that translation memory should help in such cases and not some trickery based on someones assumptions not matching reality.

> > I don't know what exactly you mean by "you need to give up the idea of
> > having unique accesskeys in a panel" but I'm pretty sure I don't agree with
> > you about implications.
> 
> It's not "my implication", it's a design choice. There are too many strings
> to have enough characters in every single panel.

You can believe that but that doesn't change much and do not answer the question what should be done about them.

> Have you tried opening the preferences on Nightly? That would answer most of
> your questions (blocks, accesskeys).

I did, if you would actually read my comments you would know that and it ends this discussion for me.

Updated

2 months ago
Depends on: 1353805

Updated

2 months ago
Depends on: 1353808
Depends on: 1353845
Depends on: 1353860
Depends on: 1354062
Depends on: 1354064
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #91)
> (In reply to Camelia Badau, QA [:cbadau] from comment #88)
> > I've tested on Windows 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.12 using
> > latest Nightly 55.0a1 (2017-04-05) and I have the following mentions: 
> > 
> > - based on slide 3 of this slide deck:
> > https://docs.google.com/presentation/d/1R-
> > HofYuN0bJcROlwUXStxr5CqcOF6WKooZgciikFJgw/edit , the first category from
> > GENERAL is "Browser" -> we don't have this category; any thought? it is
> > expected? 
> 
> Hi Camelia, looking at slide 4 it shows what would go in to the "Browser"
> section. Can you please file a bug for moving the "default browser" options
> to their own "Browser" section?
>  
> > - under "Downloads & Links" only Downloads category is displayed (which
> > apparently contains the Application options) -> I think Application (File
> > Type Handler, Link Type Handler) title is missing; we should differentiate
> > downloads from applications -> What do you think?
> 
> This is a link to the spec that we should use to verify this bug:
> https://mozilla.invisionapp.com/share/5RA0R4HAE#/screens/214905995
> 
> I notice now that "Downloads & Links" has been renamed to "Files &
> Applications". Also, the applications part of that view should have the
> "Applications" header.
> 
> Could you file bugs for these issues, as well as any other issues you find
> when comparing the spec I linked to above? You should mark the new bugs as
> blocking this bug. Thanks!


I filled bug 1354062 and bug 1354064 .

Updated

2 months ago
Depends on: 1354887
Depends on: 1356006
Depends on: 1356008
Depends on: 1356009
Depends on: 1356020
Depends on: 1356021
Depends on: 1355522
Depends on: 1356507
Blocks: 1356508
No longer blocks: 1356508
Depends on: 1356508
No longer depends on: 1356508
No longer depends on: 1356507
No longer depends on: 1356006
No longer depends on: 1356008
No longer depends on: 1356009
No longer depends on: 1356020
No longer depends on: 1356021

Comment 98

a month ago
Hi! Where can I express my feelings about this reorganization of about:preferences ? Thank you!
Flags: needinfo?(jaws)
(In reply to Julien L. from comment #98)
> Hi! Where can I express my feelings about this reorganization of
> about:preferences ? Thank you!

Please send any comments you may have regarding these changes to the firefox-dev mailing list. More information about the mailing list can be found at https://mail.mozilla.org/pipermail/firefox-dev/

Thanks!
Flags: needinfo?(jaws)

Comment 100

a month ago
Is this reorg why I can't open Privacy and Security in about:preferences?
(In reply to matt thul from comment #100)
> Is this reorg why I can't open Privacy and Security in about:preferences?

Can you please explain the steps that you are taking? For example, when I open the Firefox menu and choose "Options" ("Preferences" if I was on Mac), I can then click on Privacy & Security on the left side and the specific preferences are visible. Does this not work for you? Are you getting to the preferences via another route? Also, what version are you running and on what operating system?
Flags: needinfo?(floridamatt)

Comment 102

a month ago
I choose options and then click on Privacy and Security, and nothing happens.  All the other menu choices work, but going back to Privacy and Security and clicking leaves the previously selected choice on the screen.

55.0a1 (2017-04-17) (64-bit)  running on Win10 X64, up to date

Works the same way in safe mode.

Release version of Firefox using same profile does not have the issue (but, then, it's got the older code with privacy and security as discreet items.  

I've also tried about:preferences#privacy and #security and #privacysecurity having seen "&panePrivacySecurity.title;" in the page source.
(In reply to matt thul from comment #102)

Please file a new bug for this and we can investigate there. Thanks!
Flags: needinfo?(floridamatt)
(In reply to matt thul from comment #100)
> Is this reorg why I can't open Privacy and Security in about:preferences?

That is probably bug 1355795.
Depends on: 1357905

Updated

a month ago
Depends on: 1358394
Depends on: 1362571
No longer depends on: 1362571

Updated

15 days ago
Depends on: 1363960
Depends on: 1364041
Depends on: 1364061
relnote-firefox: ? → ---
FYI. This unfortunately will be not be shipped in fx55 (to be converted Nightly only in bug 1363850) because of bug 1365133.

Comment 106

13 hours ago
looks like this will cause lots of documentation work for sumo (targeting firefox 56 according to https://bugzilla.mozilla.org/show_bug.cgi?id=1365133#c2 currently)
Flags: needinfo?(jsavage)
You need to log in before you can comment on or make changes to this bug.