Closed Bug 1332151 Opened 6 years ago Closed 6 years ago

Fix layout issues for network monitor sidebar panels

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox53 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Iteration:
53.5 - Jan 23
Tracking Flags:
Tracking Status
firefox53 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(3 files)

Bug 1332151 - Fix layout issues for network monitor sidebar panels
59 bytes, text/x-review-board-request
Honza
: review+
Details
double-scrollbar-response-panel.png
74.06 KB, image/png
Details
white-bottom-gap.png
62.85 KB, image/png
Details 
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.
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.
Flags: qe-verify+
Priority: -- → P1
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 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?
Attachment #8828231 - Flags: feedback?(johngraciliano)
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.
Attachment #8828231 - Flags: review?(odvarko)
There is double scrollbar in the Response panel because of the additional border (adding 1px). See the screenshot.

Honza
Attached image 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 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
Attachment #8828231 - Flags: review?(odvarko) → review+
Blocks: netmonitor-html
Iteration: --- → 53.5 - Jan 23
QA Contact: ciprian.georgiu
Whiteboard: [netmonitor][triage] → [netmonitor]
(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);
    }
  }
}

/****************************************************************************************/
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.
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/937f596dc88a
Fix layout issues for network monitor sidebar panels r=Honza
https://hg.mozilla.org/mozilla-central/rev/937f596dc88a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.