Web Inspector: Replace isAncestor and isDescendant with native DOM contains method

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

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

[Devin Rousso]

Created attachment 332224 [details]
Patch

[Nikita Vasilyev]

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.

[BJ Burg]

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.

[Joseph Pecoraro]

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.");

[Devin Rousso]

(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.

[Devin Rousso]

Created attachment 333250 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 333250 [details]
Patch

Clearing flags on attachment: 333250

Committed r228216: <https://trac.webkit.org/changeset/228216>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37298898>