Add color-sheme background to HTTP status code in Console

RESOLVED FIXED in Firefox 59

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: rustam_faizr, Assigned: ctlusto, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

57 Branch
Firefox 59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Reporter

Description

2 years ago
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
Component: Untriaged → Developer Tools: Console
Blocks: 1362036
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: DevTools http error color → Console doesn't show http errors in red
Whiteboard: [console-html]
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.
Summary: Console doesn't show http errors in red → Add color-sheme background to HTTP status code in Console
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 ?
Priority: -- → P3
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
Flags: needinfo?(hkirschner)
> 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.
Flags: needinfo?(hkirschner)
Whiteboard: [console-html] → [console-html] [triage]
Mentor: nchevobbe
Keywords: good-first-bug
Whiteboard: [console-html] [triage]
Assignee

Comment 6

2 years ago
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.
Assignee

Updated

2 years ago
Attachment #8931028 - Flags: review?(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 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)
Attachment #8931028 - Flags: review?(nchevobbe) → review-
Assignee: nobody → ctlusto
Severity: normal → enhancement
Status: NEW → ASSIGNED
Assignee

Comment 10

2 years ago
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.
Assignee

Comment 11

2 years ago
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8931946 [details]
Highlight status code only

This looks good to me :) 
Let's see what Victoria and Honza think
Attachment #8931946 - Flags: ui-review?(victoria)
Attachment #8931946 - Flags: ui-review?(odvarko)
Attachment #8931358 - Attachment is obsolete: true
Attachment #8931028 - Attachment is obsolete: true

Comment 15

2 years ago
mozreview-review
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.
Attachment #8931956 - Flags: review+

Comment 16

2 years ago
mozreview-review
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 :)
Attachment #8931955 - Flags: review-
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8931956 - Attachment is obsolete: true
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
Attachment #8931946 - Flags: ui-review?(odvarko) → ui-review+
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
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.)
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

2 years ago
mozreview-review
Comment on attachment 8931955 [details]
Bug 1417805 - Add highlighting to HTTP response status codes in console

https://reviewboard.mozilla.org/r/203016/#review208890
Attachment #8931955 - Flags: review+

Comment 23

2 years ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df01e312536b
Add highlighting to HTTP response status codes in console r=nchevobbe
Attachment #8931946 - Flags: ui-review?(victoria) → ui-review+
Assignee

Comment 24

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

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df01e312536b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
@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 :)
Duplicate of this bug: 1422132

Comment 29

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

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.