Web Inspector: DOM Storage Editing is broken in many ways, frustrating
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Oct 2014 17:34:06 +0000 (17:34 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Oct 2014 17:34:06 +0000 (17:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=137547

Patch by Joseph Pecoraro <pecoraro@apple.com> on 2014-10-09
Reviewed by Timothy Hatcher.

* UserInterface/Models/DOMStorageObject.js:
(WebInspector.DOMStorageObject.prototype.getEntries): Deleted.
When getting entires, populate the model object with these keys/values
so we can accurately detect duplicates in editing.

* UserInterface/Views/DataGrid.js:
(WebInspector.DataGrid.prototype.determineNextCell):
(WebInspector.DataGrid.prototype.moveToNextCell):
When the users uses "Enter" to commit, stop editing.

(WebInspector.DataGrid.prototype._editingCommitted):
Fix title property accessor. This is not a map. This fixes an exception
when showing context menus on editable data grid rows.

* UserInterface/Views/DOMStorageContentView.js:
(WebInspector.DOMStorageContentView.prototype.cleanup):
(WebInspector.DOMStorageContentView.prototype.restoreOriginalValues):
(WebInspector.DOMStorageContentView.prototype._editingCallback):
Completely rewrite editing here. As soon as an edit is made, enter
an uncommitted state with the original key/value. When a commit is
allowed, update as appropriate based on the original values.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@174510 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Models/DOMStorageObject.js
Source/WebInspectorUI/UserInterface/Views/DOMStorageContentView.js
Source/WebInspectorUI/UserInterface/Views/DataGrid.js

index ddcef51..1054633 100644 (file)
@@ -1,3 +1,32 @@
+2014-10-09  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Web Inspector: DOM Storage Editing is broken in many ways, frustrating
+        https://bugs.webkit.org/show_bug.cgi?id=137547
+
+        Reviewed by Timothy Hatcher.
+
+        * UserInterface/Models/DOMStorageObject.js:
+        (WebInspector.DOMStorageObject.prototype.getEntries): Deleted.
+        When getting entires, populate the model object with these keys/values
+        so we can accurately detect duplicates in editing.
+
+        * UserInterface/Views/DataGrid.js:
+        (WebInspector.DataGrid.prototype.determineNextCell):
+        (WebInspector.DataGrid.prototype.moveToNextCell):
+        When the users uses "Enter" to commit, stop editing.
+
+        (WebInspector.DataGrid.prototype._editingCommitted):
+        Fix title property accessor. This is not a map. This fixes an exception
+        when showing context menus on editable data grid rows.
+
+        * UserInterface/Views/DOMStorageContentView.js:
+        (WebInspector.DOMStorageContentView.prototype.cleanup):
+        (WebInspector.DOMStorageContentView.prototype.restoreOriginalValues):
+        (WebInspector.DOMStorageContentView.prototype._editingCallback):
+        Completely rewrite editing here. As soon as an edit is made, enter
+        an uncommitted state with the original key/value. When a commit is
+        allowed, update as appropriate based on the original values.
+
 2014-10-08  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: CSS Pretty Printing fails to put space between value functions around ending parenthesis
index 82829a7..b3f8a40 100644 (file)
@@ -74,11 +74,25 @@ WebInspector.DOMStorageObject.prototype = {
 
     getEntries: function(callback)
     {
+        function innerCallback(error, entries)
+        {
+            if (error)
+                return;
+
+            for (var entry of entries) {
+                if (!entry[0] || !entry[1])
+                    continue;
+                this._entries.set(entry[0], entry[1]);
+            }
+
+            callback(error, entries);
+        }
+
         // COMPATIBILITY (iOS 6): The getDOMStorageItems function was later renamed to getDOMStorageItems.
         if (DOMStorageAgent.getDOMStorageEntries)
-            DOMStorageAgent.getDOMStorageEntries(this._id, callback);
+            DOMStorageAgent.getDOMStorageEntries(this._id, innerCallback.bind(this));
         else
-            DOMStorageAgent.getDOMStorageItems(this._id, callback);
+            DOMStorageAgent.getDOMStorageItems(this._id, innerCallback.bind(this));
     },
 
     removeItem: function(key)
index 2b136a7..7c7ce11 100644 (file)
@@ -185,58 +185,95 @@ WebInspector.DOMStorageContentView.prototype = {
 
     _editingCallback: function(editingNode, columnIdentifier, oldText, newText, moveDirection)
     {
-        var key = editingNode.data["key"].trim();
-        var value = editingNode.data["value"].trim();
-        var previousValue = oldText.trim();
-        var enteredValue = newText.trim();
-        var columnIndex = this._dataGrid.orderedColumns.indexOf(columnIdentifier);
-        var mayMoveToNextRow = moveDirection === "forward" && columnIndex === this._dataGrid.orderedColumns.length - 1;
-        var mayMoveToPreviousRow = moveDirection === "backward" && columnIndex === 0;
-        var willMoveRow = mayMoveToNextRow || mayMoveToPreviousRow;
-        var shouldCommitRow = willMoveRow && key.length && value.length;
+        var key = editingNode.data["key"].trim().removeWordBreakCharacters();
+        var value = editingNode.data["value"].trim().removeWordBreakCharacters();
+        var previousValue = oldText.trim().removeWordBreakCharacters();
+        var enteredValue = newText.trim().removeWordBreakCharacters();
+        var hasUncommittedEdits = editingNode.__hasUncommittedEdits;
+        var hasChange = previousValue !== enteredValue;
+        var isEditingKey = columnIdentifier === "key";
+        var isEditingValue = !isEditingKey;
+        var domStorage = this.representedObject;
 
-        // Remove the row if its values are newly cleared, and it's not a placeholder.
-        if (!key.length && !value.length && willMoveRow) {
-            if (previousValue.length && !editingNode.isPlaceholderNode)
-                this._dataGrid.removeChild(editingNode);
+        // Nothing changed, just bail.
+        if (!hasChange && !hasUncommittedEdits)
             return;
+
+        // Something changed, save the original key/value and enter uncommitted state.
+        if (hasChange && !editingNode.__hasUncommittedEdits) {
+            editingNode.__hasUncommittedEdits = true;
+            editingNode.__originalKey = isEditingKey ? previousValue : key;
+            editingNode.__originalValue = isEditingValue ? previousValue : value;
         }
 
-        // If the key field was deleted, restore it when committing the row.
-        if (key === enteredValue && !key.length) {
-            if (willMoveRow && !editingNode.isPlaceholderNode) {
-                editingNode.data.key = previousValue;
-                editingNode.refresh();
-            } else
-                editingNode.element.classList.add(WebInspector.DOMStorageContentView.MissingKeyStyleClassName);
-        } else if (key.length) {
+        function cleanup()
+        {
             editingNode.element.classList.remove(WebInspector.DOMStorageContentView.MissingKeyStyleClassName);
-            editingNode.__previousKeyValue = previousValue;
+            editingNode.element.classList.remove(WebInspector.DOMStorageContentView.MissingValueStyleClassName);
+            editingNode.element.classList.remove(WebInspector.DOMStorageContentView.DuplicateKeyStyleClassName);
+            delete editingNode.__hasUncommittedEdits;
+            delete editingNode.__originalKey;
+            delete editingNode.__originalValue;
         }
 
-        if (value === enteredValue && !value.length)
-            editingNode.element.classList.add(WebInspector.DOMStorageContentView.MissingValueStyleClassName);
-        else
-            editingNode.element.classList.remove(WebInspector.DOMStorageContentView.MissingValueStyleClassName);
+        function restoreOriginalValues()
+        {
+            editingNode.data.key = editingNode.__originalKey;
+            editingNode.data.value = editingNode.__originalValue;
+            editingNode.refresh();
+            cleanup();
+        }
 
-        if (editingNode.isPlaceholderNode && previousValue !== enteredValue)
-            this._dataGrid.addPlaceholderNode();
+        // If the key/value field was cleared, add "missing" style.
+        if (isEditingKey) {
+            if (key.length)
+                editingNode.element.classList.remove(WebInspector.DOMStorageContentView.MissingKeyStyleClassName);
+            else
+                editingNode.element.classList.add(WebInspector.DOMStorageContentView.MissingKeyStyleClassName);
+        } else if (isEditingValue) {
+            if (value.length)
+                editingNode.element.classList.remove(WebInspector.DOMStorageContentView.MissingValueStyleClassName);
+            else
+                editingNode.element.classList.add(WebInspector.DOMStorageContentView.MissingValueStyleClassName);
+        }
 
-        if (!shouldCommitRow)
-            return; // One of the inputs is missing, or we aren't moving between rows.
+        // Check for key duplicates. If this is a new row, or an existing row that changed key.
+        var keyChanged = key !== editingNode.__originalKey;
+        if (keyChanged) {
+            if (domStorage.entries.has(key))
+                editingNode.element.classList.add(WebInspector.DOMStorageContentView.DuplicateKeyStyleClassName);
+            else
+                editingNode.element.classList.remove(WebInspector.DOMStorageContentView.DuplicateKeyStyleClassName);
+        }
 
-        var domStorage = this.representedObject;
-        if (domStorage.entries.has(key)) {
-            editingNode.element.classList.add(WebInspector.DOMStorageContentView.DuplicateKeyStyleClassName);
+        // See if we are done editing this row or not.
+        var columnIndex = this._dataGrid.orderedColumns.indexOf(columnIdentifier);
+        var mayMoveToNextRow = moveDirection === "forward" && columnIndex === this._dataGrid.orderedColumns.length - 1;
+        var mayMoveToPreviousRow = moveDirection === "backward" && columnIndex === 0;
+        var doneEditing = mayMoveToNextRow || mayMoveToPreviousRow || !moveDirection;
+
+        // Expecting more edits on this row.
+        if (!doneEditing)
             return;
-        }
 
-        editingNode.element.classList.remove(WebInspector.DOMStorageContentView.DuplicateKeySyleClassName);
+        // Key and value were cleared, remove the row.
+        if (!key.length && !value.length && !editingNode.isPlaceholderNode) {
+            this._dataGrid.removeChild(editingNode);
+            domStorage.removeItem(editingNode.__originalKey);
+            return;                
+        }
 
-        if (editingNode.__previousKeyValue !== key)
-            domStorage.removeItem(editingNode.__previousKeyValue);
+        // Done editing but leaving the row in an invalid state. Leave in uncommitted state.
+        var isDuplicate = editingNode.element.classList.contains(WebInspector.DOMStorageContentView.DuplicateKeyStyleClassName);
+        if (!key.length || !value.length || isDuplicate)
+            return;
 
+        // Commit.
+        if (keyChanged && !editingNode.isPlaceholderNode)
+            domStorage.removeItem(editingNode.__originalKey);
+        if (editingNode.isPlaceholderNode)
+            this._dataGrid.addPlaceholderNode();
+        cleanup();
         domStorage.setItem(key, value);
-        // The table will be re-sorted when the backend fires the itemUpdated event.
     }
 };
index af204e5..9e324ed 100644 (file)
@@ -316,8 +316,8 @@ WebInspector.DataGrid.prototype = {
                 return {shouldSort: true, editingNode: previousDataGridNode || currentEditingNode, columnIndex: this.orderedColumns.length - 1};
             }
 
-            // If we are not moving in any direction, then sort but don't move.
-            return {shouldSort: true, editingNode: currentEditingNode, columnIndex: columnIndex};
+            // If we are not moving in any direction, then sort and stop.
+            return {shouldSort: true};
         }
 
         function moveToNextCell(valueDidChange) {
@@ -326,7 +326,8 @@ WebInspector.DataGrid.prototype = {
                 this._sortAfterEditingCallback();
                 delete this._sortAfterEditingCallback;
             }
-            this._startEditingNodeAtColumnIndex(moveCommand.editingNode, moveCommand.columnIndex);
+            if (moveCommand.editingNode)
+                this._startEditingNodeAtColumnIndex(moveCommand.editingNode, moveCommand.columnIndex);
         }
 
         this._editingCancelled(element);
@@ -1145,7 +1146,7 @@ WebInspector.DataGrid.prototype = {
                 else {
                     var element = event.target.enclosingNodeOrSelfWithNodeName("td");
                     var columnIdentifier = element.__columnIdentifier;
-                    var columnTitle = this.dataGrid.columns.get(columnIdentifier).get("title");
+                    var columnTitle = this.dataGrid.columns.get(columnIdentifier)["title"];
                     contextMenu.appendItem(WebInspector.UIString("Edit ā€œ%sā€").format(columnTitle), this._startEditing.bind(this, event.target));
                 }
             }