1328828 - Implement Properties View for reusing in network details panels

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

A breakdown task from ParamsPanel a.k.a bug 1317650 for creating a shareable and customized TreeView component to be reused by Headers, Cookies, Params and Response panels.

- This component will import TreeView and SourceEditor. Append SourceEditor inside TreeView so that the SourceEditor can be collapsible.
- We can benefit from it like getting rid of duplicated methods getRowClass(), onFilter(), renderRow() and renderValue() in network details panels.
- Right now, I'd prefer to use PropertiesView as component name, but any suggestions would be appreciated.

Comment 1

[Tim Nguyen :ntim]

It would be nice if the searchbox were included in it as well.

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1328828 - Implement Properties View

Review commit: https://reviewboard.mozilla.org/r/102498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102498/

Comment 4

[Ricky Chien [:rickychien] (inactive)]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #1)
> It would be nice if the searchbox were included in it as well.

Nice suggestion!

I'm still thinking this component is not generic enough to reusable out of netmonitor, so it should be part of netmonitor bug. And I also found that those components in client/shared/components are designed for common purpose (reps, splitter, tabs and tree), so it would be better if we just put properties-view in net monitor/shared/.

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102498/diff/1-2/

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Upload patch for PropertiesView component, and please note that this patch is unable to be tested without combining with ParamsPanel. So please apply this patch with https://bugzilla.mozilla.org/show_bug.cgi?id=1317650#c82 together to see how properties view works.

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Attachment: source-editor.png

Honza found an issue in source editor https://bugzilla.mozilla.org/show_bug.cgi?id=1328500#c13.

Right now, source editor is unable to take the rest of the vertical available space. We should find out a way to work it out.

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

https://reviewboard.mozilla.org/r/102498/#review103208

::: devtools/client/netmonitor/shared/components/properties-view.js:100
(Diff revision 2)
> +  // FIXME: Get rid of name and value properties due to bug 1325930
> +  let jsonString = JSON.stringify({ [name]: value }).toLowerCase();
> +  return jsonString.includes(filterText.toLowerCase());
> +}
> +
> +function renderRow(props) {

I think it'd be a better approach to pass in renderRow as prop to this component, since the current one is quite specific to the Params panel.

::: devtools/client/netmonitor/shared/components/properties-view.js:118
(Diff revision 2)
> +  if (typeof value === "object" &&
> +      (open || !path.includes(L10N.getStr("jsonScopeName")))) {
> +    return null;
> +  }

This check could be a function to pass in as prop:

if (!props.shouldRenderValue({value, path}))
  return null;
}

The component can be more generic like this.

Comment 9

[Tim Nguyen :ntim]

(In reply to Ricky Chien [:rickychien] from comment #7)
> Created attachment 8824302 [details]
> source-editor.png
> 
> Honza found an issue in source editor
> https://bugzilla.mozilla.org/show_bug.cgi?id=1328500#c13.
> 
> Right now, source editor is unable to take the rest of the vertical
> available space. We should find out a way to work it out.

I'm investigating using flexbox (or any better alternative) for the treetable in bug 1329068, since table layouts are really hard to manipulate.

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102498/diff/2-3/

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

https://reviewboard.mozilla.org/r/102498/#review103208

> This check could be a function to pass in as prop:
> 
> if (!props.shouldRenderValue({value, path}))
>   return null;
> }
> 
> The component can be more generic like this.

I'm not going to change to props.shouldRenderValue() here but use if (props.member.level === 0) to hide first level rep summary instead.

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102498/diff/3-4/

Comment 13

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

https://reviewboard.mozilla.org/r/102498/#review103392

Looks good, thanks. I have just a few comments.

::: devtools/client/netmonitor/shared/components/editor.js:40
(Diff revision 4)
> +      lineNumbers: true,
> +      readOnly: true,
> +      value: text,
> +    });
> +
> +    this.deferEditor = this.editor.appendTo(this.refs.editorElement);

It would be nice if we could use appendToLocalElement here, just like debugger.html does. It would not create a special iframe for the editor, and no async code with promises would be needed.

I'm pretty sure that the problems with appendToLocalElement are caused by XUL. If you look at the editor elements created by appendToLocalElement, they are divs and spans, but in the XUL namespace, not HTML.

Please file a followup bug, so that we don't forget to fix this after netmonitor.xul is fully converted to HTML.

::: devtools/client/netmonitor/shared/components/properties-view.js:41
(Diff revision 4)
> +  filterText = "",
> +  filterPlaceHolder = "",
> +  onFilterChange,

This component would be more reusable and easy to use if the filterText/onFilterChange logic didn't go out to Redux and back, but was in the local component state instead.

::: devtools/client/netmonitor/shared/components/properties-view.js:103
(Diff revision 4)
> +  // FIXME: Get rid of name and value properties due to bug 1325930
> +  let jsonString = JSON.stringify({ [name]: value }).toLowerCase();
> +  return jsonString.includes(filterText.toLowerCase());

Is bug 1325930 really relevant here? When I debug the code, I see that there is no JS object that would have "name" and "value" properties - there are exactly the data that were in the request params.

Comment 14

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

https://reviewboard.mozilla.org/r/102498/#review103450

::: devtools/client/netmonitor/shared/components/editor.js:40
(Diff revision 4)
> +      lineNumbers: true,
> +      readOnly: true,
> +      value: text,
> +    });
> +
> +    this.deferEditor = this.editor.appendTo(this.refs.editorElement);

Thanks for finding the root cause! I'll file bug for getting rid of deferEditor and use appendToLocalElement instead.

::: devtools/client/netmonitor/shared/components/properties-view.js:103
(Diff revision 4)
> +  // FIXME: Get rid of name and value properties due to bug 1325930
> +  let jsonString = JSON.stringify({ [name]: value }).toLowerCase();
> +  return jsonString.includes(filterText.toLowerCase());

Yes, bug 1325930 is a similar bug in JSON View.

The object parameter passes in onFilter will contain name and value properties, which is incorrect.

Ah, could you check it again? I still see { name: xxx, value: xxx } when inserting console.log(object)

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102498/diff/4-5/

Comment 16

[Ricky Chien [:rickychien] (inactive)]

(In reply to Jarda Snajdr [:jsnajdr] from comment #13)
> Comment on attachment 8823990 [details]
> Bug 1328828 - Implement Properties View
> 
> https://reviewboard.mozilla.org/r/102498/#review103392
> 
> Looks good, thanks. I have just a few comments.
> 
> ::: devtools/client/netmonitor/shared/components/editor.js:40
> (Diff revision 4)
> > +      lineNumbers: true,
> > +      readOnly: true,
> > +      value: text,
> > +    });
> > +
> > +    this.deferEditor = this.editor.appendTo(this.refs.editorElement);
> 
> It would be nice if we could use appendToLocalElement here, just like
> debugger.html does. It would not create a special iframe for the editor, and
> no async code with promises would be needed.
> 
> I'm pretty sure that the problems with appendToLocalElement are caused by
> XUL. If you look at the editor elements created by appendToLocalElement,
> they are divs and spans, but in the XUL namespace, not HTML.
> 
> Please file a followup bug, so that we don't forget to fix this after
> netmonitor.xul is fully converted to HTML.

Bug 1329211 has filed for addressing this problem.

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

https://reviewboard.mozilla.org/r/102498/#review103626

I like the patch. Just a few inline comments.


Honza

::: devtools/client/netmonitor/shared/components/editor.js:16
(Diff revision 5)
> +
> +/**
> + * CodeMirror editor as a React component
> + */
> +const Editor = createClass({
> +

nit: please remove this empty line

::: devtools/client/netmonitor/shared/components/properties-view.js:38
(Diff revision 5)
> + * Source editor - Enable by specifying object's 1 level property name to "editorText".
> + * Rep - Default enabled.
> + */
> +const PropertiesView = createClass({
> +
> +  displayName: "PropertiesView",

nit: please remove the empty line

::: devtools/client/netmonitor/shared/components/properties-view.js:74
(Diff revision 5)
> +    let filterText = this.state.filterText;
> +
> +    if (!filterText || whiteList.includes(name)) {
> +      return true;
> +    }
> +    // FIXME: Get rid of name and value properties due to bug 1325930

Note that bug 1325930 has been fixed so, changes here need to reflect that.

::: devtools/client/netmonitor/shared/components/properties-view.js:154
(Diff revision 5)
> +            enableInput,
> +            expandableStrings,
> +            expandedNodes: new Set(sectionNames.map((sec) => "/" + sec)),
> +            onFilter: (props) => this.onFilter(props, sectionNames),
> +            renderRow: renderRow || this.renderRowWithEditor,
> +            renderValue: renderValue || this.renderValueWithRep,

I am thinking if the support for 'enableInput' shouldn't be part of this component and not the generic tree. Or do you think it can be used somewhere outside of the network panel?

::: devtools/client/themes/netmonitor.css:1175
(Diff revision 5)
> +.tree-container .treeTable .treeRow.tree-section > .treeLabelCell > .treeLabel:hover {
> +  color: var(--theme-body-color-alt);
> +}
> +
> +.tree-container .treeTable .treeValueCell {
> +  /* FIXME: Make value cell can be reduced to shorter witdh */

typo: witdh -> width

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102498/diff/5-6/

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102498/diff/6-7/

Comment 20

[Ricky Chien [:rickychien] (inactive)]

(In reply to Jan Honza Odvarko [:Honza] from comment #17)
> I am thinking if the support for 'enableInput' shouldn't be part of this
> component and not the generic tree. Or do you think it can be used somewhere
> outside of the network panel?

Input filed is able to benefit to user when they want to select a long value in TreeView, so we can have this feature built-in TreeView component so that developer can enable this feature easily. In short, I have no idea how many use cases outside of network panel will need this feature, but it's still a good option to people who needs to deal with very long string in TreeCell.

I'm ok if we want TreeView to be more generic and get rid of `enableInput` in TreeView, but someone must take responsibility for supporting it anyway, the candidate might be PropertiesView itself.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2f372cf7314
Implement Properties View r=Honza,jsnajdr

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ricky Chien [:rickychien] from comment #20)
> I'm ok if we want TreeView to be more generic and get rid of `enableInput`
> in TreeView, but someone must take responsibility for supporting it anyway,
> the candidate might be PropertiesView itself.
Yep, this is precisely what I mean. No big priority but, we might do in at some point.

Honza

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2f372cf7314