1495797 - Headers panel should use proper styling

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

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

Similarly to what is reported in bug 1493931 we should:

1) Use proper styling for "Edit and Resend" and "Raw Headers" buttons
2) User proper font and background color.

Honza

Comment 1

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

@Matt: could we get mockup for this panel too?

Honza

Comment 2

[mcroud]

Absolutely, it's on my radar

Comment 3

[Lenka Pelechova]

@Honza, is this a beginner bug for me to work on? All the other good first bugs for outreachy are already assigned so I am looking for something easy to get going. thanks. Lenka

Comment 4

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

Yes, but we need mockups from Matt first.

Honza

Comment 5

[mcroud]

Attachment: Headers panel 1.png

Here is the first iteration of the Headers panel revision. 
This mockup and the questions it asks are currently being discussed. 
The amendments include better Photon 4px grid conformance, actual buttons, correct font usage for property values and correct background color usage for panel headers. 
Following critique I will respond with the next iteration!

Comment 6

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

Thanks for the mockup Matt, looks great!

Two comments:
1) Do we also need a mockup for the light theme?
2) What about moving the buttons to the top - just like we do in 'Edit and Resend' - following the same UI/UX pattern?

Honza

Comment 7

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

Re: question #1) Yeah, this is hard one. I feel like we are being inconsistent with the color. But, having all the ? icons visible all the time might have negative impact on readability. Curious what others think.

Re: question #2) Yes, I think that both (name, value) should be monospace.

Honza

Comment 8

[Lenka Pelechova]

Hello Matt, thank you it looks nice.
Although the one in 2 columns is nicer it seems that other tabs are also in one column only:
Cookies, Cache, Params and Security are all like Headers right now - in one column only. 
Same way - in one column is also the detail of the request header (see Request URL, request method, status code, version)
I don't know right now where the computed styles are :-)
thanks,
Lenka

Comment 9

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

Ah, forgot to comment on 1 vs. 2 columns.

I vote for one column (the current state) since header value can be long and there is more space (no gap between columns).

Honza

Comment 10

[mcroud]

@Honza, thank you for the feedback, to answer your questions:

1) Oh absolutely, I was waiting to see what/if layout changes would be required before mocking up both, but both light and dark will be provided for final designs

2) I suppose this depends on whether the two buttons relate specifically to the "Status code" field. They are currently inline with this field and so appear to share a relationship. If this isn't the case then there is certainly an argument to move them up to the top.

@Lenka

Thank you for the input!

Comment 11

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

(In reply to mcroud from comment #10)
> 2) I suppose this depends on whether the two buttons relate specifically to
> the "Status code" field. They are currently inline with this field and so
> appear to share a relationship. If this isn't the case then there is
> certainly an argument to move them up to the top.

There is no relation between the 'Status code' and the buttons.
'Edit and Resend' - opens the Edit and Resend form
'Raw Headers' - Two state button used to switch between nicely formatted/colorized headers and raw header-text sent over the wire.

Honza

Comment 12

[Lenka Pelechova]

Info from Matt Croud: (from Slack)

Just sharing what I placed on #ux below, the discussion of button placement and the user journey has forked ideas, there are prototypes below which show what has been discussed.

As the Edit/Resend and Headers panel are all apart of the same journey, It paid to consider both of these mockups when thinking about the button placement.

My thoughts on each is perhaps too much for slack but I am sharing these prototypes here and invite you to run through them. On each prototype the “Edit and resend”, “Raw headers” and “Cancel” buttons are all clickable. I do find Invision a tad flakey so if for some reason the buttons suddenly stop being clickable just refresh the page.

Proto 1
Separating “Raw Headers” from “Edit and Resend”
https://mozilla.invisionapp.com/share/2YONI9OX96A#/326350849_Headres_Panel_Journey_1a

Proto 2
“Raw headers” above Filter headers input bar
https://mozilla.invisionapp.com/share/M3ONIC8K4DC#/326353973_Headres_Panel_Journey_2a

Proto 3
“Raw headers” toggle, primary button for “Edit and Resend”
https://mozilla.invisionapp.com/share/KBONIE3HP5E#/326367343_Headres_Panel_Journey_3a

Proto 1 I find has a usability issue in that revealing the raw headers boxes separates the Filter headers input from the actual properties panel it affects.

The scope of these tasks is to update the individual components to match current styling. I would love to see a toggle which swaps between raw and pretty printed headers with the same pane but that’s perhaps a discussion for another time

Comment 13

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

(In reply to lba_2 from comment #12)
> Info from Matt Croud: (from Slack)
Thanks for the analysis!

There is also this mockup, which I think depicts what we want:
https://mozilla.invisionapp.com/share/C7OT12ISM3N#/screens/327991804_Headers_Panel_7

- Raw headers checkbox is placed within Request & Response header titles
- Edit and Resend button is placed at the right side above the filter-headers input box
- There should be 2 more pixels of white space (padding) below the Filter input

There are other two bugs filled:
1) Bug 1503554 - Network header tab does not work in small viewport heights
2) Bug 1506073 - URL in the Headers panel should break onto multiple lines

Both should be mostly CSS work. Use Browser Toolbox to inspect the UI of the Headers panel
https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

In case of the #1 you need to play with y-overflow, remove existing 'auto' from inner parts of the panel and then set it on to the root .panel-container element (not that there is yet one `panel-container` element within the inner parts, so you might need to come up with new class name).


Honza

Comment 14

[mcroud]

Just posting an update here as I am aware there has been radio silence from me for a while. 
The remaining discussion point with this panel has been the left/right padding for the pane content, and the padding around the pane header and the collapse/expand arrow. There is a fair amount of variation among the panes and this particular project has motivated an effort to set a standard moving forward.  

The latest mockup: https://mozilla.invisionapp.com/share/C7OT12ISM3N#/screens/327991804_Headers_Panel_7 is 99% accurate, only the padding remains undecided. Ideally I wanted to produce a final mockup which would set in stone the decisions made by UX. As only a decision on padding needs to be concluded there is the argument that this can be worked on now and the padding can be tweaked in line with the other panes at a later date.

Comment 15

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

(In reply to mcroud from comment #14)
> As only a decision on padding needs to be concluded there is the argument that this can be worked on now
> and the padding can be tweaked in line with the other panes at a later date.
Yes, agree. Thanks for the update Matt!

@Lenka, just use what's on the screenshot and we can create a follow up bug for the padding and fix it as soon as a decision has been made.

Honza

Comment 16

[Lenka Pelechova]

Honza,
I am working on the first step - to move the 'Raw headers' button down next to Response/Request headers (and change it into toggle). 

I see the button is rendered in 
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/HeadersPanel.js#246 and should be probably moved to PropertiesView panel below.

I also see that the Response/Request headers labels are built in PropertiesView.js somewhere in the TreeView.
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/PropertiesView.js#209

Can you please point me to where/how exactly are the labels/values passed into the tree? So I can maybe insert the button/toggle inside the row with label 'Response headers'?

thanks,
Lenka

Comment 17

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

(In reply to lba_2 from comment #16)
> Honza,
> I am working on the first step - to move the 'Raw headers' button down next
> to Response/Request headers (and change it into toggle). 
> 
> I see the button is rendered in 
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/ and should be probably moved to PropertiesView panel below.
The link is mangled, can you please paste it again?
(you can use the preview tab before you send your comment)

Honza

Comment 18

[Lenka Pelechova]

Sorry about that. It messes up when I add the line number e.g. #246 at the end 

I see the button is rendered in 
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/HeadersPanel.js line 246

and should be probably moved to PropertiesView panel below.

I also see that the Response/Request headers labels are built in PropertiesView.js somewhere in the TreeView.
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/PropertiesView.js  line 209

Can you please point me to where/how exactly are the labels/values passed into the tree? So I can maybe insert the button/toggle inside the row with label 'Response headers'?

Comment 19

[Lenka Pelechova]

Honza, I think I am figuring it out. Found the  place in the code. So for now, I am good. Thanks.

Comment 20

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

(In reply to lba_2 from comment #19)
> Honza, I think I am figuring it out. Found the  place in the code. So for
> now, I am good. Thanks.
Excellent!
Honza

Comment 21

[Lenka Pelechova]

Updates from a dependent Bug https://bugzilla.mozilla.org/show_bug.cgi?id=1506073;

See mockup showing how it should look like:
https://mozilla.invisionapp.com/share/C7OT12ISM3N#/screens/327991804_Headers_Panel_7

I noticed much more differences between the last mock-up (above) and the current state in Nightly. 
(Matt Croud) >I believe the wider amends to this panel come under this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1495797

1. different wrapping in the lower section `properties-view' - in mock-up the value wraps underneath the label

2. the color of info question mark is lighter in mock-up
(Matt Croud) >The (?) icons are now at 40% opacity (100% on row hover), this reduces their intensity in a cluster and follows the Photon guidelines.

3. the padding on the left of the header section is aligned with Response headers title

4. the Filter headers bar is round in mock-up
(Matt Croud) >Search/filter inputs are rounded on Mac but remain square on Windows. Best leave that one then!

5. lower case used now for properties labels but in mock-up is used upper case letters also in Response headers

6. in the light mode, the color of labels in header summary is black (bold?) while in Nightly it is grey
(Matt Croud) >I'm not 100% sure what you are referring to as the labels in header summary. If you are referring to "Request URL", "Request method", "Request method" etc I see this as Grey-70 in Nightly which is the same in the mockups.

Comment 22

[mcroud]

> 1. different wrapping in the lower section `properties-view' - in mock-up the value wraps underneath > the label

Ah yes, would it be a pain if I say hold fire on this one? I made this change as I felt the varying wrap positions made it more difficult to follow so I mockep up a consistent method of simply appearing as a newline. No comments were made but if I may, let me quickly send this to Slack for some quick feedback to see if there are strong opinions for/against.

> 5. lower case used now for properties labels but in mock-up is used upper
> case letters also in Response headers

Maybe I'm not looking in the right place, but the properties (Accept-Ranges, Cache-Control...) within the Response header and Request header sections use the same title case as the Mockup in my Nightly?

Comment 23

[mcroud]

Ok I've shared the property line wrapping mockup with the UX team and early feedback seems to be in favour of the mockup version (wraps as a new line underneath the property name). I will keep an eye out for any strong opinions but it looks good to go ahead.

Apologies for the interruption late in the process!

Comment 24

[Lenka Pelechova]

Attachment: First attempt - not complete, just for review

Comment 25

[Lenka Pelechova]

Attachment: headers-panel-Bug1495797.patch

Please review:

• this patch introduces the toggle buttons for raw headers - one button for each (request/response)
• different styling changes per Mockup

@Honza - please have a look:
There is still one bug in HeadersPanel.js with following STR:
1.open browser
2.open devtools
3.load a webpage
4.click on any request
5.toggle raw headers
=> when it is done in this order, for some reason the value used (for raw headers display) in method renderRow is not present.
=> if a user first loads a page and then opens devtools and looks at raw header all is fine.
Please let me know if you see what is the problem.
Thanks,
Lenka

Comment 26

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

Comment on attachment 9035613 [details] [diff] [review]
headers-panel-Bug1495797.patch

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

Thanks for the patch Lenka, looks great!

Some comments:

1) The `Raw headers` label is not vertically aligned with the toggle button. 
lso, it uses `font-weight: 600;` (coming from .network-monitor .tabpanel-summary-label) , which isn't right.

2) Also note that `font-weight: 600;` for the summary labels (e.g. Request URL, etc.) doesn't work well for me. I am not seeing bold font for the labels as it was before. Perhaps it should rather be: `font-weight: bold;`?

3) The textarea for raw headers should not have border

4) The font-family for raw headers text should be: the same as for the summary at the top of the form - font-family: var(--monospace-font-family);

Please see also my inline comments


Honza

::: devtools/client/netmonitor/src/components/HeadersPanel.js
@@ +60,5 @@
>  
>  /**
> +* Headers panel component
> +* Lists basic information about the request
> +*/

nit: use proper indentation for the comment
(missing spaces at the beginning)

@@ +115,5 @@
> +
> +      if (title === RESPONSE_HEADERS && this.state.rawResponseHeadersOpened) {
> +        propertiesResult = {
> +          [headerKey]: { RAW_HEADERS_ID: headers.rawHeaders },
> +          // [headerKey]: headers.rawHeaders,

Should we remove this line?

@@ +120,5 @@
> +        };
> +      } else if (title === REQUEST_HEADERS && this.state.rawRequestHeadersOpened) {
> +        propertiesResult = {
> +          [headerKey]: { RAW_HEADERS_ID: headers.rawHeaders },
> +          // [headerKey]: headers.rawHeaders,

Should we remove this line?

@@ +161,5 @@
>    }
>  
> +  renderRow(props) {
> +    const { level, value, path } = props.member;
> +    console.log("renderRow - member", props.member);

This looks like forgotten log, please remove it.

@@ +188,5 @@
> +  renderToggleRawHeadersBtn(path) {
> +    let inputElement;
> +
> +    if (path.includes(REQUEST_HEADERS)) {
> +      // This is a request header

Is this comment ok? Should it rather say something about the raw headers toggle button?

Comment 27

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

(In reply to Lenka Pelechova from comment #25)

@Honza - please have a look:
There is still one bug in HeadersPanel.js with following STR:
I can't reproduce this on my machine (Win10)

Honza

Comment 28

[Lenka Pelechova]

Attachment: headers-panel-Bug1495797-v2.patch

New patch reflecting suggested improvements.
However - the bug described in the previous patch still there.
Please review.

Comment 29

[Lenka Pelechova]

Honza,
Re:
2) Also note that font-weight: 600; for the summary labels (e.g. Request URL, etc.) doesn't work well for me. I am not seeing bold font for the labels as it was before. Perhaps it should rather be: font-weight: bold;?

I have not changed the font for summary labels, it was font-weight:600 before (semi-bold) and it is that also currently. I see it semi-bold both in light and dark theme. Not sure if we should change it to bold since other places use font-weight:600

3. The textarea for raw headers should not have border

the last patch should have no border already, I do not see a border

I will reproduce the bug I mentioned at our next meeting...

@Honza, please could you push to try so I can see if there are other things that pop out? I do NOT have a try server access currently - either because it's a new computer or because I am using another user name (before it was [e-mail], now it is [e-mail]). I'll have to check if I need to request a new level 1 access or if I can just copy ssh key to this machine.

Thank you,
Lenka

Comment 30

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a176be850f97535564d1de808dfca37bb1b7ea7

Comment 31

[Lenka Pelechova]

Attachment: headers-panel-plus-tests.patch

• Added support for display of raw headers for UPLOAD headers;
• code refactoring;
• changed 2 tests according to the new DOM structure and logic for raw headers

@Honza, please review. Thanks.
Lenka

Comment 32

[Lenka Pelechova]

Here is push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=839e42d15680213ca032502859cb1dca3e3f3ff6

One (inspector) test shows failure: browser_animation_logic_mutations_fast.js
Don't know what should be done in this case. I can do one more push to try to see if it fails again.

Comment 33

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

Comment on attachment 9037902 [details] [diff] [review]
headers-panel-plus-tests.patch

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

Great job here, thanks for the patch Lenka!

R+ 

I think that the animation test failures is an intermittent.
I retriggered the test run, so we'll see yet other results.

Honza

Comment 34

[Lenka Pelechova]

Further test runs were green. Ready for check-in.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb87ba30098c
change Raw headers buttons, plus styling changes. r=honza

Comment 36

[Tim Nguyen :ntim]

There's a standalone/data-standalone attribute you could have used to get the button styling for free.

Comment 37

[Lenka Pelechova]

@Tim, I'd love to learn more. Can you please be more specific? Thanks.
Lenka

Comment 38

[Tim Nguyen :ntim]

(In reply to Lenka Pelechova from comment #37)

@Tim, I'd love to learn more. Can you please be more specific? Thanks.
Lenka

https://searchfox.org/mozilla-central/source/devtools/client/themes/common.css#329-335

You could have added the data-standalone attribute to the "Edit and Resend" and "Raw headers" buttons instead of adding the extra border/height CSS.

See how it's done in other React tools: https://searchfox.org/mozilla-central/search?q=data-standalone&case=false&regexp=false&path=

Comment 39

[Lenka Pelechova]

(In reply to Tim Nguyen :ntim from comment #38)

(In reply to Lenka Pelechova from comment #37)

@Tim, I'd love to learn more. Can you please be more specific? Thanks.
Lenka

https://searchfox.org/mozilla-central/source/devtools/client/themes/common.css#329-335

You could have added the data-standalone attribute to the "Edit and Resend" and "Raw headers" buttons instead of adding the extra border/height CSS.

See how it's done in other React tools: https://searchfox.org/mozilla-central/search?q=data-standalone&case=false&regexp=false&path=

Thanks for pointing that out. I think it would be applicable for Edit and Resend button. The Raw headers button however has been changed to 2 toggle, so it's not a button anymore.

Comment 40

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/fb87ba30098c

Comment 41

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/projects/cedar/rev/fb87ba30098c91ee7f8a3b8c59b9e344d3a5525a
Bug 1495797 - change Raw headers buttons, plus styling changes. r=honza