Closed Bug 1247065 Opened 8 years ago Closed 8 years ago

Implement Tree Component

Categories

(DevTools :: Shared Components, defect)

Product:
DevTools
Component:
Shared Components
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox47 affected, firefox48 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 48
Tracking Flags:
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files, 12 obsolete files)

table-expander.mov
1.49 MB, video/quicktime
Details
spinner.png
2.97 KB, image/png
Details
Diagram - moving expansion logic out of component
212.41 KB, application/pdf
Details
table-to-div.patch
5.69 KB, patch
Details | Diff | Splinter Review
bug1247065.patch
64.48 KB, patch
Honza
: review+
Details | Diff | Splinter Review 
DevTools framework needs generic tree component (based on ReactJS) that is flexible, customizable and can be reused across various tools and perhaps also exposed to devtools extensions.
Attached patch bug1247065.patch (obsolete) — — Splinter Review 
Tree implementation based on experience from many scenarios in DevTools as well as in DevTools extensions. There is a tree in FireQuery, RDP Inspector (also inline packet trees), JSON Viewer, XHR Inspector, Web Socket Monitor and now also in the upcoming DOM panel. Using both synchronous and asynchronous access to data (through RDP).

Patch from bug 1211525 is needed.

The patch contains:
* TreeView: the basic tree component
* TreeRow: component for rendering tree rows/nodes
* ObjectProvider: default provider for rendering regular JS objects
* Images

Note: The components uses components from 'jsonview/reps' directory. All these components will be moved into 'shared/components/reps' directory as soon as bug 1240494 is resolved. These components are rendering basic JS data types (e.g. number, string, etc.) and DOM types (e.g. window, element, attribute, etc.)

Basic example usage:

const ReactDOM = require("devtools/client/shared/vendor/react-dom");
const { createFactories } = require("devtools/client/jsonview/components/reps/rep-utils");
const { TreeView } = createFactories(require("devtools/client/shared/components/tree/tree-view"));

ReactDOM.render(TreeView({data: {
  name: "John",
  age: "40"
}}), document.getElementById("content"));


The component has customizable UI as well as data access.

The tree is also used for implementing the DOM panel, see bug 1201475


Honza
It would be great to have a feedback.

The best way how to see the tree in action is probably run the DOM panel, see bug 1201475 (working patch attached)

Honza
Attached patch bug1247065-1.patch (obsolete) — — Splinter Review 
Just archiving some progress on the tree component.

Honza
Assignee: nobody → odvarko
Attachment #8717628 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Great work, thanks for filing this bug. Personally I really think we should use react-virtualized, I've been using it and it's great. We'd benefit from offloading a lot of the complex work for virtualizing, and it has a few different components that we might benefit from.

I haven't dug into your document too much yet. It also seems like "providers" could be made more like React components. Anyway, I think we should talk more about the API, but this shouldn't block anything that you need to land. We can tweak it in the near future.

I'm going to call into the meeting on Thursday this week even though I'll be off, and I'll be working again later next week and we can talk in more detail then.
(In reply to James Long (:jlongster) from comment #5)
> Great work, thanks for filing this bug. Personally I really think we should
> use react-virtualized, I've been using it and it's great. We'd benefit from
> offloading a lot of the complex work for virtualizing, and it has a few
> different components that we might benefit from.
I was looking at react-virtualized and yes it's great and it's well written, but it focuses on virtual lists and tabular data (grids) not hierarchical data. In any case we can get great inspiration from it. 

Also, I think that requirements for tree like components are specific. Various projects often end up implementing its own trees due to specific requirements and I believe that DevTools isn't an exception. The 'virtual' aspect of the component is just one of many we need.

Here is a list of general requirements:
* UI/UX flexibility (API for rendering customization)
* Data consumption flexibility (e.g. sync/async)
* Support for Dark/Light/Firebug themes
* Displaying hierarchical as well as tabular data
* Support for virtual viewport
* Inline editing

Note that an important decision is that we want to base the component on <table> element.

Here are existing scenarios in DevTools where we want to use the tree component:
* Inline object previews in the Console panel (bug 1243802)
* JSON/XML/SVG HTTP response preview in the Net panel also used in the Console panel (bug 1211525)
* Basic layout of the Net panel
* JSON Viewer
* DOM panel as well as DOM side panel e.g. for the Inspector panel (bug 1201475)
* Variables View for the Console panel
* Watch view for the Debugger panel
* Memory panel (virtual viewpoert)
* Performance panel (virtual viewport)
* console.table()
* Anything else?

- At some point we might also want to use the tree for the Inspector markup view as well, but I agree with Victor that the markup view is special so, let's wait with this.

There are several other requirements like sorting, filtering, displaying header for tabular data and so, I belive we need our own specific component to adopt all.

> I haven't dug into your document too much yet. It also seems like
> "providers" could be made more like React components. Anyway, I think we
> should talk more about the API,
Yes, please! I started using functional stateless components (nobody against from our last components meeting) for customization and it improves the code style (see my tutorial below). And there is definitely more space for improvements.

> but this shouldn't block anything that you
> need to land. We can tweak it in the near future.
Great, I'll yet attach my last version here, but the code is quite ready for general review.

I have created an online tutorial with live examples that should help you to understand the current API:
http://janodvarko.cz/devtools/components/tree/

Btw. do we stil want to use gitbook for docs?

Please read it and we can discuss on Thursday.

> I'm going to call into the meeting on Thursday this week even though I'll be
> off, and I'll be working again later next week and we can talk in more
> detail then.
Excellent

Honza
Attached patch bug1247065-2.patch (obsolete) — — Splinter Review 
Updating the patch
Honza
Attachment #8719572 - Attachment is obsolete: true
Honza, I have a lot of thoughts about this but I'm going back on paternity leave for the next 5 days. A very short summary:

* I like the direction of this and it's very nice to have actual code to start working on!
* Why do we want to actually use a <table> element? That will introduce more constraints than helpful features. We don't need the autosizing feature of tables, if we want a cell to fill up space we can use flexbox, but we want fixed-width resizable columns anyway.
* I really, really think we should build on top of react-virtualized's VirtualScroll. It's getting a lot of love to be very performant and feature-complete. We need to build on top of virtualization from the start, and I'm worried that we're going to far into implementation without it. I think it should be fine to use it any dynamically generate rows (not using <table>).

Obviously we need to talk more about this. Would you be ok if I took your current implementation and proposed another version of it, with an architecture that I have been thinking about building?

I will call into the meeting tomorrow and we can talk some then, but I won't be able to do much until I'm back.
Attached patch bug1247065.patch (obsolete) — — Splinter Review 
I am attaching updated patch for review.

* It can be seen in action together with the DOM panel (bug 1201475). The DOM panel patch is updated so, it can be tested together.

* Docs that could help the review:
Google doc: https://docs.google.com/document/d/1Qi5_mMq03xoCFGoCX_bhtxBQ8Wtvt07q8JbOvdGlCks/edit#
Live demo: http://janodvarko.cz/devtools/components/tree/

* Lin, Brian: the tree is needed for the DOM panel and the DOM panel patch has all the Reps I mentioned (for the Console rendering work).

Honza
Attachment #8720207 - Attachment is obsolete: true
Attachment #8721363 - Flags: review?(lclark)
Attachment #8721363 - Flags: review?(bgrinstead)
(In reply to James Long (:jlongster) from comment #8)
> Honza, I have a lot of thoughts about this but I'm going back on paternity
> leave for the next 5 days. A very short summary:

Unanswered question from our meeting:

Why we should use the react-virtualized for our tree-widget when it doesn't have any support for hierarchical data?

The component's description clearly says:
React components for efficiently rendering large, scrollable lists and tabular data 

I like how react-virtualized is written and I believe we should rather take an inspiration of how the component does its virtualization and extend our tree (not trying to extend the grid).

But, I think that we can use the component (as is) for the Console panel. The console panel isn't a tree, it's rather a list of logs (where trees can be inside these logs, e.g. groups, inline object inspection, stack-trace, etc.)

I've put together a list of use-cases (in our UI) where the tree should be used (see comment #6) and we should have those in mind when optimizing the inner guts of the tree.


Honza
Attached video table-expander.mov — 
Video showing unexpected expansion
Comment on attachment 8721363 [details] [diff] [review]
bug1247065.patch

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

There are a few things that keep me from being confident about a review here:

1. I tried manually testing the demo page. It wasn't reliably reproducible, but multiple times I saw the wrong thing expand when I clicked on something else. I asked Brian and he said he thought he'd seen that too, but he had figured that he was just mistaken. I've attached a video that shows this. When I click on the item labeled 3, both 1 and 0 expand.

2. I'm not sure that I understand the functional requirements enough.

3. Related to that, I'd like to see tests that cover the functional requirements and ensure that we maintain the required baseline of functionality.

4. I'm not sure I understand the provider pattern used here. I'm not sure whether the complexity is necessary or not.
Attachment #8721363 - Flags: review?(lclark) → review-
Comment on attachment 8721363 [details] [diff] [review]
bug1247065.patch

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

Did a pass over the style changes

::: devtools/client/shared/components/tree/images/twisty-closed-firebug.svg
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>

Are these images going to be reused across tools in the firebug theme as a replacement for the current twisty icon?   Maybe they should be added to a themes/images/firebug/ directory or something like that.  I know it's tricky because we want the component to be self-contained, but if so then we'll also need to copy/paste icons for light and dark theme into this component (and all other components that want this icon).  Which would mean extra maintenance for UX

Could we move the image declarations in CSS to top level variables so that there could be a mode in tree which uses existing images and an easy way to change the variable to use a self-contained image when deployed to a web page?

::: devtools/client/shared/components/tree/tree-view.css
@@ +9,5 @@
> +/* TreeView */
> +
> +.treeTable {
> +  font-size: 11px;
> +  font-family: Lucida Grande, Tahoma, sans-serif;

Why does this have it's own fonts?  It should inherit from the theme

@@ +42,5 @@
> +  text-decoration: underline;
> +}
> +
> +.treeTable .treeRow:hover {
> +  background-color: #EFEFEF;

Please use theme variables

@@ +72,5 @@
> +
> +/* Spinner (used for async fetch). Needs to have higher priority than
> +   theme toggle icons */
> +.treeTable .treeRow.hasChildren.loading > .treeLabelCell > .treeIcon {
> +  background-image: url(./images/spinner.png) !important;

Could this use an existing spinner image in the tree?  See comment about the other images

@@ +101,5 @@
> +  -moz-user-select: none;
> +  border-bottom: 1px solid rgba(0, 0, 0, 0.2);
> +  padding: 0 !important;
> +  background: linear-gradient(
> +          rgba(255, 255, 255, 0.05),

Please move all hardcoded colors into variables at the top of the file, and ideally give those default values to theme variables

@@ +123,5 @@
> +  background-color: #AAC3DC;
> +}
> +
> +.treeTable .treeHeaderSorted > .treeHeaderCellBox {
> +  background: url(chrome://firebug/skin/arrowDown.svg) no-repeat calc(100% - 4px);

Here are hardcoded firebug icons
Attachment #8721363 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #13)
> Comment on attachment 8721363 [details] [diff] [review]
> bug1247065.patch
> 
> Review of attachment 8721363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did a pass over the style changes
Thanks Brian!

> 
> ::: devtools/client/shared/components/tree/images/twisty-closed-firebug.svg
> @@ +1,1 @@
> > +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> 
> Are these images going to be reused across tools in the firebug theme as a
> replacement for the current twisty icon?
Yes

> Maybe they should be added to a
> themes/images/firebug/ directory or something like that.  I know it's tricky
> because we want the component to be self-contained, but if so then we'll
> also need to copy/paste icons for light and dark theme into this component
> (and all other components that want this icon).  Which would mean extra
> maintenance for UX
Agree. I like the idea of self contained and independent components, but
since we are also supporting themes we need to share the stuff somewhere.
I've created new images/firebug dir and put all 'firebug' image files into it.

> 
> Could we move the image declarations in CSS to top level variables so that
> there could be a mode in tree which uses existing images and an easy way to
> change the variable to use a self-contained image when deployed to a web
> page?
Yes, done.

> 
> ::: devtools/client/shared/components/tree/tree-view.css
> @@ +9,5 @@
> > +/* TreeView */
> > +
> > +.treeTable {
> > +  font-size: 11px;
> > +  font-family: Lucida Grande, Tahoma, sans-serif;
> 
> Why does this have it's own fonts?  It should inherit from the theme
True, fixed.

> 
> @@ +42,5 @@
> > +  text-decoration: underline;
> > +}
> > +
> > +.treeTable .treeRow:hover {
> > +  background-color: #EFEFEF;
> 
> Please use theme variables
> 
> @@ +72,5 @@
> > +
> > +/* Spinner (used for async fetch). Needs to have higher priority than
> > +   theme toggle icons */
> > +.treeTable .treeRow.hasChildren.loading > .treeLabelCell > .treeIcon {
> > +  background-image: url(./images/spinner.png) !important;
> 
> Could this use an existing spinner image in the tree?  See comment about the
> other images
Where we have the spinner image?

> 
> @@ +101,5 @@
> > +  -moz-user-select: none;
> > +  border-bottom: 1px solid rgba(0, 0, 0, 0.2);
> > +  padding: 0 !important;
> > +  background: linear-gradient(
> > +          rgba(255, 255, 255, 0.05),
> 
> Please move all hardcoded colors into variables at the top of the file, and
> ideally give those default values to theme variables
Done

> 
> @@ +123,5 @@
> > +  background-color: #AAC3DC;
> > +}
> > +
> > +.treeTable .treeHeaderSorted > .treeHeaderCellBox {
> > +  background: url(chrome://firebug/skin/arrowDown.svg) no-repeat calc(100% - 4px);
> 
> Here are hardcoded firebug icons
Fixed

These changes touches the area of new firebug theme (bug 1244054) and I think it's good timing to already think/discuss about it here.

Here are related comments:

1) I've created a new directory for all Firebug theme images: devtools/client/themes/images/firebug

Stuff in this directory will be accessed through as e.g.:
chrome://devtools/skin/images/firebug/twisty-closed-firebug.svg

2) There is a new file 'firebug-theme.css' for all Firebug theme rules. It's now based on the Light theme (simply importing theme-light.css). Only the font declaration is moved here.

3) There is a new set of variables for the Firebug theme in variables.css
:root.theme-firebug {
  /* Firebug theme vars */
}

Not sure how to sync with commandline.css and commandline.inc.css

Values are copied from the Light theme and I'll replace them step by step.

Brian does that make sense? I'll make the actual firebug-theme registration as part of the bug 1244054. Any other global setup for the theme needed? 

I'll attach updated patch as soon as I have the spinner icon (I can't find it)

Honza
(In reply to Lin Clark [:linclark] from comment #12)
> Comment on attachment 8721363 [details] [diff] [review]
> bug1247065.patch
> 
> Review of attachment 8721363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are a few things that keep me from being confident about a review here:
> 
> 1. I tried manually testing the demo page. It wasn't reliably reproducible,
> but multiple times I saw the wrong thing expand when I clicked on something
> else. I asked Brian and he said he thought he'd seen that too, but he had
> figured that he was just mistaken. I've attached a video that shows this.
> When I click on the item labeled 3, both 1 and 0 expand.
I can't repro that. I'll look into it.

> 2. I'm not sure that I understand the functional requirements enough.
The list of requirements is mostly fetched from collection of trees I've been working on (both in devtools and Firebug) and others identified in our UI (comment #6). I can provide more detailed docs if that would help.

> 3. Related to that, I'd like to see tests that cover the functional
> requirements and ensure that we maintain the required baseline of
> functionality.
Alright, I was waiting with tests, but can do it on Monday.

> 4. I'm not sure I understand the provider pattern used here. I'm not sure
> whether the complexity is necessary or not.
The provider methods (getChildren, hasChildren, getLabel, getValue, getKey, etc.) represent an interface used to access the underlying data. These methods are naturally needed for trees and you can see that Jordan's tree has pretty much the same set of methods (https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/shared/components/tree.js#L147).

The provider is composing these methods into generic interface instead of having them spread within other tree props.

The main purpose of the provider is to separate the tree from specific set of data. It's stateless and only performs necessary computation to 'transform' input data (an object) to a hierarchical data. The tree knows nothing about the input data and it uses the provider to get children for an expanded node (by calling getChildren), get a label (by calling getLabel) to display a node label, etc.

You can see it like as follows: specify generic object as an input for the tree component (tree.prop.object property), specify a provider (tree.prop.provider property) that know how to deal with that input object and done - the tree can display all the data as a hierarchy.

The plan is having several generic providers and use them to populate various trees in our UI.

I am thinking about following providers atm:

1) ObjectProvider (already in the patch) synchronously provides data from generic JS object. Such provider can be used by the JSON Viewer that renders JSON object as well as by HTTP inspector that displays HTTP JSON response.

2) GripProvider (part of the DOM panel patch) asynchronously provides data from given RDP grip object. This provider knows how to asynchronously fetch data from our backend (over RDP) and provide children (using 'prototypeAndProperties' packet), labels etc. so, the tree is able to render any remote JS/DOM object. This provider can be used by the DOM (side)panel, variables view/watch view, inline object inspection in the Console panel and anywhere where we want to display remote JS/DOM object in the UI.

3) XmlProvider (should be part of bug 1247392) this provider should provider data for XML doc preview for HTTP responses. The input object in this case is the result of DOMParser() function. This can be extended to HtmlProvider to render HTML structure as a tree (just support for more rep-types needed),

Does that make sense?

---

There might me more providers and we can nicely share them across the UI and also across components. E.g. a grip provider can provide a list or child-items for a tree component as well as a plain list of items for a drop-down component.

Finally, the Decorator is designed similarly, but for *decorating* the tree (i.e. customizing its visual aspects).

James was suggesting to make the provider more like React component (comment #5) and I am keen to hear more details! Perhaps I can do it before I write the tests? James are you here? :)

Honza
Brian, just NI you due to the spinner icon.
Honza
Flags: needinfo?(bgrinstead)
(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> (In reply to Brian Grinstead [:bgrins] from comment #13)
> > @@ +72,5 @@
> > > +
> > > +/* Spinner (used for async fetch). Needs to have higher priority than
> > > +   theme toggle icons */
> > > +.treeTable .treeRow.hasChildren.loading > .treeLabelCell > .treeIcon {
> > > +  background-image: url(./images/spinner.png) !important;
> > 
> > Could this use an existing spinner image in the tree?  See comment about the
> > other images
> Where we have the spinner image?

There's a class called .devtools-throbber [0] that's used in various tools [1].  Looks like it's not an image but rather using some kind of CSS animation.  We could re-use that class, or ask Helen to create an image version of it if it's not going to work in this case for some reason.

[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#1019
[1]: https://dxr.mozilla.org/mozilla-central/search?q=devtools-throbber&redirect=true&case=false
Flags: needinfo?(bgrinstead)
(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> These changes touches the area of new firebug theme (bug 1244054) and I
> think it's good timing to already think/discuss about it here.
> 
> Here are related comments:
> 
> 1) I've created a new directory for all Firebug theme images:
> devtools/client/themes/images/firebug
> 
> Stuff in this directory will be accessed through as e.g.:
> chrome://devtools/skin/images/firebug/twisty-closed-firebug.svg
> 
> 2) There is a new file 'firebug-theme.css' for all Firebug theme rules. It's
> now based on the Light theme (simply importing theme-light.css). Only the
> font declaration is moved here.
>
> 3) There is a new set of variables for the Firebug theme in variables.css
> :root.theme-firebug {
>   /* Firebug theme vars */
> }

2) and 3) should make it much easier to maintain the theme.  Additionally, we could hook up the Firebug theme to mozscreenshots so we can get image comparisons in automation every night to detect and unexpected changes.

> Not sure how to sync with commandline.css and commandline.inc.css
> 
> Values are copied from the Light theme and I'll replace them step by step.

Does the command line (gcli) *need* to be styled to match Firebug?  Just curious, since it's outside of the toolbox anyway and it'd make things easier if we didn't do it.  I'm also not sure off hand if we are attaching any info about that onto the devtoolstheme attr of the root browser.xul element.
 
> Brian does that make sense? I'll make the actual firebug-theme registration
> as part of the bug 1244054. Any other global setup for the theme needed? 

I'd like to discuss the plans for individual tools in further detail.  Are they each loading a custom style sheet for Firebug?  Let's move that discussion back into Bug 1244054 though.
Attached patch bug1247065.patch (obsolete) — — Splinter Review 
Patch updated, all comments from Brian fixed.

There is only the new spinner, I'll ask Helen to help out here.

Honza
Attachment #8721363 - Attachment is obsolete: true
Attachment #8722097 - Flags: review?(bgrinstead)
Attached image spinner.png — 
(In reply to Brian Grinstead [:bgrins] from comment #17)
> There's a class called .devtools-throbber [0] that's used in various tools
> [1].  Looks like it's not an image but rather using some kind of CSS
> animation.  We could re-use that class, or ask Helen to create an image
> version of it if it's not going to work in this case for some reason.
Helen, I am trying to use an existing spinner for tree-nodes that are asynchronously loading children.

I am displaying the spinner (a rotating circle) instead of the node's toggle icon. (see the attached screenshots). The size of the spinner is currently 9x9, might be different, but I think it should be around the same size as the toggle icon.

Here is the image I am using:
http://janodvarko.cz/devtools/components/tree/www/css/images/spinner.png

I was unable to use the existing ones [1][2] as they seem to be too big and the usage isn't nice.

Can you please create one?

Thanks!
Honza


[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#1019

[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/webide/themes/throbber.svg
Flags: needinfo?(hholmes)
(In reply to Brian Grinstead [:bgrins] from comment #18)
> Does the command line (gcli) *need* to be styled to match Firebug?
No, so problem solved I guess.

Honza
Attached patch bug1247065.patch (obsolete) — — Splinter Review 
Fixing typo in the patch.

Honza
Attachment #8722097 - Attachment is obsolete: true
Attachment #8722097 - Flags: review?(bgrinstead)
Attached patch bug1247065.patch (obsolete) — — Splinter Review 
Latest greatest patch.

Honza
Attachment #8722975 - Attachment is obsolete: true
(In reply to Jan Honza Odvarko [:Honza] from comment #20)
> Created attachment 8722098 [details]
> spinner.png
> 
> (In reply to Brian Grinstead [:bgrins] from comment #17)
> > There's a class called .devtools-throbber [0] that's used in various tools
> > [1].  Looks like it's not an image but rather using some kind of CSS
> > animation.  We could re-use that class, or ask Helen to create an image
> > version of it if it's not going to work in this case for some reason.
> Helen, I am trying to use an existing spinner for tree-nodes that are
> asynchronously loading children.
> [1]
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/
> toolbars.css#1019

I think the .devtools-throbber spinner that bgrins suggested looks really nice, actually. Seems like you could just extend it and make it very small: http://codepen.io/helenvholmes/pen/NNKabW?editors=1100

Is there a reason that wouldn't work?
Flags: needinfo?(hholmes)
Comment on attachment 8724088 [details] [diff] [review]
bug1247065.patch

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

2 fundamental things I found here, still going through the code locally and will review more soon.

::: devtools/client/shared/components/tree/tree-header.js
@@ +84,5 @@
> +    });
> +
> +    return (
> +      DOM.thead({},
> +        DOM.tr({className: visible ? "treeHeaderRow" : ""},

I still think this is strange syntax that I've never seen before. The problem is most editors will want to indent the line below to line up with this. For example if I format it I get:

DOM.thead({},
          DOM.tr({className: visible ? "treeHeaderRow" : ""},
                 cells
                )
         );


Here's how I would format this:

DOM.thead({}, DOM.tr(
  { className: visible ? "treeHeaderRow" : "" },
  cells
));

Please don't the props on the same line as the component if you need to span multiple lines. Also a space between brackets.

::: devtools/client/shared/components/tree/tree-view.js
@@ +355,5 @@
> +  return typeof value == "string" && value.length > 50;
> +}
> +
> +// Exports from this module
> +exports.TreeView = TreeView;

Do not export an object of components, export the component itself. We only want 1 component per file, generally, and even if you have more than 1 component all the rest should just be internal helper components.

This is not just nit-picking; exporting an object breaks hot reloading. You need to export the React component itself for it work (`module.exports = TreeView`
Also the standard is to use `dom`, not `DOM`, because it's much easier to type. You can even destructure elements if you use them a lot (i.e. `const { div, a } = React.DOM`.
Also, btw some of these bigger changes (reformatting a bunch of the code) can happen after it lands. I know you want to land all of this because you have a big deadline and I'm not going to block you on most things.

I would definitely fix the exports, and once you do that try hot reloading! (set "devtools.loader.hotreload" to true, and you should instantly see changes to React components when saving)
Attached patch bug1247065.patch (obsolete) — — Splinter Review 
(In reply to James Long (:jlongster) from comment #25)
> Comment on attachment 8724088 [details] [diff] [review]
> bug1247065.patch
> 
> Review of attachment 8724088 [details] [diff] [review]:
> -----------------------------------------------------------------
Thanks for the review James!

> 
> 2 fundamental things I found here, still going through the code locally and
> will review more soon.
> 
> ::: devtools/client/shared/components/tree/tree-header.js
> @@ +84,5 @@
> > +    });
> > +
> > +    return (
> > +      DOM.thead({},
> > +        DOM.tr({className: visible ? "treeHeaderRow" : ""},
> 
> I still think this is strange syntax that I've never seen before. The
> problem is most editors will want to indent the line below to line up with
> this. For example if I format it I get:
> 
> DOM.thead({},
>           DOM.tr({className: visible ? "treeHeaderRow" : ""},
>                  cells
>                 )
>          );
> 
> 
> Here's how I would format this:
> 
> DOM.thead({}, DOM.tr(
>   { className: visible ? "treeHeaderRow" : "" },
>   cells
> ));
Done

But frankly I don't think that two components on the same line is nice code style. Btw. I don't have any issues with my editor (SublimeText3)

> Please don't the props on the same line as the component if you need to span
> multiple lines. Also a space between brackets.
Done

We should really have some docs for the code style (and/or more eslint rules if possible).

> 
> ::: devtools/client/shared/components/tree/tree-view.js
> @@ +355,5 @@
> > +  return typeof value == "string" && value.length > 50;
> > +}
> > +
> > +// Exports from this module
> > +exports.TreeView = TreeView;
> 
> Do not export an object of components, export the component itself. We only
> want 1 component per file, generally, and even if you have more than 1
> component all the rest should just be internal helper components.
> 
> This is not just nit-picking; exporting an object breaks hot reloading. You
> need to export the React component itself for it work (`module.exports =
> TreeView`
Agreed, done


(In reply to James Long (:jlongster) from comment #26)
> Also the standard is to use `dom`, not `DOM`, because it's much easier to
> type. You can even destructure elements if you use them a lot (i.e. `const {
> div, a } = React.DOM`.
Done (using destructure)


Honza
Attachment #8724088 - Attachment is obsolete: true
Attachment #8725256 - Flags: review?(jlong)
Attached patch bug1247065.patch (obsolete) — — Splinter Review 
Patch updated (one file missing)

Honza
Attachment #8725256 - Attachment is obsolete: true
Attachment #8725256 - Flags: review?(jlong)
Attachment #8725274 - Flags: review?(jlong)
Posting some notes from the meetings yesterday (so we have them when we look back in 6 months).

In Comment #6 Honza provided a list of use cases for this component. Here's a rough take on where the use cases overlap and where they differ:

https://docs.google.com/spreadsheets/d/1gnksUxAowNX9jP0JafXn6tI1TyX_n9hSob7wLEyIJOg/edit

Different use cases will have different needs for expansion, filtering, and sorting. We discussed moving the logic for expansion/filtering/sorting out of the tree components to something else. Separating the logic out would make it easier to reuse the tree components across different use cases.

I'm attaching a rough diagram of what this could look like. We could have methods on the reducer that handle the logic, or it could be handled by something else (e.g. a component higher in the tree).

The diagram uses names from react-virtualized components. We talked about mimic-ing some of that that structure (providing components that take some of the same basic properties) so it would be easy to swap react-virtualized components in.
Going through the code more and will post more comments soon. What Lin mentioned above looks good, I think there is a lot of cool stuff we can do and we can work on it over time. I'm fine landing this as a v1 though before we abstract it out more. I'm going to write some code for this and if I can improve it at all in time, that works too.

(In reply to Jan Honza Odvarko [:Honza] from comment #28)
> Created attachment 8725256 [details] [diff] [review]
> bug1247065.patch
> 
> (In reply to James Long (:jlongster) from comment #25)
> > Comment on attachment 8724088 [details] [diff] [review]
> > bug1247065.patch
> > 
> > Review of attachment 8724088 [details] [diff] [review]:
> > -----------------------------------------------------------------
> Thanks for the review James!
> 
> > 
> > 2 fundamental things I found here, still going through the code locally and
> > will review more soon.
> > 
> > ::: devtools/client/shared/components/tree/tree-header.js
> > @@ +84,5 @@
> > > +    });
> > > +
> > > +    return (
> > > +      DOM.thead({},
> > > +        DOM.tr({className: visible ? "treeHeaderRow" : ""},
> > 
> > I still think this is strange syntax that I've never seen before. The
> > problem is most editors will want to indent the line below to line up with
> > this. For example if I format it I get:
> > 
> > DOM.thead({},
> >           DOM.tr({className: visible ? "treeHeaderRow" : ""},
> >                  cells
> >                 )
> >          );
> > 
> > 
> > Here's how I would format this:
> > 
> > DOM.thead({}, DOM.tr(
> >   { className: visible ? "treeHeaderRow" : "" },
> >   cells
> > ));
> Done
> 
> But frankly I don't think that two components on the same line is nice code
> style. Btw. I don't have any issues with my editor (SublimeText3)
> 
> > Please don't the props on the same line as the component if you need to span
> > multiple lines. Also a space between brackets.
> Done
> 
> We should really have some docs for the code style (and/or more eslint rules
> if possible).

Definitely agree, I don't know if there are any eslint rules for that kind of thing.

I like the consistency between elements with a single property and multiple properties (if you have multiple properties, you need to bump the properties down). I looked at the memory tool and they are indeed inconsistent as well. I think this is rooted in that Sublime works OK with it, while other tools encourage the above style.

We should fine a baseline that works consistently across many tools though, and the above tweaks works in many more tools (as far as I know Vim's smart indentation has the same problem as I do with Emacs, and other IDEs too).

Nick votes for the style I suggested. I hate that we have to decide on anything, but I think it's best to have consistent code, especially in React `render` methods.
Comment on attachment 8725274 [details] [diff] [review]
bug1247065.patch

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

::: devtools/client/shared/components/tree/tree-row.js
@@ +40,5 @@
> +    // The row doesn't have to be re-rendered, all we really need
> +    // to do is toggling a class name.
> +    if (nextProps.member.hidden != this.props.member.hidden) {
> +      let row = ReactDOM.findDOMNode(this);
> +      row.classList.toggle("hidden");

This is definitely not going to work. `shouldComponentUpdate` should never have any side effects, and additionally you should never mutate the DOM that React is managing. We will certainly hit problems with this in the future as this breaks assumptions.

There are other ways we can make stuff like this performant. We just need to make sure that React is doing minimal work when we are filtering. I can't say much more as I don't have a good idea of how everything works yet.
Just a comment from quick IRC conversation with Nick. The existing tree has nice keyboard navigation that should be used: https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/shared/components/tree.js#L403

Honza
(In reply to James Long (:jlongster) from comment #31)
> Going through the code more and will post more comments soon. What Lin
> mentioned above looks good, I think there is a lot of cool stuff we can do
> and we can work on it over time. I'm fine landing this as a v1 though before
> we abstract it out more. I'm going to write some code for this and if I can
> improve it at all in time, that works too.
Sounds great to me.

(In reply to James Long (:jlongster) from comment #32)
> Comment on attachment 8725274 [details] [diff] [review]
> bug1247065.patch
> 
> Review of attachment 8725274 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/shared/components/tree/tree-row.js
> @@ +40,5 @@
> > +    // The row doesn't have to be re-rendered, all we really need
> > +    // to do is toggling a class name.
> > +    if (nextProps.member.hidden != this.props.member.hidden) {
> > +      let row = ReactDOM.findDOMNode(this);
> > +      row.classList.toggle("hidden");
> 
> This is definitely not going to work. `shouldComponentUpdate` should never
> have any side effects, and additionally you should never mutate the DOM that
> React is managing. We will certainly hit problems with this in the future as
> this breaks assumptions.
> 
> There are other ways we can make stuff like this performant. We just need to
> make sure that React is doing minimal work when we are filtering. I can't
> say much more as I don't have a good idea of how everything works yet.
Yeah, I am aware of this. I can remove it or file a follow up, whatever works for you.

Thanks James!

Honza
Ok, I'm hoping to help code this, I'm working on the component locally right now but I'm not sure I'll get anywhere before I need to move on to something else. I'll let you know what I come up with in the next few days. (nothing radical, just showing a few things and maybe helping with the filtering part)
(In reply to James Long (:jlongster) from comment #35)
> Ok, I'm hoping to help code this, I'm working on the component locally right
> now but I'm not sure I'll get anywhere before I need to move on to something
> else. I'll let you know what I come up with in the next few days. (nothing
> radical, just showing a few things and maybe helping with the filtering part)
OK, great!

Honza
I know Lin told you to use ES6 classes... but are we sure that's something we want to do? You don't get auto-bound methods, which is pretty annoying. Additionally, hot module reloading does not work with native ES6 classes yet: https://github.com/gaearon/react-proxy/pull/42

Everything else is using `React.createClass` right now. I'd recommend sticking with that until we make a decision as a team to use something else. There's already too much divergence of style.
I take that back, Lin never suggested it, sorry :) She was working on some eslint rules and considered using classes. Sorry for the mis-attribution!

But we should definitely use `createClass` -- others on IRC mentioned they are only available on Nightly and we aren't 100% confident they work correctly yet. So please switch these components back.
OMG, so confusing. I meant to type "But we should definitely NOT use `createClass`". They are not ready for production yet (evidently they are not shipping, but with perf problems)
Oh boy, I need to quit work today. They ARE shipping, but with perf problems.
The .eslintrc is configured to enforce using ES6 classes. I don't really have a preference, and since it sounds like there are perf issues with our ES6 classes at the moment, I put up a patch to enforce using createClass instead (Bug 1253693).
Attached patch bug1247065.patch (obsolete) — — Splinter Review 
Sure, React.createClass used, new patch attached.

Honza
Attachment #8725274 - Attachment is obsolete: true
Attachment #8725274 - Flags: review?(jlong)
Attachment #8727485 - Flags: review?(jlong)
Jan, I experimented with switching from a table to divs. With very little code I was able to make it work enough to the point where I couldn't find any place that differed (barring minor style tweaks, like rows not having the border and things like that). When it comes to layout it was easy make it work the same way.

I really think we should go this direction, as it will make it easier to compose in arbitrary ways. I'm sure there are a few problems with my attempt, but easily solvable with a little more CSS (I didn't set any kind of `overflow` on the cell, etc).
Okay, I'll check it out tomorrow.

Btw. you can test it together with the DOM panel patch (bug 1201475).

Honza
I did, that's the table I was using as a reference and got working the same :)
Attached patch bug1247065.patch (obsolete) — — Splinter Review 
A little issue fixed (see bug 1201475 comment #23)

Honza
Attachment #8727485 - Attachment is obsolete: true
Attachment #8727485 - Flags: review?(jlong)
Attachment #8727830 - Flags: review?(jlong)
(In reply to James Long (:jlongster) from comment #43)
> Created attachment 8727599 [details] [diff] [review]
> table-to-div.patch
> 
> Jan, I experimented with switching from a table to divs. With very little
> code I was able to make it work enough to the point where I couldn't find
> any place that differed (barring minor style tweaks, like rows not having
> the border and things like that). When it comes to layout it was easy make
> it work the same way.
> 
> I really think we should go this direction, as it will make it easier to
> compose in arbitrary ways. I'm sure there are a few problems with my
> attempt, but easily solvable with a little more CSS (I didn't set any kind
> of `overflow` on the cell, etc).
It's ok for me to use divs as long as it works for all our use cases.

I've noticed the following errors when testing the patch:

Warning: validateDOMNesting(...): <thead> cannot appear as a child of <div>. See TreeView > div > TreeHeader > thead. react.js:18794:9

Warning: validateDOMNesting(...): <tbody> cannot appear as a child of <div>. See TreeView > div > tbody. react.js:18794:9

Warning: validateDOMNesting(...): <div> cannot appear as a child of <tbody>. See TreeView > tbody > TreeRow > div.


I can retest when above errors are fixed.

Honza
Yep, I just did a quick sweep and didn't remove the thead or tbody. All you need to do is remove those.
@Honza I'm not going to spend time fixing up tiny details like converting tbody/thead, my patch was a proof of concept to show that it's not hard. You can test the existing patch and see that almost everything works, but you'd need to tweak styles to match the new DOM structure. If you don't want to convert it right now that's ok, we can do it later. At some point we'll need to integrate this with another tool so we can "formalize" it more then.
(In reply to James Long (:jlongster) from comment #49)
> @Honza I'm not going to spend time fixing up tiny details like converting
> tbody/thead, my patch was a proof of concept to show that it's not hard. You
> can test the existing patch and see that almost everything works, but you'd
> need to tweak styles to match the new DOM structure. If you don't want to
> convert it right now that's ok, we can do it later. At some point we'll need
> to integrate this with another tool so we can "formalize" it more then.
Alright, I've reported Bug 1254951 to cover this.

Honza
Comment on attachment 8727830 [details] [diff] [review]
bug1247065.patch

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

There's only one last major concern of mine here, but otherwise, in the effort of helping you land this so we can ship e10s I'm going to go ahead and r+. I think there are some other things we can do for the "official" tree component but let's go ahead and land this.

::: devtools/client/shared/components/tree/tree-row.js
@@ +65,5 @@
> +    // The row doesn't have to be re-rendered, all we really need
> +    // to do is toggling a class name.
> +    if (nextProps.member.hidden != this.props.member.hidden) {
> +      let row = ReactDOM.findDOMNode(this);
> +      row.classList.toggle("hidden");

I'd like to see this gone before we land this. Can you remove this, and just make `tree-view` rerender and exclude filtered rows? That seems easy enough: check of `hidden` is true in the same place that you check `open`. It should be as fast to filter as it is to expand rows. Is it really that much slower? If true than expanding rows is slow as well, as it rerenders the whole tree then.
Attachment #8727830 - Flags: review?(jlong) → review+
Attached patch bug1247065.patch (obsolete) — — Splinter Review 
(In reply to James Long (:jlongster) from comment #51)
> I'd like to see this gone before we land this. Can you remove this, and just
> make `tree-view` rerender and exclude filtered rows? That seems easy enough:
> check of `hidden` is true in the same place that you check `open`. It should
> be as fast to filter as it is to expand rows. Is it really that much slower?
> If true than expanding rows is slow as well, as it rerenders the whole tree
> then.

So, according to our quick IRC conv, the code is moved into componentWillReceiveProps().

I am yet assigning you for a review so, you can see this bit.

Thanks!
Honza
Attachment #8727830 - Attachment is obsolete: true
Attachment #8728459 - Flags: review?(jlong)
Comment on attachment 8728459 [details] [diff] [review]
bug1247065.patch

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

Yeah that's a lot better. Works for now, thanks.
Attachment #8728459 - Flags: review?(jlong) → review+
Was going to check this in, but:
- The SVGs are not cleaned up (not sure if they are even relevant here, as they belong to the firebug theme)
- The patch seems to contain some unrelated firebug CSS variables changes.
- The reviewer name is wrong (I fixed that locally, but please be careful as not all sheriffs will check the reviewer name)
Keywords: checkin-needed
Attached patch bug1247065.patch — — Splinter Review 
(In reply to Tim Nguyen [:ntim] (busy, email me instead) from comment #54)
> Was going to check this in, but:
> - The SVGs are not cleaned up (not sure if they are even relevant here, as
> they belong to the firebug theme)
> - The patch seems to contain some unrelated firebug CSS variables changes.
Firebug theme (bug 1244054) has high priority and the tree is implemented with support for it already. I think it's alright if this lands now and we continue with bug 1244054 to do the rest.

> - The reviewer name is wrong (I fixed that locally, but please be careful as
> not all sheriffs will check the reviewer name)
Ah, good catch, James was missing in the list of reviewers, fixed.

Honza
Attachment #8728459 - Attachment is obsolete: true
Attachment #8728951 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3ef4546272c1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
theme-light.css should be light-theme.css

tree-view.css still refer to firebug specific images:
77 /* Spinner (used for async fetch). Needs to have higher priority than
78    theme toggle icons */
79 .treeTable .treeRow.hasChildren.loading > .treeLabelCell > .treeIcon {
80   background-image: url(chrome://devtools/skin/images/firebug/spinner.png) !important;
81   background-position: 2px 1px !important;
82   background-size: 9px 9px !important;
83 }
84 
85 /* Default toggle icon. The immediate children operator must be
86  used here since there might be nested tree components inside
87  a tree and we don't want to alter those. */
88 .treeTable .treeRow.hasChildren > .treeLabelCell > .treeIcon {
89   background-image: url(chrome://devtools/skin/images/firebug/twisty-closed-firebug.svg);
90 }
91 
92 .treeTable .treeRow.hasChildren.opened > .treeLabelCell > .treeIcon {
93   background-image: url(chrome://devtools/skin/images/firebug/twisty-open-firebug.svg);
94 }
Depends on: 1255975
Depends on: 1255976
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: