147174 - "nodeName" column header in DOM inspector is confusing

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[石庭豐 (Seak, Teng-Fong)]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc3)
Gecko/20020523
BuildID:    20020523

In DOM inspector, Tab "Object" > "Object - DOM Node", we have "nodeName" and
"nodeValue" as column headers, but IMHO, "nodeName" is quite confusing.  I think
"Node Attribute", "Attribute" or "Attribute name" are more appropriate but by
"nodeName", I would think of the DOM property "nodeName" which gives the name of
element.

Similarly, "Attribute Value" or "Value" are more appropriate than "nodeValue".

Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

Those columns _do_ list the nodeName and nodeValue properties of the attribute
nodes...  but agreed that the proposed phrasing is a lot clearer.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Mass re-assigning bugs to [e-mail]

Comment 3

[Javi Rueda :javirid]

Change from nodeValue to "Atribute name"
and from nodeValue to "Atribute value"?

Comment 4

[石庭豐 (Seak, Teng-Fong)]

(In reply to Javi Rueda from comment #3)
> Change from nodeValue to "Atribute name"
> and from nodeValue to "Atribute value"?

No, from nodeName to "Atribute name" and from nodeValue to "Atribute value"

Comment 5

[Javi Rueda :javirid]

Column header labels are hard-coded, although in the corresponding .dtd file there exists a friendly localizable label.

In http://mxr.mozilla.org/comm-central/source/mozilla/extensions/inspector/resources/content/viewers/domNode/domNode.xul#83 it is stated that there is not need to use that label.

The patch is easy, but is still needed?

Comment 6

[石庭豐 (Seak, Teng-Fong)]

I'm not sure how to answer your question.  Let me elaborate.

At the time this bug was reported (in 2002!), DOM Inspector was a part of Firefox.  Now DOM Inspector is an add-on.

Nevertheless, I've just tried DOM Inspector 2.0.10 and those confusing labels are still there. So, as far as those labels are concerned, yes, the patch is needed.

But I have no answer as to whether it is that particular label in that particular file (domNode.xul) that we need to change.

Comment 7

[Javi Rueda :javirid]

(In reply to 石庭豐 (Seak, Teng-Fong) from comment #6)
> 
> But I have no answer as to whether it is that particular label in that
> particular file (domNode.xul) that we need to change.

The label is used everywhere. For example, it appears in the dialog box for insert a node.

My first approach was to change both in domNode.dtd, removing the trailing ":" (and adding it to the corresponding XUL description for said dialog box), also referencing to the .label in domNode.xul. This worked in my compiled XPI.

Other way was to hard-code the strings for the column headers.

As those strings are used for 2 different things -headers and labels for text-edit boxes- maybe some specific headerNodeValue and headerNodeName could be created only for this use. This way only the "problematic" files would be modified.

Comment 8

[石庭豐 (Seak, Teng-Fong)]

As long as those labels are changed for final users (including me), whatever approach is good for me :)
Thanks

Comment 9

[Javi Rueda :javirid]

Attachment: Changing headers strings in Object Viewer

Could you take a look at this or shifting to a proper reviewer, Neil? Thank you.

Changing column headers in Object Viewer.

Creates new strings as this labels are only being used here.

Localization will be needed.

Comment 10

[Javi Rueda :javirid]

Attachment: Changing headers strings in Object Viewer

Same comment as obsoleted previous patch (I didn't refreshed changes to files, sorry).

Comment 11

[[e-mail]]

Comment on attachment 570269 [details] [diff] [review]
Changing headers strings in Object Viewer

I don't think I'm the right reviewer for this.
(But if you want my opinion, then you'd have to change all of the labels.)

Comment 12

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 570269 [details] [diff] [review]
Changing headers strings in Object Viewer

Review of attachment 570269 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, we should change all the treecol labels away from being IDL-centric, not just the name and value columns.

::: resources/locale/en-US/viewers/domNode.dtd
@@ +42,5 @@
>  <!ENTITY namespaceURI.label "Namespace URI:">
>  <!ENTITY nodeType.label "Node Type:">
>  <!ENTITY nodeValue.label "Node Value:">
> +<!ENTITY nodeNameColHeader.label "Attribute Name">
> +<!ENTITY nodeValueColHeader.label "Attribute Value">

The problem with changing all of them like this, though, means that they will all start with "Attribute...", which is pretty redundant and may require more space to display than what's available, depending on window size.

It may be better to just put the attribute tree inside a groupbox with an "Attributes" caption.  Let me know if you need me to explain further.

Also, please change "nodeNameColHeader" to something like just "attributeName".

Comment 13

[石庭豐 (Seak, Teng-Fong)]

(In reply to Colby Russell :crussell from comment #12)
> Also, please change "nodeNameColHeader" to something like just
> "attributeName".

Yes, I agree with Colby that it's better to avoid strings beginning with "node..." so that other programmers won't be confused.

Comment 14

[Javi Rueda :javirid]

Attachment: Changing columns header

Changes the attribute header column labels and puts the tree into a groupbox (which will be hidden when appropiate).

Reformatting the code affected by the inserted groupbox.

Translation needed, so removing the directories in the Makefile.

Comment 15

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 570727 [details] [diff] [review]
Changing columns header

Review of attachment 570727 [details] [diff] [review]:
-----------------------------------------------------------------

::: resources/locale/en-US/viewers/domNode.dtd
@@ +42,5 @@
>  <!ENTITY namespaceURI.label "Namespace URI:">
>  <!ENTITY nodeType.label "Node Type:">
>  <!ENTITY nodeValue.label "Node Value:">
> +<!ENTITY attributeName.label "Name">
> +<!ENTITY attributeValue.label "Value">

These should either stay hardcoded in the XUL and be lowercased (to reflect the names from IDL), or all treecol headers should be made localizable, properly capitalized, natural language strings (vs. the names from IDL, per comment 12).

Comment 16

[Colby Russell :crussell (no longer Mozilla-ing)]

I'm referring to this bit:

(From comment #12)
> Yeah, we should change all the treecol labels away from being IDL-centric,
> not just the name and value columns.

Comment 17

[Javi Rueda :javirid]

(In reply to Colby Russell :crussell from comment #15)
 
> These should either stay hardcoded in the XUL and be lowercased (to reflect

Easier easy this one :-)

Comment 18

[Javi Rueda :javirid]

Attachment: Changing columns header v1.1

Comment 19

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 572238 [details] [diff] [review]
Changing columns header v1.1

Review of attachment 572238 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't been able to dedicate much time to Mozilla stuff recently.  So, sorry for taking so long to get to this.

::: resources/content/viewers/domNode/domNode.xul
@@ +73,5 @@
>          </rows>
>        </grid>
>      
> +      <groupbox id="grpAttr" flex="1">
> +        <caption label="Attributes"/>

The treecol labels can be hardcoded because they reflect IDL, but this needs to be localizable.  Sorry, I should have caught this before.

@@ +86,1 @@
>                   by DOM APIs -->

The rest looks good, but we should extend this comment to mention that even though we're really accessing the |nodeName| and |nodeValue| properties, it's fine because they're guaranteed to match the attributes' |name| and |value|, respectively.

(Well, that's true so long as there is such a thing as an attribute node, which is set to change soon.  We can deal with that when the time comes, though.)

Comment 20

[Javi Rueda :javirid]

Attachment: Patch for bug 147174 v1.2 [Checkin: comment 21]

Making the group caption localizable and changing the commentary.

Comment 21

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 575495 [details] [diff] [review]
Patch for bug 147174 v1.2 [Checkin: comment 21]

http://hg.mozilla.org/dom-inspector/rev/d1b51e82be02

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Leaving it up to you whether or when to close the bug.