DOM Node viewer doesn't support all node types

NEW
Unassigned

Status

Other Applications
DOM Inspector
15 years ago
10 years ago

People

(Reporter: WeirdAl, Unassigned)

Tracking

Trunk
x86
Windows 98

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
Created attachment 120148 [details] [diff] [review]
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.
(Reporter)

Updated

15 years ago
Attachment #120148 - Flags: superreview?(bzbarsky)
Attachment #120148 - Flags: review?(timeless)

Comment 2

15 years ago
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).
(Reporter)

Comment 3

15 years ago
Created attachment 121603 [details] [diff] [review]
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).
Attachment #120148 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #121603 - Flags: superreview?(bzbarsky)
Attachment #121603 - Flags: review?(caillon)
Attachment #120148 - Flags: superreview?(bzbarsky)
Attachment #120148 - Flags: review?(timeless)

Comment 4

15 years ago
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.
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 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.
Attachment #121603 - Flags: superreview?(bzbarsky)
Attachment #121603 - Flags: review?(caillon)
(Reporter)

Comment 7

15 years ago
Created attachment 122329 [details] [diff] [review]
patch, v2.1

answering caillon's and jruderman's concerns.
Attachment #121603 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #122329 - Flags: superreview?(bryner)
Attachment #122329 - Flags: review?(caillon)
(Reporter)

Comment 8

15 years ago
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"/>
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?
(Reporter)

Comment 10

15 years ago
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 on attachment 122329 [details] [diff] [review]
patch, v2.1

This needs to be localized
Attachment #122329 - Flags: superreview?(bryner)
Attachment #122329 - Flags: review?(caillon)
Attachment #122329 - Flags: review-
(Reporter)

Comment 12

15 years ago
Created attachment 125982 [details] [diff] [review]
patch, v2.2

Localized.  Also a couple minor tweaks.
Attachment #122329 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #125982 - Flags: review?(caillon)
(Reporter)

Comment 13

15 years ago
Created attachment 126337 [details] [diff] [review]
patch, v2.3

CVS-compatible patch from Patch Maker 2.0.
Attachment #125982 - Attachment is obsolete: true
(Reporter)

Comment 14

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

Due to fix for bug 156072, this bug now has high visibility.
Attachment #126337 - Flags: review?(caillon)
(Reporter)

Updated

15 years ago
Attachment #125982 - Flags: review?(caillon)
(Reporter)

Comment 15

15 years ago
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.
Attachment #126337 - Attachment is obsolete: true
Attachment #126337 - Flags: review?(caillon)
(Reporter)

Comment 16

15 years ago
Created attachment 126340 [details] [diff] [review]
patch, v2.4

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

Updated

15 years ago
Attachment #126340 - Flags: review?(caillon)
(Reporter)

Comment 17

15 years ago
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

15 years ago
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.
(Reporter)

Updated

15 years ago
Target Milestone: --- → mozilla1.5beta
(Reporter)

Updated

15 years ago
Attachment #126340 - Attachment is obsolete: true
Attachment #126340 - Flags: review?(caillon)
(Reporter)

Comment 19

15 years ago
Created attachment 127641 [details] [diff] [review]
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.
(Reporter)

Updated

15 years ago
Attachment #127641 - Flags: review?(caillon)
(Reporter)

Comment 20

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

rejected by caillon based on screenshot.
Attachment #127641 - Attachment is obsolete: true
Attachment #127641 - Flags: review?(caillon)
(Reporter)

Comment 21

14 years ago
Created attachment 128806 [details] [diff] [review]
patch, v2.6

addressing comments by caillon
(Reporter)

Updated

14 years ago
Attachment #128806 - Flags: review?(caillon)

Comment 22

14 years ago
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 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.
Attachment #128806 - Flags: review?(caillon) → review-
Product: Core → Other Applications
(Reporter)

Comment 24

13 years ago
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;
(Reporter)

Comment 25

12 years ago
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.
Assignee: ajvincent → dom-inspector
Target Milestone: mozilla1.5beta → ---
Assignee: dom-inspector → nobody
QA Contact: timeless → dom-inspector
You need to log in before you can comment on or make changes to this bug.