1332151 - Fix layout issues for network monitor sidebar panels

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

There are some layout issues appear on sidebar panels after refactoring.

* Vertical splitter is disabled when image shows up in response panel, we should remove all view height workaround `height: calc(100vh - xxx);`.  https://bugzilla.mozilla.org/show_bug.cgi?id=1331821#c12
* Security panel's width doesn't fill the horizontal space.
* Preview panel's width doesn't fill the horizontal space.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1332151 - Fix layout issues for network monitor sidebar panels

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

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Summarize the changes:

* Wrapped the rest of panels (security, timings and preview) with .panel-container class.
* Unify the implementation for security panel. Used PropertiesView instead of TreeView.
  * But there is an extra bottom border in every tree row as same as the other panels, should we keep this?
* Instead of using `height: calc(100vh - 68px);`, I set the limitations for image dimensions with max-width and max-height.
  * It's hard to apply an adjustable image box to fit our need, and make sure we don't break somethings like vertical splitter. So far this is the best outcome I've tried.

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8828231 [details]
Bug 1332151 - Fix layout issues for network monitor sidebar panels

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

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8828231 [details]
Bug 1332151 - Fix layout issues for network monitor sidebar panels

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

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8828231 [details]
Bug 1332151 - Fix layout issues for network monitor sidebar panels

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

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Before this patch, timeline displays incorrectly since the width didn't fill parent. 

This issue is fixed but I also noticed that timeline animation is laggy when switching requests, because CodeMirror in response panel will reload to render source code behind so it slows down the animation.

IMO, this problem could be fixed after details view migration is completed. We're not going to load every tab-panel at the beginning, so CodeMirror will enable only when selecting the right panel.

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8828231 [details]
Bug 1332151 - Fix layout issues for network monitor sidebar panels

John, 

could you please apply my patch to see if it fix the issue you described in https://bugzilla.mozilla.org/show_bug.cgi?id=1331821#c12?

Comment 8

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

Comment on attachment 8828231 [details]
Bug 1332151 - Fix layout issues for network monitor sidebar panels

https://reviewboard.mozilla.org/r/105694/#review106648

I couldn't spot any issues on Windows, just one comment related to tree-sections & borders.

Nice work here!

Honza

::: devtools/client/themes/netmonitor.css:1149
(Diff revision 4)
>    background-color: var(--theme-toolbar-background);
>  }
>  
> -.properties-view .devtools-searchbox,
> +.properties-view .devtools-searchbox {
> -.tree-container .treeTable tr:not(:last-child) td:not([class=""]) {
>    border-bottom: 1px solid var(--theme-splitter-color);

Please keep border-top amd boreder-bottom for tree-section. But tree-section that is displayed first in <tbody> should not have border-top.

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8828231 [details]
Bug 1332151 - Fix layout issues for network monitor sidebar panels

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

Comment 10

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

Attachment: double-scrollbar-response-panel.png

There is double scrollbar in the Response panel because of the additional border (adding 1px). See the screenshot.

Honza

Comment 11

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

Attachment: white-bottom-gap.png

Just a note. The following CSS rule:

  /* Apply flex to table will create an anonymous table element outside of tbody
   * See also http://stackoverflow.com/a/30851678
   * Therefore, we set height with this magic number in order to remove the
   * redundant scrollbar when source editor appears.
   */
  height: calc(100% - 4px);


... is causing white gap at the bottom of the Response side panel, see the attached screenshot. It's ok for now, but we should make a note somewhere and check it again when the entire layout is done properly (planned to be done when Netmonitor.xhtml is in place).

Honza

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8828231 [details]
Bug 1332151 - Fix layout issues for network monitor sidebar panels

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

Comment 13

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

Comment on attachment 8828231 [details]
Bug 1332151 - Fix layout issues for network monitor sidebar panels

https://reviewboard.mozilla.org/r/105694/#review106668

Works for me, thanks Ricky!

Honza

Comment 14

[johngraciliano]

(1) After trying the latest Nightly, "[Bug 1331821] Details Panel covers most of the Network Monitor when docked on the side" appears fixed except when the transfered file is an image.

(2) I found in netmonitor.css the display value for the #react-* nodes was changed from -moz-box to flex.  This has a negative impact in the Security and the Preview tabs: their content may not fill the horizontal space.

(3) When dock on the side and the tool is made narrow, some of the details pane may become hidden beyond the right of the screen.  That is because the pane width matches the width of the inspector's toolbar on the top (that starts with the trash bin).  It should be bound to the view width.

(4) I found the extra code gets the "Response payload" fill the tab panel.  I still believe is too weird, especially because it makes elements be displayed in ways different to their standard appearance.  I may post a simpler alternative in a later comment.  I found it may cause redundant scrollbars to appear.  

The following code can be added (e.g., with Stylish) to solve those issues.  
It has extra code that even allows the "Response payload" shrink when the inspector is in a short space at the bottom of the screen.

/****************************************************************************************/

/* Allow images in the response tab to shrink: */
.response-image-box {
  height: auto; /* Add this line or remove the property setting elsewhere */
}

/* To allow the Security and the Preview tabs fill horizontally */
#react-security-tabpanel-hook,
#react-preview-tabpanel-hook {
  display: -moz-box;
}

/* Stop the details pane from extending beyond visible horizontal space: */
#details-pane {
  max-width: 100vw;
}

/* To prevent redundant scrollbars: */
#react-response-tabpanel-hook > div > div > div > .treeTable > 
      tbody > tr > td > div > .editor-mount.devtools-monospace {
  overflow: hidden;
}
/* The magic number is not needed with that fix */
.tree-container .treeTable tbody {
  height: 100%; /* This should be done elsewhere */
}

/* To allow the "Response payload" shrink vertically */
/* Only works with the Horizontal (Bottom) Layout */
@media not all and (max-width: 700px) {
  @media (max-height: 250px) { /* Only needed when the table is vertically small */
    #react-response-tabpanel-hook > div > div > div > .treeTable > 
          tbody > tr > td > div > .editor-mount.devtools-monospace,
    #react-response-tabpanel-hook > div > div > div > .treeTable > 
          tbody > tr > td > div > .editor-mount.devtools-monospace > iframe {
      max-height: calc(100vh - 70px);
    }
    .theme-firebug #react-response-tabpanel-hook > div > div > div > .treeTable > 
          tbody > tr > td > div > .editor-mount.devtools-monospace,
    .theme-firebug #react-response-tabpanel-hook > div > div > div > .treeTable > 
          tbody > tr > td > div > .editor-mount.devtools-monospace > iframe {
      max-height: calc(100vh - 76px);
    }
  }
}

/****************************************************************************************/

Comment 15

[johngraciliano]

I am sorry. I wrote my comment above without paying attention to anything any of the comments for the bug.  I am surprised to notice the issues may have been already addressed.  Please consider my code; it has plenty of comments.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/937f596dc88a
Fix layout issues for network monitor sidebar panels r=Honza

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/937f596dc88a

Comment 18

[Ciprian Georgiu, Desktop QA]

This fix looks good to me on 53.0a2 (20170125210232) across platforms: Windows 10 x64, Ubuntu 16.04 x64 and Mac OS 10.11.6. Marking here accordingly.