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

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.

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 ?

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

> 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.

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.

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)

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 on attachment 8931946 [details] Highlight status code only This looks good to me :) Let's see what Victoria and Honza think

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 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 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

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 on attachment 8931955 [details] Bug 1417805 - Add highlighting to HTTP response status codes in console https://reviewboard.mozilla.org/r/203016/#review208890

Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/df01e312536b Add highlighting to HTTP response status codes in console r=nchevobbe

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

@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 :)

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