Paul, if you are busy, then I can work on this early next week.

(In reply to Girish Sharma [:Optimizer] from comment #1) > Paul, if you are busy, then I can work on this early next week. Yes!

Not working on this atm

So is there a case when there *should* be ellipsis next to text in the markup view? We could remove the ellipsis functionality altogether and always show the full text of a text node.

(In reply to Brian Grinstead [:bgrins] from comment #4) > So is there a case when there *should* be ellipsis next to text in the > markup view? We could remove the ellipsis functionality altogether and > always show the full text of a text node. There's one thing I like in Firebug about nodes that only contain text, is that you don't need to expand them to see the text, it's displayed on the same line, in between the open and close tags. I think it helps identifying things quicker. Also, many times the text is really short, and it looks really strange sometimes to have to expand the node just to see 2 or 3 words. So of course this poses the question of what to do when the text is really long. I don't know if Firebug has an ellipsis in that case, but if it has, the threshold is quite high, and the text still is displayed inline.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #5) > (In reply to Brian Grinstead [:bgrins] from comment #4) > > So is there a case when there *should* be ellipsis next to text in the > > markup view? We could remove the ellipsis functionality altogether and > > always show the full text of a text node. > There's one thing I like in Firebug about nodes that only contain text, is > that you don't need to expand them to see the text, it's displayed on the > same line, in between the open and close tags. > I think it helps identifying things quicker. Also, many times the text is > really short, and it looks really strange sometimes to have to expand the > node just to see 2 or 3 words. > So of course this poses the question of what to do when the text is really > long. I don't know if Firebug has an ellipsis in that case, but if it has, > the threshold is quite high, and the text still is displayed inline. How about if we keep the possibility for shortened text, but we keep a much higher limit https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#157. We could use 1000 or 10000, as they are used as defaults for long string actors: https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#170

Increase the number of characters allowed for text nodes, comments, etc to 1000. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=bb24eb034a15

The bug is originally about expanding the node by default when the node is a text node and contains no children nodes. Not about the ellipsis present on the text of the node which we see after expanding the node manually.

(In reply to Girish Sharma [:Optimizer] from comment #8) > The bug is originally about expanding the node by default when the node is a > text node and contains no children nodes. Not about the ellipsis present on > the text of the node which we see after expanding the node manually. Ah, I was confused by the title, which mentions ellipsis, and the lack of a description in the bug.

The ellipses we are talking about are: <node prop1=val1 prop2=val2>...</node>

(although you might wanna let remain the summary length to 1000 chars too )

(In reply to Girish Sharma [:Optimizer] from comment #10) > The ellipses we are talking about are: > > <node prop1=val1 prop2=val2>...</node> I've made a demo here: http://jsfiddle.net/bgrins/XL8EV/. If I inspect the 'text with no whitespace' element, which is close to what you are talking about, I don't see any ellipsis. Am I missing something?

There are no ellipsis now. Anywhere.. They were removed sometime in the past with some refactoring. not sure if by mistake or purposefully. You can grab an old firefox (like 27 or 28) Any non empty node used to have a ... like I pasted in comment 11 when the node was in collapsed state in the markup view.

This always bugged me... imagine you have this markup: <div>Here is some text</div> We display it as <div></div> and allow it to be expanded. We should simply display it as: <div>Here is some text</div> It makes the source a lot easier to navigate.

(In reply to Girish Sharma [:Optimizer] from comment #13) > There are no ellipsis now. Anywhere.. They were removed sometime in the past > with some refactoring. not sure if by mistake or purposefully. Removed on purpose in bug 855523. > > You can grab an old firefox (like 27 or 28) > > Any non empty node used to have a ... like I pasted in comment 11 when the > node was in collapsed state in the markup view. Yes. So, the fact that nodes became a little easier to select/expand/collapse with this bug made us remove the ellipsis. My take on this bug is we should use it to auto-expand nodes that do not contain element children like I said in comment 5. Even though we don't have the ellipsis symbol we used to have before, the original idea of this bug still holds. I also agree that we should increase the limit for long text nodes as Brian proposed in his patch.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #14) > This always bugged me... imagine you have this markup: > <div>Here is some text</div> > > We display it as <div></div> and allow it to be expanded. > > We should simply display it as: > <div>Here is some text</div> > > It makes the source a lot easier to navigate. Yes, I also agree with this, inlining text nodes would be better.

OK, so this patch should auto expand nodes that do not contain element children. In addition it could possibly also display the open / close tags on the same line with the text (not sure how easy that will be with the current layout of the markup tree).

Auto expands nodes that only have text node children. I have a few thoughts / questions here: 1) should we only auto expand if it is all text nodes, or should we check instead to see if there 0 element children? In other words, should we be looking for the specific case of an element with all text nodes, or do we count comments (or any other node types) the same way. 2) Should we negate the naming on the server to make backwards compat easier (at the expense of having a negated variable name like noExpandableChildren) 3) Should we treat <br> tags the same as text nodes for this purpose? If we had markup like <p>top<br>bottom</p>, then do we want that to auto expand or not? 3a) Going further, if we did want to treat <br>s in this way, would we also want to treat children that contain only text nodes in the same manner? So, should the p tag in <p><i>a</i>b</p> be auto expanded?

(In reply to Brian Grinstead [:bgrins] from comment #18) > Created attachment 8420990 [details] [diff] [review] > auto-expand.patch > > Auto expands nodes that only have text node children. I have a few thoughts > / questions here: > > 1) should we only auto expand if it is all text nodes, or should we check > instead to see if there 0 element children? In other words, should we be > looking for the specific case of an element with all text nodes, or do we > count comments (or any other node types) the same way. I would only auto-expand if it's all text nodes, I think this should cover the most important use case for this. > 2) Should we negate the naming on the server to make backwards compat easier > (at the expense of having a negated variable name like noExpandableChildren) I like the way you've done it in the patch, with 'hasExpandableChildren'. > 3) Should we treat <br> tags the same as text nodes for this purpose? If we > had markup like <p>top<br>bottom</p>, then do we want that to auto expand or > not? My gut feeling tells me to say no, that we should only auto-expand nodes that contain text nodes only, but I can't really say why. I guess it feels weird to me to treat <br/> differently than other self-closing nodes (like <input /> for instance). > 3a) Going further, if we did want to treat <br>s in this way, would we also > want to treat children that contain only text nodes in the same manner? So, > should the p tag in <p><i>a</i>b</p> be auto expanded? I'd say no here too.

Comment on attachment 8420990 [details] [diff] [review] auto-expand.patch Review of attachment 8420990 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/inspector.js @@ +221,5 @@ > nodeType: this.rawNode.nodeType, > namespaceURI: this.rawNode.namespaceURI, > nodeName: this.rawNode.nodeName, > numChildren: numChildren, > + hasExpandableChildren: hasExpandableChildren, This makes me think that we'll soon need to check our "bandwidth consumption". The NodeActor's form is sent over the wire in many cases. Indeed, the walker's method often have it as a response type (or a NodeList) but client-side consumers often don't use all of the form's properties. I'm thinking about this now because I'm also contemplating another property in bug 911209, and it worries me that we'll add too much. I think it's worth investigating at some stage if we can pass a flag to some methods saying we only care about the node actor's ID, rather than the whole form. Anyway, probably out of this bug.

Comment on attachment 8420990 [details] [diff] [review] auto-expand.patch Review of attachment 8420990 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/inspector.js @@ +221,5 @@ > nodeType: this.rawNode.nodeType, > namespaceURI: this.rawNode.namespaceURI, > nodeName: this.rawNode.nodeName, > numChildren: numChildren, > + hasExpandableChildren: hasExpandableChildren, Can't this be achieved by a combination of nodeType and numChildren ?

I just tested the patch locally, I have to say that this would look and feel a lot better if the auto-expanded text node was on the same line as the open/close tags, in between them. Right now, it is a little bit surprising when expanding a node and seeing some of its children being auto-expanded automatically, because this makes the UI jump a little bit right beneath where the user's attention is. Why don't we just make a change in the markup-view (client-side only) to check numChildren and if 0, then display the form.shortValue inline? (unless numChildren also counts text nodes?). This would actually be invalid, because some text nodes wouldn't be represented by MarkupContainers, and it might make some tests fail, but I think it may actually be what we need here.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #22) > I just tested the patch locally, I have to say that this would look and feel > a lot better if the auto-expanded text node was on the same line as the > open/close tags, in between them. > > Right now, it is a little bit surprising when expanding a node and seeing > some of its children being auto-expanded automatically, because this makes > the UI jump a little bit right beneath where the user's attention is. > > Why don't we just make a change in the markup-view (client-side only) to > check numChildren and if 0, then display the form.shortValue inline? (unless > numChildren also counts text nodes?). > This would actually be invalid, because some text nodes wouldn't be > represented by MarkupContainers, and it might make some tests fail, but I > think it may actually be what we need here. Exactly, we should rely on numChildren and nodeType client side itself.

(In reply to Girish Sharma [:Optimizer] from comment #21) > Comment on attachment 8420990 [details] [diff] [review] > auto-expand.patch > > Review of attachment 8420990 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/devtools/server/actors/inspector.js > @@ +221,5 @@ > > nodeType: this.rawNode.nodeType, > > namespaceURI: this.rawNode.namespaceURI, > > nodeName: this.rawNode.nodeName, > > numChildren: numChildren, > > + hasExpandableChildren: hasExpandableChildren, > > Can't this be achieved by a combination of nodeType and numChildren ? (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #22) > Why don't we just make a change in the markup-view (client-side only) to > check numChildren and if 0, then display the form.shortValue inline? (unless > numChildren also counts text nodes?). > This would actually be invalid, because some text nodes wouldn't be > represented by MarkupContainers, and it might make some tests fail, but I > think it may actually be what we need here. numChildren includes all node types. It is using childNodes instead of children. Unfortunately the naming isn't consistent with the DOM properties (otherwise it would currently be numChildNodes and we could just add a numChildren and check > 0).

(In reply to Girish Sharma [:Optimizer] from comment #23) > (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #22) > > I just tested the patch locally, I have to say that this would look and feel > > a lot better if the auto-expanded text node was on the same line as the > > open/close tags, in between them. > > > > Right now, it is a little bit surprising when expanding a node and seeing > > some of its children being auto-expanded automatically, because this makes > > the UI jump a little bit right beneath where the user's attention is. > > > > Why don't we just make a change in the markup-view (client-side only) to > > check numChildren and if 0, then display the form.shortValue inline? (unless > > numChildren also counts text nodes?). > > This would actually be invalid, because some text nodes wouldn't be > > represented by MarkupContainers, and it might make some tests fail, but I > > think it may actually be what we need here. > > Exactly, we should rely on numChildren and nodeType client side itself. At this stage, the nodeType of the current node doesn't matter (we already know it is an element if we are considering expansion). We care about the nodeTypes of the child nodes

(In reply to Brian Grinstead [:bgrins] from comment #24) > numChildren includes all node types. It is using childNodes instead of > children. Unfortunately the naming isn't consistent with the DOM properties > (otherwise it would currently be numChildNodes and we could just add a > numChildren and check > 0). Alright, so we need a server-side change. Maybe we can use this opportunity to do as you said, change numChildren to be the number of element children, and rename the current numChildren into numChildNodes. The client will have to be changed, but numChildren is only used in one place today on the client-side, so even if we have to do a sort of 'if (old debuggee) { ... } else { ... }' it's not too bad.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #26) > (In reply to Brian Grinstead [:bgrins] from comment #24) > > numChildren includes all node types. It is using childNodes instead of > > children. Unfortunately the naming isn't consistent with the DOM properties > > (otherwise it would currently be numChildNodes and we could just add a > > numChildren and check > 0). > Alright, so we need a server-side change. Maybe we can use this opportunity > to do as you said, change numChildren to be the number of element children, > and rename the current numChildren into numChildNodes. > The client will have to be changed, but numChildren is only used in one > place today on the client-side, so even if we have to do a sort of 'if (old > debuggee) { ... } else { ... }' it's not too bad. I'm not opposed to doing this to make sure our naming is consistent with the DOM properties. This doesn't give us the same picture that the patch is looking at right now though. If we passed just numChildren and numChildNodes, then compared them to determine if a node is expandable, this would count comments and other node types the same as text nodes. We will need either another count that has just text nodes, or a boolean that determines whether children are expandable. Since it sounds like we are going to need to make some changes to the views in the frontend, maybe it would be better to be more explicit and have the server return numChildNodes (renamed from numChildren) and numTextNodes.

This patch takes a slightly different approach - if a node only has one text child, it just serializes that node as a singleTextChild attribute. A little less generic, and it doesn't solve the numChildren problem, but it degrades nicely in the face of old servers (they just won't ever set the singleTextChild property) and saves some round trips. Thoughts?

Worth mentioning: This ends up creating two editors for a single text node - one in the parent container and one for the node itself. We just never show the node's container. This lets us keep all the tree invariants in place, at the cost of a bit of weirdness keeping the inline editor up-to-date. I also tried doing fancy css to inline-block all the lists etc. to make the child container appear inline, but that started getting really messy. In particular, getting selection right was nasty. Clicking on the tags would select the element node, clicking on the text selects the text node. Other devtools (rightfully, I think) select the element node no matter where you click.

Comment on attachment 8596309 [details] [diff] [review] auto-expand.patch Review of attachment 8596309 [details] [diff] [review]: ----------------------------------------------------------------- I like the server-side changes. Especially that they take care of backward compatibility. Overall, I like approach. The text-editor weirdness isn't too bad. We just need to make sure we have the right comments to explain what's going on in the update and updateTextEditor methods. There could be another approach too: create a new type of MarkupContainer implementation (maybe called MarkupElementContainerWithText) which we'd use for element nodes that have 1 child text node only (in importNode). This would break the 1 node = 1 container assumption, which you preserved in your patch, but would it be such a big problem? It would simplify the UI code a little bit. But as I said, I'm not against this approach either, I would probably r+ this. One detail: pressing the right arrow on one of these nodes in the markup-view still expands it, and ends up showing twice the text.

> There could be another approach too: create a new type of MarkupContainer > implementation (maybe called MarkupElementContainerWithText) which we'd use > for element nodes that have 1 child text node only (in importNode). This > would break the 1 node = 1 container assumption, which you preserved in your > patch, but would it be such a big problem? > It would simplify the UI code a little bit. I think that would get hairy when a second node is appended to the parent node and you had to reassign nodes/containers. But I'll play around today and see if I can make that work. > But as I said, I'm not against this approach either, I would probably r+ > this. > One detail: pressing the right arrow on one of these nodes in the > markup-view still expands it, and ends up showing twice the text. Ah, yeah.

Comment on attachment 8596309 [details] [diff] [review] auto-expand.patch Review of attachment 8596309 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/markupview/markup-view.js @@ +2079,5 @@ > > this.tagLine.appendChild(this.editor.elt); > } > > +MarkupTextContainer.prototype = Heritage.extend(MarkupContainer.prototype, { No problem with the general approach here. I would consider creating a new SingleTextChildEditor type that handles this custom update functionality (and does basically nothing else). And then set `this.editor` equal to that in the case of `node.parentNode().singleTextChild == node` in the MarkupTextContainer constructor. This way we wouldn't need to do any extra work of actually templating and creating the inplace editor in the TextEditor for something that's never going to be used.

(In reply to Brian Grinstead [:bgrins] from comment #33) > Comment on attachment 8596309 [details] [diff] [review] > auto-expand.patch > > Review of attachment 8596309 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/markupview/markup-view.js > @@ +2079,5 @@ > > > > this.tagLine.appendChild(this.editor.elt); > > } > > > > +MarkupTextContainer.prototype = Heritage.extend(MarkupContainer.prototype, { > > No problem with the general approach here. I would consider creating a new > SingleTextChildEditor type that handles this custom update functionality > (and does basically nothing else). And then set `this.editor` equal to that > in the case of `node.parentNode().singleTextChild == node` in the > MarkupTextContainer constructor. This way we wouldn't need to do any extra > work of actually templating and creating the inplace editor in the > TextEditor for something that's never going to be used. Yeah, that seems reasonable. Will take a bit of finesse dealing with addition of a second child, but I'll see if I can make that clean.

(In reply to Dave Camp (:dcamp) from comment #34) > Yeah, that seems reasonable. Will take a bit of finesse dealing with > addition of a second child, but I'll see if I can make that clean. Just checked with the current patch applied, and it isn't handling that situation. If you highlight a node with single text child and run `$0.appendChild(document.createElement("i"))` it isn't getting updated. There is a childList mutation coming through, but the frontend doesn't seem to be reacting properly.

Haven't had a chance to open my editor yet but I think the childList mutation needs to update singleTextChild

New version allows an element container to act as the container for a single child text node. This breaks that fundamental 1:1 assumption, but gets rid of a decent chunk of nastiness, so it should be worth it. Also fixed child updates. Should switch cleanly between single child mode and multiple children mode. Still haven't fixed or added tests yet.

Comment on attachment 8603202 [details] [diff] [review] auto-expand.patch Review of attachment 8603202 [details] [diff] [review]: ----------------------------------------------------------------- Found some more things when running through this, but looking good. There is some extra space between the tag opening and closing and the text nodes. Looks like this can be solved by adding this css to browser/devtools/markupview/markup-view.css: .editor.text { display: inline-block; } ::: browser/devtools/markupview/markup-view.js @@ +1289,5 @@ > + // text child, back that out. > + this._containers.delete(aContainer.singleTextChild); > + aContainer.clearSingleTextChild(); > + > + aContainer.setExpanded(true); I'm not sure if this should be expanded automatically when an element goes from single text child to non-single-text child (like if a new child is added or if the element is emptied out). Maybe only if it's currently selected and there are more than 0 children @@ +1387,5 @@ > * Return a list of the children to display for this container. > */ > _getVisibleChildren: function(aContainer, aCentered) { > + // If the backend has told us that there's only one child, just use that child. > + if (aContainer.node.singleTextChild) { Nice, I'm assuming it'll be a perf win to pull the child down in the initial request and not do it here, since it's going to need to be shown anyway. OTOH, if we passed down singleTextChild as a boolean and then attached the child node to the container / front via a call to _getVisibleChildren it would simplify the server code. But adding extra async stuff in the frontend could very well end up being messier and more code. @@ +2532,5 @@ > + if (node && !this.textEditor) { > + // Create a text editor added to this editor. > + // This editor won't receive an update automatically, so we rely on > + // child text editors to let us know that we need updating. > + this.textEditor = new TextEditor(this.container, node, "text"); The text editor isn't getting expanded for long node values.. ElementEditor needs this attached to it's prototype for that to work: set selected(aValue) { if (this.textEditor) { this.textEditor.selected = aValue; } }, @@ +2536,5 @@ > + this.textEditor = new TextEditor(this.container, node, "text"); > + this.elt.insertBefore(this.textEditor.elt, this.elt.firstChild.nextSibling.nextSibling); > + } > + > + if (this.textEditor) { Another thing about the text editor: it seems to cause a mutation flash on the parent node if you double click into it and click out without doing anything. Oddly, this isn't an issue if you press 'enter' in the textarea ::: toolkit/devtools/server/actors/inspector.js @@ +2723,5 @@ > > mutation.removed = removedActors; > mutation.added = addedActors; > + > + if (change.target.childNodes.length == 1 && change.target.childNodes[0].nodeType === Ci.nsIDOMNode.TEXT_NODE) { This doesn't handle the case where I highlight one of these elements and then run `$0.innerHTML = ""`. Which is weird because it looks like it would

(In reply to Brian Grinstead [:bgrins] from comment #38) > ::: browser/devtools/markupview/markup-view.js > @@ +1289,5 @@ > > + // text child, back that out. > > + this._containers.delete(aContainer.singleTextChild); > > + aContainer.clearSingleTextChild(); > > + > > + aContainer.setExpanded(true); > > I'm not sure if this should be expanded automatically when an element goes > from single text child to non-single-text child (like if a new child is > added or if the element is emptied out). Maybe only if it's currently > selected and there are more than 0 children Agreed, will do it if selected and more than 0 children. > > @@ +1387,5 @@ > > * Return a list of the children to display for this container. > > */ > > _getVisibleChildren: function(aContainer, aCentered) { > > + // If the backend has told us that there's only one child, just use that child. > > + if (aContainer.node.singleTextChild) { > > Nice, I'm assuming it'll be a perf win to pull the child down in the initial > request and not do it here, since it's going to need to be shown anyway. > OTOH, if we passed down singleTextChild as a boolean and then attached the > child node to the container / front via a call to _getVisibleChildren it > would simplify the server code. But adding extra async stuff in the > frontend could very well end up being messier and more code. Actually this isn't necessary anymore - the current patch doesn't even do a getVisibleChildren if singleTextChild happens. I'll get rid of that, it's unnecessary complexity. > @@ +2536,5 @@ > > + this.textEditor = new TextEditor(this.container, node, "text"); > > + this.elt.insertBefore(this.textEditor.elt, this.elt.firstChild.nextSibling.nextSibling); > > + } > > + > > + if (this.textEditor) { > > Another thing about the text editor: it seems to cause a mutation flash on > the parent node if you double click into it and click out without doing > anything. Oddly, this isn't an issue if you press 'enter' in the textarea That's weird. I'll take a look. > > ::: toolkit/devtools/server/actors/inspector.js > @@ +2723,5 @@ > > > > mutation.removed = removedActors; > > mutation.added = addedActors; > > + > > + if (change.target.childNodes.length == 1 && change.target.childNodes[0].nodeType === Ci.nsIDOMNode.TEXT_NODE) { > > This doesn't handle the case where I highlight one of these elements and > then run `$0.innerHTML = ""`. Which is weird because it looks like it would ok, will take a look at that too.

(In reply to Dave Camp (:dcamp) from comment #39) > (In reply to Brian Grinstead [:bgrins] from comment #38) > > Another thing about the text editor: it seems to cause a mutation flash on > > the parent node if you double click into it and click out without doing > > anything. Oddly, this isn't an issue if you press 'enter' in the textarea > > That's weird. I'll take a look. OK, it looks like what's happening here is: When you click away we apply changes. We send a mutation even when the new text is identical. This triggers a mutation flash. Selection overrides mutation flash, so you don't see it when you press enter (leaving the item selected). As far as I can tell, this is the current behavior without this patch. Let's handle it in a different bug. a) We suppress the > > > > > ::: toolkit/devtools/server/actors/inspector.js > > @@ +2723,5 @@ > > > > > > mutation.removed = removedActors; > > > mutation.added = addedActors; > > > + > > > + if (change.target.childNodes.length == 1 && change.target.childNodes[0].nodeType === Ci.nsIDOMNode.TEXT_NODE) { > > > > This doesn't handle the case where I highlight one of these elements and > > then run `$0.innerHTML = ""`. Which is weird because it looks like it would > > ok, will take a look at that too.

This is also breaking anonymous content. We probably shouldn't set singleTextNode if there is anonymous content. Is there a quick way to check if a node has anonymous content?

(In reply to Dave Camp (:dcamp) from comment #41) > This is also breaking anonymous content. > > We probably shouldn't set singleTextNode if there is anonymous content. Is > there a quick way to check if a node has anonymous content? Depending on exactly what you want, LayoutHelpers.isAnonymous(childNode) on the single child node should do the trick. This would mean this feature won't work at all once we've entered into any anonymous content, but that's probably alright. Something more aggressive would be to check !!LayoutHelpers.getBindingParent(childNode) [isAnonymous]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/LayoutHelpers.jsm#538 [getBindingParent]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/LayoutHelpers.jsm#516

I'll actually ask for review for this one, since tests are updated (though I haven't done a full try run yet). But I don't like how the "has single text child" check is implemented on the server anymore. Because of anonymous content it feels kinda blah. So if you can think of a better way to detect nodes with anonymous content let me know, otherwise this is as best as I know how to do it.

Obviously I wasn't reading my bugmail while I was working. Will upload a new version with your suggestion after some sleep.

(In reply to Brian Grinstead [:bgrins] from comment #42) > (In reply to Dave Camp (:dcamp) from comment #41) > > This is also breaking anonymous content. > > > > We probably shouldn't set singleTextNode if there is anonymous content. Is > > there a quick way to check if a node has anonymous content? > > Depending on exactly what you want, LayoutHelpers.isAnonymous(childNode) on > the single child node should do the trick. I understand the problem better now after seeing the patch. I think the way you've done it is the right approach - the walker is the right place to consult about anonymous children. Getting them in other ways would be tricky, and error-prone since the walker is configured to know about what types of anonymous content we care about.

Comment on attachment 8603697 [details] [diff] [review] v3 Review of attachment 8603697 [details] [diff] [review]: ----------------------------------------------------------------- Just a couple notes about the server side changes, haven't had a chance to do a full review ::: toolkit/devtools/server/actors/inspector.js @@ +219,5 @@ > return this.rawNode.ownerDocument && > this.rawNode.ownerDocument.documentElement === this.rawNode; > }, > > + getSingleTextChild: function() { I think this is the right idea, but I'd move this function onto the WalkerActor object instead of the node, especially now that it needs to deal with traversal. @@ +224,5 @@ > + if (this.isBeforePseudoElement || this.isAfterPseudoElement) { > + return undefined; > + } > + > + // This feels like a lot of work to have to do to check this. There could be an early return case where this.rawNode.children.length > 0 since we don't need to bother with the code below if there is a child element node.

Fixed previous comments.

Try push with patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=736a010c706b. mochitest-dt seems good, only errors appear to be in oth: 3006 INFO TEST-UNEXPECTED-FAIL | toolkit/devtools/server/tests/mochitest/test_inspector-release.html | Ownership tree should have dropped by 27 nodes - got 3, expected 29 3021 INFO TEST-UNEXPECTED-FAIL | toolkit/devtools/server/tests/mochitest/test_inspector-remove.html | Ownership tree should have dropped by 25 nodes - got 5, expected 31

I'll take a look at the test failures and other things I notice in review and then update it and send it over to Patrick for a final review

Updated some of the tests along with various small changes throughout the code. Try is currently closed but based on the last push it should be pretty close.

Comment on attachment 8604456 [details] [diff] [review] markupview-auto-expand.patch Review of attachment 8604456 [details] [diff] [review]: ----------------------------------------------------------------- Some general remarks: - Works great! And looks pretty nice, the markup-view is more compact like this. - There's a bug when text has an ellipsis, the ellipsis is supposed to go away when entering edit mode and the full text should be displayed instead. This doesn't happen for inlined single text nodes (related to this, but not to your patch: entering/leaving edit mode on non-inlined text nodes makes the text display in full afterwards, the ellipsis doesn't come back). - Single-clicking on an inlined text node doesn't focus it (focus ring goes to the tagname of the parent node instead). You can fix this by replacing: target.closest(".open [tabindex]") with target.closest(".editor [tabindex]") in MarkupContainer.prototype._onMouseDown Other than this, I only have a couple of minor remarks about the code below. Once the full-text in edit mode is fixed and try is green, we should land this. ::: toolkit/devtools/server/actors/inspector.js @@ +760,5 @@ > + this.singleTextChild = > + types.getType("domnode").read(form.singleTextChild, ctx); > + } else { > + this.singleTextChild = undefined; > + } I somehow would find this more suited as a getter, next to all the other NodeFront getters we have that already deal with accessing things from the form. get singleTextChild() { if (form.singleTextChild) { return types.getType("domnode").read(form.singleTextChild, ctx); } } Is 'cx' we can find out from outside the form method? If there's a performance concern with calling read everytime, we could always continue caching the result but still using the getter. Another advantage is that WalkerFront.getMutations could be simplified to just delete the cached version when change.singleTextChild is truthy, instead of having to basically duplicate the code here. Anyway, this is a minor remark, won't prevent this from landing as is. @@ +1477,5 @@ > + node.rawNode.children.length > 0) { > + return undefined; > + } > + > + let docWalker = this.getDocumentWalker(node.rawNode); Brian would know better than I, but do we really need to instantiate a doc walker for this? Wouldn't this be enough: if (node.rawNode.childNodes.length === 1&& node.rawNode.firstChild.nodeType === Ci.nsIDOMNode.TEXT_NODE) { return this._ref(node.rawNode.firstChild); }

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #51) > @@ +1477,5 @@ > > + node.rawNode.children.length > 0) { > > + return undefined; > > + } > > + > > + let docWalker = this.getDocumentWalker(node.rawNode); > > Brian would know better than I, but do we really need to instantiate a doc > walker for this? > Wouldn't this be enough: > > if (node.rawNode.childNodes.length === 1&& > node.rawNode.firstChild.nodeType === Ci.nsIDOMNode.TEXT_NODE) { > return this._ref(node.rawNode.firstChild); > } Unfortunately we do, since the documentWalker knows about extra nodes to include (anonymous content) and nodes to exclude (whitespace only text nodes). It is "the place" to consult with what is going to show up in the markup tree.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #51) > ::: toolkit/devtools/server/actors/inspector.js > @@ +760,5 @@ > > + this.singleTextChild = > > + types.getType("domnode").read(form.singleTextChild, ctx); > > + } else { > > + this.singleTextChild = undefined; > > + } > > I somehow would find this more suited as a getter, next to all the other > NodeFront getters we have that already deal with accessing things from the > form. > > get singleTextChild() { > if (form.singleTextChild) { > return types.getType("domnode").read(form.singleTextChild, ctx); > } > } > > Is 'cx' we can find out from outside the form method? > > If there's a performance concern with calling read everytime, we could > always continue caching the result but still using the getter. > Another advantage is that WalkerFront.getMutations could be simplified to > just delete the cached version when change.singleTextChild is truthy, > instead of having to basically duplicate the code here. > > Anyway, this is a minor remark, won't prevent this from landing as is. ctx in this case is the WalkerFront (which could be attached to the front for access from a getter). I put together a patch that makes a change like this, but since ctx isn't being passed into the initializer it seems like there could be a case where this._ctx is wrong (if for some reason form() hasn't been called or of form() was called with a different context). I doubt this would ever be the case, but let's treat this as follow up material that we can discuss in more detail separately.

This should address the previous comments. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c1294b496c2

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8604456&action=interdiff&newid=8604872&headers=1

Comment on attachment 8604872 [details] [diff] [review] markupview-auto-expand.patch Review of attachment 8604872 [details] [diff] [review]: ----------------------------------------------------------------- Those last changes look good to me, try seems green, and my local tests worked fine, r=me. Just a couple of remarks. ::: browser/devtools/markupview/markup-view.js @@ +2444,5 @@ > } > > ElementEditor.prototype = { > > + set selected(aValue) { I failed to see where this was used. ::: browser/devtools/markupview/test/browser_markupview_textcontent_edit_01.js @@ +25,5 @@ > + selector: "#node17", > + newValue: "New text", > + oldValue: "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec posuere placerat magna et imperdiet.", > + shortValue: true > + }); We should probably test right after this that selecting another node makes the shortValue on #node17 come back.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #56) > Comment on attachment 8604872 [details] [diff] [review] > markupview-auto-expand.patch > > Review of attachment 8604872 [details] [diff] [review]: > ----------------------------------------------------------------- > > Those last changes look good to me, try seems green, and my local tests > worked fine, r=me. > Just a couple of remarks. > > ::: browser/devtools/markupview/markup-view.js > @@ +2444,5 @@ > > } > > > > ElementEditor.prototype = { > > > > + set selected(aValue) { > > I failed to see where this was used. > This is needed to get the ellipses to expand on the text editor. So when you click on the element (that contains the singleTextChild), the element container is selected: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#1171, which in turn sets selected on it's editor: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#1956. The logic to expand the ellipses happens in the TextEditor's selected setter, so we need to pass along that value to the element's text editor (if available). This logic is now covered in the browser_markupview_textcontent_edit_01.js test by making sure the ellipses are expanded when selecting a node with a single text child. > ::: > browser/devtools/markupview/test/browser_markupview_textcontent_edit_01.js > @@ +25,5 @@ > > + selector: "#node17", > > + newValue: "New text", > > + oldValue: "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec posuere placerat magna et imperdiet.", > > + shortValue: true > > + }); > > We should probably test right after this that selecting another node makes > the shortValue on #node17 come back. Yeah, I'll add that

Ready for checkin

We're going to uplift this to 40, right?

Comment on attachment 8605524 [details] [diff] [review] markupview-auto-expand.patch Approval Request Comment [Feature/regressing bug #]: Developer Edition 40.1 June release. [User impact if declined]: The markup view is much less compact. This combines nicely with Bug 1046803 (already in 40) to make the markup view more scannable and compare well with other devtools. [Describe test coverage new/current, TreeHerder]: There's test coverage for this change, and lots of existing test coverage for the inspector and markup view [Risks and why]: The risk is contained in that it only affects the inspector panel inside of devtools. It does introduce a new concept in the inspector backend and frontend, but we're pretty confident that it won't cause problems since it's been baking in nightly for a week and we haven't had any issues come up [String/UUID change made/needed]: