Updated General Preferences

- Show home button now disables itself when home page is cleared (null/empty)
- Removed appearance settings; moved to under "My home page is" per the mockup on #2108
- Bookmarks toolbar switches were changed into a single drop down

Unit tests are in place and working + I've manually at least two of the scenarios outlined in the attached steps

Fixes #2108
Fixes #3008
Fixes #2111
Possibly fixes #3544

Auditors:
@BrendanEich - you had previously reviewed the settings code; LMK how the changes in `./js/settings.js` look
@bbondy - small changes EXCEPT the new config value for BTB. This only had two touchpoints (main.js, contextMenus.js) versus the password manager changes

Tweaks made per review w/ Brad. Includes:
- reducing space between label and field in preferences
- the lone toggle switch was pulled over to the left
- import button is moved up
- import button area is no longer a separate grouping

Upgrade test plan:
1. Install a previous version of Brave (0.12.4 or older); ENABLE favicons and ENABLE show only favicons
2. Upgrade to this version (0.12.5?). Confirm it consolidates preference to "Favicons only" in preferences drop down
3. Install a previous version of Brave (0.12.4 or older); ENABLE favicons but DISABLE show only favicons
4. Upgrade to this version (0.12.5?). Confirm it consolidates preference to "Text and Favicons" in preferences drop down
5. Install a previous version of Brave (0.12.4 or older); DISABLE favicons and DISABLE show only favicons
6. Upgrade to this version (0.12.5?). Confirm it consolidates preference to "Text only" in preferences drop down
7. Uninstall Brave completely; move session file out of the way

Regular test plan:
** consolidation of bookmarks toolbar settings**
1. Install a clean version of Brave (0.12.5?)
2. Launch preferences, confirm preference defaults to "Text only"
3. Confirm no favicons show (only text):
  - at the root level of the  bookmarks toolbar
  - when expanding a folder inside the bookmarks toolbar
4. Repeat step 2/3 for the other two settings. NOTE: show only favicon does not apply to items inside bookmark folders

** bookmark toolbar enable/disable change **
5. Go to preferences and toggle "Always show the bookmarks toolbar"
6. If disabled, notice how drop down is now disabled too (not allowing you to change)
7. When enabled, notice how drop down is re-enabled

** disabling of show home button **
8. Set your home page to brave.com and enable "show home button"
9. Erase out the value in home page and confirm "show home button" is:
  - is turned off
  - disabled completely (not clickable)
10. Type a non-whitespace character or characters into home page box
11. Confirm the "show home button" is clickable again

** overall general settings tab **
12. Ensure screen matches corresponding parts of mockup shown in #2108
13. Ensure screen matches corresponding parts of mockup shown in #2111

app/common/constants/bookmarksToolbarMode.js

@@ -0,0 +1,11 @@
+/* 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/. */
+
+const settings = {
+  TEXT_ONLY: 'textOnly',
+  TEXT_AND_FAVICONS: 'textAndFavicons',
+  FAVICONS_ONLY: 'faviconsOnly'
+}
+
+module.exports = settings

app/extensions/brave/locales/en-US/preferences.properties

@@ -153,7 +153,7 @@ managePasswords=Manage passwords…
 sitePermissions=Saved Site Permissions
 sitePermissionsExceptions=Saved Site Exceptions
 selectedLanguage=Language:
-bookmarkToolbarSettings=Bookmarks Bar Settings
+bookmarkToolbarSettings=Bookmarks Bar
 bookmarkToolbar=Always show the bookmarks bar
 bookmarkToolbarShowFavicon=Favicons
 bookmarkToolbarShowOnlyFavicon=Show only favicon
@@ -220,7 +220,6 @@ allowUntilRestart=Allow until restart
 flashAllowAlways=Allow until {{time}}
 alwaysAllow=Always allow
 alwaysDeny=Always deny
-appearanceSettings=Appearance Settings
 autoHideMenuBar=Hide the menu bar by default
 disableTitleMode=Always show the URL bar
 tabsSettings=Tabs Settings
@@ -247,3 +246,6 @@ importNow=Import now…
 clearAll=Clear all
 sendCrashReports=Send anonymous crash reports to Brave (requires browser restart)
 sendUsageStatistics=Automatically send usage statistics to Brave
+bookmarksBarTextOnly=Text only
+bookmarksBarTextAndFavicon=Text and Favicons
+bookmarksBarFaviconOnly=Favicons only

js/about/aboutActions.js

@@ -210,7 +210,7 @@ const aboutActions = {
     ipc.sendToHost(messages.CLEAR_BROWSING_DATA_NOW, clearBrowsingDataDetail)
   },
 
-  importBrowerDataNow: function () {
+  importBrowserDataNow: function () {
     ipc.send(messages.IMPORT_BROWSER_DATA_NOW)
   },
 

js/about/preferences.js

@@ -20,6 +20,7 @@ const messages = require('../constants/messages')
 const settings = require('../constants/settings')
 const coinbaseCountries = require('../constants/coinbaseCountries')
 const {passwordManagers, extensionIds} = require('../constants/passwordManagers')
+const bookmarksToolbarMode = require('../../app/common/constants/bookmarksToolbarMode')
 const aboutActions = require('./aboutActions')
 const getSetting = require('../settings').getSetting
 const SortableTable = require('../components/sortableTable')
@@ -138,7 +139,14 @@ class SettingCheckbox extends ImmutableComponent {
   }
 
   render () {
-    return <div style={this.props.style} className='settingItem'>
+    const props = {
+      style: this.props.style,
+      className: 'settingItem'
+    }
+    if (this.props.id) {
+      props.id = this.props.id
+    }
+    return <div {...props}>
       <SwitchControl id={this.props.prefKey}
         disabled={this.props.disabled}
         onClick={this.onClick}
@@ -572,10 +580,22 @@ class GeneralTab extends ImmutableComponent {
   constructor (e) {
     super()
     this.importBrowserDataNow = this.importBrowserDataNow.bind(this)
+    this.onChangeSetting = this.onChangeSetting.bind(this)
   }
 
   importBrowserDataNow () {
-    aboutActions.importBrowerDataNow()
+    aboutActions.importBrowserDataNow()
+  }
+
+  onChangeSetting (key, value) {
+    // disable "SHOW_HOME_BUTTON" if it's enabled and homepage is blank
+    if (key === settings.HOMEPAGE && getSetting(settings.SHOW_HOME_BUTTON, this.props.settings)) {
+      const homepage = value && value.trim()
+      if (!homepage || !homepage.length) {
+        this.props.onChangeSetting(settings.SHOW_HOME_BUTTON, false)
+      }
+    }
+    this.props.onChangeSetting(key, value)
   }
 
   enabled (keyArray) {
@@ -588,7 +608,12 @@ class GeneralTab extends ImmutableComponent {
         <option data-l10n-id={lc} value={lc} />
       )
     })
+    const homepageValue = getSetting(settings.HOMEPAGE, this.props.settings)
+    const homepage = homepageValue && homepageValue.trim()
+    const disableShowHomeButton = !homepage || !homepage.length
+    const disableBookmarksBarSelect = !getSetting(settings.SHOW_BOOKMARKS_TOOLBAR, this.props.settings)
     const defaultLanguage = this.props.languageCodes.find((lang) => lang.includes(navigator.language)) || 'en-US'
+
     return <SettingsList>
       <div className='sectionTitle' data-l10n-id='generalSettings' />
       <SettingsList>
@@ -604,32 +629,38 @@ class GeneralTab extends ImmutableComponent {
           <input spellCheck='false'
             data-l10n-id='homepageInput'
             value={getSetting(settings.HOMEPAGE, this.props.settings)}
-            onChange={changeSetting.bind(null, this.props.onChangeSetting, settings.HOMEPAGE)} />
+            onChange={changeSetting.bind(null, this.onChangeSetting, settings.HOMEPAGE)} />
+        </SettingItem>
+        <SettingCheckbox dataL10nId='showHomeButton' prefKey={settings.SHOW_HOME_BUTTON}
+          settings={this.props.settings} onChangeSetting={this.props.onChangeSetting}
+          disabled={disableShowHomeButton} />
+        {
+          isDarwin ? null : <SettingCheckbox dataL10nId='autoHideMenuBar' prefKey={settings.AUTO_HIDE_MENU} settings={this.props.settings} onChangeSetting={this.props.onChangeSetting} />
+        }
+        <SettingCheckbox dataL10nId='disableTitleMode' prefKey={settings.DISABLE_TITLE_MODE} settings={this.props.settings} onChangeSetting={this.props.onChangeSetting} />
+        <SettingItem dataL10nId='bookmarkToolbarSettings'>
+          <select id='bookmarksBarSelect' value={getSetting(settings.BOOKMARKS_TOOLBAR_MODE, this.props.settings)}
+            disabled={disableBookmarksBarSelect}
+            onChange={changeSetting.bind(null, this.props.onChangeSetting, settings.BOOKMARKS_TOOLBAR_MODE)} >
+            <option data-l10n-id='bookmarksBarTextOnly' value={bookmarksToolbarMode.TEXT_ONLY} />
+            <option data-l10n-id='bookmarksBarTextAndFavicon' value={bookmarksToolbarMode.TEXT_AND_FAVICONS} />
+            <option data-l10n-id='bookmarksBarFaviconOnly' value={bookmarksToolbarMode.FAVICONS_ONLY} />
+          </select>
+          <SettingCheckbox id='bookmarksBarSwitch' dataL10nId='bookmarkToolbar'
+            prefKey={settings.SHOW_BOOKMARKS_TOOLBAR} settings={this.props.settings}
+            onChangeSetting={this.props.onChangeSetting} />
         </SettingItem>
         <SettingItem dataL10nId='selectedLanguage'>
           <select value={getSetting(settings.LANGUAGE, this.props.settings) || defaultLanguage}
             onChange={changeSetting.bind(null, this.props.onChangeSetting, settings.LANGUAGE)} >
             {languageOptions}
           </select>
         </SettingItem>
+        <SettingItem dataL10nId='importBrowserData'>
+          <Button l10nId='importNow' className='primaryButton importNowButton'
+            onClick={this.importBrowserDataNow} />
+        </SettingItem>
       </SettingsList>
-      <div className='sectionTitle' data-l10n-id='bookmarkToolbarSettings' />
-      <SettingsList>
-        <SettingCheckbox dataL10nId='bookmarkToolbar' prefKey={settings.SHOW_BOOKMARKS_TOOLBAR} settings={this.props.settings} onChangeSetting={this.props.onChangeSetting} />
-        <SettingCheckbox dataL10nId='bookmarkToolbarShowFavicon' style={{ display: this.enabled([settings.SHOW_BOOKMARKS_TOOLBAR]) ? 'block' : 'none' }} prefKey={settings.SHOW_BOOKMARKS_TOOLBAR_FAVICON} settings={this.props.settings} onChangeSetting={this.props.onChangeSetting} />
-        <SettingCheckbox dataL10nId='bookmarkToolbarShowOnlyFavicon' style={{ display: this.enabled([settings.SHOW_BOOKMARKS_TOOLBAR, settings.SHOW_BOOKMARKS_TOOLBAR_FAVICON]) ? 'block' : 'none' }} prefKey={settings.SHOW_BOOKMARKS_TOOLBAR_ONLY_FAVICON} settings={this.props.settings} onChangeSetting={this.props.onChangeSetting} />
-      </SettingsList>
-      <div className='sectionTitle' data-l10n-id='appearanceSettings' />
-      <SettingsList>
-        <SettingCheckbox dataL10nId='showHomeButton' prefKey={settings.SHOW_HOME_BUTTON} settings={this.props.settings} onChangeSetting={this.props.onChangeSetting} />
-        {
-          isDarwin ? null : <SettingCheckbox dataL10nId='autoHideMenuBar' prefKey={settings.AUTO_HIDE_MENU} settings={this.props.settings} onChangeSetting={this.props.onChangeSetting} />
-        }
-        <SettingCheckbox dataL10nId='disableTitleMode' prefKey={settings.DISABLE_TITLE_MODE} settings={this.props.settings} onChangeSetting={this.props.onChangeSetting} />
-      </SettingsList>
-      <div className='sectionTitle' data-l10n-id='importBrowserData' />
-      <Button l10nId='importNow' className='primaryButton importNowButton'
-        onClick={this.importBrowserDataNow} />
     </SettingsList>
   }
 }

js/components/main.js

@@ -53,6 +53,7 @@ const dragTypes = require('../constants/dragTypes')
 const keyCodes = require('../../app/common/constants/keyCodes')
 const keyLocations = require('../../app/common/constants/keyLocations')
 const isWindows = process.platform === 'win32'
+const bookmarksToolbarMode = require('../../app/common/constants/bookmarksToolbarMode')
 
 // State handling
 const basicAuthState = require('../../app/common/state/basicAuthState')
@@ -812,8 +813,9 @@ class Main extends ImmutableComponent {
     const nonPinnedFrames = this.props.windowState.get('frames').filter((frame) => !frame.get('pinnedLocation'))
     const tabsPerPage = Number(getSetting(settings.TABS_PER_PAGE))
     const showBookmarksToolbar = getSetting(settings.SHOW_BOOKMARKS_TOOLBAR)
-    const showFavicon = getSetting(settings.SHOW_BOOKMARKS_TOOLBAR_FAVICON)
-    const showOnlyFavicon = getSetting(settings.SHOW_BOOKMARKS_TOOLBAR_ONLY_FAVICON)
+    const btbMode = getSetting(settings.BOOKMARKS_TOOLBAR_MODE)
+    const showFavicon = (btbMode === bookmarksToolbarMode.TEXT_AND_FAVICONS || btbMode === bookmarksToolbarMode.FAVICONS_ONLY)
+    const showOnlyFavicon = btbMode === bookmarksToolbarMode.FAVICONS_ONLY
     const siteInfoIsVisible = this.props.windowState.getIn(['ui', 'siteInfo', 'isVisible'])
     const braveShieldsDisabled = this.braveShieldsDisabled
     const braveryPanelIsVisible = !braveShieldsDisabled && this.props.windowState.get('braveryPanelDetail')

js/constants/settings.js

@@ -11,6 +11,9 @@ const settings = {
   DEFAULT_DOWNLOAD_SAVE_PATH: 'general.downloads.default-save-path',
   AUTO_HIDE_MENU: 'general.autohide-menu',
   DISABLE_TITLE_MODE: 'general.disable-title-mode',
+  BOOKMARKS_TOOLBAR_MODE: 'general.bookmarks-toolbar-mode',
+  SHOW_BOOKMARKS_TOOLBAR: 'bookmarks.toolbar.show',
+  LANGUAGE: 'general.language',
   // Search tab
   DEFAULT_SEARCH_ENGINE: 'search.default-search-engine',
   OFFER_SEARCH_SUGGESTIONS: 'search.offer-search-suggestions',
@@ -37,16 +40,6 @@ const settings = {
   SHUTDOWN_CLEAR_SITE_SETTINGS: 'shutdown.clear-site-settings',
   // Autofill
   AUTOFILL_ENABLED: 'privacy.autofill-enabled',
-  // Security Tab: DEPRECATED but still required (for now)
-  PASSWORD_MANAGER_ENABLED: 'security.passwords.manager-enabled',
-  ONE_PASSWORD_ENABLED: 'security.passwords.one-password-enabled',
-  DASHLANE_ENABLED: 'security.passwords.dashlane-enabled',
-  LAST_PASS_ENABLED: 'security.passwords.last-pass-enabled',
-  // Other settings
-  SHOW_BOOKMARKS_TOOLBAR: 'bookmarks.toolbar.show',
-  SHOW_BOOKMARKS_TOOLBAR_FAVICON: 'bookmarks.toolbar.showFavicon',
-  SHOW_BOOKMARKS_TOOLBAR_ONLY_FAVICON: 'bookmarks.toolbar.showOnlyFavicon',
-  LANGUAGE: 'general.language',
   // Payments Tab
   PAYMENTS_ENABLED: 'payments.enabled',
   PAYMENTS_NOTIFICATIONS: 'payments.notifications',
@@ -61,7 +54,21 @@ const settings = {
   SEND_USAGE_STATISTICS: 'advanced.send-usage-statistics',
   ADBLOCK_CUSTOM_RULES: 'adblock.customRules',
   MINIMUM_VISIT_TIME: 'advanced.minimum-visit-time',
-  MINIMUM_VISTS: 'advanced.minimum-visits'
+  MINIMUM_VISTS: 'advanced.minimum-visits',
+
+  // DEPRECATED settings
+  // ########################
+  // these constants should not appear outside of this file, ../settings.js, and our tests
+  // NOTE: these settings rely on default values being set in ./appConfig.js
+  // ########################
+  // > phased out with 0.11.4
+  PASSWORD_MANAGER_ENABLED: 'security.passwords.manager-enabled',
+  ONE_PASSWORD_ENABLED: 'security.passwords.one-password-enabled',
+  DASHLANE_ENABLED: 'security.passwords.dashlane-enabled',
+  LAST_PASS_ENABLED: 'security.passwords.last-pass-enabled',
+  // > phased out with 0.12.5
+  SHOW_BOOKMARKS_TOOLBAR_FAVICON: 'bookmarks.toolbar.showFavicon',
+  SHOW_BOOKMARKS_TOOLBAR_ONLY_FAVICON: 'bookmarks.toolbar.showOnlyFavicon'
 }
 
 module.exports = settings

js/contextMenus.js

@@ -34,6 +34,7 @@ const urlParse = require('url').parse
 const eventUtil = require('./lib/eventUtil')
 const currentWindow = require('../app/renderer/currentWindow')
 const config = require('./constants/config')
+const bookmarksToolbarMode = require('../app/common/constants/bookmarksToolbarMode')
 
 const isDarwin = process.platform === 'darwin'
 
@@ -337,7 +338,8 @@ function showBookmarkFolderInit (allBookmarkItems, parentBookmarkFolder, activeF
 }
 
 function bookmarkItemsInit (allBookmarkItems, items, activeFrame) {
-  const showFavicon = getSetting(settings.SHOW_BOOKMARKS_TOOLBAR_FAVICON) === true
+  const btbMode = getSetting(settings.BOOKMARKS_TOOLBAR_MODE)
+  const showFavicon = (btbMode === bookmarksToolbarMode.TEXT_AND_FAVICONS || btbMode === bookmarksToolbarMode.FAVICONS_ONLY)
   return items.map((site) => {
     const isFolder = siteUtil.isFolder(site)
     let faIcon

js/settings.js

@@ -6,24 +6,45 @@ const appConfig = require('./constants/appConfig')
 const Immutable = require('immutable')
 const settings = require('./constants/settings')
 const {passwordManagers, defaultPasswordManager, extensionIds, displayNames} = require('./constants/passwordManagers')
+const bookmarksToolbarMode = require('../app/common/constants/bookmarksToolbarMode')
 
-// Retrofit the new single setting; we don't want to erase values set by the user.
 const passwordManagerDefault = (settingKey, settingsCollection) => {
   const onePasswordEnabled = resolveValue(settings.ONE_PASSWORD_ENABLED, settingsCollection) === true
-  if (onePasswordEnabled) { return passwordManagers.ONE_PASSWORD }
+  if (onePasswordEnabled) return passwordManagers.ONE_PASSWORD
 
   const dashlaneEnabled = resolveValue(settings.DASHLANE_ENABLED, settingsCollection) === true
-  if (dashlaneEnabled) { return passwordManagers.DASHLANE }
+  if (dashlaneEnabled) return passwordManagers.DASHLANE
 
   const lastPassEnabled = resolveValue(settings.LAST_PASS_ENABLED, settingsCollection) === true
-  if (lastPassEnabled) { return passwordManagers.LAST_PASS }
+  if (lastPassEnabled) return passwordManagers.LAST_PASS
 
   const disabled = resolveValue(settings.PASSWORD_MANAGER_ENABLED, settingsCollection) === false
-  if (disabled) { return passwordManagers.UNMANAGED }
+  if (disabled) return passwordManagers.UNMANAGED
 
   return defaultPasswordManager
 }
 
+const bookmarksBarDefault = (settingKey, settingsCollection) => {
+  const faviconsOnly = resolveValue(settings.SHOW_BOOKMARKS_TOOLBAR_ONLY_FAVICON, settingsCollection) === true
+  if (faviconsOnly) return bookmarksToolbarMode.FAVICONS_ONLY
+
+  const favicons = resolveValue(settings.SHOW_BOOKMARKS_TOOLBAR_FAVICON, settingsCollection) === true
+  if (favicons) return bookmarksToolbarMode.TEXT_AND_FAVICONS
+
+  return bookmarksToolbarMode.TEXT_ONLY
+}
+
+// Retrofit a new setting based on old values; we don't want to lose existing user settings.
+const getDefaultSetting = (settingKey, settingsCollection) => {
+  switch (settingKey) {
+    case settings.ACTIVE_PASSWORD_MANAGER:
+      return passwordManagerDefault(settingKey, settingsCollection)
+    case settings.BOOKMARKS_TOOLBAR_MODE:
+      return bookmarksBarDefault(settingKey, settingsCollection)
+  }
+  return undefined
+}
+
 const resolveValue = (settingKey, settingsCollection) => {
   const appSettings = (process.type === 'browser'
       ? require('./stores/appStore').getState().get('settings')
@@ -38,13 +59,9 @@ const resolveValue = (settingKey, settingsCollection) => {
 }
 
 module.exports.getSetting = (settingKey, settingsCollection) => {
-  if (settingKey === settings.ACTIVE_PASSWORD_MANAGER) {
-    const currentValue = resolveValue(settingKey, settingsCollection)
-    return !currentValue
-      ? passwordManagerDefault(settingKey, settingsCollection)
-      : currentValue
-  }
-  return resolveValue(settingKey, settingsCollection)
+  const setting = resolveValue(settingKey, settingsCollection)
+  if (setting) return setting
+  return getDefaultSetting(settingKey, settingsCollection)
 }
 
 module.exports.getActivePasswordManager = (settingsCollection) => {

less/about/preferences.less

@@ -169,6 +169,7 @@ body {
       width: 100%;
       background: transparent;
       padding: 10px 0 0 20px;
+      -webkit-user-select: none;
 
       &:before,
       &::before {
@@ -376,6 +377,10 @@ span.browserButton.primaryButton {
     margin-top: 20px;
     padding: 5px 20px;
   }
+
+  &.importNowButton {
+    margin-top: 5px;
+  }
 }
 
 .settingsList {
@@ -418,7 +423,6 @@ span.browserButton.primaryButton {
       height: 2.25em;
       font-size: 0.9em;
       padding: 0.4em;
-      margin: 10px 0;
       border: solid 1px @lightGray;
       background: white;
       color: @darkGray;
@@ -530,12 +534,14 @@ table.sortableTable {
   }
 
   .default {
-    width: 150px;
+    width: 95x;
   }
   .searchEngine {
     width: 304px !important;
+    padding-left: 10px;
   }
   .engineGoKey {
+    padding-left: 4px;
     width: 250px;
   }
 }
@@ -1019,6 +1025,7 @@ div.nextPaymentSubmission {
     .switchControlRightText, .switchControlLeftText {
       opacity: 0.3;
     }
+    cursor: default;
   }
 
   &:not(.disabled) {