Introduce column resizer in request list
Categories
(DevTools :: Netmonitor, defect, P1)
Tracking
(firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 verified)
People
(Reporter: rickychien, Assigned: lba_2)
References
(Depends on 2 open bugs, Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete, Whiteboard: [netmonitor])
Attachments
(2 files, 30 obsolete files)
Updated•8 years ago
|
Comment 3•7 years ago
|
||
Comment 4•7 years ago
|
||
Comment 5•7 years ago
|
||
Comment 6•7 years ago
|
||
Comment 7•7 years ago
|
||
Comment 8•7 years ago
|
||
Reporter | ||
Comment 9•7 years ago
|
||
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 10•7 years ago
|
||
Reporter | ||
Comment 11•7 years ago
|
||
Comment 12•7 years ago
|
||
Reporter | ||
Comment 13•7 years ago
|
||
Reporter | ||
Comment 14•7 years ago
|
||
Reporter | ||
Comment 15•7 years ago
|
||
Reporter | ||
Comment 16•7 years ago
|
||
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 17•7 years ago
|
||
Updated•7 years ago
|
Comment 19•7 years ago
|
||
Comment 20•7 years ago
|
||
Updated•7 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 22•6 years ago
|
||
Comment 23•6 years ago
|
||
Comment 25•6 years ago
|
||
A quick note about performance. I'm expecting this work to have a significant impact on netmonitor performance. Both possibly positive and negative. So I would suggest pushing work in progress patches early to DAMP in order to have a sense of its impact on perf.
Comment 26•6 years ago
|
||
As I know it is not easy to do so, I pushed the current patch to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=261e05c2f0b71d362ccb6839d4331048298e819c
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=261e05c2f0b71d362ccb6839d4331048298e819c
Comment 27•6 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #26)
As I know it is not easy to do so, I pushed the current patch to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=261e05c2f0b71d362ccb6839d4331048298e819c
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=261e05c2f0b71d362ccb6839d4331048298e819c
Thanks Alex for the help here!
Note that the patch is currently very preliminary, so the results doesn't have to be useful. But, it's great to watch the impact since the very beginning.
Honza
Comment 28•6 years ago
|
||
Here is the actual results page:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=261e05c2f0b71d362ccb6839d4331048298e819c&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=12&selectedTimeRange=172800
The patch may have an impact on simple.requestsFinish which isn't a big deal.
Updated•6 years ago
|
Assignee | ||
Comment 29•6 years ago
|
||
This patch is still just a WIP:
@Alex please have a look. In order for the table to resize - it is currently using display: table-row
in class .request-list-item
(RequestList.css) that you have commented out in regards to the bug 1431132. Please share your thoughts on this.
Thank you,
Lenka
Comment 30•6 years ago
|
||
A follow up for comment #29
Here are some Talos results showing how much the performance regress when we use display: table-row;
CSS prop for .request-list-item class in the Network panel list.
complicated.netmonitor.reload.DAMP 11%
complicated.netmonitor.requestsFinished.DAMP 13%
Note that this props was removed in bug 1431132 to avoid unnecessary repainting. But, it's extremely useful for column resizing - as we can set width on header only and make all <td>s be adjusted automatically.
There is another Talos push, using table-layout: fixed;
, but the same tests are regressing.
@Alex, is this table-layout: fixed;
change the one you had in mind when talking about possible perf optimization?
Honza
Comment 31•6 years ago
|
||
TBH. I never really understood why removing display: table-row; prevented reflows when adding a new row in the table.
We must be asking help from someone that better understand CSS than me.
Daniel, Would you have cycle to help us understand the netmonitor layout bottlenecks? Or know someone who would?
This is about this rule:
https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/devtools/client/netmonitor/src/assets/styles/RequestList.css#572-582
.request-list-item {
position: relative;
/*
display: table-row;
Bug 1431132: Disabling this rule is surprising as we no longer have "rows".
But this helps preventing reflowing the whole requests list anytime we append
a new request/row.
*/
height: 24px;
line-height: 24px;
}
Here is an overview of netmonitor DOM (only a subset of it and I may have forgot important rules...):
<div class="requests-list-table" style="display: table; position: relative; width/height: 100%;">
<div class="requests-list-contents"
style="display: table-row-group; width: XXXpx; height: XXXpx; /* size set by JS */
position: absolute; overflow-x: hidden; overflow-y: auto;">
<div class="devtools-toolbar requests-list-headers-wrapper" style="position: sticky; width: 100%; display: flex;">
<div class="request-list-item" style="display: block; position: relative; height/line-height: 24px;">
<div class="request-list-item" style="display: block; position: relative; height/line-height: 24px;">
<div class="request-list-item" style="display: block; position: relative; height/line-height: 24px;">
...
// <= this is when we add a new div.request-list-item that we get full table reflow when it is display: table-row;
One surprising thing is that div.requests-list-contents is having display: table-row-group; set, but display: block is shown in the computed view?!
The complexity of netmonitor UI is mostly around its sticky column titles (div.requests-list-headers-wrapper). Modifying anything easily break it and/or crush the performance. I'm pretty confident there is way too many hacks that piled up over time and that the final rules we are using make no sense anymore.
So may be we should go from a blank page and see what would be the best rules to use to implement a table with a sticky header and go from there? Break the UI, but see if the performance is better.
Comment 32•6 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #31)
One surprising thing is that div.requests-list-contents is having display: table-row-group; set, but display: block is shown in the computed view?!
I can reproduce this issue too. The Rule panel seems to work well.
Honza
Assignee | ||
Comment 33•6 years ago
|
||
@Alex, thanks for your comments.
I was doing some googling re:table performance and these are some interesting suggestions I found. Maybe some of you with better understanding of how React works can tell, how is the table row being created in our case.
... // <= this is when we add a new div.request-list-item that we get full table reflow when it is display: table-row;
Stack overflow this issue
- it suggests replacing .insertRow() with .createElement('tr')
- also some useful CSS improvements
- also suggests maybe using .createDocumentFragment
- and more, please take a look
Another interesting article
Another discussion
This article is more complex, but might be useful.
Someone who has more deeper understanding can get some suggestions from here for our situation with netmonitor?
Comment 34•6 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #31)
<div class="requests-list-contents"
style="display: table-row-group; width: XXXpx; height: XXXpx; /* size set by JS */
position: absolute; overflow-x: hidden; overflow-y: auto;">
[...]
One surprising thing is that div.requests-list-contents is having display: table-row-group; set, but display: block is shown in the computed view?!
That's because it has 'position:absolute', which converts its display
value to the closest block-level thing. (e.g. inline-flex--> flex, inline-->block, etc) For table-row-group, there is no block-level equivalent, so it just gets directly converted to display:block.
So its display:table-row-group
styling is ignored/unused, as you noticed, as long as it has position:absolute
basically. Table parts (like display:table-row
elements) inside of it will trigger the generation of anonymous table wrapper frames as-needed, so that they can make sense of their world.
RE the slow table-row insertion:
// <= this is when we add a new div.request-list-item that we get full table reflow when it is display: table-row;
You're probably triggering something like this case:
https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/layout/base/nsCSSFrameConstructor.cpp#11296
...which causes anonymous table wrapper frames to be regenerated.
If you can ensure that you're inserting this new table-row into an element that is definitely a table-row-group (in its computed style), you may see better performance because we won't trigger the "rebuild our anonymous table wrappers for all the adjacent improperly-wrapped table parts that maybe should be grouped together" codepath.
Assignee | ||
Comment 35•6 years ago
|
||
This is a patch that's currently not working - I am attaching it for debugging and review with Honza.
Here I am implementing a function that should autoResize the columns, so they take up 100% of the available space. Also should autoResize the columns when the browser window is resized. But it has bugs currently.
Assignee | ||
Comment 36•6 years ago
|
||
Hello Daniel,
That's because it has 'position:absolute', which converts its
display
value to the closest block-level thing. (e.g. inline-flex--> flex, inline-->block, etc) For table-row-group, there is no block-level equivalent, so it just gets directly converted to display:block.
If you can ensure that you're inserting this new table-row into an element that is definitely a table-row-group (in its computed style), you may see better performance because we won't trigger the "rebuild our anonymous table wrappers for all the adjacent improperly-wrapped table parts that maybe should be grouped together" codepath.
I am not sure yet, why the position is set to absolute nor how can I get rid of it and not lose what it was implemented for.
However, I have a question in regards to our table structure in netmonitor.
Current hierarchy of elements:
Hierarchy CSS display prop Table element
<div.requests-list-table> -> table <table>
<div.requests-list-contents> -> table-row-group++ <tbody>
<div.requests-list-headers-wrapper> -> table-header-group** <thead>
<div.requests-list-headers> -> table-row <tr>
<div.request-list-column> -> table-cell <td>
</div.requests-list-column> </td>
</div.requests-list-headers> </tr>
</div.requests-list-headers-wrapper> </thead>
<div.request-list-item> -> table-row <tr>
<div.requests-list-column> -> table-cell <td>
</div.requests-list-column> </td>
</div.request-list-item> </tr>
</div.requests-list-contents> </tbody>
</div.requests-list-table> </table>
++ currently (computed -> block) because position: absolute
** position: sticky
.requests-list-column has no position specified (default is static)
All others have position: relative
Could that be problem (in regards to table behavior) that the <thead> is within <tbody> ? I do not know the reason behind this structure.
Thanks for any insights.
Lenka
Assignee | ||
Updated•6 years ago
|
Comment 37•6 years ago
|
||
(In reply to Lenka Pelechova from comment #36)
Could that be problem (in regards to table behavior) that the <thead> is within <tbody> ?
That is a problem, yes. I don't know if it's an important part of the main problem that you're investigating here, but it's definitely kinda-broken.
Specifically: let's ignore the extra position:absolute
wrinkle for a minute.... The table nesting that you diagrammed above means that we actually end up with two nested tables here, because the inner table-header-group
finds itself without the correct type of table-part parent box that it expects to have. So, it reacts to that by generating an anonymous display:table
box to wrap itself in. And in the frame tree, that anonymous box ends up being the child of the table-row-group
, and the parent of the table-header-group
.
Now, if we take the position:absolute
into consideration, we've still got a version of that same misparenting issue (but now our table-header-box
is just parented by a block
rather than the wrong sort of table-part) -- so the table-header-group
still ends up generating an anonymous display:table
wrapper box as its parent, either way.
So your intuition is right, I think, that it's probably saner for the thead
to not be a child of the tbody
(to avoid generating unexpected extra nested tables). Having said that, there may be organizational reasons that the current structure depends on the grouping that it's got, but I imagine those could be addressed by other tweaks.
Assignee | ||
Comment 38•6 years ago
|
||
for review with Honza
Assignee | ||
Comment 39•6 years ago
|
||
WIP - update for review
Update:
- when resizing 1 column, the added/substracted width should be compensated by the last column before waterfall (if waterfall visible). Otherwise it is the last column visible.
Assignee | ||
Comment 40•6 years ago
|
||
Update: Please apply this patch on top of the previous 9039508: WIP-resize-columns-01-28-19v2.patch
Change in this patch: the proffered widths are stored in % rather than in px.
This is work in progress, for code review for Honza.
thank you,
Lenka
Comment 41•6 years ago
|
||
Comment 42•6 years ago
|
||
Assignee | ||
Comment 43•6 years ago
|
||
Work in progress - this patch is attached for debugging with Honza at a review meeting.
Status update: Switching from using px for column widths to %. However, currently dealing with problem, that the headers (columns) don't stretch across all width of the table.
Comment 44•6 years ago
|
||
Hey, here is a crazy idea: if the width of all columns is to be known, maybe it's not necessary to keep this structure with a real table element and instead use flex. This could reduce the performance problems by removing this very long table with a lot of lines.
Comment 45•6 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #44)
Hey, here is a crazy idea: if the width of all columns is to be known, maybe it's not necessary to keep this structure with a real table element and instead use flex. This could reduce the performance problems by removing this very long table with a lot of lines.
Do you have more details about how the panel content DOM structure with flex would look like? Note that one of the advantages of table based layout is that when changing col-width -> only size of the header (one element) needs to be updated and all rows are adopted automatically.
Honza
Comment 46•6 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #44)
Hey, here is a crazy idea: if the width of all columns is to be known, maybe it's not necessary to keep this structure with a real table element and instead use flex. This could reduce the performance problems by removing this very long table with a lot of lines.
I'd rather say this shouts for grid, not flex, because it's a two-dimensional layout. All that needs to be done then is to adjust the grid-template-columns
property accordingly when resizing or adding/removing columns. I've created a simple example for this.
Sebastian
Comment 47•6 years ago
|
||
Honza, Sebastian, this was exactly my crazy idea: not using a grid-based layout (table or not table). Indeed we'd need to repeat the widths on each lines. But the advantage is that the layout engine doesn't need to compute the whole table and only needs to look at individual lines.
It's totally possible that grid layout with a grid-template-columns
value containing fixed pixels values (or even fr
values which is similar from a layout point of view) can give the same performance. All I'm saying is we should try this and measure.
Comment 48•6 years ago
|
||
The grid layout looks promising. Does grid support "sticky header"? (i.e. the header still visible even if the user scrolls down)
Honza
Comment 49•6 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #48)
Does grid support "sticky header"?
Yeah, position:sticky works in grid and can achieve this sort of thing. There's a demo at e.g. https://codepen.io/stephanmax/pen/odyxWE -- each purple block there is a position:sticky grid item.
Comment 50•6 years ago
|
||
Here's :sebo's example, modified to have sticky headers:
https://jsfiddle.net/c4tp3x7q/
Comment 51•6 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #50)
Here's :sebo's example, modified to have sticky headers:
https://jsfiddle.net/c4tp3x7q/
Actually, you don't even need the extra scroll container, but in that case you need to set the height of the rows via grid-auto-rows
to min-content
or a fixed value to avoid the rows stretching over the height of the grid container when there's no overflow. See https://jsfiddle.net/SebastianZ/zoe9pa78/.
(In reply to Julien Wajsberg [:julienw] from comment #47)
Honza, Sebastian, this was exactly my crazy idea: not using a grid-based layout (table or not table). Indeed we'd need to repeat the widths on each lines. But the advantage is that the layout engine doesn't need to compute the whole table and only needs to look at individual lines.
I'm not sure how this could be faster regarding layout. Do you mean, if there are many rows the layout engine doesn't have to compute the ones that are outside the viewport?
But I agree that different possible solutions should be tested regarding performance, especially because there can be a huge amount of rows.
Sebastian
Assignee | ||
Comment 52•6 years ago
|
||
Work in progress - patch for review at a meeting with Honza.
Status update:
- Inserting thead at the right position immediately fixed the 100% header size
- position:absolute removed from .requests-list-contents
- Using
width
notminWidth
- min-width set for all columns in prefs
- implementing changes from our working prototype in react app into devtools:
Now the table has corrected structure :
<table>
<thead>
<tr>.....</tr>
</thead>
<tbody>
<tr>.....</tr>
<tr>.....</tr>
</tbody>
</table>
We are correctly assigning width to all columns in %
But disabling position:absolute for .requests-list-contents (display:table-row-group) causes no ability to scroll the rows and the row height gets ruined when resizing either the browser window or the height of the devtools window.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 53•6 years ago
|
||
corrected patch for honza
Assignee | ||
Comment 54•6 years ago
|
||
update: This patch has corrected problems with scrolling & row height & status bar that were introduced when we removed position:absolute from .requests-list-contents. This is a working prototype (still WIP :-)
Comment 55•6 years ago
|
||
I refactored the panel DOM structure on top of the suggested Grid layout (see the attached patch) I tried to do only minimal set of changes to see how performant it is.
But, I am seeing perf regressions on the following tests:
complicated.netmonitor.reload.DAMP 12.03%
complicated.netmonitor.requestsFinished.DAMP 17.87%
(these tests usually indicate that rendering/CSS layout is slower)
I'd love to get feedback on this experiment.
E.g. does that patch look ok to you?
Honza
Assignee | ||
Comment 56•6 years ago
|
||
(In reply to Lenka Pelechova from comment #54)
Created attachment 9041447 [details] [diff] [review]
WIP-resize-columns-02-05-19-v2.patchupdate: This patch has corrected problems with scrolling & row height & status bar that were introduced when we removed position:absolute from .requests-list-contents. This is a working prototype (still WIP :-)
Here are Talos results (if I've done it correctly - I am still new at this) for this patch (using html table layout):
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=85490ae15c45ba63e973694386456083d89c00fc&newProject=try&newRevision=a33c5cff8c539393a7c4b9515e543c98b6a86491&originalSignature=1759151&newSignature=1759151&framework=12
Comment 57•6 years ago
|
||
I'm not sure how this could be faster regarding layout. Do you mean, if there are many rows the layout engine doesn't have to compute the ones that are outside the viewport?
Thanks for your question, this made me thing more about why I was suggesting this. I think that the main thing is that the Netmonitor isn't a fixed view, it's changing a lot, especially when loading a page: new rows are added very often. Of course, it's slower and slower as the table is growing. So I think that's where my suggestion came from: by adding independant rows, the layout engine doesn't have to recompute the whole table at each new row, instead it can just compute the new added row.
Again, this is just a wild guess and a crazy idea, and maybe using a grid layout will provide just the same thing, and I agree this is the best way if it works like we want. When testing the various scenarios, enabling paint flashing on chrome could help looking at whether Gecko repaints the whole table or just the changed parts.
Comment 58•6 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #57)
I'm not sure how this could be faster regarding layout. Do you mean, if there are many rows the layout engine doesn't have to compute the ones that are outside the viewport?
Thanks for your question, this made me thing more about why I was suggesting this. I think that the main thing is that the Netmonitor isn't a fixed view, it's changing a lot, especially when loading a page: new rows are added very often. Of course, it's slower and slower as the table is growing. So I think that's where my suggestion came from: by adding independant rows, the layout engine doesn't have to recompute the whole table at each new row, instead it can just compute the new added row.
Ok, I've created three new examples, one with table, one with grid and one with flex layout, same HTML structure, same JavaScript code, only different CSS. They both have a button to add 1000 new rows at once and a second one that adds them continuously:
Table: https://jsfiddle.net/SebastianZ/567spj10/
Grid: https://jsfiddle.net/SebastianZ/eab4j32f/
Flexbox: https://jsfiddle.net/SebastianZ/oaz2grnq/
On my machine there is no big difference between flex and grid (~120ms for the last layout), but the normal table layout is about twice as fast. Having said that, the HTML structure in these examples is not optimized for flex and grid, so they might get a little bit more performant after optimizations.
Sebastian
Assignee | ||
Comment 59•6 years ago
|
||
Update:
-
this patch fixes some (not all) of the issues from our last review
-
some clean up of CSS
Here are talos results for this patch (the regression numbers went down some). -
intended for next review w/Honza
Comment 60•6 years ago
|
||
(In reply to Sebastian Zartner [:sebo] from comment #58)
Table: https://jsfiddle.net/SebastianZ/567spj10/
Grid: https://jsfiddle.net/SebastianZ/eab4j32f/
Flexbox: https://jsfiddle.net/SebastianZ/oaz2grnq/On my machine there is no big difference between flex and grid (~120ms for the last layout), but the normal table layout is about twice as fast. Having said that, the HTML structure in these examples is not optimized for flex and grid, so they might get a little bit more performant after optimizations.
Great analysis, thanks Sebastian!
Honza
Comment 61•6 years ago
|
||
Comment 62•6 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #55)
I'd love to get feedback on this experiment.
E.g. does that patch look ok to you?
I've not looked at the patch itself, only at the end result in term of rendering.
With nglayout.debug.paint_flashing_chrome turned on, you can see that there is a repaint on the far right of the netmonitor, everytime a new request is added.
Here is a handy test page to check that:
data:text/html,<script>window.onclick=()=>{x=new XMLHttpRequest();x.open("GET","http://mozilla.org",false);x.send();</script>
Clicking on the page will add a new request, it is important to have only the scrollbars to be repainted once the list overflow and requests get added out of the viewport.
Otherwise the end result looks broken, the column are mixed up and the timeline isn't shown.
I believe that it would really help trying to understand and prototype anything if we first get rid of all the cruft:
- useless DOM Elements thanks to react now supporting returning list of elements
For example this wrapper:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListContent.js#278
div({ className: "requests-list-wrapper" },
Or this other wrapper:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListHeader.js#170
div({ className: "devtools-toolbar requests-list-headers-wrapper" },
Or may be all these intermediate containers:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/netmonitor.css#34-37
#mount,
.launchpad-root,
.network-monitor,
.monitor-panel { - all non-mandatory CSS rules
I imagine we should read carefully these two files:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/netmonitor.css
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/RequestList.css
And try removing anything that may not be mandatory.
Especially the rules: overflow, (min-/max-)width/height, display, position.
Also, I remember that I was having an hard time preventing any unecessary reflow because of the toolbar having a variable height. Depending on the width of the window, it can be one or two lines.
It is based on this rule (if I remember correctly)
.devtools-toolbar-container {
height: auto;
}
So if you are having extra repaint, it is always worth removing the header to see if that's a side effect of this complex layout or not.
We do focus a lot of the request list layout, but that may not be the real source of our issues.
We should also look at the very complex setup we have around it! From the <table> up to <body>, we have 8 elements with a lot of various CSS rules, especially flex layouts and some usages of overflow.
Comment 63•6 years ago
|
||
On my machine there is no big difference between flex and grid (~120ms for the last layout), but the normal table layout is about twice as fast. Having said that, the HTML structure in these examples is not optimized for flex and grid, so they might get a little bit more performant after optimizations.
Love the test cases as well! The constrains that right now cause overhead are not scrolling but (as Alex pointed out in his test) the overpainting when adding/updating rows and the layout overhead by table cells with overflow (can't find the bug atm). If we have a layout that is more expensive initially but has less overhead when updating, it might be a viable trade off.
Assignee | ||
Comment 64•6 years ago
|
||
Update 02/11/19:
- fix the resizer in scenario for very narrow browser window
- fixed missing text ellipsis in header labels
- calling resizeWaterfall() when the width of waterfall column changes
- when resizing waterfall - all columns are affected (although still not quite right)
patch for review in the meeting with Honza
Assignee | ||
Comment 65•6 years ago
|
||
Update 02-12-19:
- The header is not be displayed if there are not data (requests)
- when Add/remove columns, widths are recalculated
- in componentDidUpdate(), check if widths need to be updated. Recalculate columns widths, store in prefs
- resizeWaterfall in onStopMove()
- Introduce this.getVisibleColumns()
Work in progress - patch for code review in meeting
Comment 66•6 years ago
|
||
Store columns data into UI reducer + auto save to preferences.
Honza
Comment 67•6 years ago
|
||
Updated version
Updated•6 years ago
|
Comment 68•6 years ago
|
||
@Lenka, please look at this last patch. If all is working fine we should merge both together.
Honza
Updated•6 years ago
|
Assignee | ||
Comment 69•6 years ago
|
||
02-15-19 Update:
- added redux functionality patch
- some fixed tests
Note: one new test browser_net_headers-resize.js is still not finished, and one test browser_net_autoscroll.js is still failing (Work in Progress)
Comment 70•6 years ago
|
||
Assignee | ||
Comment 71•6 years ago
|
||
added missing test file browser_net_headers-resize.js
Comment 72•6 years ago
|
||
@Lenka: please merge this into your patch, it should fix the auto-scroll issue and the related test.
Honza
Assignee | ||
Comment 74•6 years ago
|
||
02-18-19
Thank you @Honza for your help.
Here is a patch with integrated fix for auto-scroll feature, plus the new test for browser_net_headers-resize.js
Lenka
Comment 75•6 years ago
|
||
Comment 76•6 years ago
|
||
I've opened bug 1529018 in order to focus on the current implementation of the netmonitor and why it is so hard to prevent the repaints when new requests are added. Once we will figure this out, it will be easier to ensure we keep preventing these repaints in the column resize refactoring.
Assignee | ||
Comment 77•6 years ago
|
||
02-20-19 update:
- code refactor
- still working on the test where we simulate mouse resize
- work in progress
Comment 78•6 years ago
|
||
Assignee | ||
Comment 79•6 years ago
|
||
02-21-19 update
- patch for review
- code refactoring
- still working on test browser_net_headers-resize.js
Assignee | ||
Comment 80•6 years ago
|
||
02-24-19 Update:
- I have corrected one small bug that caused the new test browser_net_headers-resize.js to fail but the test is still failing. I suspect the problem could be in method moveWaterfall(). I will look at it some more. But here is the current patch.
Assignee | ||
Comment 81•6 years ago
|
||
Honza,
please see if you can spot something... there is a slight change in width that I wouldn't expect and atm I can't figure out why
-
install the latest patch
-
console.log in method moveWaterfall():
new computed width:
const newWaterfall = Math.max(waterfallRefRect.width - diff, minWaterfall);
console.log("moveWaterfall - new width:" + newWaterfall);
and after setting style, console log again:
waterfallRef.style.width =${this.px2percent(newWaterfall, parentWidth)}%
;
console.log("set style for waterfall:" + waterfallRef.style.width);
console.log("new real waterfall width:" + waterfallRef.getBoundingClientRect().width); -
build and test like this:
- open Netmonitor & read a request
- reset columns
- resize waterfall column and check the console
- you will see that the real waterfall width is slightly higher than new width which was computed and set in style (translated to %)
- what I would expect is to see the same real width as the newWaterfall width that was set in .style.width
Similar slight adjustment in width is also happening in onMove with any column.
It is not significant, because the resizing works for the user as expected and the "automatic" resize is just tiny but as it adds up it causes the test to fail. That's how I found out about it.
thanks,
Lenka
Assignee | ||
Comment 82•6 years ago
|
||
needs to be applied on top of these 2 patches
1.first apply this Bug 1529018 - Simplify netmonitor DOM/CSS around table elements
2. then this patch Bug 1529018 - Use table DOM elements instead of div.
3. and after that attachement in this patch
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 83•6 years ago
|
||
03-04-19 update
- This is a resize-columns patch rebased on top of changes from [Bug 1529018] (Figure out netmonitor layout issues)
- just noticed that it doesn't include a fix for missing border on headers from (will add that tomorrow to the patch)
Assignee | ||
Comment 84•6 years ago
|
||
Update [03-06-19]
- updated code + fixed a bug that cause intermittent failure of test devtools/client/netmonitor/test/browser_net_background_update.js
- also confirmed on Talos that removal of onResize() caused indeed a perf regression, so I have put it back in.
Comment 85•6 years ago
|
||
(In reply to Lenka Pelechova from comment #84)
- also confirmed on Talos that removal of onResize() caused indeed a perf regression, so I have put it back in.
I opened bug 1532914 in order to focus on just and only that.
This isn't something you should care if the size set by javascript doesn't interfere with your patch.
Assignee | ||
Comment 86•6 years ago
|
||
Update [03-07-19]
- fixed couple more bugs discovered by testing some edge cases
- @Honza, please review the test browser_net_headers-resize.js
Thanks. - looking into more testing now
Assignee | ||
Comment 87•6 years ago
|
||
Comment 88•6 years ago
|
||
Assignee | ||
Comment 89•6 years ago
|
||
Update [03-08-19]
- the functionality of resizing columns is hidden behind the pref
devtools.netmonitor.features.resizeColumns
where true means enabled. - the push to Try is green
- Talos results are slightly optimistic because the functionality of resizing is disabled (pref is turned off)
- for running all the tests however -> we turn the pref on
- comments from last code review are fixed
Assignee | ||
Comment 90•6 years ago
|
||
Adding feature to netmonitor for resizing of columns. In this patch the functionality is hidden behind the pref devtools.netmonitor.features.resizeColumns. This feature is currently turned off - false.
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 91•6 years ago
|
||
Comment 92•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Updated•6 years ago
|
Comment 93•6 years ago
|
||
Verified - fixed on latest Firefox Beta 67.0b3 (64-bit) on Windows 7/10, Mac OS 10.13, Ubuntu 16.04.
The columns of the request list can now be resized and reset. Tested by switching on the pref mentioned in Comment 89.
Comment 94•5 years ago
|
||
Added:
You can also change the width of the columns to help make the information you are looking for easier to view. The Reset Columns command on the context menu also resets the width of the columns to the default values.
Made a video showing the feature, uploaded to YouTube, and linked to it from the page.
Added the feature to the release notes as well.
Description
•