Reselect logic when editing a tagname in the markup view is brittle
Categories
(DevTools :: Inspector, defect, P3)
Tracking
(firefox130 fixed)
| Tracking | Status | |
|---|---|---|
| firefox130 | --- | fixed |
People
(Reporter: nchevobbe, Assigned: nchevobbe)
References
Details
Attachments
(1 file)
When editing a node tagname in the markup view, we actually create a new node, insert it before the old one, move all the children from the old node to the new one, and finally remove the old node.
Because we're actually getting a new node, we want to select it again once we get it on the client.
this.markup.reselectOnRemoved(this.node, "edittagname");
For this we're listening for mutations, and try to reselect the node if we get a childList mutation on the original parent of the node we renamed
reselectOnRemoved(removedNode, reason) {
...
const onMutations = (this._removedNodeObserver = mutations => {
...
if (
this.inspector.selection.nodeFront === parentContainer.node ||
...
) {
...
this._markContainerAsSelected(childContainer, reason);
...
}
});
...
this.inspector.on("markupmutation", onMutations);
},
The issue is that we can get other mutations in the meantime, that would cause another node to be selected and make us fail the "reselection"
Selection.prototype = {
_onMutations(mutations) {
...
for (const m of mutations) {
...
if (m.type == "childList") {
if (!detached && !this.isConnected()) {
if (this.isNode()) {
parentNode = m.target;
}
detached = true;
}
}
...
}
...
if (detached) {
this.emit("detached-front", parentNode);
}
here we'll emit detached-front with the parent node front of the first mutation item we get, given the node isn't connected.
which triggers the selection of the parent
/**
* When a node is deleted, select its parent node or the defaultNode if no
* parent is found (may happen when deleting an iframe inside which the
* node was selected).
*/
onDetached(parentNode) {
...
this.selection.setNodeFront(nodeFront, { reason: "detached" });
},
In one of my test, I was hitting this test case as we were getting mutation when anonymouscontentroot were created (e.g. scrollbar, moz-caret-accessible), which we don't show in regular toolboxes, so were seen as "disconnected".
So here I'm not sure what's the proper fix:
- should we consider it very unlikely that just after editing the tagname the user selected another node?
- or maybe we should not emit mutations for node we wouldn't show in the markup view?
- or making sure that we don't consider the selection as "changed" if the change in the selection wasn't due to the user actually interacting with the UI?
|
Assignee | |
Comment 1•4 months ago
|
||
When editing a tagname, we're actually removing the node and replacing it with
a new one, with the new tagname.
This triggers a mutation, and on the client, we detect that the currently
selected node was detached, and we select its parent.
The markup view then has some logic to know that it should reselect a node,
by checking that the currently selected node was the parent of the edited node.
Unfortunately, the way we were selecting the parent node could be "highjacked" if
other mutations are reported at the same time (mutations are throttled); we were
picking the first node who was mutated as the one we should select, although the
mutations array could actually contain the parent of the renamed node, which is
the one we want to reselect.
To fix this, we flag the tag we rename so we can identify it in the mutations
we get, and send the flag to the client. On the client, we'd then select this
parent node with the expected mutation in it.
We're not adding a specific test for it, the next patch on the queue (for Bug 1173057),
has a test which was failing intermittently without this fix.
|
||
Updated•4 months ago
|
|
||
Updated•4 months ago
|
|
|
||
Updated•4 months ago
|
|
||
Comment 3•4 months ago
|
||
| bugherder | ||
Description
•