Styling for about:networking dashboard

RESOLVED FIXED in Firefox 46

Status

()

Core
Networking
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: Boriss, Assigned: valentin)

Tracking

unspecified
mozilla46
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 672066 [details]
about:net-internals Networking Dashboard Styling: table and graph

The about:net-internals networking dashboard should be styled similarly to other Firefox in-content pages.  Attached is a guideline for the its styling in Windows.  OSX and Gnome need color/texture changes only (see http://people.mozilla.com/~shorlander/firefox-ui-design/firefox-visual-design-across-platforms.html )
What is the status of this bug?
(Assignee)

Comment 2

4 years ago
Sorry I forgot about this bug. The about:networking page is currently available in nightly and aurora, but with minimal styling. Jennifer, is the CSS and layout for the image you attached still available? (the link is dead)
Flags: needinfo?(jboriss)
(Assignee)

Updated

3 years ago
Depends on: 898237
(Assignee)

Comment 3

3 years ago
Created attachment 8385769 [details] [diff] [review]
Styling for about:net-internals Networking dashboard
(Assignee)

Comment 4

3 years ago
Comment on attachment 8385769 [details] [diff] [review]
Styling for about:net-internals Networking dashboard

This is the best I could do with my not-so-good CSS skills.
I made it look a bit like about:addons.
It's still not great, but better than before.
Attachment #8385769 - Flags: review?(ttaubert)
Flags: needinfo?(jboriss)
(Assignee)

Comment 5

3 years ago
Created attachment 8385775 [details]
about-net-style.png

Attaching a preview of how I see the sections.
(Assignee)

Updated

3 years ago
Depends on: 907050
(Assignee)

Updated

3 years ago
Blocks: 907050
No longer depends on: 907050
Comment on attachment 8385769 [details] [diff] [review]
Styling for about:net-internals Networking dashboard

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

This looks like it definitely needs more work to look like Boriss' mockup. CSS isn't necessarily my strength and I'm not that familiar with our current styling guide lines.

Jared, I know you're quite busy at the moment so feel free to redirect or take your time but I think you would be a better fit for this :)
Attachment #8385769 - Flags: review?(ttaubert) → review?(jaws)
Comment on attachment 8385769 [details] [diff] [review]
Styling for about:net-internals Networking dashboard

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

::: toolkit/content/aboutNetworking.xhtml
@@ +25,5 @@
>                  <button id="confpref">&aboutNetworking.ok;</button>
>              </div>
>          </div>
>          <div id="menu">
> +            Statistics:

We can't hardcode things like "Statistics:" because localizers won't be able to adapt the text for their locales. Please move this to a DTD file.

::: toolkit/themes/shared/aboutNetworking.css
@@ +6,5 @@
> +  background-image:
> +    /* Texture */
> +    url("chrome://global/skin/inContentUI/background-texture.png"),
> +    /* Gradient */
> +    linear-gradient(#ADB5C2, #BFC6D1);

You'll want to adjust this for Windows 8 so that it doesn't have the same linear-gradients, etc (should be mostly flat).

Also, the inline comments here can be removed.

@@ +72,5 @@
>    text-align: center;
>  }
>  
>  .tab {
>    display: none;

This and .warningBackground both have display:none, how are these styles doing anything here? Do they both use .active? We can be using the HTML5 "hidden" attribute instead of doing this.
Attachment #8385769 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 8

3 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> >  
> >  .tab {
> >    display: none;
> 
> This and .warningBackground both have display:none, how are these styles
> doing anything here? Do they both use .active? We can be using the HTML5
> "hidden" attribute instead of doing this.

The display attribute is overridden by the .active class for both .tab and .warningBackground.
This is done in: http://dxr.mozilla.org/mozilla-central/source/toolkit/content/aboutNetworking.js
(In reply to Valentin Gosu [:valentin] from comment #8)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> > >  
> > >  .tab {
> > >    display: none;
> > 
> > This and .warningBackground both have display:none, how are these styles
> > doing anything here? Do they both use .active? We can be using the HTML5
> > "hidden" attribute instead of doing this.
> 
> The display attribute is overridden by the .active class for both .tab and
> .warningBackground.
> This is done in:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/content/
> aboutNetworking.js

Sure, but we should be using .hidden=true/.hidden=false instead of this.
(Assignee)

Comment 10

3 years ago
Created attachment 8397432 [details] [diff] [review]
Styling for about:net-internals Networking dashboard
(Assignee)

Updated

3 years ago
Attachment #8385769 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
Comment on attachment 8397432 [details] [diff] [review]
Styling for about:net-internals Networking dashboard

I updated it with the hidden attributes.
Any idea on how I can use a solid background for Win8 only?

Thanks!
Attachment #8397432 - Flags: review?(jaws)
Summary: Styling for about:net-internals Networking dashboard → Styling for about:networking dashboard
Comment on attachment 8397432 [details] [diff] [review]
Styling for about:net-internals Networking dashboard

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

Please move aboutNetworking.css to /toolkit/themes/shared/aboutnetworking/aboutNetworking.css.

Windows 8 should be the default styling that we use. When you need to add some styling that is for < Windows 8, you should use @media (-moz-os-version: windows-win7), etc. See http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#624 for an example.

::: toolkit/content/aboutNetworking.xhtml
@@ +17,5 @@
>          <link rel="stylesheet" href="chrome://mozapps/skin/aboutNetworking.css" type="text/css" />
>          <script type="application/javascript;version=1.7" src="chrome://global/content/aboutNetworking.js" />
>      </head>
>      <body id="body">
> +        <div id="warning_message" class="warningBackground" hidden="hidden">

This should be hidden="true" (here and elsewhere).

::: toolkit/themes/shared/aboutNetworking.css
@@ +30,5 @@
> +  background: transparent;
> +  border-radius: 0.3em;
> +}
> +
> +#menu .selected {

Does anything outside of #menu ever get the 'selected' CSS class name? If not, then you should remove the '#menu ' portion of this selector.

@@ -46,5 @@
>  }
>  
>  .active {
> -  display: block;
> -}

Please delete the .active ruleset since it is now empty.
Attachment #8397432 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 13

2 years ago
Created attachment 8707701 [details] [diff] [review]
Styling for about:net-internals Networking dashboard
Attachment #8707701 - Flags: review?(jaws)
(Assignee)

Updated

2 years ago
Attachment #8397432 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Comment on attachment 8397432 [details] [diff] [review]
> Styling for about:net-internals Networking dashboard
> 
> Review of attachment 8397432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please move aboutNetworking.css to
> /toolkit/themes/shared/aboutnetworking/aboutNetworking.css.

I kept it in the shared directory, same as the other about* stylesheet files. It feels sad for it to be alone in its own directory :)

> Windows 8 should be the default styling that we use. When you need to add
> some styling that is for < Windows 8, you should use @media
> (-moz-os-version: windows-win7), etc. See

I removed the gradient completely, since the texture file is gone anyway. Let me know if you think more changes are necessary.

Thanks!
cool!
(Assignee)

Updated

2 years ago
Blocks: 1239686
Comment on attachment 8707701 [details] [diff] [review]
Styling for about:net-internals Networking dashboard

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

r- because the styling of aboutNetworking is so different than the other internal pages of Firefox. Most of them should move towards looking like about:preferences/about:addons or about:support. The only one that may deviate from this is about:memory since that page is specifically geared towards viewing memory details and we want to keep that page as low profile memory-wise as possible.

The initial styling in the mockups provided by Boriss in this bug are in the direction of about:addons, though that was based off an earlier iteration of about:addons that she spec'd out. The changes from that spec to what got shipped are minor color and border details. You should be able to use the in-content stylesheets that about:addons and about:preferences are using to reduce the amount of work that you are doing here.

::: toolkit/content/aboutNetworking.js
@@ +132,5 @@
>    gDashboard.enableLogging = true;
>    if (gPrefs.getBoolPref("warnOnAboutNetworking")) {
>      let div = document.getElementById("warning_message");
>      div.classList.add("active");
> +    div.removeAttribute("hidden");

Please use `div.hidden = false;`

@@ +169,5 @@
>  
>  function confirm () {
>    let div = document.getElementById("warning_message");
>    div.classList.remove("active");
> +  div.setAttribute("hidden", "hidden");

div.hidden = true;

@@ +180,5 @@
>    let content = document.getElementById(button.value);
>    if (current_tab == content)
>      return;
>    current_tab.classList.remove("active");
> +  current_tab.setAttribute("hidden", "hidden");

current_tab.hidden = true;

@@ +185,2 @@
>    content.classList.add("active");
> +  content.removeAttribute("hidden");

content.hidden = false;

::: toolkit/content/aboutNetworking.xhtml
@@ +25,5 @@
>                  <button id="confpref">&aboutNetworking.ok;</button>
>              </div>
>          </div>
>          <div id="menu">
> +            &aboutNetworking.statistics;:

The colon should be part of the localized string.

::: toolkit/themes/shared/aboutNetworking.css
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +html {
> +  background-color: #ADB5C2;

Where did this color come from?

The general styling of this page doesn't match the other in-content pages like about:preferences or about:addons. It would be better if we had a consistent styling of these internal pages instead of introducing a fourth variant (about:config, about:preferences/about:addons/, about:support, and now about:networking).

@@ +21,1 @@
>    color: gray;

Gray text on #ADB5C2 is barely legible, there needs to be more contrast.
Attachment #8707701 - Flags: review?(jaws) → review-
(Assignee)

Comment 17

2 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Review of attachment 8707701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- because the styling of aboutNetworking is so different than the other
> internal pages of Firefox. Most of them should move towards looking like
> about:preferences/about:addons or about:support. The only one that may
> deviate from this is about:memory since that page is specifically geared
> towards viewing memory details and we want to keep that page as low profile
> memory-wise as possible.
> 
> The initial styling in the mockups provided by Boriss in this bug are in the
> direction of about:addons, though that was based off an earlier iteration of
> about:addons that she spec'd out. The changes from that spec to what got
> shipped are minor color and border details. You should be able to use the
> in-content stylesheets that about:addons and about:preferences are using to
> reduce the amount of work that you are doing here.
> 

I did my best to do that in my previous version of the patch. I am quite limited by my knowledge of the front-end code, and by my general webdev skills. I'll do my best to touch up the patch, but I don't see it going very well :)
Do you think it's possible to land this in more-or-less its current state, and have someone from the Firefox team help out with this?

> > +html {
> > +  background-color: #ADB5C2;
> 
> Where did this color come from?

From what I remember, it was copied from a previous about:preferences style sheet.
Flags: needinfo?(jaws)
Sorry, I realize that switching over to the in-content prefs design is not necessarily an easy task. I don't think the styling implemented by this patch makes incremental progress towards the in-content prefs design though.

You can refactor the patch to remove the CSS changes you have made and leave the structural changes in place. Maybe Paenglab or ntim might want to help with moving this to the in-content prefs styling.
Flags: needinfo?(richard.marti)
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(jaws)
(Assignee)

Comment 19

2 years ago
Thanks for the info, and for the review.
I would love some help on getting this in the proper shape to land. Hopefully Richard or ntim could give me a hand with this. I'd really appreciate it!

Comment 20

2 years ago
Created attachment 8709629 [details] [diff] [review]
dashboard_style.patch - incontent style

Something like this ?
Flags: needinfo?(ntim.bugs)
Comment on attachment 8709629 [details] [diff] [review]
dashboard_style.patch - incontent style

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

Yes, this looks better. Thanks Tim. Valentin, please make the following two changes as well as the ones that I mentioned in my latest review and re-request review.

We should have the category names at the top of the page like you see in about:preferences too.

::: toolkit/locales/en-US/chrome/global/aboutNetworking.dtd
@@ +28,5 @@
>  <!ENTITY aboutNetworking.messagesSent          "Messages Sent">
>  <!ENTITY aboutNetworking.messagesReceived      "Messages Received">
>  <!ENTITY aboutNetworking.bytesSent             "Bytes Sent">
>  <!ENTITY aboutNetworking.bytesReceived         "Bytes Received">
> +<!ENTITY aboutNetworking.statistics            "Statistics">

While you're changing this file, please change
<!ENTITY aboutNetworking.http                  "Http">
to
<!ENTITY aboutNetworking.http2                 "HTTP">
as it should be full uppercase. Note that you'll have to change the entity name here (http to http2) so localizers will know to update their copy. aboutNetworking.xhtml will need to have the reference updated to http2 as well.
Attachment #8709629 - Flags: feedback+
Flags: needinfo?(richard.marti)
(Assignee)

Comment 22

2 years ago
Created attachment 8709706 [details] [diff] [review]
Styling for about:networking
Attachment #8709706 - Flags: review?(jaws)
(Assignee)

Comment 23

2 years ago
Thanks a lot, Tim! This looks great!

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> > +<!ENTITY aboutNetworking.statistics            "Statistics">
I removed this string, since it's not used anymore.

> While you're changing this file, please change
> <!ENTITY aboutNetworking.http                  "Http">
> to
> <!ENTITY aboutNetworking.http2                 "HTTP">
> as it should be full uppercase. Note that you'll have to change the entity name here
I changed it to aboutNetworking.HTTP since http2 means something else entirely. I hope that is ok.
I also fixed the intentation in the xhtml file, and moved the table attributes to the CSS file

Let me know if there's a better way of updating the title for each section.

Thanks!
(Assignee)

Updated

2 years ago
Attachment #8707701 - Attachment is obsolete: true
Comment on attachment 8709706 [details] [diff] [review]
Styling for about:networking

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

r+ with the following changes.

::: toolkit/themes/shared/aboutNetworking.css
@@ +31,5 @@
> +  pointer-events: none;
> +}
> +
> +/* XXX: a lot of this is duplicated from info-pages.css since that stylesheet
> +is incompatible with that type of layout */

/* XXX: A lot of this is duplicated from info-pages.css since that stylesheet
        is incompatible with this type of layout */

The second "that" should be changed to "this" and please indent the second line of the comment to line up with the first character of the note.

@@ +101,5 @@
> +
> +th, td, table {
> +  border-collapse: collapse;
> +  border: none;
> +  text-align: left;

text-align: start;
Attachment #8709706 - Flags: review?(jaws) → review+
Attachment #8385775 - Attachment is obsolete: true
Attachment #8709629 - Attachment is obsolete: true
(Assignee)

Comment 25

2 years ago
Created attachment 8710217 [details] [diff] [review]
Styling for about:networking

r+d in comment 24 by :jaws
(Assignee)

Updated

2 years ago
Attachment #8709706 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 26

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/7d1360284c1e
Keywords: checkin-needed

Comment 27

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7d1360284c1e
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Updated

a year ago
Depends on: 1242073
Depends on: 1303356
You need to log in before you can comment on or make changes to this bug.