[tabbed options] more work on content settings exceptions lists

- tie "edit" mode to "selected"
-- selecting enters into edit mode
-- blurring exits edit mode and deselects
- fix up layout within rows (faux columns)
- add a special "add new exception" row (this should be at the bottom of the table, but it's currently at the top)
- change behavior when the user attempts to commit an invalid exception. Instead of trying to regrab focus, just revert the row to its previous state.

TODO: list should be sized dynamically, i.e. no scrollbar.
TODO: list should be in sub-sub dialog
TODO: rows should have delete X

BUG=64153
TEST=manual

Review URL: http://codereview.chromium.org/5699004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69332 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 220 insertions, 131 deletions

diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
index 110f36b..e6237af 100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -5329,9 +5329,12 @@ Keep your key file in a safe place. You will need it to create new versions of y
       <message name="IDS_EXCEPTIONS_OTR_LABEL" desc="A label informing the user that the table below the label shows incognito-only exceptions">
         Exceptions below only apply to the current incognito session.
       </message>
-      <message translateable="false" name="IDS_EXCEPTIONS_PATTERN_EXAMPLE" desc="Placeholder text when the user adds a new exception" >
+      <message translateable="false" name="IDS_EXCEPTIONS_PATTERN_EXAMPLE" desc="Example pattern when the user is adding a new exception" >
         [*.]example.com
       </message>
+      <message name="IDS_EXCEPTIONS_ADD_NEW_INSTRUCTIONS" desc="Placeholder text before the user adds a new exception" >
+        Add a new exception pattern
+      </message>
       <message name="IDS_EXCEPTIONS_NOT_SET_BUTTON" desc="A label to display in the exception page's action column when a site's content setting has not yet been set for a given domain.">
         Not set
       </message>
diff --git a/chrome/browser/dom_ui/options/content_settings_handler.cc b/chrome/browser/dom_ui/options/content_settings_handler.cc
index cc5a6b1..3e5f8d3 100644
--- a/chrome/browser/dom_ui/options/content_settings_handler.cc
+++ b/chrome/browser/dom_ui/options/content_settings_handler.cc
@@ -198,6 +198,8 @@ void ContentSettingsHandler::GetLocalizedValues(
       l10n_util::GetStringUTF16(IDS_EXCEPTIONS_OTR_LABEL));
   localized_strings->SetString("examplePattern",
       l10n_util::GetStringUTF16(IDS_EXCEPTIONS_PATTERN_EXAMPLE));
+  localized_strings->SetString("addNewExceptionInstructions",
+      l10n_util::GetStringUTF16(IDS_EXCEPTIONS_ADD_NEW_INSTRUCTIONS));
 
   // Cookies filter.
   localized_strings->SetString("cookies_tab_label",
diff --git a/chrome/browser/resources/options/content_settings.js b/chrome/browser/resources/options/content_settings.js
index 96c2279..efc85e8 100644
--- a/chrome/browser/resources/options/content_settings.js
+++ b/chrome/browser/resources/options/content_settings.js
@@ -94,7 +94,7 @@ cr.define('options', function() {
         document.querySelector('div[contentType=' + type + ']' +
                                ' list[mode=normal]');
 
-    exceptionsList.clear();
+    exceptionsList.reset();
     for (var i = 0; i < list.length; i++) {
       exceptionsList.addException(list[i]);
     }
@@ -108,7 +108,7 @@ cr.define('options', function() {
 
     exceptionsList.parentNode.classList.remove('hidden');
 
-    exceptionsList.clear();
+    exceptionsList.reset();
     for (var i = 0; i < list.length; i++) {
       exceptionsList.addException(list[i]);
     }
@@ -129,7 +129,7 @@ cr.define('options', function() {
     var otrLists = document.querySelectorAll('list[mode=otr]');
 
     for (var i = 0; i < otrLists.length; i++) {
-      otrLists[i].clear();
+      otrLists[i].reset();
       otrLists[i].parentNode.classList.add('hidden');
     }
   };
@@ -154,8 +154,8 @@ cr.define('options', function() {
   ContentSettings.patternValidityCheckComplete =
       function(type, mode, pattern, valid) {
     var exceptionsList =
-        document.querySelector('div[contentType=' + type + '][mode=' + mode +
-                               '] list');
+        document.querySelector('div[contentType=' + type + '] ' +
+                               'list[mode=' + mode + ']');
     exceptionsList.patternValidityCheckComplete(pattern, valid);
   };
 
diff --git a/chrome/browser/resources/options/content_settings_exceptions_area.css b/chrome/browser/resources/options/content_settings_exceptions_area.css
index ee8ca82..63fc87b 100644
--- a/chrome/browser/resources/options/content_settings_exceptions_area.css
+++ b/chrome/browser/resources/options/content_settings_exceptions_area.css
@@ -4,11 +4,13 @@ Use of this source code is governed by a BSD-style license that can be
 found in the LICENSE file.
 */
 
-.exceptionInput {
-  /* TODO(estade): need something here to make it fill the remaining space. */
+.exceptionPattern {
+  display: -webkit-box;
+  -webkit-box-flex: 1;
 }
 
 .exceptionSetting {
-  float: right;
-  margin-right: 30px;
+  display: inline-block;
+  width: 100px;
+  margin-right: 20px;
 }
diff --git a/chrome/browser/resources/options/content_settings_exceptions_area.js b/chrome/browser/resources/options/content_settings_exceptions_area.js
index f727476..7077559 100644
--- a/chrome/browser/resources/options/content_settings_exceptions_area.js
+++ b/chrome/browser/resources/options/content_settings_exceptions_area.js
@@ -39,21 +39,27 @@ cr.define('options.contentSettings', function() {
     decorate: function() {
       ListItem.prototype.decorate.call(this);
 
-      // Labels for display mode.
-      var patternLabel = cr.doc.createElement('span');
-      patternLabel.textContent = this.pattern;
-      this.appendChild(patternLabel);
-
-      var settingLabel = cr.doc.createElement('span');
-      settingLabel.textContent = this.settingForDisplay();
-      settingLabel.className = 'exceptionSetting';
-      this.appendChild(settingLabel);
+      // Labels for display mode. |pattern| will be null for the 'add new
+      // exception' row.
+      if (this.pattern) {
+        var patternLabel = cr.doc.createElement('span');
+        patternLabel.textContent = this.pattern;
+        patternLabel.className = 'exceptionPattern';
+        this.appendChild(patternLabel);
+        this.patternLabel = patternLabel;
+
+        var settingLabel = cr.doc.createElement('span');
+        settingLabel.textContent = this.settingForDisplay();
+        settingLabel.className = 'exceptionSetting';
+        this.appendChild(settingLabel);
+        this.settingLabel = settingLabel;
+      }
 
       // Elements for edit mode.
       var input = cr.doc.createElement('input');
       input.type = 'text';
       this.appendChild(input);
-      input.className = 'exceptionInput hidden';
+      input.className = 'exceptionPattern hidden';
 
       var select = cr.doc.createElement('select');
       var optionAllow = cr.doc.createElement('option');
@@ -93,26 +99,23 @@ cr.define('options.contentSettings', function() {
       // empty input.
       this.inputIsValid = true;
 
-      this.patternLabel = patternLabel;
-      this.settingLabel = settingLabel;
       this.input = input;
       this.select = select;
       this.optionAllow = optionAllow;
       this.optionBlock = optionBlock;
 
       this.updateEditables();
-      if (!this.pattern)
-        input.value = templateData.examplePattern;
 
       var listItem = this;
-      this.ondblclick = function(event) {
+
+      this.addEventListener('selectedChange', function(event) {
         // Editing notifications and geolocation is disabled for now.
         if (listItem.contentType == 'notifications' ||
             listItem.contentType == 'location')
           return;
 
-        listItem.editing = true;
-      };
+        listItem.editing = listItem.selected;
+      });
 
       // Handle events on the editable nodes.
       input.oninput = function(event) {
@@ -132,32 +135,14 @@ cr.define('options.contentSettings', function() {
           case 'U+001B':  // Esc
             // Reset the inputs.
             listItem.updateEditables();
-            if (listItem.pattern)
-              listItem.maybeSetPatternValid(listItem.pattern, true);
+            listItem.setPatternValid(true);
           case 'Enter':
-            if (listItem.parentNode)
-              listItem.parentNode.focus();
+            listItem.ownerDocument.activeElement.blur();
         }
       }
 
-      function handleBlur(e) {
-        // When the blur event happens we do not know who is getting focus so we
-        // delay this a bit since we want to know if the other input got focus
-        // before deciding if we should exit edit mode.
-        var doc = e.target.ownerDocument;
-        window.setTimeout(function() {
-          var activeElement = doc.activeElement;
-          if (!listItem.contains(activeElement)) {
-            listItem.editing = false;
-          }
-        }, 50);
-      }
-
       input.addEventListener('keydown', handleKeydown);
-      input.addEventListener('blur', handleBlur);
-
       select.addEventListener('keydown', handleKeydown);
-      select.addEventListener('blur', handleBlur);
     },
 
     /**
@@ -199,20 +184,12 @@ cr.define('options.contentSettings', function() {
     },
 
     /**
-     * Update this list item to reflect whether the input is a valid pattern
-     * if |pattern| matches the text currently in the input.
-     * @param {string} pattern The pattern.
+     * Update this list item to reflect whether the input is a valid pattern.
      * @param {boolean} valid Whether said pattern is valid in the context of
      *     a content exception setting.
      */
-    maybeSetPatternValid: function(pattern, valid) {
-      // Don't do anything for messages where we are not the intended recipient,
-      // or if the response is stale (i.e. the input value has changed since we
-      // sent the request to analyze it).
-      if (pattern != this.input.value)
-        return;
-
-      if (valid)
+    setPatternValid: function(valid) {
+      if (valid || !this.input.value)
         this.input.setCustomValidity('');
       else
         this.input.setCustomValidity(' ');
@@ -221,10 +198,18 @@ cr.define('options.contentSettings', function() {
     },
 
     /**
+     * Set the <input> to its original contents. Used when the user quits
+     * editing.
+     */
+    resetInput: function() {
+      this.input.value = this.pattern;
+    },
+
+    /**
      * Copy the data model values to the editable nodes.
      */
     updateEditables: function() {
-      this.input.value = this.pattern;
+      this.resetInput();
 
       if (this.setting == 'allow')
         this.optionAllow.selected = true;
@@ -237,6 +222,17 @@ cr.define('options.contentSettings', function() {
     },
 
     /**
+     * Fiddle with the display of elements of this list item when the editing
+     * mode changes.
+     */
+    toggleVisibilityForEditing: function() {
+      this.patternLabel.classList.toggle('hidden');
+      this.settingLabel.classList.toggle('hidden');
+      this.input.classList.toggle('hidden');
+      this.select.classList.toggle('hidden');
+    },
+
+    /**
      * Whether the user is currently able to edit the list item.
      * @type {boolean}
      */
@@ -248,54 +244,41 @@ cr.define('options.contentSettings', function() {
       if (oldEditing == editing)
         return;
 
-      var listItem = this;
-      var pattern = this.pattern;
-      var setting = this.setting;
-      var patternLabel = this.patternLabel;
-      var settingLabel = this.settingLabel;
       var input = this.input;
-      var select = this.select;
-      var optionAllow = this.optionAllow;
-      var optionBlock = this.optionBlock;
-      var optionSession = this.optionSession;
-      var optionAsk = this.optionAsk;
-
-      // Just delete this row if it was added via the Add button.
-      if (!editing && !pattern && !input.value) {
-        var model = listItem.parentNode.dataModel;
-        model.splice(model.indexOf(listItem.dataItem), 1);
-        return;
-      }
 
-      // Check that we have a valid pattern and if not we do not change the
-      // editing mode.
-      if (!editing && (!this.inputValidityKnown || !this.inputIsValid)) {
-        input.focus();
-        input.select();
-        return;
-      }
-
-      patternLabel.classList.toggle('hidden');
-      settingLabel.classList.toggle('hidden');
-      input.classList.toggle('hidden');
-      select.classList.toggle('hidden');
-
-      var doc = this.ownerDocument;
-      var area = doc.querySelector('div[contentType=' +
-          listItem.contentType + '][mode=' + listItem.mode + ']');
-      area.enableAddAndEditButtons(!editing);
+      this.toggleVisibilityForEditing();
 
       if (editing) {
         this.setAttribute('editing', '');
         cr.ui.limitInputWidth(input, this, 20);
-        input.focus();
-        input.select();
+        // When this is called in response to the selectedChange event,
+        // the list grabs focus immediately afterwards. Thus we must delay
+        // our focus grab.
+        window.setTimeout(function() {
+          input.focus();
+          input.select();
+        }, 50);
+
+        // TODO(estade): should we insert example text here for the AddNewRow
+        // input?
       } else {
         this.removeAttribute('editing');
 
+        // Check that we have a valid pattern and if not we do not, abort
+        // changes to the exception.
+        if (!this.inputValidityKnown || !this.inputIsValid) {
+          this.updateEditables();
+          this.setPatternValid(true);
+          return;
+        }
+
         var newPattern = input.value;
 
         var newSetting;
+        var optionAllow = this.optionAllow;
+        var optionBlock = this.optionBlock;
+        var optionSession = this.optionSession;
+        var optionAsk = this.optionAsk;
         if (optionAllow.selected)
           newSetting = 'allow';
         else if (optionBlock.selected)
@@ -305,26 +288,103 @@ cr.define('options.contentSettings', function() {
         else if (optionAsk && optionAsk.selected)
           newSetting = 'ask';
 
-        // Empty edit - do nothing.
-        if (pattern == newPattern && newSetting == this.setting)
-          return;
+        this.finishEdit(newPattern, newSetting);
+      }
+    },
 
-        this.pattern = patternLabel.textContent = newPattern;
-        this.setting = newSetting;
-        settingLabel.textContent = this.settingForDisplay();
+    /**
+     * Editing is complete; update the model.
+     * @type {string} newPattern The pattern that the user entered.
+     * @type {string} newSetting The setting the user chose.
+     */
+    finishEdit: function(newPattern, newSetting) {
+      // Empty edit - do nothing.
+      if (newPattern == this.pattern && newSetting == this.setting)
+        return;
 
-        if (pattern != this.pattern) {
-          chrome.send('removeExceptions',
-                      [this.contentType, this.mode, pattern]);
-        }
+      this.patternLabel.textContent = newPattern;
+      this.settingLabel.textContent = this.settingForDisplay();
+      var oldPattern = this.pattern;
+      this.pattern = newPattern;
+      this.setting = newSetting;
 
-        chrome.send('setException',
-                    [this.contentType, this.mode, this.pattern, this.setting]);
+      if (oldPattern != newPattern) {
+        chrome.send('removeExceptions',
+                    [this.contentType, this.mode, oldPattern]);
       }
+
+      chrome.send('setException',
+                  [this.contentType, this.mode, newPattern, newSetting]);
     }
   };
 
   /**
+   * Creates a new list item for the Add New Item row, which doesn't represent
+   * an actual entry in the exceptions list but allows the user to add new
+   * exceptions.
+   * @param {string} contentType The type of the list.
+   * @param {string} mode The browser mode, 'otr' or 'normal'.
+   * @param {boolean} enableAskOption Whether to show an 'ask every time'
+   *     option in the select.
+   * @constructor
+   * @extends {cr.ui.ExceptionsListItem}
+   */
+  function ExceptionsAddRowListItem(contentType, mode, enableAskOption) {
+    var el = cr.doc.createElement('li');
+    el.mode = mode;
+    el.contentType = contentType;
+    el.enableAskOption = enableAskOption;
+    el.dataItem = [];
+    el.__proto__ = ExceptionsAddRowListItem.prototype;
+    el.decorate();
+
+    return el;
+  }
+
+  ExceptionsAddRowListItem.prototype = {
+    __proto__: ExceptionsListItem.prototype,
+
+    decorate: function() {
+      ExceptionsListItem.prototype.decorate.call(this);
+
+      this.input.placeholder = templateData.addNewExceptionInstructions;
+      this.input.classList.remove('hidden');
+      this.select.classList.remove('hidden');
+
+      // Do we always want a default of allow?
+      this.setting = 'allow';
+    },
+
+    /**
+     * Clear the <input> and let the placeholder text show again.
+     */
+    resetInput: function() {
+      this.input.value = '';
+    },
+
+    /**
+     * No elements show or hide when going into edit mode, so do nothing.
+     */
+    toggleVisibilityForEditing: function() {
+      // No-op.
+    },
+
+    /**
+     * Editing is complete; update the model. As long as the pattern isn't
+     * empty, we'll just add it.
+     * @type {string} newPattern The pattern that the user entered.
+     * @type {string} newSetting The setting the user chose.
+     */
+    finishEdit: function(newPattern, newSetting) {
+      if (newPattern == '')
+        return;
+
+      chrome.send('setException',
+                  [this.contentType, this.mode, newPattern, newSetting]);
+    },
+  };
+
+  /**
    * Creates a new exceptions list.
    * @constructor
    * @extends {cr.ui.List}
@@ -340,11 +400,29 @@ cr.define('options.contentSettings', function() {
     decorate: function() {
       List.prototype.decorate.call(this);
 
-      this.dataModel = new ArrayDataModel([]);
+      this.contentType = this.parentNode.getAttribute('contentType');
+      this.mode = this.getAttribute('mode');
+
+      var exceptionList = this;
+      function handleBlur(e) {
+        // When the blur event happens we do not know who is getting focus so we
+        // delay this a bit until we know if the new focus node is outside the
+        // list.
+        var doc = e.target.ownerDocument;
+        window.setTimeout(function() {
+          var activeElement = doc.activeElement;
+          if (!exceptionList.contains(activeElement))
+            exceptionList.selectionModel.clear();
+        }, 50);
+      }
+
+      this.addEventListener('blur', handleBlur, true);
 
       // Whether the exceptions in this list allow an 'Ask every time' option.
       this.enableAskOption = (this.contentType == 'plugins' &&
                               templateData.enable_click_to_play);
+
+      this.reset();
     },
 
     /**
@@ -352,10 +430,16 @@ cr.define('options.contentSettings', function() {
      * @param {Object} entry The element from the data model for this row.
      */
     createItem: function(entry) {
-      return new ExceptionsListItem(this.contentType,
-                                    this.mode,
-                                    this.enableAskOption,
-                                    entry);
+      if (entry) {
+        return new ExceptionsListItem(this.contentType,
+                                      this.mode,
+                                      this.enableAskOption,
+                                      entry);
+      } else {
+        return new ExceptionsAddRowListItem(this.contentType,
+                                            this.mode,
+                                            this.enableAskOption);
+      }
     },
 
     /**
@@ -364,16 +448,6 @@ cr.define('options.contentSettings', function() {
      */
     addException: function(entry) {
       this.dataModel.push(entry);
-
-      // When an empty row is added, put it into editing mode.
-      if (!entry['displayPattern'] && !entry['setting']) {
-        var index = this.dataModel.length - 1;
-        var sm = this.selectionModel;
-        sm.anchorIndex = sm.leadIndex = sm.selectedIndex = index;
-        this.scrollIndexIntoView(index);
-        var li = this.getListItemByIndex(index);
-        li.editing = true;
-      }
     },
 
     /**
@@ -384,18 +458,23 @@ cr.define('options.contentSettings', function() {
      *     a content exception setting.
      */
     patternValidityCheckComplete: function(pattern, valid) {
-      for (var i = 0; i < this.dataModel.length; i++) {
-        var listItem = this.getListItemByIndex(i);
-        if (listItem)
-          listItem.maybeSetPatternValid(pattern, valid);
+      var listItems = this.items;
+      for (var i = 0; i < listItems.length; i++) {
+        var listItem = listItems[i];
+        // Don't do anything for messages for the item if it is not the intended
+        // recipient, or if the response is stale (i.e. the input value has
+        // changed since we sent the request to analyze it).
+        if (pattern == listItem.input.value)
+          listItem.setPatternValid(valid);
       }
     },
 
     /**
      * Removes all exceptions from the js model.
      */
-    clear: function() {
-      this.dataModel = new ArrayDataModel([]);
+    reset: function() {
+      // The null creates the Add New Exception row.
+      this.dataModel = new ArrayDataModel([null]);
     },
 
     /**
@@ -434,6 +513,7 @@ cr.define('options.contentSettings', function() {
 
   return {
     ExceptionsListItem: ExceptionsListItem,
+    ExceptionsAddRowListItem: ExceptionsAddRowListItem,
     ExceptionsList: ExceptionsList,
   };
 });
diff --git a/chrome/browser/resources/options/options_page.css b/chrome/browser/resources/options/options_page.css
index 491b543..e7aa459 100644
--- a/chrome/browser/resources/options/options_page.css
+++ b/chrome/browser/resources/options/options_page.css
@@ -322,7 +322,7 @@ section > div:only-of-type label {
 }
 
 .hidden {
-  display: none;
+  display: none !important;
 }
 
 .touch-slider {
diff --git a/chrome/browser/resources/shared/js/cr/ui/list.js b/chrome/browser/resources/shared/js/cr/ui/list.js
index 5db46a3..749d0ea 100644
--- a/chrome/browser/resources/shared/js/cr/ui/list.js
+++ b/chrome/browser/resources/shared/js/cr/ui/list.js
@@ -223,12 +223,14 @@ cr.define('cr.ui', function() {
     },
 
     /**
-     * The HTML elements representing the items. This is just all the element
+     * The HTML elements representing the items. This is just all the list item
      * children but subclasses may override this to filter out certain elements.
      * @type {HTMLCollection}
      */
     get items() {
-      return this.children;
+      return Array.prototype.filter.call(this.children, function(child) {
+        return !child.classList.contains('spacer');
+      });
     },
 
     batchCount_: 0,