Closed Bug 1309188 Opened 8 years ago Closed 8 years ago

Implement Security Panel

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox52 affected, firefox53 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Iteration:
53.3 - Dec 26
Tracking Flags:
Tracking Status
firefox52 --- affected
firefox53 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(2 files, 1 obsolete file)

Bug 1309188 - Implement Security Panel
58 bytes, text/x-review-board-request
Honza
: review+
Details
security.png
77.62 KB, image/png
Details 
This is a breakdown task for bug 1308450 in order to integrate HTTP inspector into the Net panel.

- Implement Security Panel
Whiteboard: [netmonitor]
Flags: qe-verify+
QA Contact: ciprian.georgiu
Priority: -- → P2
Seems like we could use the tree-table component for this.
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #1)
> Seems like we could use the tree-table component for this.
I like the idea

Couple of links:

Displaying JSON as an expandable tree is done for the JSON Viewer.
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/jsonview/components/json-panel.js#L116

The DOM panel is using the same tree component here:
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/dom/content/components/dom-tree.js#L69

Honza
Priority: P2 → P1
Blocks: 1317645
No longer blocks: 1308450
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Iteration: --- → 53.1 - Nov 28
Here is my patch which has already integrated with Jarda's requests view. Security panel is using shared TreeView component, but there is a little bit customizations for it such as 

- adding an additional warning icon after tree-value-cell.
- removing hardcode ":" in order to get rid of duplicated ":" since we've defined the ":" along with l10n string (I don't know whether is a good idea to change this part in l10n file.
- Because there is no id or data-tag props support for each cell in TreeView, only thing I can do is to use textboxes[x] to find element in mochitest.


I'll rebase Jarda's patch once it is landed in m-c and the current patch is for gathering early feedback to speed up review progress.
Comment on attachment 8814569 [details]
Bug 1309188 - Implement Security Panel

https://reviewboard.mozilla.org/r/95764/#review96086

Very nice!

Just one comment related to width.

Honza

::: devtools/client/netmonitor/components/http-inspector/security-panel.js:30
(Diff revision 3)
> +  if (!securityInfo || !url) {
> +    return div();
> +  }
> +  // FIXME: A workaround to get the pane width since the width of html div
> +  // is unable to fit into the parent xul element automatically
> +  const tabboxWidth = $("#details-pane").getAttribute("width");

Width of the element is not changed if width of the sidebar changes and so, the tree view item values (textbox-input) use ellipses even when it isn't necessary.

Could we register resize handler and update the width?

This is also related to
https://bugzilla.mozilla.org/show_bug.cgi?id=1309187#c9
Attachment #8814569 - Flags: review?(odvarko)
Iteration: 53.1 - Nov 28 → 53.2 - Dec 12
Comment on attachment 8814569 [details]
Bug 1309188 - Implement Security Panel

https://reviewboard.mozilla.org/r/95764/#review96450

It's better but, there is still one problem.

STR:


1) Open the Net monitor and the side bar
2) Make the browser window narrow so, the 'vertical' mode applies.
3) Check out the tree, its width is not updated (it's doesn't take the entire width)

Could it be that the "width" attribute of the details-view isn't updated properly?

Honza

::: devtools/client/netmonitor/details-view.js:159
(Diff revision 4)
> +    ReactDOM.unmountComponentAtNode(this._securityPanelNode);
>      this.sidebar.destroy();
>      $("tabpanels", this.widget).removeEventListener("select",
>        this._onTabSelect);
> +    this._splitter.removeEventListener("mouseup", this._onResize);
> +    window.removeEventListener("mouseup", this._onResize);

Remove 'resize' event listener.

::: devtools/client/netmonitor/details-view.js:299
(Diff revision 4)
>  
>    /**
> +   * Listener handling the resize event.
> +   */
> +  _onResize: function () {
> +    const width = $("#details-pane").getAttribute("width");

I can see that the "width" attribute is initialized in NetMonitorview._initializePanes but, I don't understand where it's updated. Is the value *always* up to date?

::: devtools/client/themes/netmonitor.css:1079
(Diff revision 4)
>    }
>  }
> +
> +/* Overwrite tree-view cell colon and use l10n string instead */
> +.treeTable .treeLabelCell::after {
> +  content: "";

It would be great to get rid of the colon charatcters from the localized strings - in a follow up.
Attachment #8814569 - Flags: review?(odvarko)
Comment on attachment 8814569 [details]
Bug 1309188 - Implement Security Panel

https://reviewboard.mozilla.org/r/95764/#review96456

Do we really need to render the tree-table component here? With expandable items and editable values (although they are read-only)? It looks overengineered to me. Wouldn't a simple <ul> list do the same job equally well? It may not be expandable, but I'm not sure if that's really needed and if it justifies the complexity.

The layout shouldn't need any JS code (resize listeners) and should be doable with pure CSS. Setting a XUL flex on the top-level HTML:DIV element (react-security-tabpanel-hook) seems to fix the layout instantly:

div {
  display: -moz-box;
  -moz-box-orient: vertical;
  -moz-box-flex: 1;
}

(In the original XUL version, there is a <vbox flex="1"> element at this place)

After the document is fully HTML, this will be converted to standard flex properties. This should work equally well also for other tabs.

::: devtools/client/netmonitor/components/http-inspector/security-panel.js:94
(Diff revision 5)
> +        })
> +      )
> +    );
> +  }
> +
> +  return div({ style: { width: sidebarWidth } },

This extra DIV can be removed when the "width" style is no longer needed.

Also, consider splitting the two variants (property list vs error) into two components - makes the render() function more readable.
Attachment #8814569 - Flags: review?(jsnajdr)
Honza, 

see comment 12.

Should we go back to use <li> or maybe we can keep tree-view and modify tree-view component to have the ability to disable the expandable button?

how do you think?
Flags: needinfo?(odvarko)
I think there is one useful use case for using TreeView. TreeView is able to fold information which we doesn't want to see to provide more readable interface. After a row has been folded, the folded state for that row can be memorized and bring the benefit to user when comparing data or focusing on a particular value.
Attached image security.png — 
(In reply to Ricky Chien [:rickychien] from comment #13)
> see comment 12.
> Should we go back to use <li> or maybe we can keep tree-view and modify
> tree-view component to have the ability to disable the expandable button?
I actually quite liked the new UI when I was testing it. Specifically the layout - values are rendered in a column, which nicely separates them from labels. And yes, comment #14 makes sense to me.

True is that we should focus on refactoring and avoid changing the existing UI/UX.

---

@Helen, what do you think about this UI change? Can we keep it or
should we rather prefer the previous UI?

To summarize:

* Security information displayed in Net panel side bar were rendered as a hierarchical structure (see attached screenshot)
* They are still displayed hierarchically after refactoring and it's possible to collapse/expand them. Also values are rendered in separate column (see screenshot)
* The new expandable/collapsible state can be restored across Firefox sessions and so, the user can close sections that are not interested.


Honza
Flags: needinfo?(odvarko) → needinfo?(hholmes)
(In reply to Jan Honza Odvarko [:Honza] from comment #15)
> I actually quite liked the new UI when I was testing it. Specifically the
> layout - values are rendered in a column, which nicely separates them from
> labels. And yes, comment #14 makes sense to me.

Well, if you decide to keep the tree-table, I'd recommend to improve at least these things:

- use a CSS only layout instead of implementing it in Javascript
- don't render the values as readonly <input> elements
- why do the field names have a pink color? It's not used anywhere else in devtools, except maybe JS property names in JSON and DOM panels
- the expandable fields' names have a text-decoration:underline when hovered, although they are not links.

I'm still uncomfortable with the fact that a rather heavyweight component is used at a place where a simple <ul>/<li> markup would do just fine.
(In reply to Jarda Snajdr [:jsnajdr] from comment #12)
> Comment on attachment 8814569 [details]
> Bug 1309188 - Implement Security Panel
> 
> https://reviewboard.mozilla.org/r/95764/#review96456
> 
> Do we really need to render the tree-table component here? With expandable
> items and editable values (although they are read-only)? It looks
> overengineered to me. Wouldn't a simple <ul> list do the same job equally
> well? It may not be expandable, but I'm not sure if that's really needed and
> if it justifies the complexity.
I like being able to expand/collapse sections. It allows you to keep only the info that is relevant to you, and UX-wise it's consistent with how we display data in key-value pairs in other tools.
This is why I've originally suggested the tree-table.

> The layout shouldn't need any JS code (resize listeners) and should be
> doable with pure CSS. Setting a XUL flex on the top-level HTML:DIV element
> (react-security-tabpanel-hook) seems to fix the layout instantly:
> 
> div {
>   display: -moz-box;
>   -moz-box-orient: vertical;
>   -moz-box-flex: 1;
> }
> 
> (In the original XUL version, there is a <vbox flex="1"> element at this
> place)
> 
> After the document is fully HTML, this will be converted to standard flex
> properties. This should work equally well also for other tabs.
I agree we shouldn't need any resize listener for this layout.

(In reply to Jarda Snajdr [:jsnajdr] from comment #16)
> (In reply to Jan Honza Odvarko [:Honza] from comment #15)
> > I actually quite liked the new UI when I was testing it. Specifically the
> > layout - values are rendered in a column, which nicely separates them from
> > labels. And yes, comment #14 makes sense to me.
> 
> Well, if you decide to keep the tree-table, I'd recommend to improve at
> least these things:
> 
> - use a CSS only layout instead of implementing it in Javascript
I agree.

> - don't render the values as readonly <input> elements
This makes it easier to copy/paste, and it's consistent with what we had before.

> - why do the field names have a pink color? It's not used anywhere else in
> devtools, except maybe JS property names in JSON and DOM panels
> - the expandable fields' names have a text-decoration:underline when
> hovered, although they are not links.
I agree with those 2.

> I'm still uncomfortable with the fact that a rather heavyweight component is
> used at a place where a simple <ul>/<li> markup would do just fine.
I think using the tree-table is an usability improvement over the simple <ul>/<li> markup.
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #17)
> > - don't render the values as readonly <input> elements
> This makes it easier to copy/paste, and it's consistent with what we had
> before.

If the aim is to make copy&paste easier, then let's get rid of the blue frame around the input and the text cursor. These are affordances suggesting that the value is editable, which it isn't.

The state we had before looks more like buggy and carelessly implemented UI, rather that something desirable. The security info values look like editable, but they aren't. The header values in the Headers tab are actually editable (you can change the value), but it's not saved anywhere - it just doesn't make sense. It's a VariablesView bug that's visible also when inspecting JS objects - the values are editable, but they shouldn't be.

> > I'm still uncomfortable with the fact that a rather heavyweight component is
> > used at a place where a simple <ul>/<li> markup would do just fine.
> I think using the tree-table is an usability improvement over the simple
> <ul>/<li> markup.

OK OK, seems that everybody loves the tree-table :)
Blocks: 1321162
Comment on attachment 8814569 [details]
Bug 1309188 - Implement Security Panel

https://reviewboard.mozilla.org/r/95764/#review96450

> It would be great to get rid of the colon charatcters from the localized strings - in a follow up.

I'd prefer to drop this in current patch but fix it in another follow up.

Does it mean we need to change property name as well?

netmonitor.security.connection=Connection:  ->  netmonitor.security.connection.new=Connection
Comment on attachment 8814569 [details]
Bug 1309188 - Implement Security Panel

https://reviewboard.mozilla.org/r/95764/#review96456

> This extra DIV can be removed when the "width" style is no longer needed.
> 
> Also, consider splitting the two variants (property list vs error) into two components - makes the render() function more readable.

I reused TreeView for both requirements to render property list and error. It only introduced a little condition in renderValue(), but the extra error component has been removed.
(In reply to Ricky Chien [:rickychien] from comment #19)
> Comment on attachment 8814569 [details]
> Bug 1309188 - Implement Security Panel
> 
> https://reviewboard.mozilla.org/r/95764/#review96450
> 
> > It would be great to get rid of the colon charatcters from the localized strings - in a follow up.
> 
> I'd prefer to drop this in current patch but fix it in another follow up.
Sure, read the end of my comment: "in a follow up"

> Does it mean we need to change property name as well?
> 
> netmonitor.security.connection=Connection:  -> 
> netmonitor.security.connection.new=Connection
Yes.

You might also checkout this MDN page:
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

> netmonitor.security.connection.new=Connection
We should rather use a number in the key as follows:
netmonitor.security.connection2=Connection

Which allows us to change the string/key again in the future simply by increasing the number.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> It would be great to get rid of the colon charatcters from the localized
> strings - in a follow up.

Is this really a good idea? The colon character should be always part of the l10n string. Some languages have different conventions how to display the colon. In French, there is space before the colon. In Japanese, a different, non-ASCII character is used.

See, for example, how the about:preferences pages are localized:

http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/main.dtd#7,13
(In reply to Jarda Snajdr [:jsnajdr] from comment #24)
> (In reply to Jan Honza Odvarko [:Honza] from comment #11)
> > It would be great to get rid of the colon charatcters from the localized
> > strings - in a follow up.
> 
> Is this really a good idea? The colon character should be always part of the
> l10n string. Some languages have different conventions how to display the
> colon. In French, there is space before the colon. In Japanese, a different,
> non-ASCII character is used.
> 
> See, for example, how the about:preferences pages are localized:
> 
> http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/preferences/main.dtd#7,13

You're right. In Chinese, it could also be different to use "：" instead of ":" I think we should not touch l10n strings here and further close the bug 1321162.
Comment on attachment 8814569 [details]
Bug 1309188 - Implement Security Panel

https://reviewboard.mozilla.org/r/95764/#review96730

::: devtools/client/netmonitor/components/http-inspector/security-panel.js:115
(Diff revision 7)
> +  return div({ className: "security-info-value" },
> +    member.name === L10N.getStr("netmonitor.security.error") ?
> +      value : input({
> +        className: "textbox-input",
> +        readonly: "true",
> +        value,
> +      })
> +    ,
> +    weaknessReasons.indexOf("cipher") !== -1 &&
> +    member.name === L10N.getStr("netmonitor.security.cipherSuite") ?
> +      div({
> +        id: "security-warning-cipher",
> +        className: "security-warning-icon",
> +        title: L10N.getStr("netmonitor.security.warning.cipher"),
> +      })
> +      :
> +      null
> +  );

This code is almost unreadable. It says: render the value, but in one special case, do something completely different. And it does that as a combo of cryptic ternary operators.

Better design would be to decide about the rendered data (render nothing, render text, render text with icon) when constructing the tree data.

::: devtools/client/themes/netmonitor.css:1097
(Diff revision 7)
> +.theme-light .treeTable .treeLabel,
> +.theme-light .treeTable .treeRow.hasChildren > .treeLabelCell > .treeLabel:hover {
> +  color: var(--theme-highlight-red);
> +}
> +
> +.theme-dark .treeTable .treeLabel,
> +.theme-dark .treeTable .treeRow.hasChildren > .treeLabelCell > .treeLabel:hover {
> +  color: var(--theme-highlight-purple);
> +}

I think that using the aggressive highlight colors (especially the red in the light theme) is a design mistake. Why not use one of the basic text colors from the theme?

I know that this color scheme is already used in the Headers and Cookies panel, but that should be fixed, too.

Helen is already ni? here, I'd like to know her opinion on this.
Attachment #8814569 - Flags: review?(jsnajdr)
(In reply to Jarda Snajdr [:jsnajdr] from comment #26)
> I think that using the aggressive highlight colors (especially the red in
> the light theme) is a design mistake. Why not use one of the basic text
> colors from the theme?
> 
> I know that this color scheme is already used in the Headers and Cookies
> panel, but that should be fixed, too.

Aggressive highlight colors may be a design mistake, but if we changed color here that would introduce the style inconsistency. 

I prefer to keep style as is and don't touch too many UI if possible :) (although I've chosen to TreeView).
(In reply to Ricky Chien [:rickychien] from comment #27)
> Aggressive highlight colors may be a design mistake, but if we changed color
> here that would introduce the style inconsistency. 

If we change some UI in the XUL->React process, it should be a clear improvement. For me, this change is for worse.

By the way, I found a small bug - the "SHA-256 Fingerprint" and "SHA1 Fingerprint" rows have an expansion arrow, although they're not expandable. I don't know where that's coming from.
Comment on attachment 8814569 [details]
Bug 1309188 - Implement Security Panel

https://reviewboard.mozilla.org/r/95764/#review96758

The patch looks already good to me.

R+ assuming try is green.

A few comments:

1) Agree, the code style of the renderValue() methods could be improved. Also because, having comma and colon on separate lines isn't something I'd have seen in our code base.
2) I am ok with the colors. Red color has been used for Cookies and HTTP Headers for ages. We might use blue for the values too, but I don't want to spend too much time discussing what color is the best, I'll let Ricky to decide (I went through such discussions in the past and it wasn't fun). After we are done with the panel refactoring we can ask Helen for UI/UX audit (yet before our changes go to Aurora/DevEdition).
3) We need follow up for remembering the tree state.
4) I've no problem with borders around values. I like that Ctrl+A selects only the value (again, this can be part of the UI/UX audit)


In general we should avoid UI/UX changes during refactoring (and we can see this panel as an exception that shows how hard it is).

Honza
Attachment #8814569 - Flags: review?(odvarko) → review+
(In reply to Jarda Snajdr [:jsnajdr] from comment #26)
> This code is almost unreadable. It says: render the value, but in one
> special case, do something completely different. And it does that as a combo
> of cryptic ternary operators.

Name of renderValue() function came from the TreeView method, I prefer to keep the same name rather than create a new one.
 
> Better design would be to decide about the rendered data (render nothing,
> render text, render text with icon) when constructing the tree data.

Because the condition should be in renderValue() and I think it doesn't make sense to break into smaller functions, ternary operator is a good pattern to solve this problem AFAIK.
BTW, I added a comment on each condition of ternary operator, and that would be helpful to understand and more readable.
This issue needs to be addressed in TreeView component itself. There is an exception condition at 

http://searchfox.org/mozilla-central/source/devtools/client/shared/components/tree/tree-view.js#204

which makes a labelCell be expandable if string.length > 50. I'd prefer to add an extra props here to skip this code.
Attachment #8814806 - Attachment is obsolete: true
(In reply to Ricky Chien [:rickychien] from comment #33)
> This issue needs to be addressed in TreeView component itself. There is an
> exception condition at 
Can you please file a bug report for this.

R+ already, but I think we should rename the 'http-inspector' dir to 'shared'

Honza
longString props has been included in this patch in order to control the visibility of long string in some special cases.

Honza, I think it's a small changes for TreeView and could be fixed along with this patch. It looks like your the owner of TreeView component, so you could take a look in this changes.
Flags: needinfo?(odvarko)
Comment on attachment 8814569 [details]
Bug 1309188 - Implement Security Panel

https://reviewboard.mozilla.org/r/95764/#review97484

Yep, we can do the tree change as part of this patch.

Please see my inline comment.

Honza

::: devtools/client/shared/components/tree/tree-view.js:101
(Diff revision 10)
>        // Custom sorting callback
>        onSort: PropTypes.func,
>        // A header is displayed if set to true
>        header: PropTypes.bool,
> +      // Long string is displayed if set to true
> +      longString: PropTypes.bool,

Can we rename to something like: `expandableStrings`?

Honza
Attachment #8814569 - Flags: review+ → review?(odvarko)
Flags: needinfo?(odvarko)
Comment on attachment 8814569 [details]
Bug 1309188 - Implement Security Panel

https://reviewboard.mozilla.org/r/95764/#review97510

::: devtools/client/netmonitor/components/shared/security-panel.js:97
(Diff revisions 10 - 11)
>          id: "value",
>          width: "100%",
>        }],
>        renderValue: renderValue.bind(null, securityInfo.weaknessReasons),
>        expandedNodes: getExpandedNodes(object),
> -      longString: true,
> +      expandableStrings: true,

Could we flip the value prop? 

true => There is a toggle button displayed when the string is long

false => The string is always fully rendered, no toggle button.

Honza
As comment 40 mentioned, the expandableStrings prop has flipped and React component has moved to shared/components.
Iteration: 53.2 - Dec 12 → 53.3 - Dec 26
I am seeing a lot of conflicts. Can you please rebase on m-c?

Honza
Patch has rebased on m-c. please try again:)
Comment on attachment 8814569 [details]
Bug 1309188 - Implement Security Panel

https://reviewboard.mozilla.org/r/95764/#review99066

Thanks for resolving my comments, r+ assuming try is green.


I personally like how the UI/UX is done in this patch. I am also planning overall Network panel UI/UX audit for January (need to sync this effort with Patrick and Bryan).

Honza
Attachment #8814569 - Flags: review?(odvarko) → review+
Comment on attachment 8814569 [details]
Bug 1309188 - Implement Security Panel

Clearing the review request for me - Honza did the review, I have nothing more to add.
Attachment #8814569 - Flags: review?(jsnajdr)
Clearing Helen's request, we'll solve the UI/UX as part of general audit in Jan.

Honza
Flags: needinfo?(hholmes)
https://hg.mozilla.org/mozilla-central/rev/45624e3ab760
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
This issue is verified fixed on latest Nightly 53.0a1 (2016-12-19) across platforms:
- Windows 10 x64
- Ubuntu 16.04 x64 LTS
- Mac OS X 10.11.6

Marking here accordingly.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: qe-verify+
Blocks: 1325914
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: