201129 - DOM Node viewer doesn't support all node types

Status: RESOLVED INCOMPLETE

(Other Applications :: DOM Inspector, defect)

Description

[Alex Vincent [:WeirdAl]]

While examining issues for bug 156072, I discovered the DOM Node viewer doesn't
handle the various node types very well.  It has a panel for Element nodes, and
dumps every other node type into a Text node panel.

This works for several node types, but not all.

ELEMENT_NODE  ok
ATTRIBUTE_NODE n/a
TEXT_NODE ok
CDATA_SECTION_NODE ok
ENTITY_REFERENCE_NODE n/a
ENTITY_NODE n/a
PROCESSING_INSTRUCTION_NODE broken
COMMENT_NODE ok
DOCUMENT_NODE broken
DOCUMENT_TYPE_NODE broken
DOCUMENT_FRAGMENT_NODE n/a (probably)
NOTATION_NODE n/a

The fix will be fairly simple, and will convert to using deck.selectedPanel
instead of deck.selectedIndex to set the correct node type display.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

Lots of new views, particularly for Document and DocumentType.	For Document, I
specifically added the option of HTMLDocument, NSHTMLDocument, and
XULDocument-specific information.  Should be nice.  About the only
consideration I'm unsure of what to put in the panel when I have an XMLDocument
with no doctype (typical for XUL documents).  Right now it just shows a blank
panel.

Comment 2

[Jesse Ruderman]

Alex tells me this patch makes it so the "Node Type" textbox only appears for
Element nodes.  It should either be removed from Element's DOM Node panel, or it
should be added to the other DOM Node panels.

I think it would be best to add "Node Type" to all DOM Node panels, even though
the left panel uses different colors for different node types, because
* Not everyone knows that the colors indicate different node types.
* Not everyone knows what each color means.
* Accessibility (don't rely on color alone).
* It would make it clear why the rest of the panel looks so different for Text
nodes and Element nodes.

The textbox should show a name ("ELEMENT_NODE", etc) in addition to or instead
of a number ("1", etc).

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2

bz came up with nsINSHTMLDocument for a few features for generic documents. 
Included here.

I disagree with jruderman in his suggestion for adding node type entries to
each field; there's a node type column available in the left panel under DOM
Nodes (dom.xul).  I simply removed the node type and node value fields for
Element (elements have null node values).

Comment 4

[Jesse Ruderman]

Alex: the nodeType column is not shown by default and shows a number rather than
a name.  Also, showing multiple columns in a deeply nested tree tends to suck
(cf bug 201832).  An ugly, non-default option is not a good excuse for lack of
clarity in the default setup.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Note that I've always been opposed to displaying integers in the nodeType
column.  See bug 113793 to which I finally caved in.  I might retract that now
since I do agree that the constants are much more usable than integers.  Maybe a
new bug for that would be in order?

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 121603 [details] [diff] [review]
patch, v2

Alex, can you please clean this patch up by actually removing stuff you are
commenting out?  You have quite a few places with that stuff going on.	I also
agree with jesse in that we should be consistent.  Either show it or not for
all elements.  I lean toward showing because whether or not they are null, all
nodes are expected to have a nodeValue.

Comment 7

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2.1

answering caillon's and jruderman's concerns.

Comment 8

[Alex Vincent [:WeirdAl]]

Comment on attachment 122329 [details] [diff] [review]
patch, v2.1

For some reason, the text of the caption elements isn't showing through.  Oops.

>+    <vbox id="bxDocument" scroll="vertical">
>+      <groupbox id="document_doctype">
>+        <caption>Document Type</caption>

+	 <caption label="Document Type"/>

>+      </groupbox>
>+
>+      <groupbox id="document_all">
>+        <caption>Basic Document Information</caption>

+	 <caption label="Basic Document Information"/>

...

>+      <groupbox id="document_HTML" display="none">
>+        <caption>HTML Document Features</caption>

+	 <caption label="HTML Document Features"/>

...

>+      <groupbox id="document_XUL" display="none">
>+        <caption>XUL Document Features</caption>

+	 <caption label="XUL Document Features"/>

Comment 9

[Brian Ryner (not reading)]

I'm not clear on what your last comment means -- did you find a problem with
your most recent patch, or is it ready for review?

Comment 10

[Alex Vincent [:WeirdAl]]

Just a tiny boo-boo.  The patch is ready for review; the amendment above is
minor enough where I didn't think it appropriate to post another attachment. 
(Chris has lectured me about bugs I am assigned to, with several attachments.)

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 122329 [details] [diff] [review]
patch, v2.1

This needs to be localized

Comment 12

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2.2

Localized.  Also a couple minor tweaks.

Comment 13

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2.3

CVS-compatible patch from Patch Maker 2.0.

Comment 14

[Alex Vincent [:WeirdAl]]

Comment on attachment 126337 [details] [diff] [review]
patch, v2.3

Due to fix for bug 156072, this bug now has high visibility.

Comment 15

[Alex Vincent [:WeirdAl]]

Comment on attachment 126337 [details] [diff] [review]
patch, v2.3

Breakage switching from one #document node to another (made possible by bug
201585).  Caused by bug 197427.  Workaround coming up.

Comment 16

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2.4

Fixed bustage switching from one #document to another #document, and from
#documentType to #documentType.

Comment 17

[Alex Vincent [:WeirdAl]]

Comment on attachment 126340 [details] [diff] [review]
patch, v2.4

Per discussion with caillon, I've made a few changes to my patch to date:

>+    this.setTextValue("nodeName", aObject.nodeName ? aObject.nodeName : "(null)");
>+    this.setTextValue("namespace", aObject.namespaceURI ? aObject.namespaceURI : "(null)");
>+    this.setTextValue("nodeType", nodeTypeToText(this.mSubject.nodeType));
>+    this.setTextValue("nodeValue", aObject.nodeValue ? aObject.nodeValue : "(null)");

I've removed the possibility for (null) entries; this now shows the current
nodeValue converted to a string.

>+    <vbox id="bxDocument" scroll="vertical">
+    <vbox id="bxDocument" style="overflow: -moz-scrollbars-vertical;">

>+            if (this.documentHTMLNames[i] in aObject) {
>+              node.removeAttribute("display");
>+              this.setTextValue("document_" + this.documentHTMLNames[i], aObject[this.documentHTMLNames[i]]);
>+            } else {
>+              node.setAttribute("display", "none");
>+            }

Now, I use node.removeAttribute("style"); and node.setAttribute("style",
"display:none;").

>+
>+*[display="none"] {
>+  display: none;
>+}
>+
>+*[scroll="vertical"] {
>+  overflow: -moz-scrollbars-vertical; 
>+}

This part is gone.  inspector.css is no longer part of my patch.

caillon asked me not to post a new patch just yet.

Comment 18

[[e-mail]]

Comment on attachment 126340 [details] [diff] [review]
patch, v2.4

Looks like I bitrotted this :-)

>+    this.setTextValue("nodeName", aObject.nodeName ? aObject.nodeName : "(null)");
Could use aObject.nodeName || "(null)" etc. here.

>+            if (this.documentHTMLNames[i] in aObject) {
>+              node.removeAttribute("display");
>+              this.setTextValue("document_" + this.documentHTMLNames[i], aObject[this.documentHTMLNames[i]]);
>+            } else {
>+              node.setAttribute("display", "none");
>+            }
We have a boolean property, no less, called node.hidden, to do this.

Comment 19

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2.5

Bitrot fixed.

neil: caillon rejected the ?: "(null)" bit, so that has been removed.

As for the hidden property... I see it in JSObject panel, and a quick check
shows we apparently inherit it from nsXULElement.  Setting it to true means the
C++ code uses DOM to set a "hidden" attribute's value to "true", which in turn
calls on the global XUL CSS stylesheet to set display:none.

Logically speaking, that is the same as what I tried to do in patch v2.4, but
using the inspector.css stylesheet instead.  The only difference is the
attribute is set from C++ instead of JS.  I think I'm better off just setting
the style attribute as appropriate.

Comment 20

[Alex Vincent [:WeirdAl]]

Comment on attachment 127641 [details] [diff] [review]
patch, v2.5

rejected by caillon based on screenshot.

Comment 21

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2.6

addressing comments by caillon

Comment 22

[[e-mail]]

The advantage of setting .hidden is that XPConnect doesn't have to convert a JS
string to the right sort of C++ string (twice for setAttribute, because the
attribute's name and value are both strings), only for it to get looked up in an
atom table, when the C++ can statically refer to the "hidden" atom.
Also, setting the style attribute just causes added CSS parsing.

Comment 23

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 128806 [details] [diff] [review]
patch, v2.6

>@@ -40,20 +41,21 @@
> * DOMNodeViewer --------------------------------------------
> *  The default viewer for DOM Nodes
> ****************************************************************/
> 
> //////////// global variables /////////////////////
> 
> var viewer;
>-
>+var deck;

I'd rather see this as an object property.

> var gPromptService;
> 
> //////////// global constants ////////////////////
> 
> const kDOMViewCID          = "@mozilla.org/inspector/dom-view;1";
>+const nsIDOMNode           = Components.interfaces.nsIDOMNode;

Don't intermingle contract ids with classes.  Group them separately.

> const kPromptServiceCID    = "@mozilla.org/embedcomp/prompt-service;1";
> 
> //////////////////////////////////////////////////
> 
> window.addEventListener("load", DOMNodeViewer_initialize, false);
> 
> function DOMNodeViewer_initialize()
>@@ -83,14 +85,16 @@
>   ////////////////////////////////////////////////////////////////////////////
>   //// Initialization
>   
>   mDOMView: null,
>   mSubject: null,
>   mPanel: null,
> 
>+  documentHTMLNames: ["aLinkColor", "vLinkColor", "linkColor", "bgColor", "fgColor", "title", "URL"],
>+

This really, really (yes, really) sucks that we have to be so HTML centric here
in a generic DOM node viewer panel.  I am not really sure what a better
solution would be though.  Perhaps a pref as to what properties for specific
things users want to get, but I don't really like that solution either.  Feel
free to come up with a better one.  In any case, this list does need to be
dynamic.

Ping me on IRC sometime and let's rehash things.

Comment 24

[Alex Vincent [:WeirdAl]]

Comment on attachment 128806 [details] [diff] [review]
patch, v2.6

caillon: what about:

var aPproperty;
for (aProperty in Components.interfaces.nsIDOMNSHTMLDocument) {
  if (typeof aDocument[aProperty] != "function")
    this.setTextValue("document_" + property, aDocument[aProperty].toString());
}
for (aProperty in Components.interfaces.nsIDOMHTMLDocument) {
  if (typeof aDocument[aProperty] != "function")
    this.setTextValue("document_" + property, aDocument[aProperty].toString());
  this.setTextValue("document_" + property, aDocument[aProperty].toString());
}

Or something like that?  That would let us replace most of this:

>+      case nsIDOMNode.DOCUMENT_NODE:
>+        deck.selectedPanel = deck.bxDocumentPanel;
>+
>+        if (aObject instanceof Components.interfaces.nsIDOMHTMLDocument) {
>+          // set HTMLDocument information
>+          deck.bxDocumentPanel.HTMLFields.removeAttribute("style");
>+          for (i = 0; i < this.documentHTMLNames.length; i++) {
>+            node = document.getElementById("tx_document_"+this.documentHTMLNames[i]).parentNode;
>+            if (this.documentHTMLNames[i] in aObject) {
>+              this.setTextValue("document_" + this.documentHTMLNames[i], aObject[this.documentHTMLNames[i]]);
>+            } else {
>+              this.setTextValue("document_" + this.documentHTMLNames[i], "");
>+            }
>+          }
>+        } else {
>+          deck.bxDocumentPanel.HTMLFields.setAttribute("style", "display:none;");
>+        }
>+        break;

Comment 25

[Alex Vincent [:WeirdAl]]

Reassigning DOM-I bugs which have stagnated in my buglist back to default owner.  Hopefully someone will pick up some of these bugs and work on them.  I'll continue to follow them.

Comment 26

[Firefox Bug Husbandry Bot]

Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023

Comment 27

[Firefox Bug Husbandry Bot]

Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023