Instead of traversing the DOM manually we should use Node.prototype.contains. Utilities.js: Object.defineProperty(Node.prototype, "isAncestor", ... Object.defineProperty(Node.prototype, "isDescendant", ... Object.defineProperty(Node.prototype, "isSelfOrAncestor", ... Object.defineProperty(Node.prototype, "isSelfOrDescendant", ... https://developer.mozilla.org/en-US/docs/Web/API/Node/contains
Created attachment 332224 [details] Patch
Comment on attachment 332224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332224&action=review > Source/WebInspectorUI/UserInterface/Base/Main.js:994 > - if (element && element.isSelfOrAncestor(this.currentFocusElement)) > + if (element && element.contains(this.currentFocusElement)) Huh, I didn't realize element.contains(element) returns true. element.isAncestor(element) and element.isDescendant(element) ruturn true (thus we have isSelfOr* methods). We should look be careful replacing isAncestor/isDescendant with contains and look case-by-case that it didn't break anything.
Comment on attachment 332224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332224&action=review r=me Please test manually before landing. in particular, the descendant checks previously null-checked the node argument, but now we are unconditionally calling .contains on the node receiver which could throw an exception if it was null. > Source/WebInspectorUI/UserInterface/Views/BoxModelDetailsSectionRow.js:308 > + if (!element.contains(selectionRange.commonAncestorContainer)) This seems unlikely to be nil. Can you add a console assertion? > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:710 > + if (event.relatedTarget && this.element.contains(event.relatedTarget)) { We should always have this.element. > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:331 > + if (nodeUnderMouse && this.element.contains(nodeUnderMouse)) We should always have this.element. > Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:284 > + if (!element.contains(range.commonAncestorContainer)) We already unconditionally call a method on element. > Source/WebInspectorUI/UserInterface/Views/LogContentView.js:451 > + if (this._selectedMessages.length && !this._selectedMessages.some(element => element.contains(event.target))) { Ugh, this is not good style in existing code. Sometime, _selectedMessages should either be renamed to _selectedMessageElements, or store a non-Node type to be less ambiguous. This change seems fine, though. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:45 > + if (this._statusElement.contains(event.target)) this._statusElement should be generated by setting this.status in the constructor. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:55 > + if (focusedElement && this.element.contains(focusedElement)) Should always exist.
I don't see this test updated: LayoutTests/inspector/view/basics.html 50: InspectorTest.expectThat(child.isDescendantOf(parent), "Child should be a descendant of the parent."); 59: InspectorTest.expectThat(grandchild.isDescendantOf(child.parentView), "Grandchild should be a descendant of it's grandparent."); 73: InspectorTest.expectThat(!child.isDescendantOf(parent), "Removed view should not be a descendant of the parent.");
(In reply to Joseph Pecoraro from comment #4) > I don't see this test updated: > > LayoutTests/inspector/view/basics.html > 50: InspectorTest.expectThat(child.isDescendantOf(parent), "Child > should be a descendant of the parent."); > 59: > InspectorTest.expectThat(grandchild.isDescendantOf(child.parentView), > "Grandchild should be a descendant of it's grandparent."); > 73: InspectorTest.expectThat(!child.isDescendantOf(parent), > "Removed view should not be a descendant of the parent."); `isDescendantOf` is a function on WI.View, which is unrelated to this change, as it doesn't even use the DOM.
Created attachment 333250 [details] Patch
Comment on attachment 333250 [details] Patch Clearing flags on attachment: 333250 Committed r228216: <https://trac.webkit.org/changeset/228216>
All reviewed patches have been landed. Closing bug.
<rdar://problem/37298898>