1479421 - Response payload not visible

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Nicolas Frandeboeuf]

Attachment: payload-pane-invisible.png

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.75 Safari/537.36

Steps to reproduce:

Load a large JSON file and look at it in the network panel, in the response tab. Make sure both "JSON" and "Response payload" are open.



Actual results:

When the JSON is long enough (more content than the height of the devtools window), the response payload is not visible, even though it is open. See attached screenshot in the bottom ("Charge utile de la réponse").


Expected results:

The response payload should have been visible

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report I can reproduce the problem on my Win10 machine.

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Some info for anyone interested in fixing this bug:

1) Both response sections are rendered/generated here:
https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/devtools/client/netmonitor/src/components/ResponsePanel.js#190-203

The code belongs to ResponsePanel, which is React component

2) The problem is that the "Response payload" section body height is 0 when the JSON section is opened. It's likely because the first section gets all the available vertical space and the "Response payload" section body doesn't have min-height set...

Use Browser Toolbox Inspector to inspect the Network monitor UI (specifically the Response side panel) to see it.
https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

Honza

Comment 3

[Amy Chan (:amychan331)]

Hi! Can I work on this one?

Comment 4

[Amy Chan (:amychan331)]

Attachment: Bug 1479421 - Making the response panel visible; r?Honza

Making the response panel visible

Comment 5

[Amy Chan (:amychan331)]

Got a bit stuck on trying to change just the height - turns out it was the overflow property that was causing the problem! Also got stuck on the change from hg to Phabricator, but I think I did it right? Please let me know if I made any mistake in the phabricator settings...

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Amy Chan from comment #5)
> Got a bit stuck on trying to change just the height - turns out it was the
> overflow property that was causing the problem! Also got stuck on the change
> from hg to Phabricator, but I think I did it right? Please let me know if I
> made any mistake in the phabricator settings...

The patch is properly uploaded in Phabricator and I just commented on it.

Thanks for the help!
Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 9005099 [details]
Bug 1479421 - Making the response panel visible; r?Honza

Jan Honza Odvarko [:Honza] has approved the revision.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

One more thing, can we also have a test for this? (to avoid regression).

Here is an example test you can get some inspiration from:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_json-long.js
(we coulc just extend this one I think)

You can also search for `#response-panel` in the devtools/client/netmonitor/test directory to see other related tests.

It's ok for me if you want to do this as part of another bug report.

Honza

Comment 9

[Amy Chan (:amychan331)]

Pushed a new update with the comment and testing - so I just add to the browser_net_json-long.js? Is it just checking for the editor container existence like this?

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 9005099 [details]
Bug 1479421 - Making the response panel visible; r?Honza

Jan Honza Odvarko [:Honza] has requested changes to the revision.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 9005099 [details]
Bug 1479421 - Making the response panel visible; r?Honza

Jan Honza Odvarko [:Honza] has approved the revision.

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Amy: The patch works for me, thanks! Don't forget to land it.

Honza

Comment 13

[Amy Chan (:amychan331)]

Unfortunately, I don't have access to make the land - it seems I don't have LDAP account to log into Lando? Is it ok if you help me land it?

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Done

Thanks!
Honza

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8a7ea258709
Making the response panel visible; r=Honza

Comment 16

[Eliza Balazs [:ebalazs_]]

Backed out changeset f8a7ea258709 (Bug 1479421) for ES lint failure in /builds/worker/checkouts/gecko/devtools/client/netmonitor/test/browser_net_json-long.js CLOSED TREE

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f8a7ea258709cd10976898c25e8618d470ae5a39&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=198418865

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

Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3f057af0ba1d3fcbfbf4b15d6e54b809f3b9865e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/netmonitor/test/browser_net_json-long.js:105:1 | Block must not be padded by blank lines. (padded-blocks)

@Amy: this is simple to fix, just remove one unnecessary empty line.
Can you please take a quick look, so we can land it again?

Thanks!
Honza

Comment 18

[Amy Chan (:amychan331)]

Just pushed the latest changes!

Comment 19

[Amy Chan (:amychan331)]

I thought this would have to be landed again because of push failure from ES lint, but I just check Phabricator & realized it was landed - I guessed since it was landed once, it remained landed? Should I changed the status of this bug to resolved?

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Amy Chan from comment #19)
> I thought this would have to be landed again because of push failure from ES
> lint, but I just check Phabricator & realized it was landed - I guessed
> since it was landed once, it remained landed?
It isn't landed.

You need to remove that line and update the patch in phabricator (you might need to reopen that in Phabricator).

> Should I changed the status of this bug to resolved?
No, it'll happen automatically as soon as it's properly landed.

Honza

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 9005099 [details]
Bug 1479421 - Making the response panel visible; r?Honza

Jan Honza Odvarko [:Honza] has requested changes to the revision.

Comment 22

[Amy Chan (:amychan331)]

Actually, I already removed the line and updated the patch earlier - it's the empty line in 104 for Diff 14070, right? Is there another line that I need to remove?
I also tried to reopen the revision in Phabricator following the instruction here: https://wiki.mozilla.org/Phabricator/FAQ#How_do_I_reopen_an_existing_revision_to_submit_more_updates_for_review_.28e.g._following_a_backout.29.3F
But my Revision Actions tab in the dropdown don't have the Reopen Revision option as indicated. Is there another way to reopen it?

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 9005099 [details]
Bug 1479421 - Making the response panel visible; r?Honza

Jan Honza Odvarko [:Honza] has approved the revision.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/255ba5eac283
Making the response panel visible; r=Honza

Comment 25

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Trying to re-land now...
Honza

Comment 26

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/255ba5eac283