951714 - DevTools Themes: Update network panel table headers to match new theme

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Brian Grinstead [:bgrins]]

Based on this mockup: https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail].

Let's focus on the headers here to free up some vertical space and match the design - there are some design elements in common with the sidebar tabs in Bug 929127.

Comment 1

[Brian Grinstead [:bgrins]]

One thing that may be worth doing is merging all three netmonitor.css files:

https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/netmonitor.css
https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/devtools/netmonitor.css
https://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/devtools/netmonitor.css

This can be a little tricky, since there are differences in each OS, so we would need to check and make sure it doesn't cause any issues.  As we did with some other bugs though, if you want to work on the designs for just linux at first we can have a separate patch that merges all the files into a single shared one.

Comment 2

[Victor Porof [:vporof][:vp]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> One thing that may be worth doing is merging all three netmonitor.css files:
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/
> netmonitor.css
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/
> devtools/netmonitor.css
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/devtools/
> netmonitor.css
> 
> This can be a little tricky, since there are differences in each OS, so we
> would need to check and make sure it doesn't cause any issues.  As we did
> with some other bugs though, if you want to work on the designs for just
> linux at first we can have a separate patch that merges all the files into a
> single shared one.

I'm doing that in bug 943883 since the netmonitor uses the widget.

Comment 3

[Brian Grinstead [:bgrins]]

The multiple netmonitor.css files are now in shared/devtools/netmonitor.inc.css, so this is no longer being blocked

Comment 4

[Albert Juhé]

Attachment: patch1.patch

Hey, I have been really busy these last weeks and I couldn't start working on this until today. The attached patch is what I have so far. I am not asking for review nor feedback because it is still too basic.

However, I am already having two issues where I need your help:

1) I am applying the same styles from the inspector sidebar tabs (bug 929127). There, I could select the tab after the active one because they were in the same level and so, I used .tab[active] + .tab. In the network panel, however, not all tabs are in the same level, I checked netmonitor.xul and I saw there are hboxes around some of them. Any advice in how to select the tab after the active one?

2) In the mockup I see there are images for the arrows. I have never worked with images in Firefox. Are they already in some folder? Shall I ask somebody for the original files?

Thank you!

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #4)
> Created attachment 8357930 [details] [diff] [review]
> patch1.patch
> 
> Hey, I have been really busy these last weeks and I couldn't start working
> on this until today. The attached patch is what I have so far. I am not
> asking for review nor feedback because it is still too basic.
> 
> However, I am already having two issues where I need your help:
> 
> 1) I am applying the same styles from the inspector sidebar tabs (bug
> 929127). There, I could select the tab after the active one because they
> were in the same level and so, I used .tab[active] + .tab. In the network
> panel, however, not all tabs are in the same level, I checked netmonitor.xul
> and I saw there are hboxes around some of them. Any advice in how to select
> the tab after the active one?
> 

It appears that the main reason they are wrapped in boxes is because there is a special styling case for requests-menu-status-and-method-header-box, where there is no border between the two leftmost columns.  In the new design, this seems to not be the case anymore, so we may be able to unwrap all of the buttons to be top level children in netmonitor.xul: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor.xul#48.

If for some reason this will not work, we could modify the sortBy function in netmonitor-view.js to also set an active attribute on the parent: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js#478.

> 2) In the mockup I see there are images for the arrows. I have never worked
> with images in Firefox. Are they already in some folder? Shall I ask
> somebody for the original files?

Darrin, a few questions about the network panel mockup here: https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail].

1) On the mockup it isn't clear to me if the sort arrow is meant to sit directly next to the text of the selected column, or if it should be attached to the far right of the column.
2) On the "Method" column it almost looks like it would have to extend the size of the column in order to squeeze an arrow in it, should the column be a little bigger by default in order to accommodate this space, or should it extend when selected? 
3) Any opinion on if we should use an icon for the up/down arrow for column sort, or just use CSS to generate the triangles?

Comment 6

[Albert Juhé]

Attachment: patch2.patch

Ok, here I attach a new patch.

> If for some reason this will not work, we could modify the sortBy function in netmonitor-view.js to also set an active attribute on the parent: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js#478.

I opted for this solution. Trying to remove the <hbox>es made the header columns to have a different widths than the results.

In the patch there are two things missing:

1) The arrows (waiting for more information).

2) 'Timeline' column header redesign. I assume that when opening devtools only "Timeline" string should be shown and after loading the page it must change to "Timeline" in one line and the time numbers in the line below as shown in the mockup. Please, correct me if I am wrong.

Comment 7

[Albert Juhé]

Attachment: patch2-screenshot.png

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8362975 [details] [diff] [review]
patch2.patch

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

I think adding the active on a different element in JS was a good call, since it makes the CSS changes much easier.  I don't think this will affect what you are doing here, but be aware that I am making some light theme changes on the network panel (including the toolbar styles) over on Bug 957117.  This shouldn't be an issue - I can just copy over the same type of changes I'm adding for the sidebar tabs for the filters.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +496,5 @@
>          target.setAttribute("sorted", direction = "ascending");
>          target.setAttribute("tooltiptext", L10N.getStr("networkMenu.sortedAsc"));
>        }
> +      if (target.id !== "requests-menu-status-button") {
> +        target.parentNode.setAttribute("active","true");

Please add a space after the comma here

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #6)

> 
> 2) 'Timeline' column header redesign. I assume that when opening devtools
> only "Timeline" string should be shown and after loading the page it must
> change to "Timeline" in one line and the time numbers in the line below as
> shown in the mockup. Please, correct me if I am wrong.

I'm not sure if we should or shouldn't make this change here.  Darrin, any thoughts about the 'Timeline' column header based on https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]?

Comment 10

[Darrin Henein [:darrin]]

> I'm not sure if we should or shouldn't make this change here.  Darrin, any
> thoughts about the 'Timeline' column header based on
> https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> [e-mail]?

My inclination is to leave out the 'Timeline' label. I don't think that it adds any value (I believe the timeline is pretty self evident) and takes away from the visual balance by having some headers with 1 row, some with 2.

We should change the dotted separators to solid lines, and adjust some of the font sizes as per the mockup, however.

Comment 11

[Brian Grinstead [:bgrins]]

Still needinfo? for Comment 5.  With the added knowledge that using CSS to generate arrows can cause some weirdness with center alignment in XUL if we want the arrow to be pinned to the far right of the column.

Comment 12

[Victor Porof [:vporof][:vp]]

I hate, despise and have a phobia for CSS triangles. Please let's not use them.

Comment 13

[Albert Juhé]

Hey, I would like to work on this bug again (sorry, last weeks I had been really busy...). There is still a needinfo from Comment 5 pending. Any news?

Comment 14

[Darrin Henein [:darrin]]

> 1) On the mockup it isn't clear to me if the sort arrow is meant to sit
> directly next to the text of the selected column, or if it should be
> attached to the far right of the column.

I would attach the triangle to the right of the column with a bit of right-padding, which should be the same value as the padding in the cells below (note the space between "www.facebook.com" and the left border of that cell).

> 2) On the "Method" column it almost looks like it would have to extend the
> size of the column in order to squeeze an arrow in it, should the column be
> a little bigger by default in order to accommodate this space, or should it
> extend when selected? 

It should probably be a bit bigger by default, to avoid things resizing while clicking around.

> 3) Any opinion on if we should use an icon for the up/down arrow for column
> sort, or just use CSS to generate the triangles?

Probably an SVG/icon though I'm not strongly inclined either way.

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Darrin Henein [:darrin] from comment #14)
> > 1) On the mockup it isn't clear to me if the sort arrow is meant to sit
> > directly next to the text of the selected column, or if it should be
> > attached to the far right of the column.
> 
> I would attach the triangle to the right of the column with a bit of
> right-padding, which should be the same value as the padding in the cells
> below (note the space between "www.facebook.com" and the left border of that
> cell).
> 
> > 2) On the "Method" column it almost looks like it would have to extend the
> > size of the column in order to squeeze an arrow in it, should the column be
> > a little bigger by default in order to accommodate this space, or should it
> > extend when selected? 
> 
> It should probably be a bit bigger by default, to avoid things resizing
> while clicking around.
> 
> > 3) Any opinion on if we should use an icon for the up/down arrow for column
> > sort, or just use CSS to generate the triangles?
> 
> Probably an SVG/icon though I'm not strongly inclined either way.

Albert, is that enough info to pick this up?  I'm guessing you will need to rebase the patch against the latest code (there may be some conflicts).

And for the icon, I'd say just use a existing image as a placeholder for now (something like chrome://browser/skin/devtools/dropmarker.png), and we will get the updated icon later.  My preference in this case would be SVG, so we don't need to have separate 1x and 2x versions.

Comment 16

[Albert Juhé]

Attachment: patch3.patch

> Albert, is that enough info to pick this up?

Yeah, I'm working on it again. :)

> We should change the dotted separators to solid lines, and adjust some of the font sizes as per the mockup, however.

I think those lines are dashed in the mockup, should they be solid?

I have another question: in the current timeline there are different units ms, seconds and minutes. In the mockup, however, all measures are in ms (1280 ms instead of 1.28s). Is that change also in the scope of this bug? If it's not, shall seconds and minutes have a different separator color like in the current design?

> chrome://browser/skin/devtools/dropmarker.png

Great. However I also need an image with a triangle pointing up, I searched on devtools folder and I didn't find any. What should I do?

----

I attached a new patch, there aren't a lot of new things, I just rebased it with the new code and changed all background-images with border-images, it makes the code much simpler and avoids adding more work to bug 975804.

I am having some trouble making the triangle fit the column space. I can't make the status column look nice. If I add space in the right side, when it's not active it doesn't look centered. If I add space to both sites, the column is too wide and "eats" the space of the method column.

Comment 17

[Albert Juhé]

Attachment: patch3-screenshot.png

Comment 18

[Brian Grinstead [:bgrins]]

> > We should change the dotted separators to solid lines, and adjust some of the font sizes as per the mockup, however.
> 
> I think those lines are dashed in the mockup, should they be solid?

OK I see that now.  It looks like they are dashed and orange in your current screenshot, they should probably be the same color but just dashed.

> I have another question: in the current timeline there are different units
> ms, seconds and minutes. In the mockup, however, all measures are in ms
> (1280 ms instead of 1.28s). Is that change also in the scope of this bug? If
> it's not, shall seconds and minutes have a different separator color like in
> the current design?
> 

Don't worry about the units for this case - I don't think the end result will look 100% like the mockup because of details like this, but it is not a big deal.

> > chrome://browser/skin/devtools/dropmarker.png
> 
> Great. However I also need an image with a triangle pointing up, I searched
> on devtools folder and I didn't find any. What should I do?
> 

It looks like you already have a placeholder for the other state, so we just need to get a resource for it - it should be easy to make this in SVG, since it is just a triangle.  Let me check something and get back to you about that.  If this ended up being a separate element for some reason we could also handle this with a css rotation.

> I am having some trouble making the triangle fit the column space. I can't
> make the status column look nice. If I add space in the right side, when
> it's not active it doesn't look centered. If I add space to both sites, the
> column is too wide and "eats" the space of the method column.

I was worried this would be a pain.  Maybe we could just add a min-width on the column to give it enough space?  If you could upload a screenshot with a few of the different configurations we could take a look at it and decide what is the best path.

Comment 19

[Brian Grinstead [:bgrins]]

Darrin, can you make Albert a sprite with the up and down arrows for the network headers, as in https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]?

We could use dropmarker.png (https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/dropmarker.png) and do something with rotating it, but I think it will be easier to just have a separate image for this.

Albert, when you get the image, please put it in browser/themes/shared/devtools/images, and add a reference to it in these three files:

* browser/themes/osx/jar.mn
* browser/themes/linux/jar.mn
* browser/themes/windows/jar.mn (it needs to be in two places in this file)

I would call it something like sort-arrows.png.  I usually just copy and paste a line in each file for an existing resource, like vview-open-inspector.png, to make sure that all of the formatting stays consistent.

Comment 20

[Darrin Henein [:darrin]]

Attachment: sort-arrows.zip

Albert can you try these assets? Please post screenshot on dark and light theme for ui-review. Thanks!

Comment 21

[Albert Juhé]

Attachment: patch4.patch

After a long delay (sorry for that!), I have a new patch. There are both dark and light styles. I think there are only two thinks missing:
1) I can not make the method text fit in its column after making the status column wider. Any tip?
2) In the light theme, the arrow is barely noticeable.

Comment 22

[Albert Juhé]

Attachment: patch4-screenshot.png

Comment 23

[Albert Juhé]

Attachment: patch4-arrowinlighttheme.png

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8408294 [details] [diff] [review]
patch4.patch

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

It looks like the sort-arrows.png file didn't get added to the patch.  Can you please include that file and reupload?

Comment 25

[Victor Porof [:vporof][:vp]]

Do we still need to change the background on the headers now that we have arrows? I don't think so.

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #25)
> Do we still need to change the background on the headers now that we have
> arrows? I don't think so.

We've been working off of https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail], which shows changed background on sorted column.

Regarding the arrow not being visible against active tab on light theme, we should be able to use the svg invert filter to make the arrow more visible against light theme background (see the filter: url(filters.svg#invert) line in toolbars.inc.css)

Comment 27

[Albert Juhé]

Attachment: patch4b.patch

Sorry, it was my first time uploading a patch with images. I think now it's ok.

Comment 28

[Albert Juhé]

Attachment: patch5.patch

> Regarding the arrow not being visible against active tab on light theme, we should be able to use the svg invert filter to make the arrow more visible against light theme background (see the filter: url(filters.svg#invert) line in toolbars.inc.css)

Done! ;)

Comment 29

[Brian Grinstead [:bgrins]]

Attachment: browser-toolbox-inspector.png

Albert, I'm not sure if you've seen this, but you can now use the Browser Toolbox to inspect devtools, which can make debugging these issues a lot easier (see screenshot).  Info on opening it is here: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox.

> 1) I can not make the method text fit in its column after making the status
> column wider. Any tip?

It looks like these 2 columns have a special layout compared to the other columns.  Checking into it, it looks like a few things can be changed to make it look better:

* There is a 8em width on .requests-menu-status-and-method that can be removed
* There is a width: 20px; on .requests-menu-status that can be removed
* There is a width: 6em on .requests-menu-method.  This can be removed
* Add min-width: 120px (I think) for 40*3 on .requests-menu-status-and-method
* On .box.requests-menu-status, set -moz-margin-start: 15px; -moz-margin-end: 15px; to = 40px with the 10px width

These changes get things pretty close.  I'm not sure if it will hold up with internationalized column names (like if "Method" was a longer string).  Some of the other columns use a pattern on the row like width: 20vw; min-width: 4em;, but I'm not sure how that it keeping the width the same as the header column.

Comment 30

[Victor Porof [:vporof][:vp]]

(In reply to Brian Grinstead [:bgrins] from comment #29)
> Created attachment 8408331 [details]
> browser-toolbox-inspector.png
> 
> Albert, I'm not sure if you've seen this, but you can now use the Browser
> Toolbox to inspect devtools, which can make debugging these issues a lot
> easier (see screenshot).  Info on opening it is here:
> https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox.
> 
> > 1) I can not make the method text fit in its column after making the status
> > column wider. Any tip?
> 
> It looks like these 2 columns have a special layout compared to the other
> columns.  Checking into it, it looks like a few things can be changed to
> make it look better:
> 

These widths are there to make all the rows the same width. Otherwise you'd get different widths between POST and GET for example.

Comment 31

[Victor Porof [:vporof][:vp]]

(as a small side-note, please ask me for review as well whenever you feel the patch is ready)

Comment 32

[Brian Grinstead [:bgrins]]

Comment on attachment 8408309 [details] [diff] [review]
patch5.patch

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

First of all, I'm loving the smaller headers and the extra room it gives for the contents in the panel.  But I think Victor may have better suggestions about how to wrap this patch up then I will.  Victor, the main issue I see is the matching the width on those first 2 headers with the entries for status, status code, and method.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +782,5 @@
>          target.setAttribute("sorted", direction = "ascending");
>          target.setAttribute("tooltiptext", L10N.getStr("networkMenu.sortedAsc"));
>        }
>      }
> +    if (target.id !== "requests-menu-status-button") {

I know there was a reason for this, but I can't remember exactly why now.  Can you add a comment explaining why we don't add active when the status header is clicked?

@@ +783,5 @@
>          target.setAttribute("tooltiptext", L10N.getStr("networkMenu.sortedAsc"));
>        }
>      }
> +    if (target.id !== "requests-menu-status-button") {
> +     target.parentNode.setAttribute("active", "true");

Nit: use 2 spaces for indentation here

Comment 33

[Victor Porof [:vporof][:vp]]

Comment on attachment 8408309 [details] [diff] [review]
patch5.patch

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

This looks very botched on OS X, I'll be attaching some screenshots.

Please don't misinterpret my question, but I don't fully understand why we need so many css changes to fix this particular bug. The way I see it, it's all just about changing the height and adding an arrow where appropriate. Since the widths of most columns is already defined, simply changing the value should do the trick. Granted, the first column is a bit trickier because of the different layout, so I suggest normalizing the actual header markup, instead of doing all these css tricks to make it match the mockup.

Comment 34

[Victor Porof [:vporof][:vp]]

Attachment: screenshot (bottom)

Comment 35

[Victor Porof [:vporof][:vp]]

Attachment: screenshot (side)

Comment 36

[Tim Nguyen :ntim]

Albert, are you still working on this ? I could finish this if you want (still with your commit info).

Comment 37

[Albert Juhé]

Hi Tim, I don't have a lot of time these days so for me it's ok if you want to continue it. I was thinking to work again on it some day of the next week, but I don't know for sure if I will have time, so probably it is better if somebody else finishes it. I can take another bug when I have more time.

Comment 38

[Girish Sharma [:Optimizer]]

Please also update headers for the TableWidget in bub 993014

Comment 39

[Albert Juhé]

Tim, I see this bug is still assigned to me. Did you start working on it?

Comment 40

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #39)
> Tim, I see this bug is still assigned to me. Did you start working on it?

Nope, I didn't have much time either. I need to finish the most important bugs I'm assigned to.

Comment 41

[Albert Juhé]

Attachment: patch6.patch

Well, after a long delay (sorry for that!) I updated a new version of the patch with the changes from Comment 29 and Comment 32.

> Granted, the first column is a bit trickier because of the different layout, so I suggest normalizing the actual header markup, instead of doing all these css tricks to make it match the mockup.
If we could move the two <button>s (method and status column headers) to use a different <hbox> it would simplify a little bit the CSS, but as you already said, the biggest problem is that the current design doesn't have space for the arrow. Do you have any idea in how to solve that? Sorry, I'm quite new in Firefox development and I need some advice. :)

> Please also update headers for the TableWidget in bub 993014
How does that bug affect this bug? I would need some advice here too...

Comment 42

[Albert Juhé]

Attachment: patch6-screenshot.png

Comment 43

[Brian Grinstead [:bgrins]]

Comment on attachment 8443023 [details] [diff] [review]
patch6.patch

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

Looks like this is the wrong patch?

Comment 44

[Albert Juhé]

Attachment: patch6-real.patch

Wow, sorry. Even checking it thousands of times I always end up uploading the wrong file!

Comment 45

[Victor Porof [:vporof][:vp]]

(In reply to Albert Juhé from comment #44)
> Created attachment 8443036 [details] [diff] [review]
> patch6-real.patch
> 
> Wow, sorry. Even checking it thousands of times I always end up uploading
> the wrong file!

There don't seem to be any substantial differences between this patch and attachment 8408309 [details] [diff] [review]. Please check comment 33 for the best direction when fixing this bug.

Comment 46

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vporof][:vp] from comment #45)
> (In reply to Albert Juhé from comment #44)

To be more specific, we should normalize the markup to not special-case certain headers or columns (like status + label, method + code). This bug is a great place to actually fix this so that there's a separate column for each header.

Comment 47

[Brian Grinstead [:bgrins]]

Comment on attachment 8443036 [details] [diff] [review]
patch6-real.patch

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

> If we could move the two <button>s (method and status column headers) to use a different <hbox>

Yes, I think based on comment 33 and comment 46, let's split it up so that the markup is more standard.  So under the "Status" header there would be both the status icon + the status code, and the method header would just have the method name underneath it.  This may also alleviate the arrow problem, since that will make the status column wider (as it would now include the code)

Comment 48

[Albert Juhé]

Attachment: patch7.patch

I attached a new patch where I normalized the markup for the headers. It makes everything easier. I marked it for review because I think there is nothing missing, but let me know if there is something you think must be changed. ;)

Comment 49

[Albert Juhé]

Attachment: patch7-screenshot.png

Comment 50

[Victor Porof [:vporof][:vp]]

From a quick glance over the screenshot, it looks like the timeline ticks (0ms, 640ms, 1.28s) are not aligned with the lines below them (in the table) anymore.

Comment 51

[Victor Porof [:vporof][:vp]]

Also, the header borders are not aligned with the cell borders in the table.

Comment 52

[Tim Nguyen :ntim]

The selected header shouldn't be bold (see inspector/toolbox tabs), and the selected header arrow shouldn't be inverted.

Comment 53

[Brian Grinstead [:bgrins]]

Comment on attachment 8444124 [details] [diff] [review]
patch7.patch

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

I'm having some issues applying the patch - can you rebuild it off of the latest fx-team?  As mentioned, there are a couple of alignment issues with the borders but I think it is on the right track.  I like hot the new table grouping looks more, and think that it simplifies the CSS.

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +308,5 @@
>    transform-origin: right center;
>  }
>  
> +.theme-dark .requests-menu-timings-division {
> +  -moz-border-start-color: #5a6169 !important; /* Light foreground text */

I know this color is taken from the sidebar tabs, so I'd be OK with using it for now and dealing with this in a follow up.. But we should be trying to use colors from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors whenever possible.

Since the string #5a6169 appears only in a few places (here and in toolbars.inc.css) I would suggest that we replace it in both files with the corresponding Grey Foreground Text color to match the light theme's use: #b6babf.  If this doesn't look good, we could use #5f7387; /* Dark grey content text */.

Comment 54

[Albert Juhé]

Attachment: patch8.patch

Ok, I solved most of the issues in this new patch. There is only one thing remaining: in the responsive sidebar, even though all the columns' width add to 100vw, they do not occupy the full width (you can see it in the screenshot). Any idea why this is happening?

> I know this color is taken from the sidebar tabs, so I'd be OK with using it for now
> and dealing with this in a follow up.. But we should be trying to use colors from
> https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors whenever possible.
> 
> Since the string #5a6169 appears only in a few places (here and in toolbars.inc.css) I
> would suggest that we replace it in both files with the corresponding Grey Foreground
> Text color to match the light theme's use: #b6babf.  If this doesn't look good, we
> could use #5f7387; /* Dark grey content text */.

Is it ok if I fill a new bug to handle this? This one is getting huge and I would like to finish it soon. :)

Comment 55

[Albert Juhé]

Attachment: patch8-issue-screenshot.png

Comment 56

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #54)
> Created attachment 8466647 [details] [diff] [review]
> patch8.patch
> 
> Ok, I solved most of the issues in this new patch. There is only one thing
> remaining: in the responsive sidebar, even though all the columns' width add
> to 100vw, they do not occupy the full width (you can see it in the
> screenshot). Any idea why this is happening?

I'm not sure I understand exactly what the issue is from the screenshot.  Is it that you are not seeing a right border on the 'size' button header?  For some reason, I'm not seeing any borders on the headers with the patch applied (light or dark theme).  Seems like maybe the border-image on .requests-menu-header-button is not visible?

> 
> > I know this color is taken from the sidebar tabs, so I'd be OK with using it for now
> > and dealing with this in a follow up.. But we should be trying to use colors from
> > https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors whenever possible.
> > 
> > Since the string #5a6169 appears only in a few places (here and in toolbars.inc.css) I
> > would suggest that we replace it in both files with the corresponding Grey Foreground
> > Text color to match the light theme's use: #b6babf.  If this doesn't look good, we
> > could use #5f7387; /* Dark grey content text */.
> 
> Is it ok if I fill a new bug to handle this? This one is getting huge and I
> would like to finish it soon. :)

Sure, it would be fine to follow up with the color changes.

Comment 57

[Albert Juhé]

(In reply to Brian Grinstead [:bgrins] from comment #56)
> I'm not sure I understand exactly what the issue is from the screenshot.  Is
> it that you are not seeing a right border on the 'size' button header?  For
> some reason, I'm not seeing any borders on the headers with the patch
> applied (light or dark theme).  Seems like maybe the border-image on
> .requests-menu-header-button is not visible?

My plan was to make the columns occupy the 100% of the available spacing making their width in vw add to 100, but I don't know why it's not working, there is always an empty space after "Size" column.

In the current design we have the same problem, so I'm not sure how important is to fix that.

I also tried adding a right border to the "Size" column header. It would not fix the problem but at least it would look a little bit better. However, it was 1px misaligned to the left and it was impossible to align it with the table border. I tried making the header column a little bit bigger but I couldn't get it.

Comment 58

[Brian Grinstead [:bgrins]]

Attachment: patch8-no-border-headers.png

This is what I was talking about (I don't see any borders at all on the headers).  Are you sure that you have the most up to date patch uploaded?

Comment 59

[Albert Juhé]

(In reply to Brian Grinstead [:bgrins] from comment #58)
> Created attachment 8467150 [details]
> patch8-no-border-headers.png
> 
> This is what I was talking about (I don't see any borders at all on the
> headers).  Are you sure that you have the most up to date patch uploaded?

I don't understand why this is happening. For me it works fine. Today I pulled the code and compiled everything from the scratch again and it's working.

The first modified lines in /browser/themes/shared/devtools/netmonitor.inc.css is where the border is added. I can't figure out what's going on.

Comment 60

[Girish Sharma [:Optimizer]]

Hi Albert, can you also update the headers for the TableWidget [0] ? The css is present in widgets.inc.css [1]

It is going to be used in both web console (bug 899753) and storage inspector (bug 970517).

The sort indicator being too dim in current headers has been already come up as a concern wrt web console.

To test, you can apply patches from either of the bugs.

Thanks.

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/TableWidget.js
[1] http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/widgets.inc.css#1226-1362

Comment 61

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #60)
> Hi Albert, can you also update the headers for the TableWidget [0] ? The css
> is present in widgets.inc.css [1]
> 
> It is going to be used in both web console (bug 899753) and storage
> inspector (bug 970517).
> 
> The sort indicator being too dim in current headers has been already come up
> as a concern wrt web console.
> 
> To test, you can apply patches from either of the bugs.
> 
> Thanks.
> 
> [0]
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/
> widgets/TableWidget.js
> [1]
> http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/
> widgets.inc.css#1226-1362

I think this should be done as a follow up.  There are enough changes here - we can tackle reskinning the TableWidget separately, especially since neither of the uses are currently live.

Comment 62

[Brian Grinstead [:bgrins]]

Comment on attachment 8466647 [details] [diff] [review]
patch8.patch

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

Looks like there is another part of netmonitor-view.js that needs to change (I see an error when loading the page):

Full message: TypeError: node is null
Full stack: RequestsMenuView.prototype<.updateMenuView<@chrome://browser/content/devtools/netmonitor-view.js:1278:9

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +86,5 @@
>  
>  .requests-menu-header-button {
>    -moz-appearance: none;
> +  background-color: transparent;
> +  border-width: 0 0 0 1px;

Figured out the issue with the borders not showing up - you need to add a border-style: solid for support in OSX (I'm guessing your UA styles were already defining this).

Comment 63

[Albert Juhé]

Attachment: 951714patch9.patch

(In reply to Brian Grinstead [:bgrins] from comment #62)
> Comment on attachment 8466647 [details] [diff] [review]
> patch8.patch
> 
> Review of attachment 8466647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like there is another part of netmonitor-view.js that needs to change
> (I see an error when loading the page):
> 
> Full message: TypeError: node is null
> Full stack:
> RequestsMenuView.prototype<.updateMenuView<@chrome://browser/content/
> devtools/netmonitor-view.js:1278:9
> 

How can I reproduce this error?

> ::: browser/themes/shared/devtools/netmonitor.inc.css
> @@ +86,5 @@
> >  
> >  .requests-menu-header-button {
> >    -moz-appearance: none;
> > +  background-color: transparent;
> > +  border-width: 0 0 0 1px;
> 
> Figured out the issue with the borders not showing up - you need to add a
> border-style: solid for support in OSX (I'm guessing your UA styles were
> already defining this).

Solved, thank you! ;)

Comment 64

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #63)
> Created attachment 8467910 [details] [diff] [review]
> 951714patch9.patch
> 
> (In reply to Brian Grinstead [:bgrins] from comment #62)
> > Comment on attachment 8466647 [details] [diff] [review]
> > patch8.patch
> > 
> > Review of attachment 8466647 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks like there is another part of netmonitor-view.js that needs to change
> > (I see an error when loading the page):
> > 
> > Full message: TypeError: node is null
> > Full stack:
> > RequestsMenuView.prototype<.updateMenuView<@chrome://browser/content/
> > devtools/netmonitor-view.js:1278:9
> > 
> 
> How can I reproduce this error?

I'm fairly sure it happened when reloading the page with the netmonitor opened, but it may have been once I selected one of the resources from the list.  It's basically trying to select a node that doesn't exist: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js#1273.  You should see the error in the browser console or in the command line that you used to mach run the process.

Comment 65

[Albert Juhé]

Attachment: 951714patch.patch

True. It should be solved now.

Comment 66

[Brian Grinstead [:bgrins]]

Comment on attachment 8467918 [details] [diff] [review]
951714patch.patch

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

This needs a simple rebase for toolbars.inc.css.rej and netmonitor.xul.rej.  The headers do look quite nice with it applied, though I think Victor should review the code changes, specifically to the netmonitor files.

Comment 67

[Victor Porof [:vporof][:vp]]

Will do! Thanks!

Comment 68

[Victor Porof [:vporof][:vp]]

Comment on attachment 8467918 [details] [diff] [review]
951714patch.patch

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

Sorry about the long delay. I had a cold.

There's a few small problems still remaining, but in general this looks good. Take a look at the attached screenshots and review comments below.

* status code shapes aren't custom colored anymore; all of them are dark gray (screenshot 1)
* the Type header's text is misaligned (screenshot 2)
* the Type header's sort arrow overlaps the right border (screenshot 3)
* the Method header is unnecessarily too wide now

Also, perhaps we can now change the checkmark symbol to read "Status" now? Can you please file a followup bug for that? Thank you!

r+ with the comments below addressed.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +796,5 @@
>        } else {
>          target.setAttribute("sorted", direction = "ascending");
>          target.setAttribute("tooltiptext", L10N.getStr("networkMenu.sortedAsc"));
>        }
> +      // Used to style the next column

Nit: add a fullstop (.) at the end of the comment.

@@ +1273,5 @@
>          node.setAttribute("code", aValue);
>          break;
>        }
>        case "statusText": {
> +        let node = $(".requests-menu-status", target);

Use .label.requests-menu-status here too.

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +43,5 @@
>  %define table_itemLightStartBorder rgba(128,128,128,0.25)
>  %define table_itemLightEndBorder transparent
> +%define smallSeparatorDark linear-gradient(transparent 15%, #5a6169 15%, #5a6169 85%, transparent 85%)
> +%define smallSeparatorLight linear-gradient(transparent 15%, #aaa 15%, #aaa 85%, transparent 85%)
> +%define solidSeparatorDark linear-gradient(#2d5b7d, #2d5b7d)

Can we please use theme colors for all of this stuff instead? Touch base with Brian in bug 947242 maybe?

Colors: https://developer.mozilla.org/en-US/docs/Tools/DevToolsAPI

@@ +54,5 @@
>  }
>  
> +#toolbar-labels .requests-menu-header {
> +  border-width: 0;
> +  box-shadow: none;

If I'm reading this correctly, you're removing the border width and box-shadow here, while they're being added below, in the rules that look like this:

.theme-dark .requests-menu-header:not(:last-child),
.theme-dark .requests-menu-subitem:not(:last-child) {
  -moz-border-end: 1px solid @table_itemDarkStartBorder@;
  box-shadow: 1px 0 0 @table_itemDarkEndBorder@;
}

The border width and box shadow end up being removed because this selector has more specificity (#toolbar-labels .requests-menu-header), but it'd be a better idea to just remove the redundant rules instead of overriding them.

@@ +87,5 @@
>  .requests-menu-header-button {
>    -moz-appearance: none;
> +  background-color: transparent;
> +  border-style: solid;
> +  border-width: 0 0 0 1px;

This isn't properly localized. Use -moz-border-start and -moz-border-end rules.

@@ +92,4 @@
>    min-width: 1px;
>    min-height: 24px;
>    margin: 0;
> +  padding: 0 17px;

Why 17? Just curious. Did you calculate this value some way?

@@ +106,3 @@
>  }
>  
> +#requests-menu-status-header-box .requests-menu-header-button {

#requests-menu-status-header-box is not necessary here AFAICT.

@@ +110,4 @@
>  }
>  
> +.theme-dark .requests-menu-header-button:hover {
> +  background-color: hsla(206,37%,4%,.2);

Again, can we use theme colors instead? See my previous comment and bug 947242.

@@ +114,4 @@
>  }
>  
> +.theme-light .requests-menu-header-button:hover {
> +  background-color: #ddd;

Ditto.

@@ +120,5 @@
> +.requests-menu-header-button::after {
> +  content: "";
> +  display: block;
> +  height: 4px;
> +  margin-top: 11px;

Is this properly centered on Linux and Windows, with custom font sizes?

@@ +121,5 @@
> +  content: "";
> +  display: block;
> +  height: 4px;
> +  margin-top: 11px;
> +  margin-right: -10px;

This is not properly localized. Need to use -moz-margin-end.

@@ +126,5 @@
> +  width: 7px;
> +}
> +
> +.requests-menu-header-button.requests-menu-waterfall::after {
> +  margin-right: 10px;

It is very interesting that you had to special case this. Is it because the collapse/expand sidebar button? Can you add a comment here describing why this is needed?

@@ +130,5 @@
> +  margin-right: 10px;
> +}
> +
> +.requests-menu-header-button[sorted=ascending]::after {
> +  background: url('chrome://browser/skin/devtools/sort-arrows.png') no-repeat -13px -3px;

You don't seem to be using the @2x images anywhere. Why? Retina support should be trivial to add, especially since this patch already contains the assets AFAICT.

@@ +138,5 @@
> +  background: url('chrome://browser/skin/devtools/sort-arrows.png') no-repeat -2px -4px;
> +}
> +
> +.theme-dark .requests-menu-header-button[sorted] {
> +  background-color: #1d4f73;

Nit: touch base with Brian for this.

@@ +143,5 @@
> +  border-image: @solidSeparatorDark@ 1 1;
> +}
> +
> +.theme-light .requests-menu-header-button[sorted] {
> +  background-color: #4c9ed9;

Ditto.

@@ +165,5 @@
>  }
>  
> +.requests-menu-header.requests-menu-status,
> +.requests-menu-header-button.requests-menu-status {
> +  width: 59px;

Why is this width different from the one above? This looks very dirty. You should use box-sizing to achieve what you need instead of hard coding the widths and subtracting an assumed 1px border size.

Also, please use em-based widths for all columns like before.

@@ +203,5 @@
>  }
>  
>  .requests-menu-type {
>    text-align: center;
> +  width: 5em;

This seems to not be enough. Why not simply make the up/down arrow not occupy any layout space?

Something like {
  width: 7px;
  -moz-margin-end: -7px;
  transform: translate(whatever px);
}
for the .requests-menu-header-button::after arrow might work as you'd expect.

@@ +299,5 @@
>  
>  /* Network requests table: waterfall header */
>  
> +.requests-menu-waterfall {
> +  padding: 0;

Hmm, looks like this is why you had to special case .requests-menu-header-button.requests-menu-waterfall::after above with a custom margin? You should keep removing the padding for all headers, not just the waterfall, like it was the case before.

@@ +316,5 @@
>    pointer-events: none;
>  }
>  
> +.requests-menu-timings-division:first-child {
> +  width: 99px;

Why is this here now, but wasn't necessary before? Looks like a content-sizing problem again, which assumes there's a 1px border somewhere. This should be more elegantly handled, rather than just hard coding a fixed width 1px smaller.

@@ +851,5 @@
>      /* To prevent all the margin hacks to hide the sidebar. */
>    }
>  
> +  .requests-menu-status {
> +    padding-left: 0;

Not localized properly.

@@ +857,5 @@
> +  }
> +
> +  .requests-menu-header.requests-menu-status,
> +  .requests-menu-header-button.requests-menu-status {
> +    width: calc(6vw - 1px);

Ugh....

Comment 69

[Victor Porof [:vporof][:vp]]

Attachment: status codes and shapes

Comment 70

[Victor Porof [:vporof][:vp]]

Attachment: miscentered type column

Comment 71

[Victor Porof [:vporof][:vp]]

Attachment: type arrow overlapping right border

Comment 73

[Albert Juhé]

Attachment: bug951714.patch

I'm working on it again (after a long time, sorry!) and I have solved almost all the issues you said in your last comment. However, I have some questions for you:

(In reply to Victor Porof [:vporof][:vp] from comment #68)
> 
> @@ +120,5 @@
> > +.requests-menu-header-button::after {
> > +  content: "";
> > +  display: block;
> > +  height: 4px;
> > +  margin-top: 11px;
> 
> Is this properly centered on Linux and Windows, with custom font sizes?
> 

What do you mean "custom font sizes"? How can I test it?

> 
> Can we please use theme colors for all of this stuff instead? Touch base with Brian in bug 947242 maybe?
> 
> Colors: https://developer.mozilla.org/en-US/docs/Tools/DevToolsAPI
> 

Few weeks ago I filed Bug 1049012 to handle color inconsistencies (maybe I should rename it to include all color inconsistencies and not only one) because I was waiting for Bug 947242 to land.

> Why is this width different from the one above? This looks very dirty. You should use box-sizing to achieve what you need instead of hard coding the widths and subtracting an assumed 1px border size.

I can't make box-sizing to work on table headers.

I'm trying to add "box-sizing: border-box" to .requests-menu-header-button but it's completely ignored. Any idea why or what I'm doing wrong?

> Also, please use em-based widths for all columns like before.

Actually when devtools are vertical, we are using vw-based width. Should we unify them to use in all cases em-based/vw-based or is it ok if they use different units?

---

Btw, what happened with the Browser toolbox tool to inspect UI elements from the chrome? I can't find it when compiling the source but in Firefox Developer Edition it's there.

Comment 74

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #73)
> Created attachment 8523545 [details] [diff] [review]
> bug951714.patch
> 
> I'm working on it again (after a long time, sorry!) and I have solved almost
> all the issues you said in your last comment. However, I have some questions
> for you:
> 
> (In reply to Victor Porof [:vporof][:vp] from comment #68)
> > 
> > @@ +120,5 @@
> > > +.requests-menu-header-button::after {
> > > +  content: "";
> > > +  display: block;
> > > +  height: 4px;
> > > +  margin-top: 11px;
> > 
> > Is this properly centered on Linux and Windows, with custom font sizes?
> > 
> 
> What do you mean "custom font sizes"? How can I test it?
On Windows, you can set a bigger font size for accessibility.
But you can set layout.css.devPixelsPerPx to something like 1.2 (or bigger) to test that.

> > Why is this width different from the one above? This looks very dirty. You should use box-sizing to achieve what you need instead of hard coding the widths and subtracting an assumed 1px border size.
> 
> I can't make box-sizing to work on table headers.
> 
> I'm trying to add "box-sizing: border-box" to .requests-menu-header-button
> but it's completely ignored. Any idea why or what I'm doing wrong?
Must be some XUL weirdness.
 
> Btw, what happened with the Browser toolbox tool to inspect UI elements from
> the chrome? I can't find it when compiling the source but in Firefox
> Developer Edition it's there.
You need to "Enable chrome and add-on debugging" and "Enable remote debugging" from the devtools options panel.

Comment 75

[Victor Porof [:vporof][:vp]]

Thanks Tim for answering some of the questions in a more timely manner :)

(In reply to Albert Juhé from comment #73)
> Created attachment 8523545 [details] [diff] [review]
> bug951714.patch
> 

> I can't make box-sizing to work on table headers.
> 
> I'm trying to add "box-sizing: border-box" to .requests-menu-header-button
> but it's completely ignored. Any idea why or what I'm doing wrong?
> 

This should work. Try playing with the property a bit more, maybe you're not applying it to the right element. Using -1px tricks is very dirty and I would much rather avoid doing that. Stuff like this has bitten us badly in the past.

> > Also, please use em-based widths for all columns like before.
> 
> Actually when devtools are vertical, we are using vw-based width. Should we
> unify them to use in all cases em-based/vw-based or is it ok if they use
> different units?
> 

We should use vw/vh everywhere.

Comment 76

[Albert Juhé]

Attachment: 951714patch12.patch

Finally, I updated a new version with the new changes. As far as I noticed, there are only two things missing - for sure you will find more though! ;):

1. When the window or devtools panel is too narrow, the text overflows the arrow. I don't know which is the best solution in those cases: should we make the column wider? In the case of the screenshot I will attach in the next comment, you will see that we would waste a lot of space in the column method if we do that. Another solution would be to trim the text like we are doing with the toolbox tabs.

2. When I set layout.css.devPixelsPerPx to 2, it makes the arrow not to be correctly aligned. However, it works fine when it's set to 1.9. Not sure if 2 is a possible value and I have to fix that or it's not possible to set it to 2.

(In reply to Tim Nguyen [:ntim] from comment #74)
> (In reply to Albert Juhé from comment #73)
> > Btw, what happened with the Browser toolbox tool to inspect UI elements from
> > the chrome? I can't find it when compiling the source but in Firefox
> > Developer Edition it's there.
> You need to "Enable chrome and add-on debugging" and "Enable remote
> debugging" from the devtools options panel.

Thanks Tim, it is very useful!

Comment 77

[Albert Juhé]

Attachment: text-arrow.png

Comment 78

[Albert Juhé]

Attachment: px2.png

Comment 79

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #76)
> Created attachment 8535863 [details] [diff] [review]
> 951714patch12.patch
> 
> Finally, I updated a new version with the new changes. As far as I noticed,
> there are only two things missing - for sure you will find more though! ;):
> 
> 1. When the window or devtools panel is too narrow, the text overflows the
> arrow. I don't know which is the best solution in those cases: should we
> make the column wider? In the case of the screenshot I will attach in the
> next comment, you will see that we would waste a lot of space in the column
> method if we do that. Another solution would be to trim the text like we are
> doing with the toolbox tabs.
You should make sure text is trimmed properly either way.

> 2. When I set layout.css.devPixelsPerPx to 2, it makes the arrow not to be
> correctly aligned. However, it works fine when it's set to 1.9. Not sure if
> 2 is a possible value and I have to fix that or it's not possible to set it
> to 2.
You need to set a background-size that is equal to the 1x image dimensions.
Btw, the values you need to test for Windows are : 1.25, 1.5, 1.75.
Note that setting the devPixelsPerPx pref to 2 allows you to emulate retina (but I guess you noticed that).

Comment 80

[Albert Juhé]

Attachment: 951714patch13.patch

(In reply to Tim Nguyen [:ntim] from comment #79)
> (In reply to Albert Juhé from comment #76)
> > Created attachment 8535863 [details] [diff] [review]
> > 951714patch12.patch
> > 
> > Finally, I updated a new version with the new changes. As far as I noticed,
> > there are only two things missing - for sure you will find more though! ;):
> > 
> > 1. When the window or devtools panel is too narrow, the text overflows the
> > arrow. I don't know which is the best solution in those cases: should we
> > make the column wider? In the case of the screenshot I will attach in the
> > next comment, you will see that we would waste a lot of space in the column
> > method if we do that. Another solution would be to trim the text like we are
> > doing with the toolbox tabs.
> You should make sure text is trimmed properly either way.

I'm having some trouble here. Should I trim the text using text-overflow: ellipsis? I don't know why but it seems not to work there and I can't figure out how the text is trimmed in the toolbox tabs.

> > 2. When I set layout.css.devPixelsPerPx to 2, it makes the arrow not to be
> > correctly aligned. However, it works fine when it's set to 1.9. Not sure if
> > 2 is a possible value and I have to fix that or it's not possible to set it
> > to 2.
> You need to set a background-size that is equal to the 1x image dimensions.
> Btw, the values you need to test for Windows are : 1.25, 1.5, 1.75.
> Note that setting the devPixelsPerPx pref to 2 allows you to emulate retina
> (but I guess you noticed that).

Fixed! Thank you.

Comment 81

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #80)
> Created attachment 8536002 [details] [diff] [review]
> 951714patch13.patch
> 
> (In reply to Tim Nguyen [:ntim] from comment #79)
> > (In reply to Albert Juhé from comment #76)
> > > Created attachment 8535863 [details] [diff] [review]
> > > 951714patch12.patch
> > > 
> > > Finally, I updated a new version with the new changes. As far as I noticed,
> > > there are only two things missing - for sure you will find more though! ;):
> > > 
> > > 1. When the window or devtools panel is too narrow, the text overflows the
> > > arrow. I don't know which is the best solution in those cases: should we
> > > make the column wider? In the case of the screenshot I will attach in the
> > > next comment, you will see that we would waste a lot of space in the column
> > > method if we do that. Another solution would be to trim the text like we are
> > > doing with the toolbox tabs.
> > You should make sure text is trimmed properly either way.
> 
> I'm having some trouble here. Should I trim the text using text-overflow:
> ellipsis? I don't know why but it seems not to work there and I can't figure
> out how the text is trimmed in the toolbox tabs.
Try adding the crop="end" attributes to the headers.

Comment 82

[Albert Juhé]

Attachment: 951714patch14.patch

(In reply to Tim Nguyen [:ntim] from comment #81)
> Try adding the crop="end" attributes to the headers.

Done! Thank you!

The only problem I see now is that sometimes the full text is trimmed so the user can only see ellipsis (see next screenshot). Any suggestion for that?

Comment 83

[Albert Juhé]

Attachment: onlyellipsisshown.png

Comment 84

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #83)
> Created attachment 8536168 [details]
> onlyellipsisshown.png

Ah, now that I see how it looks like, it's probably a better idea to just increase the width of the small columns.

Comment 85

[Tim Nguyen :ntim]

But Victor should have a better idea what to do.

Comment 86

[Tim Nguyen :ntim]

Comment on attachment 8536167 [details] [diff] [review]
951714patch14.patch

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

I think it's worth it to re-use the CSS variables from Bug 1102369 throughout the file.

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +132,5 @@
> +  }
> +}
> +
> +.theme-dark .requests-menu-header-button[sorted] {
> +  background-color: #1d4f73;

Can we use the CSS variables from Bug 1102369 ?
.requests-menu-header-button[sorted] {
  background-color: var(--theme-selection-background);
  color: var(--theme-selection-color);
}

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +835,5 @@
>  .theme-light #requests-menu-perf-notice-button .button-icon,
>  .theme-light #requests-menu-network-summary-button .button-icon,
> +.theme-light .event-tooltip-debugger-icon,
> +.theme-light .requests-menu-header-button[sorted=ascending]::after,
> +.theme-light .requests-menu-header-button[sorted=descending]::after {

This change isn't needed, since we have a blue background behind those arrows. So having those arrows white looks better anyway.

Comment 87

[Tim Nguyen :ntim]

Comment on attachment 8536168 [details]
onlyellipsisshown.png

I noticed that the timestamps are not aligned properly in the screenshot, can you fix that as well ?

Comment 88

[Albert Juhé]

Attachment: 951714patch15.patch

(In reply to Tim Nguyen [:ntim] from comment #87)
> Comment on attachment 8536168 [details]
> onlyellipsisshown.png
> 
> I noticed that the timestamps are not aligned properly in the screenshot,
> can you fix that as well ?

Fixed!

Comment 89

[Albert Juhé]

Attachment: strange-variable-processing.png

(In reply to Tim Nguyen [:ntim] from comment #86)
> Comment on attachment 8536167 [details] [diff] [review]
> 951714patch14.patch
> 
> Review of attachment 8536167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it's worth it to re-use the CSS variables from Bug 1102369
> throughout the file.
> 
> ::: browser/themes/shared/devtools/netmonitor.inc.css
> @@ +132,5 @@
> > +  }
> > +}
> > +
> > +.theme-dark .requests-menu-header-button[sorted] {
> > +  background-color: #1d4f73;
> 
> Can we use the CSS variables from Bug 1102369 ?
> .requests-menu-header-button[sorted] {
>   background-color: var(--theme-selection-background);
>   color: var(--theme-selection-color);
> }

There is something weird when I do that. It's like it replaces the word "background" before processing the variable. I will try to figure out what is happening next week.

Comment 90

[Albert Juhé]

Attachment: inverted-arrow-color-comparison.png

(In reply to Tim Nguyen [:ntim] from comment #86)
> 
> ::: browser/themes/shared/devtools/toolbars.inc.css
> @@ +835,5 @@
> >  .theme-light #requests-menu-perf-notice-button .button-icon,
> >  .theme-light #requests-menu-network-summary-button .button-icon,
> > +.theme-light .event-tooltip-debugger-icon,
> > +.theme-light .requests-menu-header-button[sorted=ascending]::after,
> > +.theme-light .requests-menu-header-button[sorted=descending]::after {
> 
> This change isn't needed, since we have a blue background behind those
> arrows. So having those arrows white looks better anyway.

For me it looks better when the arrow is dark. I made this image to compare both versions. Anyway, if you are sure we should change it, I will. ;)

Comment 91

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #90)
> Created attachment 8536197 [details]
> inverted-arrow-color-comparison.png
> 
> (In reply to Tim Nguyen [:ntim] from comment #86)
> > 
> > ::: browser/themes/shared/devtools/toolbars.inc.css
> > @@ +835,5 @@
> > >  .theme-light #requests-menu-perf-notice-button .button-icon,
> > >  .theme-light #requests-menu-network-summary-button .button-icon,
> > > +.theme-light .event-tooltip-debugger-icon,
> > > +.theme-light .requests-menu-header-button[sorted=ascending]::after,
> > > +.theme-light .requests-menu-header-button[sorted=descending]::after {
> > 
> > This change isn't needed, since we have a blue background behind those
> > arrows. So having those arrows white looks better anyway.
> 
> For me it looks better when the arrow is dark. I made this image to compare
> both versions. Anyway, if you are sure we should change it, I will. ;)

Ah, the arrow looks too faint, but I guess that also makes it nearly invisible on dark theme as well. If that's the case, I can provide assets at a higher opacity.
But in both ways, I think having the arrow color match the text color is better.

Comment 92

[Albert Juhé]

(In reply to Tim Nguyen [:ntim] from comment #91)
> But in both ways, I think having the arrow color match the text color is
> better.

Then, would it be better to make the arrows SVG so we can change the color easily with CSS? Otherwise, the colors must be:
- Dark theme: #B6BABF
- Light theme: #585959

I also need help with Comment 89. The system is always replacing the string "background" for "F1F2F0" and I can't figure out why. I checked on the code and that color only appears in linux/browser.css but it has nothing to do with variables or preprocessor definitions, so I'm completely puzzled.

That makes all the variables that contain the word "background" not to work, because it's replaced before. Any idea about what I am missing?

Comment 93

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #92)
> (In reply to Tim Nguyen [:ntim] from comment #91)
> > But in both ways, I think having the arrow color match the text color is
> > better.
> 
> Then, would it be better to make the arrows SVG so we can change the color
> easily with CSS? Otherwise, the colors must be:
> - Dark theme: #B6BABF
> - Light theme: #585959
No, the text color is always #f5f7fa (var(--theme-selection-color)) for the selected header. But I could make an SVG with that color if wanted.

> I also need help with Comment 89. The system is always replacing the string
> "background" for "F1F2F0" and I can't figure out why. I checked on the code
> and that color only appears in linux/browser.css but it has nothing to do
> with variables or preprocessor definitions, so I'm completely puzzled.
> 
> That makes all the variables that contain the word "background" not to work,
> because it's replaced before. Any idea about what I am missing?
This looks like a bug specific to Firefox DevTools, can you file one ?

Comment 94

[Tim Nguyen :ntim]

The bug doesn't really change anything in the source file, and doesn't prevent this bug from being worked on.

Comment 95

[Albert Juhé]

Sorry again for the delay, I am always busier than what I would like to be.

I have been trying to replace everywhere the colors with the proper variables. What should I do when there is a color that has no variable? Should I replace that color for the most similar variable, create a new variable or just leave it?

For example:
> .theme-dark .requests-menu-header-button:hover {
>   background-color: hsla(206,37%,4%,.2);
> }
> 
> .theme-light .requests-menu-header-button:hover {
>   background-color: #ddd;
> }

Anyway, would it be possible to make the colors replacement in another bug? I don't think that is something that blocks this one and this way we can split the work in smaller parts. :)

(In reply to Tim Nguyen [:ntim] from comment #91)
> (In reply to Albert Juhé from comment #90)
> > Created attachment 8536197 [details]
> > inverted-arrow-color-comparison.png
> > 
> > (In reply to Tim Nguyen [:ntim] from comment #86)
> > > 
> > > ::: browser/themes/shared/devtools/toolbars.inc.css
> > > @@ +835,5 @@
> > > >  .theme-light #requests-menu-perf-notice-button .button-icon,
> > > >  .theme-light #requests-menu-network-summary-button .button-icon,
> > > > +.theme-light .event-tooltip-debugger-icon,
> > > > +.theme-light .requests-menu-header-button[sorted=ascending]::after,
> > > > +.theme-light .requests-menu-header-button[sorted=descending]::after {
> > > 
> > > This change isn't needed, since we have a blue background behind those
> > > arrows. So having those arrows white looks better anyway.
> > 
> > For me it looks better when the arrow is dark. I made this image to compare
> > both versions. Anyway, if you are sure we should change it, I will. ;)
> 
> Ah, the arrow looks too faint, but I guess that also makes it nearly
> invisible on dark theme as well. If that's the case, I can provide assets at
> a higher opacity.
> But in both ways, I think having the arrow color match the text color is
> better.

So yes, could you upload the images here? I will replace them and try to see how it looks.

Comment 96

[Albert Juhé]

Victor, can I get your opinion about Comment 82 and Comment 83? Should we set a min-width for small columns or make them crop the text even when only an ellipsis is shown?

Setting a min-width means that the last column won't be displayed completely.

Comment 97

[Victor Porof [:vporof][:vp]]

There's already a width in the css iirc, so just increasing that should do the trick.

Comment 98

[Albert Juhé]

Attachment: 951714patch16.patch

I attached a new version of the patch that fixes most of the problems that we said on the last comments.

I couldn't try it on a RTL Firefox because I was not able to install this extension on Firefox 38:
https://addons.mozilla.org/en-US/firefox/addon/force-rtl/

If I'm not wrong, there are only two things missing:

- color replacement (Comment 95): I would like to do that in a follow up bug
- replace the arrow images (Comment 95): waiting for Tim.

Comment 99

[Tim Nguyen :ntim]

Attachment: sort-arrows.svg

Here's an SVG with exactly the same shapes, but with a different color. I've set the opacity to 0.8 (It was at ~0.35 before), but with the SVG, you can tweak it to your liking.

Comment 100

[Albert Juhé]

Attachment: 951714patch17.patch

Here is the patch with the new arrows.

Comment 101

[Albert Juhé]

Attachment: 951714patch17screenshot.png

And a screenshot showing the new SVG arrows. Thank you Tim!

Comment 102

[Victor Porof [:vporof][:vp]]

Comment on attachment 8554295 [details] [diff] [review]
951714patch17.patch

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

Looking solid, thank you! Just a few comments below, and this should be good to go afterwards.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +806,3 @@
>    sortBy: function(aType = "waterfall") {
>      let target = $("#requests-menu-" + aType + "-button");
>      let headers = document.querySelectorAll(".requests-menu-header-button");

Instead of accessing the parent node everywhere, it's probably safer to querySelectorAll for the requests-menu-*-header-box here, and query for requests-menu-*-button when needed, now that there's always one button inside one box for all headers.

@@ +1284,5 @@
>      let target = aItem.target || aItem;
>  
>      switch (aKey) {
>        case "method": {
> +        let node = $("label.requests-menu-method", target);

Is the label. selector necessary here? Are we using the same class for multiple labels in different places, when we shouldn't? Smells like we need to rename our classes to keep things consistent, or use ids if we have to be specific.

::: browser/devtools/netmonitor/netmonitor.xul
@@ +63,5 @@
>                      align="center">
>                  <button id="requests-menu-status-button"
>                          class="requests-menu-header-button requests-menu-status"
>                          data-key="status"
>                          label="&netmonitorUI.toolbar.status2;">

Should add flex and crop attributes here too. No guarantee we'll always have a super short label for the status, in all locales.

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +33,5 @@
>  %define table_itemDarkStartBorder rgba(0,0,0,0.2)
>  %define table_itemDarkEndBorder rgba(128,128,128,0.15)
>  %define table_itemLightStartBorder rgba(128,128,128,0.25)
>  %define table_itemLightEndBorder transparent
> +%define smallSeparatorDark linear-gradient(transparent 15%, #5a6169 15%, #5a6169 85%, transparent 85%)

Can we use theme colors for these hex values? It's ok to have hard-coded transparent blacks, whites or grays, but actual colors should come from themes.

@@ +78,4 @@
>  }
>  
> +.theme-dark .requests-menu-header-button:hover {
> +  background-color: hsla(206,37%,4%,.2);

Hardcoded colors: not anymore!

@@ +82,4 @@
>  }
>  
> +.theme-light .requests-menu-header-button:hover {
> +  background-color: #ddd;

Ditto. And everywhere else.

@@ +164,5 @@
>  .theme-light .requests-menu-icon {
>    outline: 1px solid @table_itemLightStartBorder@;
>  }
>  
> +label.requests-menu-file {

Ugh. Like I said above, need to rename some of these classes.

Comment 103

[Albert Juhé]

Attachment: 951714patch18.patch

After a long delay, a new version of the patch.


What's new:

- I rebased it
- Fixed the overqualified selectors
- Removed the preprocessing


What's missing:

> Instead of accessing the parent node everywhere, it's probably safer to querySelectorAll for the requests-menu-*-header-box here, and query for requests-menu-*-button when needed, now that there's always one button inside one box for all headers.

I'm not sure what you mean. We are looping through all headers and after finding it we get the parent node of that header, is there a better way of doing it?

> Hardcoded colors: not anymore!

I replaced some of them with variables, but for the :hover state I couldn't find any variable that fit on it. I checked other buttons in the devtools and in all of them the :hover effect is done with hardcoded colors. Should we create a new variable for all of them? Should it be in a following bug?

Comment 104

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #103)
> > Hardcoded colors: not anymore!
> 
> I replaced some of them with variables, but for the :hover state I couldn't
> find any variable that fit on it. I checked other buttons in the devtools
> and in all of them the :hover effect is done with hardcoded colors. Should
> we create a new variable for all of them? Should it be in a following bug?

I've been generally creating a variable local to the particular file on .theme-light and .theme-dark for colors that don't exist in the themes.  For example: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/animationinspector.css#6-13

Comment 105

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #104)
> (In reply to Albert Juhé from comment #103)
> > > Hardcoded colors: not anymore!
> > 
> > I replaced some of them with variables, but for the :hover state I couldn't
> > find any variable that fit on it. I checked other buttons in the devtools
> > and in all of them the :hover effect is done with hardcoded colors. Should
> > we create a new variable for all of them? Should it be in a following bug?
> 
> I've been generally creating a variable local to the particular file on
> .theme-light and .theme-dark for colors that don't exist in the themes.  For
> example:
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> devtools/animationinspector.css#6-13

Up to you and Victor if you tackle that here or in a follow up, though.  I don't see any problem moving it into a follow up personally if it's a non-trivial amount of work.

Comment 106

[Albert Juhé]

Attachment: 951714patch19.patch

Updated patch, it fixes an specificity issue from the previous one.

> Up to you and Victor if you tackle that here or in a follow up, though.  I
> don't see any problem moving it into a follow up personally if it's a
> non-trivial amount of work.

My point is that the :hover colour should be a common one for the Netmonitor table header, the Inspector sidebar tabs, the Console/Style Editor/... buttons... right? So maybe that's out of the scope of this bug to unify all of them?

Comment 107

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #106)
> Updated patch, it fixes an specificity issue from the previous one.
> 
> > Up to you and Victor if you tackle that here or in a follow up, though.  I
> > don't see any problem moving it into a follow up personally if it's a
> > non-trivial amount of work.
> 
> My point is that the :hover colour should be a common one for the Netmonitor
> table header, the Inspector sidebar tabs, the Console/Style Editor/...
> buttons... right? So maybe that's out of the scope of this bug to unify all
> of them?

Yeah, unifying those colors is outside the scope of this bug

Comment 108

[Tim Nguyen :ntim]

Comment on attachment 8634250 [details] [diff] [review]
951714patch19.patch

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

::: browser/themes/linux/devtools/netmonitor.css
@@ -22,5 @@
> -
> -.requests-menu-size {
> -  width: 6em;
> -}
> -

Are you sure other platforms don't have specific code that needs to be removed as well ?

::: browser/themes/shared/devtools/images/sort-arrows.svg
@@ +1,1 @@
> +<svg xmlns="http://www.w3.org/2000/svg" width="44px" height="22px">

Can you add a license header ?
Also, can you change the indentation to 2 spaces?

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +218,2 @@
>    border: 2px solid var(--theme-content-color2);
>    background-color: transparent;  

While you're changing this, can you remove this trailing whitespace ?

Comment 109

[Albert Juhé]

Attachment: 951714patch20.patch

> Are you sure other platforms don't have specific code
> that needs to be removed as well ?

As far as I could see, no. Correct me if I'm wrong, though.

> Can you add a license header ?

I will need help here. Which license should I add and how should the license header look like?

> Also, can you change the indentation to 2 spaces?

> While you're changing this, can you remove this trailing whitespace ?

Done both of them.

Comment 110

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #109)
> Created attachment 8638047 [details] [diff] [review]
> 951714patch20.patch
> 
> > Are you sure other platforms don't have specific code
> > that needs to be removed as well ?
> 
> As far as I could see, no. Correct me if I'm wrong, though.
Alright, just seemed a bit suspicious that only Linux code was removed.

> > Can you add a license header ?
> 
> I will need help here. Which license should I add and how should the license
> header look like?
This should be put before the <svg> tag :
<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - 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/. -->

Comment 111

[Albert Juhé]

Attachment: 951714patch21.patch

New patch with the license in the SVG file.

Comment 112

[Tim Nguyen :ntim]

Comment on attachment 8638640 [details] [diff] [review]
951714patch21.patch

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

Looks good to me. You can just mark this for checkin as :vporof already gave you a r+.

Comment 113

[Carsten Book [:Tomcat]]

this failed to apply:

patching file browser/themes/shared/devtools/netmonitor.inc.css
Hunk #1 FAILED at 38
Hunk #2 FAILED at 162
2 out of 3 hunks FAILED -- saving rejects to file browser/themes/shared/devtools/netmonitor.inc.css.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh bug951714.patch

can you take a look, thanks!

Comment 114

[Tim Nguyen :ntim]

I wanted to rebase this, but I was a bit surprised when I saw how much this broke the netmonitor on OSX :
- The timeline column timestamps weren't aligned with the table markers
- The sort arrows weren't showing up
- The table row borders were missing
- The border images weren't showing up either
- The transferred column header text was invisible

Comment 115

[Albert Juhé]

Attachment: 951714patch22.patch

I uploaded a rebased version of the patch and fixed a 1px offset in vertical mode that I just found (see #requests-menu-toolbar height change).

> I wanted to rebase this, but I was a bit surprised when I saw how much this broke the netmonitor on OSX

Not sure why this happens, on Linux it looks fine and they are both using the same CSS file.

I don't have any Mac OSX to test it, though. How can I help?

Comment 116

[Albert Juhé]

Attachment: 951714patch22-screenshot.png

Comment 117

[Tim Nguyen :ntim]

Comment on attachment 8646498 [details] [diff] [review]
951714patch22.patch

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

This patch works much better for me, so I must have done something wrong in the rebasing process.
While trying to rebase this, I had found a few things that can be changed.
Also, you should test this on Windows.

Here's how it looks on OSX : http://cl.ly/image/1u0s1Z1t1s3S
There are still some issues on OSX :
- The table headers aren't cropped properly, the text disappears if the column header is too small (that's the case for the Transferred column)
- The Method column takes too much space, I feel like it's a bit wasted for a 4-letter word (POST), if it can simply fit the word "Method", that would be awesome.

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +53,5 @@
> +  background-color: transparent;
> +  border-image: linear-gradient(transparent 15%, var(--theme-splitter-color) 15%, var(--theme-splitter-color) 85%, transparent 85%) 1 1;
> +  border-style: solid;
> +  border-width: 0;
> +  -moz-border-start-width: 1px;

nit : border-inline-start-width is the new standard way

@@ +70,4 @@
>  }
>  
> +.theme-dark .requests-menu-header-button:hover {
> +  background-color: hsla(206,37%,4%,.2);

rgba(0,0,0,0.1) (with a higher opacity if needed) will work out fine for both themes (tested it)

Comment 118

[Albert Juhé]

Attachment: 951714patch23.patch

Excuse me for the long delays before replying, I don't have as much free time as I would like to.

> you should test this on Windows.

I have Windows on a laptop but I have never built Firefox on it. When I have time, I will try it but maybe it takes a while.

> - The table headers aren't cropped properly, the text disappears if the
> column header is too small (that's the case for the Transferred column)

That's weird and I can't reproduce it on Linux.

The text is cropped in the <button> with the attribute crop="end" (/browser/devtools/netmonitor/netmonitor.xul).

I don't own any Mac OS, any advice about how can I solve this?

> - The Method column takes too much space, I feel like it's a bit wasted
> for a 4-letter word (POST), if it can simply fit the word "Method",
> that would be awesome.

I set a max-width for all columns except file, domain and the timeline. I think it looks much better now. I was even thinking in setting a max-width for file and domain columns, so in a very wide screen the timeline would be wider. What do you think?

> nit : border-inline-start-width is the new standard way

Changed everywhere in the patch.

> rgba(0,0,0,0.1) (with a higher opacity if needed) will work out fine
> for both themes (tested it)

Done, thank you!

The patch is not rebased (not sure if needed), but I post it anyway to get your feedback about the questions above.

Comment 119

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #118)
> > you should test this on Windows.
> 
> I have Windows on a laptop but I have never built Firefox on it. When I have
> time, I will try it but maybe it takes a while.
I can push to try if you can rebase the patch. Pushing to try allows you to get binaries for all platforms using a simple SSH push (and run tests, but in you won't need that here).

> > - The table headers aren't cropped properly, the text disappears if the
> > column header is too small (that's the case for the Transferred column)
> 
> That's weird and I can't reproduce it on Linux.
> 
> The text is cropped in the <button> with the attribute crop="end"
> (/browser/devtools/netmonitor/netmonitor.xul).
> 
My guess is that ::after doesn't play well with XBL bindings, maybe removing the use of ::after could fix the issue. For that, what you could try doing is to set the list-style-image of .button-icon for the arrow image, then move .button-icon after the label using the -moz-box-ordinal-group CSS property. There may be a simpler fix, so you should ask on #content on IRC.
 
> > - The Method column takes too much space, I feel like it's a bit wasted
> > for a 4-letter word (POST), if it can simply fit the word "Method",
> > that would be awesome.
> 
> I set a max-width for all columns except file, domain and the timeline. I
> think it looks much better now. I was even thinking in setting a max-width
> for file and domain columns, so in a very wide screen the timeline would be
> wider. What do you think?
The ideal solution would be having those columns fit the widest content in them. But if that's not possible, I could go with your solution as long as it works for all locales (some locales may have longer headers).

> The patch is not rebased (not sure if needed), but I post it anyway to get
> your feedback about the questions above.
You'll need rebasing, since :bgrins recently eliminated the platform specific files.

Comment 120

[Albert Juhé]

Attachment: 951714patch24.patch

Another version of the patch. Excuse me for the delay again, I can only work on it when I have free time, and I have much less than what I would like to.

This patch is a rebased version of the previous one replacing the way the arrow is added, instead of a CSS pseudo element, now it's a normal <image>.

I ask for vporof for review, but maybe ntim.bugs should take a look too?

Comment 121

[Tim Nguyen :ntim]

Comment on attachment 8684603 [details] [diff] [review]
951714patch24.patch

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

::: devtools/client/themes/images/sort-arrows.svg
@@ +5,5 @@
> +  <g fill="#edf0f1" fill-opacity="0.8">
> +    <polygon points="5,9 11,15 12,15 18,9"/>
> +    <polygon points="27,14 33,8 34,8 40,14"/>
> +  </g>
> +</svg>

I've remade this image to be more pointy (like the original png) on non-retina screens (please tell me if you don't see a difference, or if it looks worse):

<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - 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/. -->
<svg xmlns="http://www.w3.org/2000/svg" width="22" height="11">
  <g fill="#edf0f1" fill-opacity="0.8">
    <polygon points="3,5 5.5,8 8,5"/>
    <polygon points="14,7 16.5,4 19,7"/>
  </g>
</svg>

::: devtools/client/themes/netmonitor.css
@@ +58,4 @@
>    min-width: 1px;
>    min-height: 24px;
>    margin: 0;
> +  padding: 2px 0 2px 13px;

Is this rtl aware ?

@@ +74,4 @@
>  }
>  
> +.requests-menu-header-button .button-box {
> +  display: flex;

Can we avoid mixing up CSS flexbox and XUL ? There's no defined behaviour when mashing up those 2 things. Since we're using XUL here, please stick with XUL flexbox.

If you're using flexbox to change the order of the button image, you can use -moz-box-ordinal-group, which works like the order property.
You can also use -moz-box-flex instead of flex.

@@ +78,4 @@
>  }
>  
> +#requests-menu-waterfall-button::before {
> +  content: "";

Seems like leftover from the older patch.

@@ +93,5 @@
>  
> +.requests-menu-header-button[sorted] > .button-box > .button-icon,
> +.requests-menu-header-button[sorted] #requests-menu-waterfall-image {
> +  background-image: url('chrome://devtools/skin/images/sort-arrows.svg');
> +  background-size: 22px 11px;

You can use list-style-image which is more appropriate for XUL images.

Then you can use width and height instead of background-size, and -moz-image-region (the syntax is on MDN) instead of background-position.

@@ +108,4 @@
>  }
>  
> +.requests-menu-header-button > .button-box > .button-text {
> +  display: block;

display: block; shouldn't be needed (with XUL layouts), and if it's needed, it should be -moz-box for XUL

@@ +113,5 @@
> +}
> +
> +#requests-menu-waterfall-label-wrapper {
> +  display: flex;
> +  flex: 1;

Same comment about flexbox

@@ +269,5 @@
> +  padding-left: 0;
> +}
> +
> +#requests-menu-waterfall-label:not(.requests-menu-waterfall-visible) {
> +  padding-left: 13px;

Have you tested if this is rtl aware ?

@@ +274,5 @@
>  }
>  
>  .requests-menu-timings-division {
>    width: 100px;
> +  padding-block-start: 2px;

I'm not quite sure we've started using this in our CSS code.

@@ +305,4 @@
>  }
>  
> +.theme-light .requests-menu-timings-division {
> +  border-inline-start-color: #585959 !important;

Seems like you can use a common rgba() color for both themes, something like rgba(128,128,128,0.15)

Comment 122

[Tim Nguyen :ntim]

Attachment: Issues

The timeline markers don't appear to look right, and the header text isn't vertically aligned. I'm sure the second issue can be fixed by using XUL flexbox instead of HTML flexbox as mentioned in the review.

Comment 123

[Victor Porof [:vporof][:vp]]

Comment on attachment 8684603 [details] [diff] [review]
951714patch24.patch

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

Agree with everything Tim said.

Comment 124

[Albert Juhé]

Attachment: 951714patch25.patch

Hey Tim, thanks for all the inputs. I fixed everything you said and a couple of other issues I saw. Just one comment:

> rgba(128,128,128,0.15)

It looks almost transparent to me, should we increase the opacity?

Comment 125

[Albert Juhé]

Attachment: Screenshot time separators.png

Screenshot showing almost invisible time separators.

Comment 126

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #124)
> Created attachment 8686764 [details] [diff] [review]
> 951714patch25.patch
> 
> Hey Tim, thanks for all the inputs. I fixed everything you said and a couple
> of other issues I saw. Just one comment:
> 
> > rgba(128,128,128,0.15)
> 
> It looks almost transparent to me, should we increase the opacity?

Yeah, that sounds good. You can undo this change if you want, it's not very important anyway since we're unifying all splitter colors in bug 1212958 anyway.

Comment 127

[Albert Juhé]

Attachment: 951714patch26.patch

> Yeah, that sounds good. You can undo this change if you want, it's not very
> important anyway since we're unifying all splitter colors in bug 1212958 anyway.

So I left it as it was before. ;)

Comment 128

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #127)
> Created attachment 8687308 [details] [diff] [review]
> 951714patch26.patch
> 
> > Yeah, that sounds good. You can undo this change if you want, it's not very
> > important anyway since we're unifying all splitter colors in bug 1212958 anyway.
> 
> So I left it as it was before. ;)

I haven't tested this, but this doesn't apply cleanly on top of latest fx-team. You'll need to rebase your patch (you don't need to re-request review, just flag your next patch with r+)

Comment 129

[Albert Juhé]

Attachment: 951714patch27.patch

I think this new patch should work fine now, however this is my first bug using git instead of hg (https://developer.mozilla.org/en-US/docs/Tools/Contributing) so maybe I did something wrong because I don't see many differences between the old and the new patch files.

Comment 130

[Tim Nguyen :ntim]

Comment on attachment 8690535 [details] [diff] [review]
951714patch27.patch

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

Looks great, thanks for your awesome work ! Found 2 minor issues.

::: devtools/client/themes/netmonitor.css
@@ +60,5 @@
>    margin: 0;
> +  padding-bottom: 2px;
> +  padding-inline-start: 13px;
> +  padding-top: 2px;
> +  overflow: hidden;

nit: overflow: hidden isn't needed.

@@ +189,5 @@
>  }
>  
>  /* Network requests table: status codes */
>  
> +.requests-menu-status-icon {

These changes seems to remove the color code from the netmonitor sidebar. (screenshot coming).

Comment 131

[Tim Nguyen :ntim]

Attachment: Issues

I'm fine with leaving out the "Method" overflow issue for another bug.

Comment 132

[Tim Nguyen :ntim]

Also, it seems like the arrows are being used backwards. The small part of the arrow is supposed to represent the smallest item (or the smallest index in the alphabet). In your patch, when the status codes are sorted with the `v` arrow (with the smallest side pointing down), the biggest status code appears last, and smallest one first, whereas it should be the other way.

Basically, you just need to swap the [sorted=ascending] and the [sorted=descending] rules.

Comment 133

[Albert Juhé]

Attachment: 951714patch28.patch

Let's see now. This patch fixes all the issues reported except this one:

> I'm fine with leaving out the "Method" overflow issue for another bug.

I couldn't reproduce it. Can it have something to do with the OS font-size? Anyway, I agree it's better to work on it in a following bug.

Comment 134

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #133)
> Created attachment 8690572 [details] [diff] [review]
> 951714patch28.patch
> 
> Let's see now. This patch fixes all the issues reported except this one:
Thanks. Marked this patch for check-in.

> I couldn't reproduce it. Can it have something to do with the OS font-size?
> Anyway, I agree it's better to work on it in a following bug.
Will file a bug.

Comment 135

[Carsten Book [:Tomcat]]

This failed to apply:

Fetching... done
Parsing... done
adding 951714 to series file
renamed 951714 -> Bug12345.patch
applying Bug12345.patch
patching file devtools/client/themes/netmonitor.css
Hunk #2 FAILED at 169
1 out of 4 hunks FAILED -- saving rejects to file devtools/client/themes/netmonitor.css.rej

could you take a look, thanks!

Comment 136

[Albert Juhé]

Attachment: 951714patch28b.patch

I created the patch again after a git pull, if it doesn't work either, maybe I'm doing something wrong.

Comment 137

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/57dbf05c9f5b

Comment 138

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/fx-team/rev/5d2e35fd9509e57c242e67ddb79035898bc17076
Backed out changeset 57dbf05c9f5b (bug 951714) for devtools bustage on a CLOSED TREE

Comment 139

[Nigel Babu [:nigelb]]

Backed out for bustages like this -> https://treeherder.mozilla.org/logviewer.html#?job_id=5903363&repo=fx-team

Comment 140

[Albert Juhé]

Attachment: 951714patch29.patch

Same patch as before but fixing the test.

Comment 141

[Albert Juhé]

Attachment: 951714patch2b.patch

I just noticed with the previous patch that the tooltip on the Method icon stopped working. This patch fixes it. I set r=+ since it was a small change (a couple of lines), let me know if that's not ok.

Comment 143

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #141)
> Created attachment 8696245 [details] [diff] [review]
> 951714patch2b.patch
> 
> I just noticed with the previous patch that the tooltip on the Method icon
> stopped working. This patch fixes it. I set r=+ since it was a small change
> (a couple of lines), let me know if that's not ok.

That's fine.

Comment 144

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/396725444e10

Comment 145

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/396725444e10

Comment 146

[Brian Grinstead [:bgrins]]

Nice job Albert, looks great!