The current network panel layout makes it difficult to visually find the header you're looking for. If the window was made a bit wider and the headings aligned, it would likely be easier to find the specific heading you need. (faaborg suggested widening the panel in order to make it possible to do this.)

Proposed patch. Made the headers nicely aligned and spaced. Makes things easier to read / scan through. Also increased the panel default width and height. Will attach the obligatory screenshot as well. Please let me know if it's fine. I only tested on Ubuntu.

holy cow... that's way more readable. thanks, Mihai!

Requesting blocking status for this bug. If you look at the screenshot and compare it with the current network panel you can see that it's a huge improvement in readability. The comparison is near the top of this document: http://mozilla.github.com/devtools/WebConsole4.html

Comment on attachment 488168 [details] [diff] [review] proposed patch >-span.property-name { >+.property-table th, #header th { > font-size: 11px; > font-weight: bold; >- padding-right: 4px; > color: #000; >+ white-space: nowrap; >+ text-align: left; >+ vertical-align: top; >+ width: 10%; > } Would "text-align: right" look better here? >-span.property-name { >+.property-table th, #header th { Avoid the descendant selector [1]. Would it be possible to make a "property-table-header" class and just use that? If so, that's preferred as I understand it. Additionally, a nit: break the line after the comma. >-span.property-value { >+.property-table td, #header #td { Did you mean "td" here instead of "#td"? Comments above apply here as well. [1]: https://developer.mozilla.org/en/Writing_Efficient_CSS

Updated patch. Made the changes you requested and a bit more. Thanks for your feedback!

mass change: filter on PRIORITYSETTING

mass change: filter on PRIORITYSETTING

Comment on attachment 491238 [details] [diff] [review] updated patch >diff --git a/toolkit/components/console/hudservice/NetworkPanel.xhtml b/toolkit/components/console/hudservice/NetworkPanel.xhtml >--- a/toolkit/components/console/hudservice/NetworkPanel.xhtml >+++ b/toolkit/components/console/hudservice/NetworkPanel.xhtml >+ <table class="property-table" id="requestHeadersContent" cellspacing="0" >+ cellpadding="3" border="0"></table> > > <div id="requestCookie" style="display:none"> > <h1>&networkPanel.requestCookie;</h1> >- <div class="property-header" id="requestCookieContent"></div> >+ <table class="property-table" id="requestCookieContent" cellspacing="0" >+ cellpadding="3" border="0"></table> > </div> > > <div id="requestBody" style="display:none"> > <h1>&networkPanel.requestBody;</h1> >- <div class="property-header" id="requestBodyContent"></div> >+ <table class="property-table" id="requestBodyContent" cellspacing="0" >+ cellpadding="3" border="0"></table> > </div> > <div id="requestFormData" style="display:none"> > <h1>&networkPanel.requestFormData;</h1> >- <div class="property-header" id="requestFormDataContent"></div> >+ <table class="property-table" id="requestFormDataContent" cellspacing="0" >+ cellpadding="3" border="0"></table> > </div> > </div> > > <div class="group" id="responseContainer" style="display:none"> > <h1> > &networkPanel.responseHeaders; > <span id="responseHeadersInfo" class="info">&Delta;</span> > </h1> >- <div class="property-header" id="responseHeadersContent"></div> >+ <table class="property-table" id="responseHeadersContent" cellspacing="0" >+ cellpadding="3" border="0"></table> > > <div id="responseBody" style="display:none"> > <h1> > &networkPanel.responseBody; > <span class="info" id="responseBodyInfo">&Delta;</span> > </h1> >- <div class="property-header" id="responseBodyContent"></div> >+ <table class="property-table" id="responseBodyContent" cellspacing="0" >+ cellpadding="3" border="0"></table> > </div> > <div id="responseBodyCached" style="display:none"> > <h1> > &networkPanel.responseBodyCached; > <span class="info" id="responseBodyCachedInfo">&Delta;</span> > </h1> >- <div class="property-header" id="responseBodyCachedContent"></div> >+ <table class="property-table" id="responseBodyCachedContent" >+ cellspacing="0" cellpadding="3" border="0"></table> > </div> > <div id="responseNoBody" style="display:none"> > <h1> > &networkPanel.responseNoBody; > <span id="responseNoBodyInfo" class="info">&Delta;</span> > </h1> > </div> > <div id="responseBodyUnknownType" style="display:none"> > <h1> > &networkPanel.responseBodyUnknownType; > <span id="responseBodyUnknownTypeInfo" class="info">&Delta;</span> > </h1> >- <div class="property-header" id="responseBodyUnknownTypeContent"></div> >+ <table class="property-table" id="responseBodyUnknownTypeContent" >+ cellspacing="0" cellpadding="3" border="0"></table> > </div> > <div id="responseImage" style="display:none"> > <h1> > &networkPanel.responseImage; > <span id="responseImageInfo" class="info"></span> > </h1> > <div id="responseImageNodeDiv"> > <img id="responseImageNode" /> nit: There's whitespace at the ends of some of these lines. On my machine, the "delta" lines aren't quite aligned with the baseline of the header. I'll attach a screenshot so you can see what I mean. Is there a quick fix? If not, we should file a followup bug. f+ with that :)

Here's a screenshot showing the alignment issue.

Updated the patch as requested by Patrick. Fixed the header title alignment issue as well. Removed trailing spaces from the XHTML. Thanks for your feedback+! Asking for review from Dao now.

Definitely a huge improvement, but not a change that justifies holding back the release any more than we already have. Blocking-. Request approval on the patch if it gets review+.

Comment on attachment 494733 [details] [diff] [review] patch update 2 >+<table id="header" cellspacing="0" cellpadding="3" border="0"> Please use CSS instead of these attributes, throughout this patch. >--- a/toolkit/themes/gnomestripe/global/webConsole_networkPanel.css >+++ b/toolkit/themes/gnomestripe/global/webConsole_networkPanel.css >+#header { > padding: 5px; > overflow-x:auto; >+ display: block; > } What does display: block achieve here?

(In reply to comment #13) > Comment on attachment 494733 [details] [diff] [review] > patch update 2 > > >+<table id="header" cellspacing="0" cellpadding="3" border="0"> > > Please use CSS instead of these attributes, throughout this patch. Will do. > >--- a/toolkit/themes/gnomestripe/global/webConsole_networkPanel.css > >+++ b/toolkit/themes/gnomestripe/global/webConsole_networkPanel.css > > >+#header { > > padding: 5px; > > overflow-x:auto; > >+ display: block; > > } > > What does display: block achieve here? Argh, that was legacy code. Will update the patch accordingly.

Updated the patch as requested. Thanks!

Comment on attachment 498375 [details] [diff] [review] patch update 3 >+<table id="header"> >+ <tr> >+ <th class="property-name" >+ scope="row">&networkPanel.requestURL;:</th> >+ <td class="property-value" >+ id="headUrl"></td> >+ </tr> >+ <tr> >+ <th class="property-name" >+ scope="row">&networkPanel.requestMethod;:</th> >+ <td class="property-value" >+ id="headMethod"></td> >+ </tr> >+ <tr> >+ <th class="property-name" >+ scope="row">&networkPanel.statusCode;:</th> >+ <td class="property-value" >+ id="headStatus"></td> >+ </tr> >+</table> The colons need to be part of the entity values. Please file a bug on fixing this. >--- a/toolkit/themes/gnomestripe/global/webConsole_networkPanel.css >+++ b/toolkit/themes/gnomestripe/global/webConsole_networkPanel.css >@@ -37,65 +37,73 @@ > * ***** END LICENSE BLOCK ***** */ > > body { > font-family: Lucida Grande, sans-serif; Please either fix the syntax here or remove this line, with a preference for the latter. I don't like the idea of using random fonts in chrome rather than following the UI settings of the OS. > font-size: 11px; > background: #EEE; > } > >-div#header { >+#header { > padding: 5px; > overflow-x:auto; > } > > h1 { > font-size: 13px; >- padding: 2px 10px; >+ line-height: 15px; >+ padding: 3px 10px; >+ vertical-align: bottom; > margin: 0px; > background: -moz-linear-gradient(top, #BBB, #999); > border-radius: 2px; > text-shadow: #FFF 0px 1px 0px; > } I don't see anything setting a text color here, which seems bogus. The body rule at the top should probably do that.

(In reply to comment #16) > Comment on attachment 498375 [details] [diff] [review] > patch update 3 > > >+<table id="header"> > >+ <tr> > >+ <th class="property-name" > >+ scope="row">&networkPanel.requestURL;:</th> > >+ <td class="property-value" > >+ id="headUrl"></td> > >+ </tr> > >+ <tr> > >+ <th class="property-name" > >+ scope="row">&networkPanel.requestMethod;:</th> > >+ <td class="property-value" > >+ id="headMethod"></td> > >+ </tr> > >+ <tr> > >+ <th class="property-name" > >+ scope="row">&networkPanel.statusCode;:</th> > >+ <td class="property-value" > >+ id="headStatus"></td> > >+ </tr> > >+</table> > > The colons need to be part of the entity values. Please file a bug on fixing > this. Reported bug 621291. > >--- a/toolkit/themes/gnomestripe/global/webConsole_networkPanel.css > >+++ b/toolkit/themes/gnomestripe/global/webConsole_networkPanel.css > >@@ -37,65 +37,73 @@ > > * ***** END LICENSE BLOCK ***** */ > > > > body { > > font-family: Lucida Grande, sans-serif; > > Please either fix the syntax here or remove this line, with a preference for > the latter. I don't like the idea of using random fonts in chrome rather than > following the UI settings of the OS. I am trying to not touch other aspects of the network panel UI. Thus, I'd like to err on the side of just fixing the syntax. If I remove the line, the network panel web page uses times new roman (or an equivalent font), which looks ugly. What I'd suggest is to remove Lucida Grande, and just put sans-serif. > > font-size: 11px; > > background: #EEE; > > } > > > >-div#header { > >+#header { > > padding: 5px; > > overflow-x:auto; > > } > > > > h1 { > > font-size: 13px; > >- padding: 2px 10px; > >+ line-height: 15px; > >+ padding: 3px 10px; > >+ vertical-align: bottom; > > margin: 0px; > > background: -moz-linear-gradient(top, #BBB, #999); > > border-radius: 2px; > > text-shadow: #FFF 0px 1px 0px; > > } > > I don't see anything setting a text color here, which seems bogus. The body > rule at the top should probably do that. Good point. Thanks for your comments! Will attach an updated patch now.

Updated the patch based on your comments.

(In reply to comment #17) > What I'd suggest is to remove Lucida Grande, and just put sans-serif. Sounds good to me, please remove it. Lucida Grande is also normally not available on Windows and Linux. I realize this isn't really what this bug is about, but it's the first time I'm having a close look at this code :/

they're good comments. Thanks for the review, dão!

(In reply to comment #19) > (In reply to comment #17) > > What I'd suggest is to remove Lucida Grande, and just put sans-serif. > > Sounds good to me, please remove it. Lucida Grande is also normally not > available on Windows and Linux. I realize this isn't really what this bug is > about, but it's the first time I'm having a close look at this code :/ No worries. Updating the patch now. Thanks for the review!

Removed the font name.

Comment on attachment 499658 [details] [diff] [review] patch update 5 >--- a/toolkit/components/console/hudservice/NetworkPanel.xhtml >+++ b/toolkit/components/console/hudservice/NetworkPanel.xhtml >+ <th class="property-name" >+ scope="row">&networkPanel.statusCode;:</th> Indentation is off.

Fixed markup indentation. Thanks for the review+! Asking for approval2.0+. This patch does important UI polish for the network panel of the Web Console.

On IRC, sdwilsh pointed out that this patch will break any themes that have already been changed to support the Web Console. Can we update the patch to minimize the changes to class names that people have to support (property-header changed to property-table, for example)?

(In reply to comment #25) > On IRC, sdwilsh pointed out that this patch will break any themes that have > already been changed to support the Web Console. Can we update the patch to > minimize the changes to class names that people have to support > (property-header changed to property-table, for example)? I can do that, but from a strict technical POV I believe such changes will not be enough because we've also changed the tags from divs and spans to proper tables. That would require completely changing the approach of the patch. That is: only touch the stylesheet. Also, I wonder if themes won't be broken anyhow ... since they run on top of the stylesheets we have, don't they? If they do not replace our styles, any change will break existing themes.

> since they run on top of the stylesheets we have, don't they? They don't. They'd copy one of our webConsole_networkPanel.css files and make their adjustments, if any. Keeping the class while changing the underlying markup doesn't help much, as the applied styling breaks anyway. The truth is that we can hardly avoid breaking themes in the last betas, as they're very often affected by these innocent polish bugs...

Mossop suggested that we should communicate any changes that could break themes (like via a blog post that shows up on the planet, etc.) What I'm really looking for here is the a+, because the network panel looks much nicer with this patch applied. I will prod people some more about it tomorrow.

(In reply to comment #28) > Mossop suggested that we should communicate any changes that could break themes > (like via a blog post that shows up on the planet, etc.) I'm afraid that's not practical. Instead I think we should point them to <http://forums.mozillazine.org/viewtopic.php?f=18&t=1418995>.

OK, thanks for the pointer!

Comment on attachment 500765 [details] [diff] [review] patch update 6 I think the theme breaking is unfortunate, but still outweighed by the benefits to the console. Let's make sure to communicate this well, in that Mozillazine thread and maybe on planet as well.

Thanks for the approval, Gavin! This patch is ready to land. Will have to coordinate with Rob/David to see when they can do the checkin, and I'll rebase (if needed) the patch at that time.