Closed Bug 1358414 Opened 7 years ago Closed 5 years ago

Introduce column resizer in request list

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 67
Tracking Flags:
Tracking Status
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)

react-dimension-slowness.png
261.34 KB, image/png
Details
Bug 1358414 - Introduce column resizer in request list; r=Honza
47 bytes, text/x-phabricator-request
Details | Review 
Chromium devtools is using column resizer to control column width. It's nice to have feature and bring flexibility, so user can adjust their request table as they want.

As comment https://bugzilla.mozilla.org/show_bug.cgi?id=1349173#c36 mentioned, column resizer is good solution to distribute available column width, it's also can fix the bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1349173#c26) when hiding column in request list.
Iteration: 55.4 - May 1 → ---
Flags: qe-verify?
Priority: P1 → P2
Did not face face issue before (54.0.1, 54.0 and previous), but faced it today after moving to 55 release.
(In reply to Alexandr Paliy from comment #3)
> Did not face face issue before (54.0.1, 54.0 and previous), but faced it
> today after moving to 55 release.

The DevTools never allowed to resize the columns in the Net Monitor. This report asks to add this feature.

Sebastian
This is a very highly requested feature. I've heard 2 requests in the past week. One today on IRC #devtools.
I think it would be worth spending time making the bug approachable at least so that people can come in and try to fix it.

Ricky: do you think you could point people in the right directions here? Which files would need to be modified for this? Which components? Whether a pref should be added to store the sizes? What are the edge cases to keep in mind? Where is the complexity? Etc.

I think this would help a lot someone (with some DevTools hacking experience) to get started.
And then maybe, setting yourself (or Honza?) as mentor on the bug would make it more visible too.
Flags: needinfo?(rchien)
Couple of notes:

* The solution should be based on React/Redux (no XUL like the Storage panel)
* We might want to get inspiration from https://github.com/react-dnd/react-dnd
* The result component should be shared among panels (e.g. used in the Storage panel)
* Good analysis prior implementation needed.

Honza
Hi Honza,
I also found this library to suit our needs
https://facebook.github.io/fixed-data-table/example-resize.html
Your thoughts on whether we can use fixed-data-table to solve this issue?
Flags: needinfo?(odvarko)
(In reply to Abhinav Koppula from comment #7)
> Hi Honza,
> I also found this library to suit our needs
> https://facebook.github.io/fixed-data-table/example-resize.html
> Your thoughts on whether we can use fixed-data-table to solve this issue.
Yep, looks promising. I don't like how the header labels are selected when dragging the column marker, but that's probably just a little detail.

One thing to note. We want to change the UI/UX so, entries in the table are expandable, and so the user can see all the HTTP details (headers, response, etc.) underneath of the request (just like in the Console panel) instead of in the side bar.

So, height of a row doesn't have to be always the same.

Honza
Flags: needinfo?(odvarko)
Sorry for the late reply. I'm busy in shipping 57 and Mozilla dev conference.

I believe Honza has mentioned useful notes on comment 6 and fixed-data-table seems like the right component we want.

Feel free to ask Honza and me!

BTW, Each column width can be stored in our pref like `visibleColumn` [1].

[1] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/index.js#24-27
Mentor: rchien, odvarko
Flags: needinfo?(rchien)
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: qe-verify? → qe-verify+
Priority: P2 → P1
Here is my observation of fixed-data-table-2 [1] and react-virtualized [2]

The mechanism behind fixed-data-table-2 is implemented by `transform: translate` to position each table row individually. Take a look at its example [1] to see how it performs while scrolling in a large amount of data (> 1,000,000 rows in this example). The transform mechanism is pretty fitted into Firefox since it doesn't cause any conflict with APZ. I'm sure fixed-data-table-2 is using recycle pool to maintain the list of data just like react-virtualized, but it won't render the blank result when scrolling too fast. It performs pretty fast and smooth!

However, we need to pay more attention on dealing with responsive table [3] since the dimension of Netmonitor flexible. The resizing performance of example [3] is noticeable janky and it's unacceptable. It's using react-dimension to adjust the table dimension [4]. Thus, we'd probably find out a better approach to make table resizing more faster and responsive.

On the other hand, react-virtualized doesn't use `transform: translate` way to position table row, it's choosing `position: absolute` to layout the table. Sounds similar but it will conflict with APZ and incurs blank screen if user scrolls the table too fast.

If we can find out a solution to deal with resizing issue of fixed-data-table-2, I believe fixed-data-table-2 is a nice library for Netmonitor. I'll keep investigating it to make sure fixed-data-table-2 can fulfill our needs.

[1] http://schrodinger.github.io/fixed-data-table-2/example-object-data.html
[2] https://bvaughn.github.io/react-virtualized/#/components/List
[3] http://schrodinger.github.io/fixed-data-table-2/example-responsive.html
[4] https://github.com/schrodinger/fixed-data-table-2/blob/master/examples/ResponsiveExample.js#L11
After modifying the number of FakeObjectDataListStore in example [1] from 1,000,000 to 1,000, I can see the speed of table resizing is way faster than before. 

P.S. In practice, cnn.com is less than 500 requests & bbc.com is less than 300 requests, so I believe 1,000 rows is a reasonable number.

If there is no other obvious issues found in fixed-data-table-2, I'm going to refactor entire request-list with using fixed-data-table-2.

[1] https://github.com/schrodinger/fixed-data-table-2/blob/master/examples/ResponsiveExample.js#L18
Ricky, please consider that the vast majority of our users have slower computers than we have.

Also, remember that the net monitor can persist the requests: if a user keeps this setting enabled and reload the page while working (usual practice, right? :) ), it can achieve a lot more lines than what a website normally is. So while 1,000,000 is likely a lot, I think you should look at numbers like 100,000 (reloading a single webpage more than 300 times in one day while working on it doesn't seem unusual).
Thank you Julien for this good insight!

I've done more investigation about adjusting configuration of react-dimension and reproduce the case of rendering 1,000,000 items in ResponsiveExample. 

After setting `debounce: 50` into react-dimension config [1], I've seen that table horizontal resizing is way better. (I'd prefer to test horizontal or vertical resizing individually since AFAIK there is no way to do horizontal and vertical resizing for toolbox at the same time.)


[1] https://github.com/schrodinger/fixed-data-table-2/blob/master/examples/ResponsiveExample.js#L61
Attachment is an example showing table resizing performance of example-object-data.html in fixed-data-table site.

As you can see in attachment, there are some red bars indicating in FPS timeline and telling us a requestAnimationFrame call from react-dimension took 56.70ms to complete a task. We got other similar red bars while resizing table.

Comparing with current request list implementation, pure flexbox layout is way faster than passing width/height props into react component. By calculating size through layout engine itself which is able to avoid redundant scripting calculation. 

Unfortunately, according to fixed-data-table document [1], width / height props seem to be required.

[1] https://github.com/schrodinger/fixed-data-table-2/tree/master/docs#create-your-table
After taking a glance at the implementation of fixed-data-table-2 [1], there are more than one places requiring `width` props to determine the width of row or width of column, which looks like the width / height props are necessary and impossible to workaround by flexbox.

However, we can enable `debounce` (comment 13) to mitigate the performance issue of horizontal resizing and efficiently reduce the occurrence frequency of red bars.

If above result is still unacceptable, we might go back to see how to implement column resizer without using library.

[1] https://github.com/schrodinger/fixed-data-table-2/tree/master/src
After netmonitor meeting today, we've reached a consensus that not to integrate third party library into our code since both react-virtualized and fixed-data-table-2 have perf downsides or constraints. Instead of adopting new library, we'll start working on column resizer feature by ourselves.
Mentor: odvarko, rchien
Depends on: 1414728
Depends on: 1416096
This feature will depend on fixed-data-table experiment but unfortunately we hit perf bottleneck based on WIP patch https://bugzilla.mozilla.org/show_bug.cgi?id=1414728#c6. Therefore, team have decided to fully focus on perf issue first before jumping into column resizer. We will come back to this soon once the perf issue gets fixed.
Assignee: rchien → nobody
No longer blocks: netmonitor-phaseII
Status: ASSIGNED → NEW
Keywords: regression
Version: 54 Branch → unspecified
Depends on: 1416161
Depends on: 1416714
Severity: normal → enhancement
Depends on: 1431395
Should bug 1308451 be closed as duplicate or should it be used to implement the widget code and this one to use the widget in the Netmonitor?

Sebastian
Flags: needinfo?(odvarko)
(In reply to Sebastian Zartner [:sebo] from comment #19)
> should it be used to implement the widget code and this one to use
> the widget in the Netmonitor?
Yep, let's keep this one open.

Honza
Flags: needinfo?(odvarko)
Depends on: 1308451
Product: Firefox → DevTools
Attached patch resizable-columns-v1.patch (obsolete) — — Splinter Review 
This patch is work in progress:
- Draggable component is introduced in RequestListHeader.js
- basic styling for splitter (draggable) in RequestList.css
Assignee: nobody → lpelechova
Comment on attachment 9034318 [details] [diff] [review]
resizable-columns-v1.patch

Review of attachment 9034318 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this Lenka!

Honza

::: devtools/client/netmonitor/src/components/RequestListHeader.js
@@ +168,5 @@
>  
>      return div({ className }, label);
>    }
>  
> +// Dragging Events

nit: wrong indentation

@@ +248,5 @@
>          div({
>            className: "devtools-toolbar requests-list-headers",
>            onContextMenu: this.onContextMenu,
>          },
>            HEADERS.filter((header) => columns[header.name]).map((header) => {

The filtering is done already above so, we could utilize the result here.

@@ +269,5 @@
> +            // Calculate splitter size
> +            const splitterSize = 5;
> +            const splitterStyle = {
> +              flex: "0 0 " + splitterSize + "px",
> +            };

This looks like something that doesn't have to be in the loop (constants are usually defined at the top of the file).

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.

(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

Status: NEW → ASSIGNED
Attached patch wip-resize-columns-v2.patch (obsolete) — — Splinter Review

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

Attachment #9034318 - Attachment is obsolete: true
Flags: needinfo?(poirot.alex)

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.

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=6d427a3639ada17a968cdf933fbcb28ceb336b47&newProject=try&newRevision=37e99ddf965d49de63358277125f37cb111f46cb&originalSignature=1759151&newSignature=1759151&framework=12

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.

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=6d427a3639ada17a968cdf933fbcb28ceb336b47&newProject=try&newRevision=2a7b1e2625a417840bed56cd93f394612b377b60&originalSignature=1759151&newSignature=1759151&framework=12

@Alex, is this table-layout: fixed; change the one you had in mind when talking about possible perf optimization?

Honza

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.

Flags: needinfo?(poirot.alex) → needinfo?(dholbert)

(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

@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?

(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.

Flags: needinfo?(dholbert)

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.

Attachment #9036770 - Attachment is obsolete: true

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

Flags: needinfo?(dholbert)

(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.

Flags: needinfo?(dholbert)

for review with Honza

Attachment #9038169 - Attachment is obsolete: true
Attachment #9039481 - Flags: review?(odvarko)

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.
Attachment #9039481 - Attachment is obsolete: true
Attachment #9039481 - Flags: review?(odvarko)
Attachment #9039508 - Flags: review?(odvarko)

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

Attachment #9039729 - Flags: review?(odvarko)
Comment on attachment 9039508 [details] [diff] [review]
WIP-resize-columns-01-28-19v2.patch

Review of attachment 9039508 [details] [diff] [review]:
-----------------------------------------------------------------

Review done at a meeting.
Honza
Attachment #9039508 - Flags: review?(odvarko)
Comment on attachment 9039729 [details] [diff] [review]
WIP-resize-columns-01-29-19.patch

Review of attachment 9039729 [details] [diff] [review]:
-----------------------------------------------------------------

Review done at a meeting.
Honza
Attachment #9039729 - Flags: review?(odvarko)

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.

Attachment #9039508 - Attachment is obsolete: true
Attachment #9039729 - Attachment is obsolete: true
Attachment #9040340 - Flags: review?(odvarko)

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.

(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

Flags: needinfo?(felash)

(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

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.

Flags: needinfo?(felash)

The grid layout looks promising. Does grid support "sticky header"? (i.e. the header still visible even if the user scrolls down)

Honza

(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.

Here's :sebo's example, modified to have sticky headers:
https://jsfiddle.net/c4tp3x7q/

(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

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 not minWidth
  • 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.

Attachment #9041401 - Flags: review?(odvarko)
Attachment #9040340 - Flags: review?(odvarko) → review-
Attached patch newpatch-02-05-19.patch (obsolete) — — Splinter Review

corrected patch for honza

Attachment #9040340 - Attachment is obsolete: true
Attachment #9041401 - Attachment is obsolete: true
Attachment #9041401 - Flags: review?(odvarko)

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 :-)

Attachment #9041404 - Attachment is obsolete: true
Attachment #9041447 - Flags: review?(odvarko)
Attached patch grid-layout.patch (obsolete) — — Splinter Review

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.

Here is Talos (DAMP) result:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=5b9cf86786296eacdfd096126cd9e90bd5fbf6a3&newProject=try&newRevision=4de845268fed6072c47a26b69a541c29e4de61a6&originalSignature=1759151&newSignature=1759151&framework=12

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

(In reply to Lenka Pelechova from comment #54)

Created attachment 9041447 [details] [diff] [review]
WIP-resize-columns-02-05-19-v2.patch

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 :-)

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

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.

(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

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

Attachment #9041447 - Attachment is obsolete: true
Attachment #9041486 - Attachment is obsolete: true
Attachment #9041447 - Flags: review?(odvarko)
Attachment #9042022 - Flags: review?(odvarko)

(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 on attachment 9042022 [details] [diff] [review]
WIP-resize-columns-02-07-19.patch

Review of attachment 9042022 [details] [diff] [review]:
-----------------------------------------------------------------

Review done at a meeting.
Honza
Attachment #9042022 - Flags: review?(odvarko)

(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:

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.

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.

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

Attachment #9042022 - Attachment is obsolete: true
Attachment #9042897 - Flags: review?(odvarko)

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

Attachment #9042897 - Attachment is obsolete: true
Attachment #9042897 - Flags: review?(odvarko)
Attachment #9043207 - Flags: review?(odvarko)
Attached patch resizeable-columns-reducer.patch (obsolete) — — Splinter Review

Store columns data into UI reducer + auto save to preferences.

Honza

Attached patch resizeable-columns-reducer.patch (obsolete) — — Splinter Review

@Lenka, please look at this last patch. If all is working fine we should merge both together.

Honza

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)

Attachment #9044103 - Flags: review?(odvarko)
Comment on attachment 9044103 [details] [diff] [review]
WIP-resize-columns-02-15-19v2.patch

Review of attachment 9044103 [details] [diff] [review]:
-----------------------------------------------------------------

@Lenka: browser_net_headers-resize.js test-file seems to be missing in the patch.

Honza
Attachment #9044103 - Flags: review?(odvarko)

added missing test file browser_net_headers-resize.js

Attachment #9043207 - Attachment is obsolete: true
Attachment #9043598 - Attachment is obsolete: true
Attachment #9044103 - Attachment is obsolete: true
Attachment #9043207 - Flags: review?(odvarko)
Attachment #9044118 - Flags: review?(odvarko)
Attached patch auto-scroll.patch (obsolete) — — Splinter Review

@Lenka: please merge this into your patch, it should fix the auto-scroll issue and the related test.

Honza

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

Attachment #9044118 - Attachment is obsolete: true
Attachment #9044201 - Attachment is obsolete: true
Attachment #9044118 - Flags: review?(odvarko)
Attachment #9044572 - Flags: data-review?(odvarko)
Comment on attachment 9044572 [details] [diff] [review]
WIP-resize-columns-02-18-19.patch

Patch discussed at a meeting.
Attachment #9044572 - Flags: data-review?(odvarko)
Depends on: 1529018

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.

02-20-19 update:

  • code refactor
  • still working on the test where we simulate mouse resize
  • work in progress
Attachment #9044572 - Attachment is obsolete: true
Attachment #9045167 - Flags: data-review?(odvarko)
Comment on attachment 9045167 [details] [diff] [review]
WIP-resize-columns-02-20-19.patch

Review done at a meeting.
Honza
Attachment #9045167 - Flags: data-review?(odvarko)

02-21-19 update

  • patch for review
  • code refactoring
  • still working on test browser_net_headers-resize.js
Attachment #9045167 - Attachment is obsolete: true

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.

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:

  1. open Netmonitor & read a request
  2. reset columns
  3. 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

Attached patch resize-columns-rebased.patch (obsolete) — — Splinter Review

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

Attachment #9046216 - Attachment is obsolete: true
Attached patch combined-resize-columns.patch (obsolete) — — Splinter Review

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)
Attachment #9047623 - Attachment is obsolete: true
Attached patch resize-column2.patch (obsolete) — — Splinter Review

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.
Attachment #9048346 - Attachment is obsolete: true

(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.

Attached patch resize-columns3.patch (obsolete) — — Splinter Review

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
Attachment #9048782 - Attachment is obsolete: true
Attached patch resize-columns6.patch (obsolete) — — Splinter Review
Attachment #9049097 - Attachment is obsolete: true
Comment on attachment 9049450 [details] [diff] [review]
resize-columns6.patch

Review of attachment 9049450 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my inline comments
Honza

::: devtools/client/netmonitor/src/actions/ui.js
@@ +140,5 @@
>  
>  /**
> + * Set width of multiple columns
> + *
> + * TODO: doc for @param

Please update the doc

::: devtools/client/netmonitor/src/assets/styles/RequestList.css
@@ +180,5 @@
> + * Make sure headers are not processing any mouse
> + * events. This is good for performance during dragging.
> + */
> +
> +.requests-list-headers.dragging {

Please remove the empty line between the comment and CSS rule

::: devtools/client/netmonitor/src/components/RequestListContent.js
@@ +130,1 @@
>    onResize() {

You can mention bug 1532914 in the comment

Also fix indentation of the comment (the last *)

::: devtools/client/netmonitor/src/components/RequestListHeader.js
@@ +195,5 @@
> +   */
> +  onStartMove() {
> +    // Set cursor to dragging
> +    const container =
> +      document.querySelector(".request-list-container");

This can be on one line:
const container = document.querySelector(".request-list-container");

@@ +236,5 @@
> +
> +    // Calculate new width (according to the mouse x-position) and set to style.
> +    // Do not allow to set it below minWidth.
> +
> +    const newWidth = Math.max(x - headerRefRect.left, minWidth);

You can remove the empty line between the comment and the code

@@ +287,5 @@
> +    const visibleColumns = this.getVisibleColumns();
> +    const lastColumn = visibleColumns[visibleColumns.length - 1].name;
> +    return (lastColumn === "waterfall") ?
> +            visibleColumns[visibleColumns.length - 2].name :
> +            lastColumn;

You can simplify this code to:

getCompensateHeader() {
  const visibleColumns = this.getVisibleColumns();
  const delta = (lastColumn === "waterfall") ? 2 : 1;
  return visibleColumns[visibleColumns.length - delta].name;
}

@@ +350,5 @@
> +  }
> +
> +  /**
> +   * Method takes the total change in width for waterfall column
> +   * and distributes it among all other columns. Retuns an array

Typo: Retuns -> Returns

@@ +360,5 @@
> +      const colWidth = headerRef.getBoundingClientRect().width;
> +      return colWidth;
> +    });
> +
> +    // Devide changeInWidth among all columns but waterfall (that's why -1).

Typo: Devide -> Divide

@@ +411,5 @@
> +      totalPercent += +widthFromStyle; // + converts it to a number
> +    });
> +
> +    // Do not update if total percent is from 99-101% or when it is 0
> +    // - it means all columns are currently hidden (e.g. other panel is opened).

nit: `(e.g. other panel is opened).` -> `(e.g. other panel is currently selected).`

::: devtools/client/netmonitor/src/create-store.js
@@ +76,5 @@
>    return state;
>  }
>  
>  /**
> + * Get column data (width, min-width)

nit: `Get column data` -> `Get columns data`

::: devtools/client/netmonitor/test/browser_net_headers-resize.js
@@ +7,5 @@
> + * Tests resizing of columns in NetMonitor.
> + */
> +add_task(async function() {
> +  // Reset visibleColumns so we only get the default ones
> +  // and not all thatare set in head.js

nit: `thatare` -> `that are`

@@ +65,5 @@
> +  checkSumOfVisibleColumns(columnsData, visibleColumns);
> +  checkAllColumnsChanged(columnsData, oldColumnsData, visibleColumns);
> +
> +  // 3. Check that all rows have the right column sizes.
> +  info("Checking alignement of columns and headers...");

typo: algnement -> alignment
Attached patch resize-columns-behind-pref.patch (obsolete) — — Splinter Review

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

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.

Attachment #9049537 - Attachment description: resize-columns ->behind the pref (Bug 1358414) → Bug 1358414 - Introduce column resizer in request list; r=Honza
Attachment #9049450 - Attachment is obsolete: true
Attachment #9049528 - Attachment is obsolete: true
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13115d00f301
Introduce column resizer in request list; r=Honza

https://hg.mozilla.org/mozilla-central/rev/13115d00f301

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

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.

Status: RESOLVED → VERIFIED
status-firefox67: fixed → verified
Flags: qe-verify+

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.

Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1586281
You need to log in before you can comment on or make changes to this bug.