Bug 192119

<rdar://problem/46327554>

Created attachment 356270 [details] Patch

(In reply to Devin Rousso from comment #0) > Created attachment 355934 [details] > [Image] Screenshot of Issue > > I see a few options: > 1. Only show $0 for the most recently selected node > 2. Make $0 into an array when more than one node is selected > 3. Change all but the most recently selected node to be $$0 (or something to > that effect) and create a $$0 available to the console that returns the > array of all selected nodes I went with 1, as it is the simplest.

Comment on attachment 356270 [details] Patch r=me

> > I see a few options: > > 1. Only show $0 for the most recently selected node > > 2. Make $0 into an array when more than one node is selected > > 3. Change all but the most recently selected node to be $$0 (or something to > > that effect) and create a $$0 available to the console that returns the > > array of all selected nodes > > I went with 1, as it is the simplest. r=m assuming that `$0` really does resolve to the last selected element.

Created attachment 356274 [details] [Image] There can be only one

(In reply to Matt Baker from comment #6) > Created attachment 356274 [details] > [Image] There can be only one LOL, oops.

Created attachment 356280 [details] Patch

Thanks for prodding Joe. While the inspected node often resolved to the last selected TreeElement, this wasn't always the case. The updated patch corrects this, and makes some progress toward simplifying some of the more convoluted aspects of TreeOutline. Eventually I'd like to eliminate all of the TreeElement `on*` overloads, and have all TreeElement selection go through TreeOutline.

Comment on attachment 356280 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356280&action=review I'd like to see my comments be addressed before I r+. Looking good! > Source/WebInspectorUI/ChangeLog:57 > +2018-11-30 Matt Baker <mattbaker@apple.com> Double ChangeLog :( > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:462 > + WI.domManager.highlightDOMNode(domNode.id); This will error if `domNode` is null (meaning there is no selected element). Only call this if `domNode` is valid. > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:464 > + this._domTreeOutline.updateSelection(); This name is pretty confusing considering all of the new `SelectionController` logic :/ > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:-630 > - this.treeOutline.suppressRevealAndSelect = true; Is this necessary? It wasn't copied over. If not, you should remove the getter/setter in `WI.DOMTreeOutline` as well. > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:-635 > - this.treeOutline.suppressRevealAndSelect = false; Ditto (<630) > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:501 > + select(omitFocus, selectedByUser, suppressNotification) While you're at it, you should turn this into an optional object. select({omitFocus, selectedByUser, suppressNotification} = {}) > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:527 > + revealAndSelect(omitFocus, selectedByUser, suppressNotification) Ditto (>501) > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:533 > + deselect(suppressNotification) Ditto (>501) > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:819 > + this._previousSelectedTreeElement.listItemElement.classList.remove("last-selected"); Perhaps now would be a good time to add something similar to `WI.GeneralTreeElement.prototype.addClassName`. > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:821 > + selectedTreeElement.listItemElement.classList.add("last-selected"); Ditto (>819) > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:823 > + this._previousSelectedTreeElement = selectedTreeElement; Style: our usual "pattern" is to assign, and then do something to the new value if present: if (this._previousSelectedTreeElement) this._previousSelectedTreeElement.listItemElement.classList.remove("last-selected"); this._previousSelectedTreeElement = selectedTreeElement; if (this._previousSelectedTreeElement) this._previousSelectedTreeElement.listItemElement.classList.add("last-selected");

Comment on attachment 356280 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356280&action=review >> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:464 >> + this._domTreeOutline.updateSelection(); > > This name is pretty confusing considering all of the new `SelectionController` logic :/ I'll change it to `updateSelectionArea()`, which matches the DOMTreeElement method. >> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:501 >> + select(omitFocus, selectedByUser, suppressNotification) > > While you're at it, you should turn this into an optional object. > > select({omitFocus, selectedByUser, suppressNotification} = {}) I'm not a huge fan of this style, and I'd rather not add any unnecessary churn to this patch. >> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:819 >> + this._previousSelectedTreeElement.listItemElement.classList.remove("last-selected"); > > Perhaps now would be a good time to add something similar to `WI.GeneralTreeElement.prototype.addClassName`. I suppose the addClassName/removeClassName methods in GeneralTreeElement could be pushed into TreElement, but I'd rather not change that here. >> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:823 >> + this._previousSelectedTreeElement = selectedTreeElement; > > Style: our usual "pattern" is to assign, and then do something to the new value if present: > > if (this._previousSelectedTreeElement) > this._previousSelectedTreeElement.listItemElement.classList.remove("last-selected"); > > this._previousSelectedTreeElement = selectedTreeElement; > > if (this._previousSelectedTreeElement) > this._previousSelectedTreeElement.listItemElement.classList.add("last-selected"); Yeah this reads better.

Created attachment 356401 [details] Patch

Comment on attachment 356401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356401&action=review r=me > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:465 > + if (domNode && selectedByUser) > + WI.domManager.highlightDOMNode(domNode.id); If `domNode` is falsy, we should hide the highlight by calling `WI.domManager.hideDOMNodeHighlight();`. Side note: I just noticed, but I think this is broken: <https://webkit.org/b/192354> > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:468 > + this._domTreeOutline.suppressRevealAndSelect = false; > + this._domTreeOutline.updateSelectionArea(); Not sure that it matters, but this is reversed from what was there before. > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:-628 > - onselect(treeElement, selectedByUser) My only concern with something like this is the order of operations, in that `onselect` was guaranteed to happen before event dispatch, but considering that this doesn't change the selection, I think it's fine. > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:81 > +.tree-outline.dom li.last-selected > span::after { The rule after this (`.tree-outline.dom:focus li.selected > span::after`) should probably also be changed to `.last-selected`. > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:501 > - select(omitFocus, selectedByUser, suppressOnSelect, suppressOnDeselect) > + select(omitFocus, selectedByUser, suppressNotification) I really would prefer that you change this to an optional object in this patch. You're already changing almost all of the callers due to the argument removal, so it's not crazy to change them to this pattern at the same time. Using a list of flags is really not very readable or future-proof, as evident in much of this patch. I had to keep going back/forth multiple times to remember what each argument meant in all of the callers of `select`, `revealAndSelect`, and `deselect` (even worse for callers that just pass `true` or `false`). Using an optional object makes it clear at the call-site what each argument means, and makes it so that the callers only have to provide values for things they want "on".

Comment on attachment 356401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356401&action=review >> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:468 >> + this._domTreeOutline.updateSelectionArea(); > > Not sure that it matters, but this is reversed from what was there before. it shouldn't matter, but I'll change it to match the original. >> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:-628 >> - onselect(treeElement, selectedByUser) > > My only concern with something like this is the order of operations, in that `onselect` was guaranteed to happen before event dispatch, but considering that this doesn't change the selection, I think it's fine. True, but in practice there is only only one event handler for the DOMTreeContentView's SelectionDidChange event.

Comment on attachment 356401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356401&action=review >> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:465 >> + WI.domManager.highlightDOMNode(domNode.id); > > If `domNode` is falsy, we should hide the highlight by calling `WI.domManager.hideDOMNodeHighlight();`. > > Side note: I just noticed, but I think this is broken: <https://webkit.org/b/192354> This matches the original. The highlight is hidden on mouseout, dragstart, and when the tree is hidden.

Comment on attachment 356401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356401&action=review >> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:501 >> + select(omitFocus, selectedByUser, suppressNotification) > > I really would prefer that you change this to an optional object in this patch. You're already changing almost all of the callers due to the argument removal, so it's not crazy to change them to this pattern at the same time. Using a list of flags is really not very readable or future-proof, as evident in much of this patch. I had to keep going back/forth multiple times to remember what each argument meant in all of the callers of `select`, `revealAndSelect`, and `deselect` (even worse for callers that just pass `true` or `false`). Using an optional object makes it clear at the call-site what each argument means, and makes it so that the callers only have to provide values for things they want "on". Refactoring this would mean touching DebuggerSidebarPanel, OpenResourceDialog, ThreadTreeElemen, ContentBrowserTabContentView, FolderizedTreeElement, and at least four other files. I'd rather not do that as part of this patch.

(In reply to Matt Baker from comment #16) > Comment on attachment 356401 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356401&action=review > > >> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:501 > >> + select(omitFocus, selectedByUser, suppressNotification) > > > > I really would prefer that you change this to an optional object in this patch. You're already changing almost all of the callers due to the argument removal, so it's not crazy to change them to this pattern at the same time. Using a list of flags is really not very readable or future-proof, as evident in much of this patch. I had to keep going back/forth multiple times to remember what each argument meant in all of the callers of `select`, `revealAndSelect`, and `deselect` (even worse for callers that just pass `true` or `false`). Using an optional object makes it clear at the call-site what each argument means, and makes it so that the callers only have to provide values for things they want "on". > > Refactoring this would mean touching DebuggerSidebarPanel, > OpenResourceDialog, ThreadTreeElemen, ContentBrowserTabContentView, > FolderizedTreeElement, and at least four other files. I'd rather not do that > as part of this patch. I guess we're modifying most of these anyway. Still, I'd prefer not to refactor old TreeOutline code at this time.

Created attachment 356476 [details] Patch

Comment on attachment 356476 [details] Patch Attachment 356476 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10263159 New failing tests: http/wpt/mediarecorder/MediaRecorder-AV-video-only-dataavailable.html

Created attachment 356495 [details] Archive of layout-test-results from ews116 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6

Created attachment 356501 [details] Patch for landing

Comment on attachment 356501 [details] Patch for landing Clearing flags on attachment: 356501 Committed r238859: <https://trac.webkit.org/changeset/238859>

All reviewed patches have been landed. Closing bug.