Skip to content
Permalink
Browse files
Web Inspector: Layout panel doesn't always match visible nodes/order …
…of nodes in document

https://bugs.webkit.org/show_bug.cgi?id=240775
rdar://93727833

Reviewed by Devin Rousso.

There were a couple issues preventing the layout panel from showing nodes that could actually have an overlay shown in
a predictable order.

First, DOMManager.prototype.nodesWithLayoutContextType provided an array of nodes sorted by nodeId, not their order in
the document. This meant that nodes created later and inserted into the page did not get placed in their logical order
relative to other containers, but at the end of the list. This is fixed with a new iterator that iterated through the
document and its tree in a way the preserves the order of elements as they would appear in a Tree view.

Second, LayoutDetailsSidebarPanel was not instrumenting the insertion/deletion of nodes, which meant that a node with a
layout context could be removed from the DOM tree, but still exist. These nodes weren't actually useful in the list
because you can't turn on the overlay for them. A similar issue existed for inserting a known node back into the DOM
tree.

Also fix an assertion reached from NodeOverlayListSection when toggling all overlays off.

* Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:
(WI.DOMManager.prototype.attachedNodes):
(WI.DOMManager.prototype.nodesWithLayoutContextType):

* Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:
(WI.LayoutDetailsSidebarPanel.prototype.attached):
(WI.LayoutDetailsSidebarPanel.prototype.detached):
(WI.LayoutDetailsSidebarPanel.prototype._handleNodeInserted):
(WI.LayoutDetailsSidebarPanel.prototype._handleNodeRemoved):
(WI.LayoutDetailsSidebarPanel.prototype._handleLayoutContextTypeChanged):
(WI.LayoutDetailsSidebarPanel.prototype._removeNodeFromNodeSets):
(WI.LayoutDetailsSidebarPanel.prototype._invalidateNodeSets):
(WI.LayoutDetailsSidebarPanel.prototype._refreshNodeSets):
- Instead of iterating all the nodes twice every time something changes, iterate all the nodes once, an only do so when
doing layout to prevent multiple iterations for an eventual single layout.

* Source/WebInspectorUI/UserInterface/Views/NodeOverlayListSection.js:
(WI.NodeOverlayListSection.prototype._handleToggleAllCheckboxChanged):
- Fix for an assertion reachable while manually testing this patch. Toggling the overlay off when it is already off is
not allowed, and turning an overlay on that is already on is needlessly chatty over the protocol (unless changing the
settings/color, which is left untouched).

* LayoutTests/inspector/dom/attachedNodes-expected.txt: Added.
* LayoutTests/inspector/dom/attachedNodes.html: Added.

* LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:
- Update to use WI.domManager.attachedNodes() iterator.

Canonical link: https://commits.webkit.org/250897@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294695 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
patrickangle committed May 23, 2022
1 parent e5db103 commit ced7fb0c25b71e3f7c73f34e9476e4771ba93f51
Showing 6 changed files with 376 additions and 45 deletions.
@@ -22,45 +22,50 @@
WI.cssManager.layoutContextTypeChangedMode = layoutContextTypeChangedMode;
}

function nodesWithLayoutContextType(type)
{
return Array.from(WI.domManager.attachedNodes({filter: (node) => node.layoutContextType === type}));
}

suite.addTestCase({
name: "CSS.setLayoutContextTypeChangedMode.queryGrid",
description: "Test that the expected number of grids are instrumented without chagning the LayoutContextTypeChangedMode.",
async test() {
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 0, "0 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 0, "0 grid nodes should be instrumented.");

// Query for the node, sending it to the frontend.
InspectorTest.log("Querying document for selector `#queryGrid`...");
let documentNode = await WI.domManager.requestDocument();
let queryNode = WI.domManager.nodeForId(await documentNode.querySelector("#queryGrid"));
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 1, "1 grid node should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 1, "1 grid node should be instrumented.");

InspectorTest.log("Changing `#queryGrid` to `display: block;`...");
await Promise.all([
queryNode.awaitEvent(WI.DOMNode.Event.LayoutContextTypeChanged),
changeElementDisplayValue("queryGrid", "block"),
]);
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 0, "0 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 0, "0 grid nodes should be instrumented.");

InspectorTest.log("Changing `#queryGrid` to `display: grid;`...");
await Promise.all([
queryNode.awaitEvent(WI.DOMNode.Event.LayoutContextTypeChanged),
changeElementDisplayValue("queryGrid", "grid"),
]);
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 1, "1 grid node should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 1, "1 grid node should be instrumented.");

InspectorTest.log("Changing `#queryGrid` to `display: block;`...");
await Promise.all([
queryNode.awaitEvent(WI.DOMNode.Event.LayoutContextTypeChanged),
changeElementDisplayValue("queryGrid", "block"),
]);
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 0, "0 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 0, "0 grid nodes should be instrumented.");

InspectorTest.log("Changing `#queryGrid` to `display: inline-grid;`...");
await Promise.all([
queryNode.awaitEvent(WI.DOMNode.Event.LayoutContextTypeChanged),
changeElementDisplayValue("queryGrid", "inline-grid"),
]);
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 1, "1 grid node should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 1, "1 grid node should be instrumented.");
}
});

@@ -70,34 +75,34 @@
async test() {
await WI.domManager.requestDocument();

InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 1, "1 grid node should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 1, "1 grid node should be instrumented.");

// Grid layout contexts are sent to the frontend when the mode is changed to all.
InspectorTest.log("Changing `layoutContextTypeChangedMode` to `All`...");
await Promise.all([
WI.domManager.awaitEvent(WI.DOMManager.Event.NodeInserted),
setLayoutContextTypeChangeMode(WI.CSSManager.LayoutContextTypeChangedMode.All),
]);
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");

// Once a node has been observed, it will always be kept up-to-date.
InspectorTest.log("Changing `layoutContextTypeChangedMode` to `Observed`...");
await setLayoutContextTypeChangeMode(WI.CSSManager.LayoutContextTypeChangedMode.Observed),
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");

InspectorTest.log("Changing `#normalGrid` to `display: block;`...");
await Promise.all([
WI.DOMNode.awaitEvent(WI.DOMNode.Event.LayoutContextTypeChanged),
changeElementDisplayValue("normalGrid", "block"),
]);
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 1, "1 grid node should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 1, "1 grid node should be instrumented.");

InspectorTest.log("Changing `#normalGrid` to `display: grid;`...");
await Promise.all([
WI.DOMNode.awaitEvent(WI.DOMNode.Event.LayoutContextTypeChanged),
changeElementDisplayValue("normalGrid", "grid"),
]);
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");
}
});

@@ -107,36 +112,36 @@
async test() {
await WI.domManager.requestDocument();

InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");

// Changes to unobserved nodes should not change the grid count.
InspectorTest.log("Changing `#normalNonGrid` to `display: grid;`...");
await changeElementDisplayValue("normalNonGrid", "grid");
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");

InspectorTest.log("Changing `#normalNonGrid` to `display: block;`...");
await changeElementDisplayValue("normalNonGrid", "block");
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");

// Enabling `All` mode should not change the grid count.
InspectorTest.log("Changing `layoutContextTypeChangedMode` to `All`...");
await setLayoutContextTypeChangeMode(WI.CSSManager.LayoutContextTypeChangedMode.All),
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");

// Changing a node to a grid after enabling `All` mode will increase the count.
InspectorTest.log("Changing `#normalNonGrid` to `display: grid;`...");
await Promise.all([
WI.DOMNode.awaitEvent(WI.DOMNode.Event.LayoutContextTypeChanged),
changeElementDisplayValue("normalNonGrid", "grid"),
]);
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 3, "3 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 3, "3 grid nodes should be instrumented.");

InspectorTest.log("Changing `#normalNonGrid` to `display: block;`...");
await Promise.all([
WI.DOMNode.awaitEvent(WI.DOMNode.Event.LayoutContextTypeChanged),
changeElementDisplayValue("normalNonGrid", "block"),
]);
InspectorTest.expectEqual(WI.domManager.nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");
InspectorTest.expectEqual(nodesWithLayoutContextType(WI.DOMNode.LayoutContextType.Grid).length, 2, "2 grid nodes should be instrumented.");
}
});

@@ -0,0 +1,118 @@
Tests for the DOMManager.attachedNodes.


== Running test suite: DOM.attachedNodes
-- Running test case: DOM.attachedNodes.Unfiltered
Dumping nodes:
#document
html
html
head
script
script
[text node]
style
[text node]
body
p#test-description
[text node]
div#a
::before
div#a1
div#a2
div#a3
::after
div#b
div#b1
div#b2
div#b3

-- Running test case: DOM.attachedNodes.Filtered
Dumping nodes:
div#a
div#a1
div#a2
div#a3
div#b
div#b1
div#b2
div#b3

-- Running test case: DOM.attachedNodes.DetachedNode
Creating detached node...
Created detached node: div#script-created-node
Dumping nodes:
#document
html
html
head
script
script
[text node]
style
[text node]
body
p#test-description
[text node]
div#a
::before
div#a1
div#a2
div#a3
::after
div#b
div#b1
div#b2
div#b3
Attaching node to DOM tree...
DOM node attached to tree.
Dumping nodes:
#document
html
html
head
script
script
[text node]
style
[text node]
body
p#test-description
[text node]
div#a
::before
div#script-created-node
div#a1
div#a2
div#a3
::after
div#b
div#b1
div#b2
div#b3
Detaching node from DOM tree...
DOM node detached from tree.
Dumping nodes:
#document
html
html
head
script
script
[text node]
style
[text node]
body
p#test-description
[text node]
div#a
::before
div#a1
div#a2
div#a3
::after
div#b
div#b1
div#b2
div#b3

@@ -0,0 +1,132 @@
<!doctype html>
<html>
<head>
<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
<script>
function createDetachedNode()
{
window.__testDetachedNode = document.createElement("div");
window.__testDetachedNode.id = "script-created-node";
return window.__testDetachedNode;
}

function insertDetachedNode()
{
let parentElement = document.getElementById("a");
parentElement.insertBefore(window.__testDetachedNode, parentElement.firstChild);
}

function removePreviouslyDetachedNode()
{
window.__testDetachedNode.remove();
}

function test()
{
let suite = InspectorTest.createAsyncSuite("DOM.attachedNodes");

async function ensureDocumentSubtree() {
let documentNode = await WI.domManager.requestDocument();

const entireSubtreeDepth = -1;
await WI.assumingMainTarget().DOMAgent.requestChildNodes(documentNode.id, entireSubtreeDepth);
}

function logNodes(nodesIterator) {
InspectorTest.log("Dumping nodes:")
for (let node of nodesIterator) {
// Each test adds more content to the DOM tree, but we don't want to clutter test output with those.
if (node.escapedIdSelector === "#end-of-prewritten-test-content")
break;

logNode(node);
}
}

function logNode(node) {
if (node.nodeType() === Node.TEXT_NODE)
InspectorTest.log(" [text node]");
else
InspectorTest.log(" " + node.unescapedSelector);
}

suite.addTestCase({
name: "DOM.attachedNodes.Unfiltered",
description: "Ensure that `attachedNodes` provides a correctly ordered list of all attached DOM nodes.",
async test() {
await ensureDocumentSubtree();
logNodes(WI.domManager.attachedNodes());
}
});

suite.addTestCase({
name: "DOM.attachedNodes.Filtered",
description: "Ensure that `attachedNodes` correctly filters nodes.",
async test() {
await ensureDocumentSubtree();
logNodes(WI.domManager.attachedNodes({filter: (node) => node.nodeNameInCorrectCase() === "div"}));
}
});

suite.addTestCase({
name: "DOM.attachedNodes.DetachedNode",
description: "Ensure that `attachedNodes` provides a correctly ordered list of all attached DOM nodes, and ignored detached nodes that are known to the backend.",
async test() {
await ensureDocumentSubtree();

InspectorTest.log("Creating detached node...");
let detachedNodeRemoteObject = await InspectorTest.evaluateInPage(`createDetachedNode()`);
let detachedNodeId = (await WI.assumingMainTarget().DOMAgent.requestNode(detachedNodeRemoteObject.objectId)).nodeId;
let detachedNode = WI.domManager.nodeForId(detachedNodeId);
InspectorTest.log("Created detached node: " + detachedNode.unescapedSelector);

logNodes(WI.domManager.attachedNodes());

InspectorTest.log("Attaching node to DOM tree...");
await Promise.all([
InspectorTest.evaluateInPage(`insertDetachedNode()`),
WI.domManager.awaitEvent(WI.DOMManager.Event.NodeInserted),
]);
InspectorTest.log("DOM node attached to tree.");

logNodes(WI.domManager.attachedNodes());

InspectorTest.log("Detaching node from DOM tree...");
await Promise.all([
InspectorTest.evaluateInPage(`removePreviouslyDetachedNode()`),
WI.domManager.awaitEvent(WI.DOMManager.Event.NodeRemoved),
]);
InspectorTest.log("DOM node detached from tree.");

logNodes(WI.domManager.attachedNodes());
}
});

suite.runTestCasesAndFinish();
}
</script>
<style>
#a::before {
content: "before";
}

#a::after {
content: "after";
}
</style>
</head>
<body onload="runTest()">
<p id="test-description">Tests for the DOMManager.attachedNodes.</p>
<div id="a">
<div id="a1"></div>
<div id="a2"></div>
<div id="a3"></div>
</div>
<div id="b">
<div id="b1"></div>
<div id="b2"></div>
<div id="b3"></div>
</div>
<div id="end-of-prewritten-test-content"></div>
</body>
</html>

0 comments on commit ced7fb0

Please sign in to comment.