113793 - Node type should display human readable name, not value

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, enhancement)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Inspector should report human readable values for Node type.  At the least
ELEMENT_NODE, ATTRIBUTE_NODE, TEXT_NODE, etc. but preferably 'Element',
'Attribute', 'Text', etc.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Taking. some good old fashioned JS magic can fix this.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Proposed fix v1.0

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

The patch I just uploaded takes care of this for both the DOM Object viewer and
Javascript Object viewer, within the DOM window and inspecting in a new window.

utils.js seemed the best place for the nodeTypeToText function.  I also included
more node types than we currently display in that function for completeness.  No
sense in re-doing it later to add more node types if we display them in the
future.  If anyone disagrees though, I can change it.

bz, would you review please?

Comment 4

[Boris Zbarsky [:bzbarsky]]

There is no need to do the toString business and then compare to random
stringified numbers:

switch (nodeType) {
  case Node.ELEMENT_NODE: return "Element";
  case Node.ATTRIBUTE_NODE: return "Attribute";
...
}

Do that and r=bzbarsky

Another note, though.  See
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/base/src/inDOMView.cpp#310
-- that has some code that already does something similar... if that can be
shared somehow, that would be great.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v2

Sorry for the delay in getting this out.  I had connectivity issues @home.

Anyway, this patch should do things the "Right Way" (tm).  I moved the function
call in jsObject.js up, to prevent nodeType getting cast to a string, and added
the proper constants in the switch.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 61539 [details] [diff] [review]
Patch v2

r=bzbarsky

Comment 7

[Joe Hewitt (gone)]

This is a great idea for the DOM Node viewer, but I'm against changing nodeType
to a string in the js object viewer - that viewer should always give back
straight values for each property.  Also, could you change your if/else syntax
to be like this:

if (1) {
} else {
}

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Can we display the constant names as opposed to the int values for the JS Object
viewer then?  Node.ELEMENT_NODE, Node.ATTRIBUTE_NODE, etc

Technically it would be a straight value as it's a constant, just not the
numeric representation, and would offer some clue as to the type of node to a
user who doesn't want to (or know how to) figure out what each nodeType is...

The downside is of course that users might not realize it is a constant as
opposed to a string containing text "Node.ELEMENT_NODE" etc, though the fact
that there won't be double quotes surrounding it should alert them to that.  I'd
love to have a more visual representation in the JS Object viewer than 1, 2, 3,
but I'll leave the decision to you.

(Yes I do realize the irony of this coming from someone who use 1, 2, 3, etc in
the first version of the patch :)

I'll change the if/else syntax in the next patch.

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v.3

This patch is just for DOM Node viewer and fixes the if/else bracketing syntax.


After some thought though, I'm wondering if we should do this in tooltipText. 
This way we can still implement this functionality for JS Object Viewer while
keeping the actual value in the <textbox>.  The JS Object viewer and DOM viewer
should be consistent IMO, and if they both display tooltips containing their
human readable names, I would be fine with that.

Comments?

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch using tooltiptext instead of altering the .value

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 63634 [details] [diff] [review]
Patch using tooltiptext instead of altering the .value

r=bzbarsky

Comment 12

[Joe Hewitt (gone)]

Comment on attachment 63634 [details] [diff] [review]
Patch using tooltiptext instead of altering the .value

sr=hewitt

Comment 13

[Boris Zbarsky [:bzbarsky]]

checked into trunk