Closed Bug 1307938 Opened 8 years ago Closed 8 years ago

Display DOMNodes in a more user friendly way (i.e. not HTMLBodyElement but <body>)

Categories

(DevTools :: Console, enhancement, P2)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: linclark, Assigned: nchevobbe)

References

Details

(Whiteboard: new-console)

Attachments

(1 file)

Bug 1307938 - Add a Rep for DOM nodes. ;
58 bytes, text/x-review-board-request
Honza
: review+
pbro
: review+
Details 
Originally posted by:captainbrosset

see https://github.com/devtools-html/gecko-dev/issues/325

I'm fairly certain this got discussed at some stage either in a bug or on IRC, but after talking to @nchevobbe he said that didn't ring a bell to him, so I'm filing this just in case.

The old front-end used to display DOMNodes in a way that makes more sense to users, it used to preview them in 2 ways:

- compact: `<tag#id.class>`
- HTML: `<tag id="id" class="class">`

Right now, they get displayed as HTMLTagElement.
Priority: -- → P2
Whiteboard: new-console
Blocks: enable-new-console
Assignee: nobody → chevobbe.nicolas
Blocks: 1307941
Blocks: 1311168
Comment on attachment 8803001 [details]
Bug 1307938 - Add a Rep for DOM nodes. ;

https://reviewboard.mozilla.org/r/87242/#review86556

I like what you do with Reps, thanks!

See my inline comments. Some of them are related to UX so, we might want to get Helen involved.

Also, not part of this bug but, I think that the ElementRep could be expandable at some point (e.g. only in the 'long' mode) and show child elements. 

Honza

::: devtools/client/shared/components/reps/element-node.js:15
(Diff revision 1)
> +  // ReactJS
> +  const React = require("devtools/client/shared/vendor/react");
> +  const { isGrip } = require("./rep-utils");
> +
> +  // Utils
> +  const nodeConstants = require("devtools/shared/dom-node-constants");

Are you sure this doesn't break JSON view? Should we wrap content of 'dom-node-constants' in define()?

The same question goes to bug 1311266

::: devtools/client/shared/components/reps/element-node.js:53
(Diff revision 1)
> +            span({className: "attr-name theme-fg-color2"}, `#${attributes.id}`));
> +        }
> +        if (attributes.class) {
> +          attributeElements.push(
> +            span({className: "attr-name theme-fg-color2"}, `.${attributes.class}`));
> +        }

Why the tiny mode uses proprietary syntax:

`<tag#id.class>`

Could we use standard CSS selector syntax instead?

`tag#id.class`

And in case of more class names use dots:

`tag#id.class1.class2`

This way, the user has can copy to clipboard and have CSS selector ready right away!

::: devtools/client/shared/components/reps/element-node.js:64
(Diff revision 1)
> +            `="`,
> +            span({className: "attr-value theme-fg-color6"}, `${value}`),
> +            `"`
> +          );
> +
> +          return arr.concat([" ", attribute]);

Firebug prioritized id and class attributes and displayed them as the first since these are used the most. What if we did the same?

::: devtools/client/shared/components/test/mochitest/test_reps_element-node.html:171
(Diff revision 1)
> +              "class": "body-class",
> +              "id": "body-id"
> +            },
> +            "attributesLength": 2
> +          }
> +        };

Do we actually need all the grip fields? Couldn't we remove: frozen, extensible, sealed from all the stubs?
Attachment #8803001 - Flags: review?(odvarko)
I belive this depends on bug 1311266 that wraps dom-node-constants.js file in define() call and so, it can be loaded in AMD environment.

Honza
Depends on: 1311266
Comment on attachment 8803001 [details]
Bug 1307938 - Add a Rep for DOM nodes. ;

https://reviewboard.mozilla.org/r/87242/#review86556

Interresting questions ! FYI , I copied the current behaviour of the console, but we could indeed think more about how we display nodes in different modes.

For the "expandable" part, I think that it is planned by Lin and devtools.html (they call it "Object inspection"), to replace the Variable View. So I guess in the end all the reps would have an "expander"

> Are you sure this doesn't break JSON view? Should we wrap content of 'dom-node-constants' in define()?
> 
> The same question goes to bug 1311266

yes, I should have add a blocker for this bug. My comment node rep will fix the issue. I'll rebase this patch when bug 1311266 lands in central.

> Why the tiny mode uses proprietary syntax:
> 
> `<tag#id.class>`
> 
> Could we use standard CSS selector syntax instead?
> 
> `tag#id.class`
> 
> And in case of more class names use dots:
> 
> `tag#id.class1.class2`
> 
> This way, the user has can copy to clipboard and have CSS selector ready right away!

I like that !
I should have a deeper look for the classes as they come as a string (e.g. "class1 class2"), and see if there's no danger to just do a `split(" ")` on them to prepend the `.` part.

> Firebug prioritized id and class attributes and displayed them as the first since these are used the most. What if we did the same?

Agree, FWIW, when I wrote the tests, I put the ids first, and then I saw they were displayed after the class :)

> Do we actually need all the grip fields? Couldn't we remove: frozen, extensible, sealed from all the stubs?

I guess we could.
As I'm working more and more on Reps, I'm wondering if this is enough to put the stubs like this.
If the backend change, the stub are out-of-sync, and we're dommed :)
In the console, we use a script to generate the stubs ( for packets and messages ) based on an input in the console, and we plan to have some automation in test to make sure the stubs are always up-to-date.
So I was wondering if we couldn't share this between the console and the Reps, or at least, having some kind of similar thing in the Reps, if we don't want to share the thing with the console.

I planned to do some improvment in Reps architecture ( stubs generation, modes as constants, ... ), so maybe I can create some tickets to make sure we keep track of those ideas
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #4)
> As I'm working more and more on Reps, I'm wondering if this is enough to put
> the stubs like this.
> If the backend change, the stub are out-of-sync, and we're dommed :)
> In the console, we use a script to generate the stubs ( for packets and
> messages ) based on an input in the console, and we plan to have some
> automation in test to make sure the stubs are always up-to-date.
> So I was wondering if we couldn't share this between the console and the
> Reps, or at least, having some kind of similar thing in the Reps, if we
> don't want to share the thing with the console.
> 
> I planned to do some improvment in Reps architecture ( stubs generation,
> modes as constants, ... ), so maybe I can create some tickets to make sure
> we keep track of those ideas

All good ideas and yes having bug reports that cover those would be great.

Also, there should also be bug/discussion about how to support other protocols besides RDP. There might be some kind of a grip-converter, or we can introduce more modes, or more reps, etc.

Honza
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #4)
> Interresting questions ! FYI , I copied the current behaviour of the
> console, but we could indeed think more about how we display nodes in
> different modes.
We need to be consistent throughout the tools on how dom nodes are displayed:

- in the console when a single node is displayed:
<tagname id="id" class="class" attr1="value1" attr2="value2">

- in the console when a collection of nodes is displayed as a result to $$ for instance:
<tagname#id.class>

- in the variables-view:
<tagname#id.class>

- in the inspector's markup-view
<tagname id="id" class="class" attr1="value1" attr2="value2"></tagname> when collapsed
<tagname id="id" class="class" attr1="value1" attr2="value2"> when expanded
(note that id, if it exists, is always first, and class, if it exists, is always second)

- in the rule-view, within the "inherited" section header:
tagname#id
(note that the .class isn't displayed in this case, but I think we should fix it)

- in the computed-view when you expand one property and it gives you the list of participating rules:
tagname#id.class

- in the animation-inspector when an animation is displayed:
tagname#id.class

Much of these are implemented using mostly duplicated code, on the long run, we should use the rep everywhere we can.
But having consistency on how things look is very important here. And I agree with Honza that the <tagname#id.class> is a bit weird. I think we should use tagname#id.class instead (this is also what Chrome does).

> For the "expandable" part, I think that it is planned by Lin and
> devtools.html (they call it "Object inspection"), to replace the Variable
> View. So I guess in the end all the reps would have an "expander"
And I hope that on the long run we can also just use this for the inspector itself.
Comment on attachment 8803001 [details]
Bug 1307938 - Add a Rep for DOM nodes. ;

https://reviewboard.mozilla.org/r/87242/#review87126

I haven't reviewed the code, so I can't really R+ this. The code changes are mostly rep and react, and I have no experience in this domain, so I'll let Honza do that.
I have however done some manual testing and can give my feedback about this:

- This might not be related: arrays of DOM elements only contain 3 previews, and then the "N more ..." label, while the "old" console contains 10 previews. More is better here. Same for NodeList (I guess any Array-like is treated in a similar way).
- This is most probably not related either, but document.body.attributes[0] (on about:newtab) does output the right thing, but clicking on the attribute shows a blank variablesview panel.
- On about:newtab, if you open the inspector, select the node `#newtab-customize-button`, go to the console and then enter `$0`, you get `<input id="newtab-customize-button" dir="ltr" title="Customize your New Tab page" type="button" value="⚙">` whereas you get `<input id="newtab-customize-button" dir="ltr" value="⚙" title="Personnaliser la page Nouvel onglet" type="button">` in the old console. The order of attributes is different. The old console matches the inspector (which is good, most consistency is good), whereas the new console seems to order attributes alphabetically.
- In the dark theme, the tagname syntax highlighting color is incorrect. With the old console, it's green (like in the inspector), with the new console, it's white.
- In compact form (i.e. `div.class` instead of `<div class="class">`), only the tagname is a link you can click. In the old console, the whole preview is clickable. Not a big deal, especially now that the compact form is different (e.g. `div.class` instead of `<div.class>`), so up to you if this needs fixing or not. This might actually be more consistent with the long form. Fwiw, I'd prefer if the link was around the whole preview every time, this makes it easier to click.

So, consider this review cancelation as an F+ based on the fact that I haven't reviewed the code.
I think this is good work, the previews are much better than before, and the only issues I found are minor anyway.
I do think they need fixing before landing still. So feel free to ping me again so I can do another round of testing, but Honza should be the one giving you the final R+ on the code changes.
Attachment #8803001 - Flags: review?(pbrosset)
> I have however done some manual testing and can give my feedback about this:

Thanks for testing it Patrick

> This might not be related: arrays of DOM elements only contain 3 previews, and then the "N more ..." label

This is the default size for array-like object (when mode is undefined, or "short"). The "long" mode displays the 300 first items. I wonder if we could bump the default, or have a new mode. It is true that in the old console, we display the 10 first elements ( probably all that is in the preview property of the grip ), even for non node containing arrays.

> The old console matches the inspector (which is good, most consistency is good), whereas the new console seems to order attributes alphabetically
> [...]
> In the dark theme, the tagname syntax highlighting color is incorrect.

I'll check what's done in the old console and make the new one behave the same way.

> In compact form (i.e. `div.class` instead of `<div class="class">`), only the tagname is a link you can click. In the old console, the whole preview is clickable

This should be easy to do.
Patrick, I pushed to TRY with some fixes, it would be really nice if you have some times to play around with it (https://treeherder.mozilla.org/#/jobs?repo=try&revision=85fbd43a5a98).

>> This might not be related: arrays of DOM elements only contain 3 previews, and then the "N more ..." label
> This is the default size for array-like object (when mode is undefined, or "short"). The "long" mode displays the 300 first 
> items. I wonder if we could bump the default, or have a new mode. It is true that in the old console, we display the 10 first > elements ( probably all that is in the preview property of the grip ), even for non node containing arrays.

I might doing that as a separate bug, to see if we add a new mode, and use it in the console, or change the default items number on array like object, because in either case it would require quite some changes

Honza, do you remember what was the rationale to have the short mode limit to 3 items ? 
I guess it was for the JSONView or the DOMPanel (maybe to match some firebug feature ? ). I think it would male sense to have the same limit everywhere (and thus modifying how array are rendered in short mode), but maybe there's a good reason to have the "3" limit.
Comment on attachment 8803001 [details]
Bug 1307938 - Add a Rep for DOM nodes. ;

https://reviewboard.mozilla.org/r/87242/#review88454

UI review only.

I've been testing this. Looks great. Thanks.
Just one remaining problem, R+ assuming you fix it.

On this page: https://www.mozilla.org/en-US/firefox/52.0a1/whatsnew/?oldversion=51.0a1

executing document.body outputs:
<body id="nightly-whatsnew" class="html-ltr ">
=> Notice the hanging space at the end of the class attribute. It's fine, if it's actually in the attribute value, it should be there.

executing [document.body] outputs:
Array [ body#nightly-whatsnew.html-ltr. ]
=> Notice the extra dot at the end of the short form which should *not* be there because document.body.classList actually contains just 1 item.
Attachment #8803001 - Flags: review?(pbrosset) → review+
> executing [document.body] outputs:
> Array [ body#nightly-whatsnew.html-ltr. ]
> => Notice the extra dot at the end of the short form which should *not* be there because document.body.classList actually > contains just 1 item.

Good catch !
BTW, we have this bug in the old console too :)
Do you think we should output hanging spaces in the Rep ? Since there could be multiple of them, it could lead to some visual weirdness : 

`document.querySelector("body, html")` will output "body#id.html-ltr ,html" . Notice the space between the extra html string. If there would be 10 spaces after "hhtml-ltr", it could get really weird : "body#id.html-ltr           ,html"
and since the string is not wrapped by angle brackets, it could be misleading.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
> > executing [document.body] outputs:
> > Array [ body#nightly-whatsnew.html-ltr. ]
> > => Notice the extra dot at the end of the short form which should *not* be there because document.body.classList actually > contains just 1 item.
> 
> Good catch !
> BTW, we have this bug in the old console too :)
> Do you think we should output hanging spaces in the Rep ? Since there could
> be multiple of them, it could lead to some visual weirdness : 
> 
> `document.querySelector("body, html")` will output "body#id.html-ltr ,html"
> . Notice the space between the extra html string. If there would be 10
> spaces after "hhtml-ltr", it could get really weird : "body#id.html-ltr     
> ,html"
> and since the string is not wrapped by angle brackets, it could be
> misleading.
I would say that when we are outputting the element as it is displayed in the markup-view:
<tagName id="value" class="value">
then we should preserve all characters that are present in the attribute values. Even if there are 10 spaces. Because that's how the markup-view will also display them.
This brings another interesting point that deserves its own bug: how do you deal with nodes with really long attribute values and/or many attributes. There's an option in the inspector to truncate this, and I think the console should also honor it.

Now, when the shorter form is used:
tagName#value.value
then that's a good point, I think we should trim the values there. The goal of this form is *not* to tell users exactly the values of the attributes. Its goal is to serve as a quick identifier to a node. So as such, we may make it look how we think it best serves that purpose.
So, yeah, trimming the extra leading/hanging spaces in attributes sound good. But in fact, we should also perhaps have a limit in the number of classes displayed? I think that limit would have to be quite high though (and probably deserves it own bug too).
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> Patrick, I pushed to TRY with some fixes, it would be really nice if you
> have some times to play around with it
> (https://treeherder.mozilla.org/#/jobs?repo=try&revision=85fbd43a5a98).
> 
> >> This might not be related: arrays of DOM elements only contain 3 previews, and then the "N more ..." label
> > This is the default size for array-like object (when mode is undefined, or "short"). The "long" mode displays the 300 first 
> > items. I wonder if we could bump the default, or have a new mode. It is true that in the old console, we display the 10 first > elements ( probably all that is in the preview property of the grip ), even for non node containing arrays.
> 
> I might doing that as a separate bug, to see if we add a new mode, and use
> it in the console, or change the default items number on array like object,
> because in either case it would require quite some changes
> 
> Honza, do you remember what was the rationale to have the short mode limit
> to 3 items ? 
> I guess it was for the JSONView or the DOMPanel (maybe to match some firebug
> feature ? ). I think it would male sense to have the same limit everywhere
> (and thus modifying how array are rendered in short mode), but maybe there's
> a good reason to have the "3" limit.
Yes, the number has been introduced for JSONv and DOM panel and it originally came from Firebug. Displaying 3 items has been usually enough especially when rendered inside a side panel (e.g. Watch panel in the Debugger panel) which doesn't have so much horizontal space.

Is the Console actually using the 'long' mode? The Console panel takes the entire horizontal space of the browser window and there is usually a lot of space (comparing to a side panel where 'short' mode should be used).

Backward compatibility with the old Console sounds like high priority so, I think it's ok to change the short mode limit to 10 if that's the only option.

Honza
The console does not specified a mode at all, so we fall back to the short mode.
You make a good point with reps being used in a side panel in the debugger, I guess it would be too much to have 10 items there.
I think we could fine tune the max property to be :
- 0 (or ignored anyway) in tiny mode
- 3 in explicit short mode
- 100 in explicit long mode
- 10 when no mode is passed

With that, we could explicitly set mode to short when Reps are used in a side panel, and enjoy a more comfortable preview in console (and maybe dom & jsonview too).

We could create a new mode ("regular"), but since it won't behave much differently than the short one (except the number of properties), it would be an overkill (we would have to test it for all the Reps).
(In reply to Patrick Brosset <:pbro> from comment #14)
> I would say that when we are outputting the element as it is displayed in
> the markup-view:
> <tagName id="value" class="value">
> then we should preserve all characters that are present in the attribute
> values. Even if there are 10 spaces. Because that's how the markup-view will
> also display them.

> Now, when the shorter form is used:
> tagName#value.value
> then that's a good point, I think we should trim the values there. The goal
> of this form is *not* to tell users exactly the values of the attributes.
> Its goal is to serve as a quick identifier to a node. So as such, we may

Yes, I like these two statements.


(In reply to Nicolas Chevobbe [:nchevobbe] from comment #19)
> The console does not specified a mode at all, so we fall back to the short
> mode.
> You make a good point with reps being used in a side panel in the debugger,
> I guess it would be too much to have 10 items there.
> I think we could fine tune the max property to be :
> - 0 (or ignored anyway) in tiny mode
> - 3 in explicit short mode
> - 100 in explicit long mode
> - 10 when no mode is passed
I think that changing the limit if no mode is specified could be confusing I'd rather make clear what mode is the default (if no mode is explicitly specified). So, any reps behavior always corresponds to an existing mode.

How about:
- 0 (or ignored anyway) in tiny mode
- 3 in short mode
- 10 in long mode


* The tiny mode is used mostly for array items and object properties.
* The short mode is used mostly for side panels or any other UI with limited horizontal space.
* The long mode is used for the Console panel or any other UI with rather larger horizontal space (e.g. taking the entire horizontal space of the browser window). 

The long mode is especially useful for the Console panel and it should be explicitly set for it. Also, I don't think that we are using it anywhere else so, it looks ok to change max limit of the long mode from 300 to 10. I'd personally suggest more than 10 since there is really a space in the console panel, but backward compatibility is important and also there is no additional risk of performance regression if we keep the max limit equal to 10.


> We could create a new mode ("regular"), but since it won't behave much
> differently than the short one (except the number of properties), it would
> be an overkill (we would have to test it for all the Reps).
Yes agree, no new mode seems to be needed.

Honza
Okay, so I created Bug 1314571 to use long mode in the console, and Bug 1314573 to make the change for the long mode in Reps.
Honza, since we have those bugs for the number of items to preview, do you need any information to review this patch ?
Flags: needinfo?(odvarko)
Comment on attachment 8803001 [details]
Bug 1307938 - Add a Rep for DOM nodes. ;

https://reviewboard.mozilla.org/r/87242/#review89610

Just one suggestion for a follow-up

R+ assuming Try is green.

Great job here!

Honza

::: devtools/client/shared/components/test/mochitest/test_reps_element-node.html:1
(Diff revision 6)
> +

Missing Mozilla license header. I noticed that it's actually missing in all Rep test files so, it would probably deserve separate bug.
Attachment #8803001 - Flags: review?(odvarko) → review+
Flags: needinfo?(odvarko)
Thanks !

> Missing Mozilla license header. I noticed that it's actually missing in all Rep test files so, it would probably deserve separate bug.

Sure, I filed bug 1314590 for that purpose
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/c22b06e7fba4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: