Closed
Bug 1332151
Opened 6 years ago
Closed 6 years ago
Fix layout issues for network monitor sidebar panels
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox53 verified)
Tracking | Status | |
---|---|---|
firefox53 | --- | verified |
People
(Reporter: rickychien, Assigned: rickychien)
References
(Blocks 1 open bug)
Details
(Whiteboard: [netmonitor])
Attachments
(3 files)
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 hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
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 hidden (mozreview-request) |
Updated•6 years ago
|
Flags: qe-verify+
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•6 years ago
|
||
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.
Assignee | ||
Comment 7•6 years ago
|
||
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 8•6 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
There is double scrollbar in the Response panel because of the additional border (adding 1px). See the screenshot. Honza
Comment 11•6 years ago
|
||
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 hidden (mozreview-request) |
Comment 13•6 years ago
|
||
mozreview-review |
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+
Updated•6 years ago
|
Blocks: netmonitor-html
Iteration: --- → 53.5 - Jan 23
QA Contact: ciprian.georgiu
Whiteboard: [netmonitor][triage] → [netmonitor]
Comment 14•6 years ago
|
||
(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•6 years ago
|
||
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•6 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/937f596dc88a Fix layout issues for network monitor sidebar panels r=Honza
Comment 17•6 years ago
|
||
bugherder |
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.
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•