Currently, cell DOM nodes of a DataGrid store the value of a 'columnIdentifier' in the 'class' attribute. This requires later parsing of attribute 'class' to know the value of 'columnIdentifier'. HTML5 provides a proper way to store custom attributes to a DOM node, which is by using the 'dataset' attribute. https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement.dataset Storing 'columnIdentifier' as a dataset attribute won't require later parsing.
<rdar://problem/15908787>
Created attachment 222197 [details] Patch
Comment on attachment 222197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222197&action=review r- because I think it breaks styles. But make it so you set classname and dataset attribute, and it is good! > Source/WebInspectorUI/UserInterface/DataGrid.js:88 > - cell.className = columnIdentifier + "-column"; > + cell.dataset["columnIdentifier"] = columnIdentifier; I think we will need to set the class name for styles. There are a lot of CSS instances of "<foo>-column": shell> ack "\-column" *.css DOMTreeDataGrid.css 62:.dom-tree-data-grid .data-container .name-column { 66:.dom-tree-data-grid .data-container .name-column .icon { 74:.dom-tree-data-grid .data-container .name-column .label { 83:.dom-tree-data-grid .data-container tr:hover .name-column .label { DetailsSection.css 251:.details-section > .content .data-grid td.value-column { 257:.details-section > .content .data-grid td.value-column > div { ... > Source/WebInspectorUI/UserInterface/DataGrid.js:155 > - td.className = columnIdentifier + "-column"; > + td.dataset["columnIdentifier"] = columnIdentifier; Same here. > Source/WebInspectorUI/UserInterface/DataGrid.js:346 > - // FIXME: Better way to do this than regular expressions? > - var columnIdentifier = parseInt(element.className.match(/\b(\d+)-column\b/)[1], 10); > + var columnIdentifier = element.dataset["columnIdentifier"]; This looks great though!
Comment on attachment 222197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222197&action=review I agree with Joe. We need the className for style reasons. I am also not a fan of dataset. There is no reason to use a DOM attribute for internal logic. I would just set cell.__columnIdentifier and use it later. > Source/WebInspectorUI/UserInterface/DataGrid.js:903 > - // The first child of the selected element is the <td class="0-column">, > + // The first child of the selected element is the <td data-columnIdentifier="0">, The original comment is not correct, the column identifier is not usually a number, it is a arbitrary string. Just remove this comment.
Lets not use dataset. Using the DOM for this is expensive. A private property on the DOM element is good.
Created attachment 222234 [details] Patch
Comment on attachment 222234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222234&action=review r=me > Source/WebInspectorUI/UserInterface/DataGrid.js:156 > + td.columnIdentifier = columnIdentifier; As Tim suggested, we should use "__columnIdentifier". We typically use "__foo" syntax for property names when one class is storing properties on another object, and that other object has no knowledge of the existence of that property. Though ".columnIdentifier" works, it is not immediately clear if you're debugging this element that the property is something we injected onto the element. And having it show up as obvious in the debugger can sometimes be helpful! Also, if in the further that object wanted to add a "columnIdentifier" property then the double underscore would avoid a naming conflict.
Created attachment 222378 [details] Patch
Comment on attachment 222378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222378&action=review > Source/WebInspectorUI/UserInterface/DataGrid.js:156 > + td.__columnIdentifier = parseInt(columnIdentifier); Whoa, there is a parseInt now? Can't columnIdentifier be a string like "name"?
Comment on attachment 222378 [details] Patch Yeah, parseInt is not correct. The old code had it in a different spot and it was wrong.
Comment on attachment 222378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222378&action=review >> Source/WebInspectorUI/UserInterface/DataGrid.js:156 >> + td.__columnIdentifier = parseInt(columnIdentifier); > > Whoa, there is a parseInt now? Can't columnIdentifier be a string like "name"? I noticed that __columnIdentifier has to be "numeric". If __columnIdentifier is a "string", there's a regression. When I try the following: * double-click on a "Key" column * Insert a value, for instance "Key1" * Press return The result is stored in the key column as "Key1" but it's also stored in the value column as "Key1". This is not correct. In the original code, parseInt was used in "_editingCommited" and "_contextMenuInDataTable" for casting the value of columnIdentifier after retrieving that value from a regular expression. var columnIdentifier = parseInt(element.className.match(/\b(\d+)-column\b/)[1], 10); Since there is going to be a field for storing columnIdentifier I thought it was better to store that field as numeric, instead of casting it everytime "_editingCommited" and "_contextMenuInDataTable" were called. Still, why columnIdentifier has to be numeric? If __columnIdentifier is a string, function "_contextMenuInDataTable" works well. The problem is in function "_editingCommited". _editingCommited calls this._editCallback. _editCallback (DOMStorageContentView.js:215) does a strict comparison of the value of columnIdentifier. _editingCallback: function(editingNode, columnIdentifier, oldText, newText) { var domStorage = this.representedObject; if (columnIdentifier === 0) { if (oldText) domStorage.removeItem(oldText); domStorage.setItem(newText, editingNode.data[1]); } else domStorage.setItem(editingNode.data[0], newText); this.update(); } If columnIdentifier is a "string" the else branch is always executed (it updates the value of the value column). That's why in the use-case explained above, when updating a key the key column gets updated (visually) and the value column gets updated too.
Comment on attachment 222378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222378&action=review >>> Source/WebInspectorUI/UserInterface/DataGrid.js:156 >>> + td.__columnIdentifier = parseInt(columnIdentifier); >> >> Whoa, there is a parseInt now? Can't columnIdentifier be a string like "name"? > > I noticed that __columnIdentifier has to be "numeric". If __columnIdentifier is a "string", there's a regression. When I try the following: > > * double-click on a "Key" column > * Insert a value, for instance "Key1" > * Press return > > The result is stored in the key column as "Key1" but it's also stored in the value column as "Key1". This is not correct. > > In the original code, parseInt was used in "_editingCommited" and "_contextMenuInDataTable" for casting the value of columnIdentifier after retrieving that value from a regular expression. > > var columnIdentifier = parseInt(element.className.match(/\b(\d+)-column\b/)[1], 10); > > Since there is going to be a field for storing columnIdentifier I thought it was better to store that field as numeric, instead of casting it everytime "_editingCommited" and "_contextMenuInDataTable" were called. > > Still, why columnIdentifier has to be numeric? If __columnIdentifier is a string, function "_contextMenuInDataTable" works well. The problem is in function "_editingCommited". _editingCommited calls this._editCallback. _editCallback (DOMStorageContentView.js:215) does a strict comparison of the value of columnIdentifier. > > _editingCallback: function(editingNode, columnIdentifier, oldText, newText) > { > var domStorage = this.representedObject; > if (columnIdentifier === 0) { > if (oldText) > domStorage.removeItem(oldText); > > domStorage.setItem(newText, editingNode.data[1]); > } else > domStorage.setItem(editingNode.data[0], newText); > > this.update(); > } > > If columnIdentifier is a "string" the else branch is always executed (it updates the value of the value column). That's why in the use-case explained above, when updating a key the key column gets updated (visually) and the value column gets updated too. Good catch! But DOMStorageContentView.js is the one that should change. Making the 0s and 1s be "0" and "1" would be fine to fix in this patch. Ideally, DOMStorageContentView.js would use identifiers like "key" and "value" instead of numbers.
Created attachment 222430 [details] Patch
Comment on attachment 222430 [details] Patch Clearing flags on attachment: 222430 Committed r162931: <http://trac.webkit.org/changeset/162931>
All reviewed patches have been landed. Closing bug.