Closed
Bug 1335907
Opened 8 years ago
Closed 8 years ago
Re-arrange sections in about:preferences according to updated spec
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mconley, Assigned: herrickz, Mentored, NeedInfo)
References
(Depends on 2 open bugs, Blocks 1 open bug)
Details
Attachments
(1 file, 7 obsolete files)
See slide 3 of this slide deck: https://docs.google.com/presentation/d/1R-HofYuN0bJcROlwUXStxr5CqcOF6WKooZgciikFJgw/edit
Updated•8 years ago
|
Assignee: nobody → herrickz
Mentor: jaws, mconley
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
mozreview-review |
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 9•8 years ago
|
||
mozreview-review |
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 13•8 years ago
|
||
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 14•8 years 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/#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•8 years 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/
Comment 16•8 years ago
|
||
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•8 years 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•8 years 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 & Links"> > +<!ENTITY panePrivacy.title "Privacy & 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•8 years ago
|
Attachment #8838234 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8838255 -
Attachment is obsolete: true
Comment 22•8 years ago
|
||
(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)
Comment 23•8 years ago
|
||
(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 24•8 years 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/#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 & 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•8 years 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•8 years 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•8 years ago
|
Attachment #8840630 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 30•8 years 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/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 31•8 years 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/#review117696
Attachment #8836376 -
Flags: review?(jaws) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 34•8 years 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 35•8 years 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/#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) |
Updated•8 years ago
|
Attachment #8836376 -
Flags: review?(mconley)
Updated•8 years ago
|
Attachment #8834653 -
Attachment is obsolete: true
Comment 37•8 years 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/#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 38•8 years 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/#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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e6ece6c5117
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 42•8 years 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/#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 44•8 years ago
|
||
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 45•8 years 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/#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•8 years 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 & 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 & 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 & 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)
Comment 48•8 years ago
|
||
I have the feeling I commented the wrong patch, but issues seem to be present in both.
Reporter | ||
Comment 49•8 years 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•8 years 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•8 years 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 52•8 years ago
|
||
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+
Comment 53•8 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Attachment #8836376 -
Attachment is obsolete: true
Comment 56•8 years ago
|
||
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.
Comment 57•8 years 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•8 years ago
|
Updated•8 years ago
|
Comment 58•8 years 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.
Comment 59•8 years ago
|
||
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.
Comment 60•8 years ago
|
||
(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.
Comment 61•8 years ago
|
||
(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•8 years 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+
Updated•8 years ago
|
Attachment #8849116 -
Flags: feedback?(benjamin)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Attachment #8850788 -
Flags: review?(jaws) → review+
Comment 65•8 years ago
|
||
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•8 years 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•8 years 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) |
Updated•8 years ago
|
Attachment #8850788 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 70•8 years ago
|
||
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 71•8 years 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/#review126904
Attachment #8850789 -
Flags: review?(jaws) → review+
Reporter | ||
Comment 72•8 years 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!
Updated•8 years ago
|
Flags: needinfo?(mconley)
Comment 73•8 years 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
![]() |
||
Comment 74•8 years ago
|
||
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)
Comment 75•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05fc26f6683dfde3a27b04b163922d497905d385
Flags: needinfo?(jaws)
Flags: needinfo?(herrickz)
Comment hidden (mozreview-request) |
Comment 77•8 years ago
|
||
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•8 years 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•8 years ago
|
Flags: needinfo?(mconley)
Comment 79•8 years ago
|
||
(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.
Comment 80•8 years ago
|
||
(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•8 years 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•8 years 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/069b6372cc15
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Reporter | ||
Comment 84•8 years 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:
--- → ?
Comment 85•8 years ago
|
||
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)
Comment 86•8 years ago
|
||
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)
Updated•8 years ago
|
Comment 87•8 years ago
|
||
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.
Comment 88•8 years ago
|
||
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)
Comment 89•8 years ago
|
||
(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.
Comment 90•8 years ago
|
||
(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)
Comment 91•8 years ago
|
||
(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)
Comment 92•8 years ago
|
||
(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)
Comment 93•8 years ago
|
||
(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.
Comment 94•8 years ago
|
||
(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.
Comment 95•8 years ago
|
||
(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).
Comment 96•8 years ago
|
||
(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.
Comment 97•8 years ago
|
||
(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•8 years ago
|
Comment 98•8 years ago
|
||
Hi! Where can I express my feelings about this reorganization of about:preferences ? Thank you!
Flags: needinfo?(jaws)
Comment 99•8 years ago
|
||
(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 101•8 years ago
|
||
(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•8 years 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.
Comment 103•8 years ago
|
||
(In reply to matt thul from comment #102) Please file a new bug for this and we can investigate there. Thanks!
Flags: needinfo?(floridamatt)
Comment 104•8 years ago
|
||
(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.
Updated•7 years ago
|
relnote-firefox:
? → ---
Comment 105•7 years ago
|
||
FYI. This unfortunately will be not be shipped in fx55 (to be converted Nightly only in bug 1363850) because of bug 1365133.
Comment 106•7 years 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)
Comment 107•7 years ago
|
||
(In reply to Camelia Badau, QA [:cbadau] from comment #97) > > 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. As per this link t0 verify bug:https://mozilla.invisionapp.com/share/5RA0R4HAE#/screens/214905995, could see changes in privacy and security tab. 1. Couldn't found "Permissions" under privacy and security. 2. "Block pop-up windows" checkbox is separated from "Permissions" and been placed under "Pop-ups". 3. "Warn you when sites try to install add-on" check box is separated from "Permissions" and been placed as one of the check box under "Security". 4. "Privacy data collection and use" has been renamed to "Reports". 5. Container tab option is in Privacy & security. The bug is tested under Firefox Nightly 55.0a1. Build ID: 20170608030205 [bugday-20170607]
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•