Closed Bug 1434885 Opened 6 years ago Closed 6 years ago

Netmonitor devtools-toolbar-bottom background is transparent in portrait mode

Categories

(DevTools :: Netmonitor, defect, P2)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 unaffected, firefox60 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- verified

People

(Reporter: magicp.jp, Assigned: glowka.tom, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(2 files)

steps-to-reproduce_20180201_205220.gif
1.70 MB, image/gif
Details
Bug 1434885 - Netmonitor list of requests width and height synced with details panel using dedicated action
59 bytes, text/x-review-board-request
Honza
: review+
Details 
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0 ID:20180131220303

Steps to reproduce:
1. Launch Nightly
2. Open Network Monitor (Ctrl+Shift+E)
3. Dock to side of browser window
4. Go to any sites (e.g. https://www.mozilla.org)
5. Select any requests

Actual results:
devtools-toolbar-bottom background is transparent.

Expected results:
devtools-toolbar-bottom background is not transparent.

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9913d09905e24a360309384695915e2f0891aa93&tochange=58e45c38f6c87481e71a8bd83b39b3ddbbc0532b
Blocks: 1431132
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox58: --- → unaffected
status-firefox59: --- → unaffected
status-firefox-esr52: --- → unaffected
Thanks for the report

Regression from bug 1434885

Honza
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P2
I would like to work on this as good-first-bug. Please assign me.

Thanks.

Spencer
Never mind. I could not reproduce this bug.

Thanks.

Spencer
It seems like the onResize() function https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListContent.js#127 is not called. If I add this.onResize(); in componentDidUpdate, this fixes the issue for me. However I'm not sure if this is the right fix.
Flags: needinfo?(odvarko)
(In reply to Vincent Lequertier from comment #4)
> It seems like the onResize() function
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/
> src/components/RequestListContent.js#127 is not called. If I add
> this.onResize(); in componentDidUpdate, this fixes the issue for me. However
> I'm not sure if this is the right fix.

So, I did the following, but it doesn't help.

  componentDidUpdate(prevProps) {
    let node = this.refs.contentEl;
    // Keep the list scrolled to bottom if a new row was added
    if (this.shouldScrollBottom && node.scrollTop !== MAX_SCROLL_HEIGHT) {
      // Using maximum scroll height rather than node.scrollHeight to avoid sync reflow.
      node.scrollTop = MAX_SCROLL_HEIGHT;
    }
    this.onResize();
  }


---

Note that the request list content is properly resized if I change width of the Toolbox (when docked at the right side), but not when I change height of the side bar (when the portrait/vertical mode is active - Toolbox docked at the right side).

Honza
Flags: needinfo?(odvarko)
Hello!
This bug looked very interesting, so I played around with it a bit and I just wanted to share what I learned.

My initial conclusion is that this event-based onResize way of setting height of RequestListContent will not just work correctly as this component is inside SplitBox which emulates some resizing of left and right (or top bottom ) boxes contained by it, without emulating any events.

Before changes introduced in bug #1431132 correct size of RequestListContent was implemented using CSS, but as the bug states it suffered from some efficiency issues. Though, I am afraid the onResize way may not be the best solution either, combining CSS and JS styling in component full of containers and wrappers can be prone to bugs, which I guess is the current case.

So I studied all the changes https://reviewboard.mozilla.org/r/213962/#review220024, tried this and that, and found solution by setting height 100% just as width 100% was in that changeset (in .requests-list-contents class) and removing all changes related to onResize.

I also tried to secure the repaint optimization, so tested it under active nglayout.debug.paint_flashing_chrome. Visually it looks much better than in current stable firefox, the effect is also similar to before adding height 100%. The conclusion would be that disabling display table-row in .request-list-item was crucial from the efficiency point of view, although it requires some more tests like https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=4bfe58b98d04&newProject=try&newRevision=aa81c3e3eb26b2ab9ee1ed5ec64ef1177495608a&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyConfident=1&framework=1
(In reply to glowka.tom from comment #6)
Sounds like a progress!
Can you please attach a patch?
Honza
Attachment #8954717 - Flags: review?(odvarko)
Comment on attachment 8954717 [details]
Bug 1434885 - Netmonitor list of requests width and height synced with details panel using dedicated action

https://reviewboard.mozilla.org/r/223842/#review230162

Thanks for working on this!

Is there any other solution to fix the transparency issue?

The patch is bad for performance. Using % introduces repaints, which causes perf penalty.

Try run of the patch with perf results:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=4335cd2430a82bb29a77fdffbf48a68f49aedcec&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=netmon&framework=1&selectedTimeRange=172800

There is a 4% regression on complicated.requestsFinished

Honza
Attachment #8954717 - Flags: review?(odvarko)
Comment on attachment 8954717 [details]
Bug 1434885 - Netmonitor list of requests width and height synced with details panel using dedicated action

https://reviewboard.mozilla.org/r/223842/#review230162

Hi! So I implemented it in much different way, actually returning to this event listening approach, but this time taking advantage of SplitBox interface. It works for the reported case and should not cause any performance regression as no CSS is added.
Version number 3
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/661f27f91e1e
Netmonitor list of requests width and height synced with details panel using dedicated action r=Honza
Backed out for ESlint failure in MonitorPanel.js:

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=def924646dff0c5ee32a82638037e611e283fe6a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=661f27f91e1eef7ebe052c6078f0e652c516caee&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=pending&filter-resultStatus=running&filter-resultStatus=success

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=166737635&repo=autoland&lineNumber=267

Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-03-08T12:57:53.031Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/netmonitor/src/components/MonitorPanel.js:103:7 | Expected indentation of 4 spaces but found 6. (indent-legacy)
[task 2018-03-08T12:57:53.031Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/netmonitor/src/components/MonitorPanel.js:103:25 | 'onNetworkDetailsResized' is missing in props validation (react/prop-types)
[task 2018-03-08T12:57:53.031Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/netmonitor/src/components/MonitorPanel.js:106:8 | Missing semicolon. (semi)
[taskcluster 2018-03-08 12:57:53.468Z] === Task Finished ===
Flags: needinfo?(odvarko)
Ups, sorry, I overlooked it due to (now fixed) ESlint misconfiguration. What's the proper flow of fixing those formatting errors? Updating the commit once more or new patch or what?
1) Reopen the review for new patch (button at the top of the review board)
2) Upload the new patch
3) Use review board to land the patch again or set 'checkin-needed' keyword here (see the field at the top of this report)

Honza
Flags: needinfo?(odvarko)
Thank you for clear instruction!

I reopened review and uploaded new patch. I would love to take step 3 as well but, I can neither land the patch nor set the keyword.

Honza, could you do this for me please? Thanks in advance!
Flags: needinfo?(odvarko)
(In reply to glowka.tom from comment #20)
> Thank you for clear instruction!
> 
> I reopened review and uploaded new patch. I would love to take step 3 as
> well but, I can neither land the patch nor set the keyword.
> 
> Honza, could you do this for me please? Thanks in advance!

Done, thanks!
Honza
Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0922530dcf5f
Netmonitor list of requests width and height synced with details panel using dedicated action r=Honza
https://hg.mozilla.org/mozilla-central/rev/0922530dcf5f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
I am seeing the following warning now:

Warning: Failed prop type: The prop `networkDetailsWidth` is marked as required in `RequestListContent`, but its value is `null`.
    in RequestListContent
    in Unknown (created by Connect(Component))
    in Connect(Component) (created by RequestList)
    in div (created by RequestList)
    in RequestList (created by MonitorPanel)
    in div (created by SplitBox)
    in div (created by SplitBox)
    in SplitBox (created by MonitorPanel)
    in div (created by MonitorPanel)
    in MonitorPanel
    in VisibilityHandler
    in Unknown (created by Connect(Component))
    in Connect(Component) (created by App)
    in div (created by DropHarHandler)
    in DropHarHandler (created by App)
    in div (created by App)
    in App
    in VisibilityHandler
    in Unknown (created by Connect(Component))
    in Connect(Component)
    in Provider


Can you please file a follow up bug and look into this?
Thanks!
Honza
Flags: needinfo?(glowka.tom)
Sure, thanks for reporting, here it is: bug 1444594
Please assign it to me.
Flags: needinfo?(glowka.tom)
Assignee: nobody → glowka.tom
Blocks: 1444594
Flags: qe-verify+
Issue cannot be reproduced on Nightly 61.0a1 (2018-03-12) (64-bit): Windows 10.0 x64, Ubuntu 16.04 and Mac OSX Sierra.

I am marking it as verified.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
Flags: qe-verify+
I verified this also on Nightly 60.0a1 (2018-03-12) and it is fixed.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: