"nodeName" column header in DOM inspector is confusing

RESOLVED FIXED

Status

--
trivial
RESOLVED FIXED
17 years ago
7 years ago

People

(Reporter: lapsap7+mz, Assigned: foss)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

17 years ago
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
Those columns _do_ list the nodeName and nodeValue properties of the attribute
nodes...  but agreed that the proposed phrasing is a lot clearer.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Mass re-assigning bugs to dom.inspector@extensions.bugs
Assignee: hewitt → dom.inspector
Product: Core → Other Applications
Assignee: dom-inspector → nobody
QA Contact: timeless → dom-inspector
(Assignee)

Comment 3

7 years ago
Change from nodeValue to "Atribute name"
and from nodeValue to "Atribute value"?
(Reporter)

Comment 4

7 years ago
(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"
(Assignee)

Updated

7 years ago
Assignee: nobody → leofigueres
Status: NEW → ASSIGNED
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

7 years ago
Created attachment 570266 [details] [diff] [review]
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.
Attachment #570266 - Flags: review?(neil)
(Assignee)

Updated

7 years ago
Attachment #570266 - Flags: review?(neil)
(Assignee)

Comment 10

7 years ago
Created attachment 570269 [details] [diff] [review]
Changing headers strings in Object Viewer

Same comment as obsoleted previous patch (I didn't refreshed changes to files, sorry).
Attachment #570266 - Attachment is obsolete: true
Attachment #570269 - Flags: review?(neil)
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.)
Attachment #570269 - Flags: review?(neil) → review?(Sevenspade)
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".
Attachment #570269 - Flags: review?(Sevenspade) → review-
(Reporter)

Comment 13

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

Comment 14

7 years ago
Created attachment 570727 [details] [diff] [review]
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.
Attachment #570269 - Attachment is obsolete: true
Attachment #570727 - Flags: review?(Sevenspade)
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).
Attachment #570727 - Flags: review?(Sevenspade) → review-
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.
(Assignee)

Comment 17

7 years ago
(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 :-)
(Assignee)

Comment 18

7 years ago
Created attachment 572238 [details] [diff] [review]
Changing columns header v1.1
Attachment #570727 - Attachment is obsolete: true
Attachment #572238 - Flags: review?(Sevenspade)
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.)
(Assignee)

Comment 20

7 years ago
Created attachment 575495 [details] [diff] [review]
Patch for bug 147174 v1.2 [Checkin: comment 21]

Making the group caption localizable and changing the commentary.
Attachment #572238 - Attachment is obsolete: true
Attachment #575495 - Flags: review?(Sevenspade)
Attachment #572238 - Flags: review?(Sevenspade)
Attachment #575495 - Flags: review?(Sevenspade) → review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Comment on attachment 575495 [details] [diff] [review]
Patch for bug 147174 v1.2 [Checkin: comment 21]

http://hg.mozilla.org/dom-inspector/rev/d1b51e82be02
Attachment #575495 - Attachment description: Patch for bug 147174 v1.2 → Patch for bug 147174 v1.2 [Checkin: comment 21]
Leaving it up to you whether or when to close the bug.
Keywords: checkin-needed
Blocks: 670224
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.