156072 - DOM inspector omits the #document node

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Håkan Waara]

The DOM inspector doesn't seem to show the mandatory #document node (i.e., the 
document root) as the parent of all nodes in the DOM inspector.

If the DOM inspector is supposed to match reality, I think it should show the 
document root too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Just to clarify the bug, the document is the parent of the root element and any
PIs in the XML prolog are siblings of the root element and children of the document.

Comment 2

[Håkan Waara]

Attachment: Patch

Here's a patch to show the document root (#document) for all documents. 

#comment nodes are green, #text nodes blue, and I saw no nodeTypes that used
purple so now #document does...

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Index: resources/skin/classic/viewers/dom/dom.css

>+treechildren:-moz-tree-cell-text(DOCUMENT_NODE) {
>+  color: purple;
>+}

So if everything else is using #rrggbb syntax, you should follow suit.

Comment 4

[Håkan Waara]

Attachment: Patch that doesn't touch the color codes

This patch also includes a fix to bug 156074.

Comment 5

[Håkan Waara]

To clarify even more, I think that this patch also fixes the rather more 
*serious* bug that PIs at the same 'level' as the root element are not shown in 
DOM inspector.

I'm going to test this now, and per request from Boris attach my testcases 
before I hunt for reviews.

Comment 6

[Håkan Waara]

Attachment: XML testcase

Yep, in current Mozilla builds we don't display sibling PIs of the root
element. That is quite serious. With my patch, since we elevate the root node
whose children the DOM inspector will display one level, sibling PIs of the
root element will display!

I used the this testcase to test this in my patched Mozilla build, and Mozilla
1.0.

Reviews?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase testing iframes as well

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 90433 [details] [diff] [review]
Patch that doesn't touch the color codes

>+    var doc = this.mDOMView.rootNode; // The root node is the document root.

"node is the top-level document node", not "node is the document root".

With that minor comment change, r/sr=bzbarsky

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 90433 [details] [diff] [review]
Patch that doesn't touch the color codes

r=caillon

Comment 10

[Scott Collins]

Comment on attachment 90433 [details] [diff] [review]
Patch that doesn't touch the color codes

a=scc for checkin to the mozilla trunk

Comment 11

[Håkan Waara]

->moi

Comment 12

[Håkan Waara]

klart och fixat.

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Just for the record, the checkin for this caused bug 158792.  Do we have a
zt4newregressions keyword yet?

Comment 14

[Boris Zbarsky [:bzbarsky]]

I've backed out this fix to fix bug 158792.  Reopening.  See bug 158792 comment
4 for some of the issues involved.

Comment 15

[Alex Vincent [:WeirdAl]]

Something interesting about this bug:  When you set the left panel to JavaScript
object, the Inspector pulls up the Document node for the root of the tree...

If I have time I'll investigate the issues involved -- retrying a version of
hwaara's patch and then debugging from there.

Comment 16

[Alex Vincent [:WeirdAl]]

I'm having a lot of "fun" with this bug.

The original issuess bug 158792 was filed for appear to have gone away as of
20030120 (the build I'm working with, via Patch Maker).  The tree does update,
and the Select Element By Click (the little magnifier icon) does work too, with
hwaara's patch.

bz's comment that the left-panel tree isn't live is partially correct; you can
execute a command on the left panel which modifies the tree.  Then collapse the
affected node's parent element in the tree and re-expand it, and the correct
tree appears.

However, I'm discovering a few other factors that make life interesting.

(1) The availability of viewers for a particular node type isn't determined in
any chrome file.  According to chrome://inspector/content/ViewerRegistry.js ,
the availability is really determined from an RDF file,
resource:///res/inspector/viewer-registry.rdf .  Naturally, that's not a file
Patch Maker handles.  (I can work around that.)

(2) For me to lock the DOM Nodes viewer (dom.xul) into the left panel only and
the DOM Node viewer (domNode.xul) into the right panel only, I need to determine
which domi-panel node is calling .registry.findViewersForObject() method
(chrome://inspector/content/inspector.xml#panel under setSubject()).  I need
some major help doing that.

(3) Of course, the DOM Node viewer isn't equipped to handle Document or
ProcessingInstruction nodes (bug 201129)...

I'm beating my head on a brick wall on (2).  My suspicion is the left panel for
dom.xul will go fully live again once we disable it for the right panel.

hwaara:  if you want to reassign the bug to me, do so.

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK, so I just talked to Alex for a bit.  This comment summarizes the conversation.

The situation is that when an object is selected in the left panel, it's set as
the subject for the right panel.  When a panel's subject is set, its type
determines which panelset is instantiated.

In particular, the default panel for Document objects is dom.xul (the node
tree).  The alternates are the stylesheet view, etc.  Which is why the node tree
shows up on the right when a #document is selected on the left.

It would be possible to change Document objects to instantiate domNode.xul
instead of dom.xul.  We would still need a way to trigger dom.xul, however; my
suggestion was to do this with the Window object.

At this point, Alex remarked that this would cause a problem with the stylesheet
view -- if we only show the #document views on the right, the main stylesheet
view also only comes up on the right, which is bad.

So the stylesheet view should really hang off the same object as dom.xul does. 
We _could_ hang it off the Window object.  This would not allow easy viewing of
sheets inside iframes, but we don't have that right now anyway...

Comment 18

[Alex Vincent [:WeirdAl]]

Attachment: Notes using bz's suggested approach, including pieces of a patch

I believe a working patch can be assembled from this notes section.  (That
means I have a working version on my Patch Maker build, but not a coherent
patch.	The relevant file changes are included here.)

I think the left-side panel for dom.xul is live again.	At least I didn't
notice anything particularly wrong with it this time around.  (But I'll recheck
it.)

A couple other things:	when the left panel is set to JSObject, the right panel
doesn't update, ever.  I believe this is due to an event not being dispatched
(this.mObsMan.dispatchEvent("something")) in JSObject that other panels, such
as dom.xul, do dispatch.  I'll look it up and probably include a fix for that
in bug 193942.

Comment 19

[timeless]

Attachment: attachment 119893 applied to cvs

attachment 119893 [details] applied to cvs

Comment 20

[Alex Vincent [:WeirdAl]]

chrome://inspector/skin/viewers/dom/dom.css

I need to add a color code for DOCUMENT_TYPE_NODE nodes.  No need to file a new
bug for that, or to edit/obsolete the patch.  I'll just create a new patch
specifically for the CSS files.

Comment 21

[Alex Vincent [:WeirdAl]]

Comment on attachment 120026 [details] [diff] [review]
attachment 119893 [details] applied to cvs

In the meantime, I might as well set my r?/sr? flags.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120026 [details] [diff] [review]
attachment 119893 [details] applied to cvs

>Index: mozilla/extensions/inspector/resources/content/viewers/dom/dom.js
>-      this.selectElementInTree(aObject.documentElement, true);
>+      this.selectElementInTree(aObject.document.documentElement, true);

Why?  Why not select the document node itself?

>Index: mozilla/extensions/inspector/resources/content/viewers/jsObject/jsObject.js
>-    var ti = this.addTreeItem(this.mTreeKids, "target", aObject, aObject);
>+    if (aObject instanceof Components.interfaces.nsIDOMWindow) {
>+      var ti = this.addTreeItem(this.mTreeKids, "target", aObject.document, aObject.document);
>+    } else {
>+      var ti = this.addTreeItem(this.mTreeKids, "target", aObject, aObject);
>+    }

Why?  Why not have a JS object view for the window?

I assume problem #1 and problem #2 from bug 158792 comment 0 do not arise with
this patch?

Or did you test them?  I'm not sure how #1 could be solved given bug 158792
comment 4...

Does this crash when showing anonymous content is enabled and a document has no
kids?  See the patch in bug 158792 first hunk (at least add some bail-out code
there!)

Comment 23

[Alex Vincent [:WeirdAl]]

>Why?  Why not select the document node itself?

I thought about selecting the document node in dom.xul, but perhaps we shouldn't
do that right off the bat.  The selection helps emphasize the document element
node in the original view, and the document node is pretty obvious anyway as the
topmost item in the tree.

It was just a personal preference, but I felt it might be more appropriate to
let the document root element be the initial node selected, as it was before. 
If you insist on that, I won't fight it.  8)

>Why?  Why not have a JS object view for the window?

Consistency, in this case.  Reference comment 15 of this bug:  currently, the
left panel uses the document element for dom.xul, the document for
stylesheets.xul and jsObject.xul -- it's far better to use the same node for the
left panel in all three viewers.  Otherwise, the user is going to be confused. 
(In other words, I silently fixed a design bug at the same time.)

>I assume problem #1 and problem #2 from bug 158792 comment 0 do not arise with
>this patch?

>Or did you test them?  I'm not sure how #1 could be solved given bug 158792
>comment 4...

Actually, when I tested hwaara's original patch on my 20030120 build, those
issues were no longer present.  See comment 16 of this bug, which occurred
before your suggestion of changing the aObject values sent to the various
viewers.  I specifically reproduced hwaara's patch exactly, and spent a half
hour testing and retesting what was reported.  It just wasn't there.

>Does this crash when showing anonymous content is enabled and a document has no
kids?  See the patch in bug 158792 first hunk (at least add some bail-out code
there!)

I wasn't aware of a crash bug in that case, and I can't hack C++ on a Patch
Maker build.  But I'll take your suggested testcase home and try it out.

Comment 24

[Alex Vincent [:WeirdAl]]

I should clarify that as regard bug 158792 comment 4, the live tree apparently
did revert to the left, once I eliminated it as a possibility on the right.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120026 [details] [diff] [review]
attachment 119893 [details] applied to cvs

"odd".

Well, if it works, sr=me, pending testing of that crash possibility.  ;)

Comment 26

[Alex Vincent [:WeirdAl]]

I was wrong about the left-panel tree being live.  But I believe I have
discovered the cause.

The inIDOMView interface works fine when you set rootNode to an Element node. 
But set it to a Document node and you are screwed.

It is the only possible explanation.  I added in the following code to dom.js
DOMViewer() function:

  this.mDOMTree.addEventListener("DOMNodeRemoved", function() {
    alert("Node Removed!");
}, true);

What this means is that whenever a node is removed from the XUL tree via the
Document Object Model, an alert would be fired.  I tested this in an XUL tree I
had constructed; the alert came up normally.  When I added this code into
Inspector and then removed a node from the document via the "Delete..." context
menu item... no alert.  The event listener did not fire.

JavaScript cannot by itself remove a node from the DOM without firing a
DOMNodeRemoved event through the ancestor nodes.  A C++ implementation is
interfering.  The only C++ implementation involved in the tree is inIDOMView.

inIDOMView removed the treeitem node representing the removed node from the tree
without using the Document Object Model.

Also, bz was right about the need for the #document node to be selected in
dom.xul, but for the wrong reason.  If you change viewers in the left panel,
from JS Object to dom.xul, you should reasonably expect the right panel to lock
in on the Document node.

helpwanted:  the patch causes a partial breakage of
http://lxr.mozilla.org/mozilla/source/extensions/inspector/base/src/inDOMView.cpp 

Tree no longer updates automatically with this patch as it should.  Somehow it
works when rootNode is an element, but when rootNode is a document node, it
doesn't update automatically (you have to collapse a parent node and then
re-expand it to see the update).

Comment 27

[Alex Vincent [:WeirdAl]]

:) Crash bz noted seems to be gone as of 20030404 builds...

Comment 28

[Alex Vincent [:WeirdAl]]

Attachment: Classic and Modern skin patch, concurrent with attachment 120026.

Comment 29

[Alex Vincent [:WeirdAl]]

Comment on attachment 120141 [details] [diff] [review]
Classic and Modern skin patch, concurrent with attachment 120026 [details] [diff] [review].

I think that's a nice color...

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120141 [details] [diff] [review]
Classic and Modern skin patch, concurrent with attachment 120026 [details] [diff] [review].

sr=me if it looks fine.  ;)

Comment 31

[Alex Vincent [:WeirdAl]]

re comment 26, bz's working on it.  Bug 201577.

Comment 32

[Alex Vincent [:WeirdAl]]

I'll take this.  It's about to be fixed, and it'll hang on my head if anything
regresses.

Comment 33

[Alex Vincent [:WeirdAl]]

*sigh* Attachment 120026 [details] [diff] causes a regression in sidebar Inspector.

Error: this.mDocPanel has no properties
Source File: chrome://inspector/content/sidebar.js
Line: 107

We can't check in while that's in effect.  

Comment 34

[Alex Vincent [:WeirdAl]]

This is interesting.  It seems sidebar.js never defines this.mDocPanel; it 
doesn't have a initViewerPanels() method (inspector.js, line 120), and the 
setTargetDocument() methods are different (sidebar.js, line 110; inspector.js, 
line 339).

Not one checkin to sidebar.js has happened since Nov 14, 2001.  Ten separate 
changes to inspector.js have happened since then.  (I just went over all ten; 
none of them appear related to the sidebar edition of Inspector.)

Looks like hewitt simply forgot about setting mDocPanel in his revival of 
Inspector back then.

I think (I'll have to wait until tomorrow to be sure) sidebar.js will need 
mutations very similar to those for inspector.js in attachment 120026 [details] [diff] [review], and also 
the initViewerPanels() method of inspector.js.  Line 125 of sidebar.js does 
what lines 128 and 130 of inspector.js do:  set the target document of the 
inspector object.

I don't see how the sidebar inspector object uses the mDocPanel property except 
to get the document.  So anytime we call inspector.document on the sidebar, 
we'd get that exception.  There's no unusual scripts attached to sidebar; if 
anything, inspectorOverlay.xul has lots more scripts, and inspector.js sets 
inspector.mDocPanel anyway.

I think that covers all the bases; I'll have to wait until tomorrow to be 
sure...

Comment 35

[Alex Vincent [:WeirdAl]]

Attachment: Sidebar patch, concurrent with attachment 120026

This is the third patch for the same bug, and all three patches go together. 
Since no patch changes files in any other patch, there are no collisions, and
probably no need to combine them all into a single patch.  This patch fixes the
sidebar regression.

Of course, if someone wants to post a single patch, that's optional.

Comment 36

[Alex Vincent [:WeirdAl]]

Comment on attachment 121024 [details] [diff] [review]
Sidebar patch, concurrent with attachment 120026 [details] [diff] [review]

Oops.

>+/*
>+    if (this.mInitTarget) {
>+      if (this.mInitTarget.nodeType == Node.DOCUMENT_NODE)
>+        this.setTargetDocument(this.mInitTarget);
>+      else if (this.mInitTarget.nodeType == Node.ELEMENT_NODE) {
>+        this.setTargetDocument(this.mInitTarget.ownerDocument);
>+        this.mDocPanel.params = this.mInitTarget;
>+      }
>+      this.mInitTarget = null;
>+    }
>+*/

...

>+/*
>+      case "subjectChange":
>+        if (aEvent.target == this.mDocPanel.viewer &&
>+            aEvent.subject && "location" in aEvent.subject) {
>+          this.locationText = aEvent.subject.location; // display document url
>+        }
>+*/

Forgot to remove those commented sections.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121024 [details] [diff] [review]
Sidebar patch, concurrent with attachment 120026 [details] [diff] [review]

sr=me, but please don't check in those commented-out sections.

Comment 38

[Alex Vincent [:WeirdAl]]

When I have a chance I'll combine all three patches into one and transfer bz's 
collective sr= into them.  (I had some trouble checking them into my Patch 
Maker build; it's possible, but unlikely, that something changed.  If it did, 
I'll point it out to bz at that time.)

Comment 39

[Alex Vincent [:WeirdAl]]

Attachment: Combined patch for Inspector

Comment 40

[Alex Vincent [:WeirdAl]]

Comment on attachment 121432 [details] [diff] [review]
Combined patch for Inspector

bz has already given sr= on the pieces of this patch; there are no changes
(aside from removing the comments).

Comment 41

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 121432 [details] [diff] [review]
Combined patch for Inspector

r=caillon provided there's been some good testing done here.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121432 [details] [diff] [review]
Combined patch for Inspector

Actually, I'd like to take a look at the whole thing together, so resetting
review flag...	This is targeted for 1.5a so I should have time for that,
right?

Comment 43

[Christopher Hoess (gone)]

I believe this dependency is correct...it probably wouldn't hurt to check this
testcase against the patch in bug 154449.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Note that nothing Inspector does will really fix bug 154449...

Comment 45

[Alex Vincent [:WeirdAl]]

I think I've arrived at the wrong solution: see bug 193942 comment 23, and this
bug, comments 17 and 18, for details.  I'm not going to obsolete the patch yet,
though.

Comment 46

[Alex Vincent [:WeirdAl]]

Solution:
(1) inspector.xml#panel, line 556, pass this.id as an argument (the panel's id
attribute)
(2) ViewerRegistry.js, line 150 passes on id attribute to objectMatchesEntry.
This last function, and probably viewer-registry.rdf, will need to be altered. 
The RDF file will need to have valid id values added for each panel, I think.

Comment 47

[Alex Vincent [:WeirdAl]]

More specifically:

inspector.xml, approx. line 556:
this.mSubject = aObject;
this.mParams = null;
           
// get the list of viewers which match the node
var entries = this.registry.findViewersForObject(aObject, this.id);
this.rebuildViewerList(entries);
 
if (entries.length == 0) {
  this.switchViewer(-1);
// ...

For ViewerRegistry.js:

findViewersForObject: function(aObject, panelId)
{
  // check each entry in the registry
  var len = this.mViewerDS.length;
  var entry;
  var urls = [];
  for (var i = 0; i < len; ++i) {
    if (this.getEntryProperty(i, "panels").indexOf(panelId) == -1) {
      continue;
    }
    if (this.objectMatchesEntry(aObject, i)) {
      if (this.getEntryProperty(i, "important")) {
        urls.unshift(i); 
      } else {
        urls.push(i);
      }
    }
  }

  return urls;
},
// ...

For viewer-registry.rdf:
<rdf:li><rdf:Description 
  ins:uid="jsObject"
  ins:panels="bxDocPanel bxObjectPanel bxObjPanel"
  ins:description="Javascript Object"
  ins:filter="return true;"/>
</rdf:li>

We need all three for jsObject viewer because sidebar uses bxObjPanel instead 
of bxObjectPanel, and jsObject should be available to both the left- and right-
hand panels.  Other viewers will have two or one of these panel id values, so 
as to restrict which panels will see the viewer.

Comment 48

[Alex Vincent [:WeirdAl]]

Attachment: New combined patch for Inspector

When in doubt, pass more arguments.

Comment 49

[Mozilla_rocks]

Comment on attachment 120026 [details] [diff] [review]
attachment 119893 [details] applied to cvs

removing obsolete review request

Comment 50

[Mozilla_rocks]

Comment on attachment 120141 [details] [diff] [review]
Classic and Modern skin patch, concurrent with attachment 120026 [details] [diff] [review].

removing obsolete review request

Comment 51

[Mozilla_rocks]

Comment on attachment 121024 [details] [diff] [review]
Sidebar patch, concurrent with attachment 120026 [details] [diff] [review]

removing obsolete review request

Comment 52

[Alex Vincent [:WeirdAl]]

caillon: I need your help.  Somehow, this patch when applied breaks the hiding
of whitespace text nodes.

Comment 53

[Alex Vincent [:WeirdAl]]

Disregard comment 52.  I was seeing bug 111411 again.  Patch is good for reviews.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Attachment: Same diff, but applies.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126212 [details] [diff] [review]
Same diff, but applies.

>Index: extensions/inspector/resources/content/ViewerRegistry.js

>   // @param Object aObject - the object being searched against
>   // @return nsIRDFResource[] - array of entries in the viewer registry

You need to add an @param aPanelId line here and clearly document exactly what
it is, what id does, and what one eats it with.  Or more to the point document
what should get passed and what will go wrong if people screw it up.

>Index: extensions/inspector/resources/content/res/viewer-registry.rdf

Please add a nice comment here documenting what all the ins:* fields to,
especially ins:panels (since I can't quite figure it out for sure even with you
describing it).

The other changes look good.  sr=me with those comments added.

Comment 56

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 126212 [details] [diff] [review]
Same diff, but applies.

Okay, I was going to test this before officially stamping this, but since bz
already did a solid job of doing that, I'll just go ahead and stamp it.  I'm
fine with the changes.	The only other comment I have really is:

>+  onEvent: function(aEvent)
>+  {
>+    switch (aEvent.type) {
>+      case "panelsetready":
>+        this.initViewerPanels();
>+        break;
>+    }

A switch may be useful later on if we get more event types, but for now, I
think an if() will suffice.

r=caillon

Comment 57

[Alex Vincent [:WeirdAl]]

Attachment: final

updates patch to meet r=caillon, sr=bz requirements

Comment 58

[Alex Vincent [:WeirdAl]]

fixed by bz's checkin of attachment 126213 [details] [diff] [review].  Thanks to caillon, bz for r=, sr=
for 1.5a.