Open Bug 1122268 Opened 9 years ago Updated 2 years ago

Make setOuterHTML actor method do less work and not remove the target element if possible

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: bgrins, Unassigned)

Details

It would be nice if the call to setOuterHTML didn't blow away the entire element that is being edited if possible.  This causes event handlers to be removed and adds some strangeness in the markup view frontend that has to do with reselecting the node (since it's been removed).

There are some virtual DOM diffing algorithms that could help, or maybe something we could do something simpler to keep the outer element around.
Transcript from a discussion with James:

jlongster
2:24 couldn't you easily at least compare the top-level element to see if it's the same as the first element in the given str, and at least reuse that?
2:24 and still iterate over it's attributes, and do a little checking there
2:24 you'd still blow away the inner contents, but at least the outer one would live

bgrins
2:26 probably after narrowing some cases down.  Here is the current code: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2114.  Notice that on document element it even modifies attribute directly since you can't set outerHTML on that
2:27 so theoretically you could do that operation on the outer node as long as it's tag name matched (attrs wouldn't even have to)

jlongster
2:29 yeah. in general, I think diffing the whole thing might turn out to be difficult, tbh. it is difficult problem, and while the algorithm may be somewhat straight-forward, it'd probably take a lot of code

bgrins
2:29 keeping the outer node around would be an improvement, in the sense that we wouldn't have to do some weirdness in the markupview frontend that involves reselecting the node (since setting outerhtml causes a childList mutation on the parent)
2:30 but then the frontend would have to *sometimes* do this weirdness (in the case of tag name change), so the code couldn't go away, but it may make it feel a little smoother in the common case
How does the user set the outer HTML of the document element? I don't see it in the inspector. (it isn't the HTML tag is it? I can replace that with a raw string).

I bet you could repurpose a lot of the code within the branch for the document element for any tag that's of the same type. You will have to lightly parse the given string though to get the tag name. Also since there can be multiple top-level elements in the string, you'll need to check the tag of the first element, and support appending sibling elements (or, maybe even easier, if there's multiple top-level elements in the given string just use outerHTML and blow away everything like we do now).
(In reply to James Long (:jlongster) from comment #2)
> How does the user set the outer HTML of the document element? I don't see it
> in the inspector. (it isn't the HTML tag is it? I can replace that with a
> raw string).

Yes, it's the HTML element.  Right click on HTML Element -> Edit as HTML -> Change text -> cmd+enter.  It works in the UI because of the workaround in the inspector actor, but if you try to run something like this in your console it doesn't work: document.documentElement.outerHTML = "<html><body>hi</body></html>".
Brian: Just checking if we are still considering this improvement?

Inspector bug triage. Filter on CLIMBING SHOES.
Severity: normal → enhancement
Flags: needinfo?(bgrinstead)
Priority: -- → P3
(In reply to Julian Descottes [:jdescottes] from comment #4)
> Brian: Just checking if we are still considering this improvement?
> 
> Inspector bug triage. Filter on CLIMBING SHOES.

I think it should still be considered, but the amount of work vs the payoff might not be great.  If we could at least treat it as an inner HTML edit if that's basically what was requested (i.e. they didn't change the tag name, attributes, or add and new sibling elements to the node) then that'd be a good start.

For instance if:

* <div id="foo"><div id="bar"></div></div> -> <div id="foo"><div id="bar">baz</div></div> then #foo is the same node but #bar is replaced
* <div id="foo"></div> -> <div id="foo" class="bar"></div> then #foo is replaced
* <div id="foo"></div> -> <p></p><div id="foo" class="bar"></div> then #foo is replaced

But, I'm not sure if solving that single case is any easier than handling it more fully with virtual dom / diffing
Flags: needinfo?(bgrinstead)
Product: Firefox → DevTools
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.