1417805 - Add color-sheme background to HTTP status code in Console

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Rustam]

Attachment: http-errors-color-in-console.JPG

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36 OPR/48.0.2685.52

Steps to reproduce:

Prior to Firefox 57, the HTTP errors in the console were displayed in red. Now they are blue. Very uncomfortable. Changing the Dark / Ligt console theme does not help.



Actual results:

Now the error "500 Internal Server Error" is very difficult to distinguish from the usual "200 OK"


Expected results:

This is a regression, return the red color of HTTP errors

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Netmonitor colored icons

Instead of adding a background color to the whole message, could we add a color-schemed background to the HTTP status code ?
My reasoning here is that we don't apply background colors on requests in the netmonitor (there is an icon, see attachment), and I think we should keep consistency between panels.

Talking to Honza, he said to me that is not a fan of the icon, so we could also apply the color on the status code in netmonitor (and save horizontal space if the icon is no longer shown).

The other reason in favor of that, is that we don't know what the status code is when we show the message, we only display it when we get the response (see https://xhr-responses-code.glitch.me/). So with the background on the whole message, it can be weird to suddenly change the color for a message after it was displayed.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

If someone is willing to work on this, here is some pointers:

We should put the colors defined in https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/devtools/client/netmonitor/src/assets/styles/RequestList.css#261-282 in https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/variables.css , which is used by both netmonitor and webconsole.

For example it can be as simple as :
```
:root {
  […]
  --status-code-color-1xx: var(--theme-highlight-blue);
  --status-code-color-2xx: var(--theme-highlight-green);
  --status-code-color-3xx: transparent;
  --status-code-color-4xx: var(--theme-highlight-red);
  --status-code-color-5xx: var(--theme-highlight-pink);
}
```

And then these should be used in RequestList.css
```
.requests-list-status-icon[data-code^="1"] {
  background-color: var(--status-code-color-1xx);
}

.requests-list-status-icon[data-code^="2"] {
  background-color: var(--status-code-color-2xxn);
}
[…]
```


In webconsole's NetworkMessage component, the status code (https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/devtools/client/webconsole/new-console-output/components/message-types/NetworkEventMessage.js#77) should be wrapped in a span with a specific classname (e.g. status-code) and a `data-code` attribute containing the status.

The CSS rule could then be added in https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/httpi.css
```
.status-code[data-code^="1"] {
  background-color: var(--status-code-color-1xx);
  text: white; /* Find the appropriate CSS variable if there is one */
}
.status-code[data-code^="2"] {
  background-color: var(--status-code-color-2xx);
  text: white; /* Find the appropriate CSS variable if there is one */
}
```

A follow up could then be filed in Netmonitor to remove icons (saving horizontal space), and apply the background color on the status code column (which could be done by modifing/adding "status-code" className and "data-code" attribute in https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/devtools/client/netmonitor/src/components/RequestListColumnStatus.js#58).

Honza, does it looks good to you ?

Comment 3

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

Looks good, some comments:

1) Yes, I am not fan of the icon and I think that a color indicating errors can be better & faster to render & not using any additional space.
2) Style of an HTTP log should be consistent in Console and Net panel
3) It could be nice to have also some consistency between console.error() and HTTP 4xx if possible.

Harald, what do you think?

Honza

Comment 4

[:Harald Kirschner :digitarald]

> 3) It could be nice to have also some consistency between console.error() and HTTP 4xx if possible.

I agree with that and Victoria also mentioned it. Re-using at least the colors to some degree (if it doesn't get too noisy, maybe for just parts of the message).

> 2) Style of an HTTP log should be consistent in Console and Net panel

I also like the idea of aligning the style between panels, but would tackle in Q1 when we do some work on panel consistency.

Comment 5

[ctlusto]

Attachment: Add highlighting to HTTP response messages in console

Comment 6

[ctlusto]

This is my first attempt at contributing here. Also new to Mercurial. I think I've successfully attached a patch, but the world is full of people who think they've succeeded at things.

Any guidance would be greatly appreciated.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Hello ctlusto, it seems that your patch well built, i'll review it very soon.
In the meantime, may I suggest you to have a look at http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html , which has a better workflow for pushing patch to review (and a better UI to do the review), instead of attaching files from bugzilla ui.

You can come and say Hi in slack if you want to chat : https://devtools-html-slack.herokuapp.com/

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931028 [details] [diff] [review]
Add highlighting to HTTP response messages in console

Thanks for the patch !
So, this looks a bit to bold to me.
Could you try to only apply the background color to the `${status}` element only ?

Also, we may want to tweak those colors in dark mode I think: the white text seems barely legible. (Attachment to come)

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Initial patch - light and dark mode

Comment 10

[ctlusto]

Second attempt:

1. Only apply highlighting to the status code itself.
2. Add a little horizontal padding and a border-radius
3. Use the theme background for the text color to help with legibility

How do you think this looks? Attachment coming.

Comment 11

[ctlusto]

Attachment: Highlight status code only

Comment 12

[ctlusto]

Attachment: Bug 1417805 - Add highlighting to HTTP response status codes in console

Review commit: https://reviewboard.mozilla.org/r/203016/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203016/

Comment 13

[ctlusto]

Attachment: Bug 1417805 - Higlight status code only; update styling

Review commit: https://reviewboard.mozilla.org/r/203018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203018/

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931946 [details]
Highlight status code only

This looks good to me :) 
Let's see what Victoria and Honza think

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931956 [details]
Bug 1417805 - Higlight status code only; update styling

https://reviewboard.mozilla.org/r/203018/#review208406

This looks good to me. 
Could you squash these 2 changeset ?
In mercurial, I use the histedit extension (https://www.mercurial-scm.org/wiki/HisteditExtension) which make it very easy.

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931955 [details]
Bug 1417805 - Add highlighting to HTTP response status codes in console

https://reviewboard.mozilla.org/r/203016/#review208408

Waiting for squashed diff :)

Comment 17

[ctlusto]

Comment on attachment 8931955 [details]
Bug 1417805 - Add highlighting to HTTP response status codes in console

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203016/diff/1-2/

Comment 18

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

Comment on attachment 8931946 [details]
Highlight status code only

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14)
> Comment on attachment 8931946 [details]
> Highlight status code only
> 
> This looks good to me :) 
> Let's see what Victoria and Honza think
I like it too.

In addition, we could make the error code (just the number in colored box) a link that points to MDN error page
(e.g. https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/501)

The rest of the text can expand/collapse the request (just like now)

Honza

Comment 19

[:Harald Kirschner :digitarald]

Drive-by review, LGTM from the product side; makes the network logs style much more interesting and easier to tell apart. We should file follow up work to revisit styling for the [HTTP… ] part

Comment 20

[Victoria Wang [:victoria]]

This looks great! Thanks for working on this ctlusto!

(It would look a bit nicer visually if the "icons" were to the left of that section, or lined up in some way, but this seems like a big enough of a win that we should go ahead with it and I can revisit this for polish later.)

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

TRY looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f66fb06fb612aea1e0b581d9de88b694cc57940 , let's land this and file follow-up bugs for the UI feedback here :)

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931955 [details]
Bug 1417805 - Add highlighting to HTTP response status codes in console

https://reviewboard.mozilla.org/r/203016/#review208890

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df01e312536b
Add highlighting to HTTP response status codes in console r=nchevobbe

Comment 24

[ctlusto]

Is there anything else I need to do here? Sorry if this is waiting on me...just let me know. :)

Comment 25

[Tiberius Oros[:tiberius_oros]]

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

Comment 26

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df01e312536b

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

@ctlusto, nope, we're good ! The bug has now landed in mozilla-central, and will be in Nightly soon !
Thanks a lot for working on this, it's a nice touch of color in the console :)

Comment 29

[Artem Skoretskiy]

It is possible to back-port these changes to Firefox 58 or even Firefox 57? It is kind of regression.