Summary: Multiple selection lost after navigating to another tab. Steps to Reproduce: 1. Open Inspector > Elements tab 2. Select more than one DOM node 3. Select another tab 4. Return to Elements tab => Multiple selection is changed to single selection
<rdar://problem/46709392>
Created attachment 362538 [details] Patch
Comment on attachment 362538 [details] Patch Looks good to me. However, I'm not very familiar with intricacies of SelectionController. Even if I was a reviewer, I'd suggest somebody else to review as well. View in context: https://bugs.webkit.org/attachment.cgi?id=362538&action=review > Source/WebInspectorUI/UserInterface/Base/Utilities.js:195 > + let iterator = this.values(); > + let result = iterator.next().value; > + for (let value of iterator) I was going to suggest to convert it to an array before iterating to make it faster, but it doesn't really matter any longer. Iterators are pretty fast nowadays. https://jsperf.com/set-iterator-vs-foreach/4 > Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:106 > + selectItems(items, extendSelection = false) Is `extendSelection` currently always false? If so, I'd prefer to remove it until we use it.
Comment on attachment 362538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362538&action=review >> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:106 >> + selectItems(items, extendSelection = false) > > Is `extendSelection` currently always false? If so, I'd prefer to remove it until we use it. Initially I included this parameter in TreeOutline.prototype.selectTreeElements(), but then removed it to limit the scope of this patch since it wasn't used. Will remove.
Created attachment 362563 [details] Patch
Comment on attachment 362563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362563&action=review > Source/WebInspectorUI/ChangeLog:11 > + (value.defaultComparator): > + (value): These ChangeLog function entires are quite useless :| Either remove them or replace them with `Set.prototype.maxValue` and `Set.prototype.union` entries. > Source/WebInspectorUI/ChangeLog:27 > + * UserInterface/Test.html: > + New files for testing. NIT: I'd put this at the end of the ChangeLog, as it's not very related to the main functionality of this patch/fix. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:195 > + for (let value of iterator) Style: missing braces. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:207 > + let result = new Set(other); Why use `other` and not `this`? > Source/WebInspectorUI/UserInterface/Base/Utilities.js:212 > + Style: unnecessary newline. > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:192 > + let domNode = oldTreeElement.representedObject; NIT: you could inline this. > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:195 > + treeElement = treeElement.children.lastValue; This is a scary assumption to make. Why don't we pass the actual `WI.DOMTreeElement` as `elementCloseTag` instead of just a true/false? That way, we can retrieve it directly here (e.g. `treeElement.associatedCloseTagTreeElement`). > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:199 > + this.selectTreeElements(selectedTreeElements); Will this interfere with the selection set by `_revealAndSelectNode` above? Should we reverse the order of the two, and maybe filter out `selectedNode` from the list of `selectedTreeElements`? > LayoutTests/inspector/unit-tests/set-utilities.html:142 > + name: "Set.prototype.maxValue", We should also test `Set.prototype.union`.
Comment on attachment 362563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362563&action=review >> Source/WebInspectorUI/ChangeLog:11 >> + (value): > > These ChangeLog function entires are quite useless :| > Either remove them or replace them with `Set.prototype.maxValue` and `Set.prototype.union` entries. I agree, but am not interested in playing games with our changelog generator, >> Source/WebInspectorUI/ChangeLog:27 >> + New files for testing. > > NIT: I'd put this at the end of the ChangeLog, as it's not very related to the main functionality of this patch/fix. Reordering change log entries isn't a good use of time. Inevitably people will disagree over what the most appropriate order is, and pretty soon we'll be peppering these entries with nits requesting they be reordered. I really don't want to go there. I'll just drop the comment. >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:195 >> + for (let value of iterator) > > Style: missing braces. Oops! >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:207 >> + let result = new Set(other); > > Why use `other` and not `this`? Why use `this` and not `other`? >> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:195 >> + treeElement = treeElement.children.lastValue; > > This is a scary assumption to make. Why don't we pass the actual `WI.DOMTreeElement` as `elementCloseTag` instead of just a true/false? That way, we can retrieve it directly here (e.g. `treeElement.associatedCloseTagTreeElement`). Nice idea. Will change. >> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:199 >> + this.selectTreeElements(selectedTreeElements); > > Will this interfere with the selection set by `_revealAndSelectNode` above? Should we reverse the order of the two, and maybe filter out `selectedNode` from the list of `selectedTreeElements`? Actually reversing the order will cause it to break: 1. Shift select some nodes, in a bottom-to-top direction. For example: <div> <p></p> <-- Then shift-click to extend the selection to this node (2) <p></p> <p></p> <-- Click to select this node first (1) </div> 2. The topmost (2) selected item's WI.DOMNode is shown in the details sidebar 3. Change to a different tab, then back to Elements => Selection restored, but the WI.DOMNode for the bottommost selected item (1) is now shown in the sidebar I'll add a comment, or find a way to rewrite it to not depend on this subtlety.
Comment on attachment 362563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362563&action=review >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:212 >> + > > Style: unnecessary newline. I wanted to be consistent with the surrounding code. I wish we had better guidelines for these cases. For example, why should return be on its own line in `maxValue` above, but not here? It feels very arbitrary.
Comment on attachment 362563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362563&action=review >>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:207 >>> + let result = new Set(other); >> >> Why use `other` and not `this`? > > Why use `this` and not `other`? Touche. The other `Set.prototype` functions follow that pattern.
Comment on attachment 362563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362563&action=review >>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:195 >>> + treeElement = treeElement.children.lastValue; >> >> This is a scary assumption to make. Why don't we pass the actual `WI.DOMTreeElement` as `elementCloseTag` instead of just a true/false? That way, we can retrieve it directly here (e.g. `treeElement.associatedCloseTagTreeElement`). > > Nice idea. Will change. r-, as it seems like you're going to make some changes
Created attachment 363813 [details] Patch
Comment on attachment 363813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363813&action=review > Source/WebInspectorUI/UserInterface/Base/Utilities.js:195 > + for (let value of iterator) Style: missing braces. > Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:120 > + this._shiftAnchorItem = null; Looking at it more, I think this may introduce unexpected behavior, as it would change the `_shiftAnchorItem`. If I select item 1, shift-click to item 4, the `_lastSelectedItem` would be item 1 and the `_shiftAnchorItem` would be also be item 1. This could would clear `_shiftAnchorItem`, even though it would keep `_lastSelectedItem` the same. > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:535 > + this._closeTagTreeElement = this.insertChildElement(this.representedObject, this.children.length, true); While you're making this change, we could also introduce an `openTagTreeElement` and have a link in both directions (it would involve passing `this` instead of `true` as the last argument). > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:202 > + this.selectTreeElements(selectedTreeElements); You said you were going to add a comment explaining why this must be called in this particular order? > LayoutTests/inspector/unit-tests/set-utilities.html:125 > + return true; NIT: in a sync test suite, the return is unnecessary (unless you want to fail the test case and stop running the other test cases).
Comment on attachment 363813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363813&action=review >> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:120 >> + this._shiftAnchorItem = null; > > Looking at it more, I think this may introduce unexpected behavior, as it would change the `_shiftAnchorItem`. If I select item 1, shift-click to item 4, the `_lastSelectedItem` would be item 1 and the `_shiftAnchorItem` would be also be item 1. This could would clear `_shiftAnchorItem`, even though it would keep `_lastSelectedItem` the same. This method isn't called when shift-clicking. That's done by handleItemMouseDown, which calls _updateSelection directly when selecting multiple items. In the future we might want to retain the anchor when programmatically selecting items, but let's wait until the need arises. >> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:202 >> + this.selectTreeElements(selectedTreeElements); > > You said you were going to add a comment explaining why this must be called in this particular order? I'm going to try refactoring this slightly to eliminate the need for a comment.
Created attachment 365941 [details] Patch
Created attachment 365945 [details] Patch
(In reply to Matt Baker from comment #15) > Created attachment 365945 [details] > Patch I simplified SelectionController.selectItem, but need to make another change for this to work correctly.
Created attachment 366063 [details] Patch
A few notes on the latest patch: - Removed Set utility methods maxValue and union. No longer used. - Added Set utility method lastValue. - SelectionController.prototype.selectItems will set _lastSelectedItem to the last item being selected. SelectionController tracks its selected items in the order in which they are selected: let sc = new SelectionController(...); let items = sc.selectedItems; let lastSelectedItem = sc.lastSelectedItem; console.assert(items.lastValue === lastSelectedItem);
Comment on attachment 363813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363813&action=review >>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:120 >>> + this._shiftAnchorItem = null; >> >> Looking at it more, I think this may introduce unexpected behavior, as it would change the `_shiftAnchorItem`. If I select item 1, shift-click to item 4, the `_lastSelectedItem` would be item 1 and the `_shiftAnchorItem` would be also be item 1. This could would clear `_shiftAnchorItem`, even though it would keep `_lastSelectedItem` the same. > > This method isn't called when shift-clicking. That's done by handleItemMouseDown, which calls _updateSelection directly when selecting multiple items. In the future we might want to retain the anchor when programmatically selecting items, but let's wait until the need arises. I understand that this path isn't called by shift-clicking, but it may be called after `_shiftAnchorItem` was set by a previous shift-click. As an example, if I shift-click a few nodes, delete one of them via JavaScript (e.g. `node.remove()`), and then try to shift-click again, I'd experience unexpected behavior.
Comment on attachment 366063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366063&action=review > Source/WebInspectorUI/UserInterface/Base/Utilities.js:176 > + let iterator = this.values(); > + for (let i = 0; i < this.size - 1; ++i) > + iterator.next(); > + return iterator.next().value; I think it would be "cleaner" to just use `return Array.from(this).lastValue`. It effectively does the same logic, albeit in a slightly different way (it's also much simpler). > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:540 > + this._closeTagTreeElement = this.insertChildElement(this.representedObject, this.children.length, true); We should clear this at the beginning of this function. If we change node type (e.g. Edit > Tag), we'd want to make sure that this isn't incorrect (or keeping some other object alive). > LayoutTests/inspector/tree-outline/tree-outline-selection.html:24 > + addObject(treeElement, child); `appendObject`? > LayoutTests/inspector/tree-outline/tree-outline-selection.html:35 > + InspectorTest.expectEqual(treeOutline.selectedTreeElement, treeElement, `TreeOutline should have selected TreeElement "${treeElement.title}".`); We should also `assert`/`expectEqual` that the size of `selectedTreeElements` (or even the actual values) matches what we'd expect. > LayoutTests/inspector/tree-outline/tree-outline-selection.html:50 > + name: "TreeOutline.FindTreeElement", What about a negative case? Where a `representedObject` doesn't exist? > LayoutTests/inspector/tree-outline/tree-outline-selection.html:51 > + description: "Find TreeElement for given represented object.", This isn't a very good description :( > LayoutTests/inspector/tree-outline/tree-outline-selection.html:69 > + description: "Select a tree element, then select a different tree element.", Ditto (>51). > LayoutTests/inspector/tree-outline/tree-outline-selection.html:76 > + InspectorTest.expectThat(treeElement.selected, `TreeElement "${treeElement.title}" should have selected = true.`); Rather than use "selected = false", how about just "should be selected"? > LayoutTests/inspector/tree-outline/tree-outline-selection.html:79 > + InspectorTest.expectThat(otherTreeElementsAreDeselected, "Other TreeElements should have selected = false."); Ditto (>76). > LayoutTests/inspector/tree-outline/tree-outline-selection.html:95 > + name: "TreeOutline.AllowsMultipleSelection", What about a test for what happens when we have a selection and change `allowsMultipleSelection` (e.g. turning it off should "clamp" the selection to just one item). > LayoutTests/inspector/tree-outline/tree-outline-selection.html:107 > + function triggerSelectTreeElements(treeOutline, treeElements) { Please put this before any test cases, as otherwise it's harder to find/distinguish. > LayoutTests/inspector/tree-outline/tree-outline-selection.html:108 > + let displayText = `${treeElements.map((x) => `"${x.title}"`)}`; This is an odd way of getting a stringified array. Please use `JSON.stringify` instead. > LayoutTests/inspector/tree-outline/tree-outline-selection.html:110 > + InspectorTest.log(`Selecting TreeElements [${displayText}].`); NIT: we normally put "..." at the end of any log before an action. > LayoutTests/inspector/tree-outline/tree-outline-selection.html:115 > + let treeElementsSelected = treeElements.every((x) => x.selected); I'd inline this. > LayoutTests/inspector/tree-outline/tree-outline-selection.html:116 > + InspectorTest.expectThat(treeElementsSelected, `TreeElements [${displayText}] should have selected = true.`); Ditto (>76). > LayoutTests/inspector/tree-outline/tree-outline-selection.html:118 > + let otherTreeElementsNotSelected = treeOutline.children.every((x) => !x.selected || treeElements.includes(x)); Ditto (>115). > LayoutTests/inspector/tree-outline/tree-outline-selection.html:119 > + InspectorTest.expectThat(otherTreeElementsNotSelected, "All other TreeElements should have selected = false."); Ditto (>76). > LayoutTests/inspector/tree-outline/tree-outline-selection.html:124 > + description: "Select two tree elements, then select two different tree elements.", Ditto (>51).
Created attachment 366864 [details] Patch
Created attachment 367164 [details] Patch
Comment on attachment 367164 [details] Patch Clearing flags on attachment: 367164 Committed r244154: <https://trac.webkit.org/changeset/244154>
All reviewed patches have been landed. Closing bug.