989629 - Object Inspector should display class names for DOM elements

Status: RESOLVED FIXED

(DevTools :: Object Inspector, defect)

Description

[Dane MacMillan [:danemacmillan]]

Attachment: Screen Shot 2014-03-29 at 11.42.44 AM.png

Since bug 682033 comment 13, the output of nodes in the Console have included their class names. This is great. I think the side panel (when clicking to see the rest of the truncated results) that also displays NodeLists should include that information, too, otherwise it's not as useful, as discussed in the bug comment referenced.

I think this is especially pertinent if the output of results in the Console do not have the cap on truncated collections (arrays, objects, NodeLists) increased or removed, via bug 989624.

Comment 1

[Sami Jaktholm]

Attachment: variablesview-show-element-classes.patch

Here's a patch that adds class names to DOMNodes in variables view with the standard DOM selector convention. A DOM node might look like <div#id.class-one.class-two>. The patch also contains a test case to ensure the nodes are rendered correctly.

Comment 2

[Sami Jaktholm]

Attachment: variablesview-show-element-classes.patch

The previous patch contained a little error. Here's a patch that calls the correct cleanup function in the attached test case.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8430567 [details] [diff] [review]
variablesview-show-element-classes.patch

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

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +3698,5 @@
>            result += "#" + attrs.id;
>          }
> +
> +        if (attrs.class) {
> +          result += "." + attrs.class.replace(" ", ".");

This would properly match class="foo bar", but not class="foo    bar". Better use class.replace(/\s+/, "."). Add another check in the test for this case, too.

Looks good otherwise.

Comment 4

[Sami Jaktholm]

Attachment: variablesview-show-element-classes.patch

That's a good point I didn't thought of. Thanks for the review. Here's a revised version of the patch with fix for the issue you pointed out.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8430948 [details] [diff] [review]
variablesview-show-element-classes.patch

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

Thanks for the updated patch. Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=65925b7716f1

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Landed: https://hg.mozilla.org/integration/fx-team/rev/767e7eb767d8

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/767e7eb767d8