1288577 - Firebug theme - Fix selection color in the net monitor

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[magicp]

Attachment: fix-selection-color-in-netmonitor.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160721030216

Steps to reproduce:

1. Start Nightly
2. Open DevTools > Network with Firebug theme
3. Go to any sites (e.g. https://bugzilla.mozilla.org)
4. Select any rows (side-menu-widget-item)


Actual results:

a. Method and Transferred cells are not applied firebug theme selection color
b. Status and Domain icons are dark for the selection background color
c. Variables headers' font color is too light if selected

Please find attached image.


Expected results:

a. apply firebug theme selection color
b. apply brightness filter to icons
c. remove selected color

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report

Honza

Comment 2

[Noah]

Haven't done this before, how would I get started on this bug?

Comment 3

[Jarda Snajdr [:jsnajdr]]

(In reply to Noah from comment #2)
> Haven't done this before, how would I get started on this bug?

Thanks for your interest!

The first thing you need to do is to checkout the Firefox sources and build them. Please follow the hacking guides here:

https://wiki.mozilla.org/DevTools/Hacking
https://developer.mozilla.org/en-US/docs/Tools/Contributing

Also, using artifact builds could speed things up for you, if you use Mercurial:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

Then, you can get to the actual bug. The attached image illustrates very nicely what needs to be fixed. You'll be changing these CSS files for Netmonitor:

devtools/client/themes/netmonitor.css
devtools/client/netmonitor/netmonitor.css

and maybe others, too.

Comment 4

[Noah]

(In reply to Jarda Snajdr [:jsnajdr] from comment #3)
> (In reply to Noah from comment #2)
> > Haven't done this before, how would I get started on this bug?
> 
> Thanks for your interest!
> 
> The first thing you need to do is to checkout the Firefox sources and build
> them. Please follow the hacking guides here:
> 
> https://wiki.mozilla.org/DevTools/Hacking
> https://developer.mozilla.org/en-US/docs/Tools/Contributing
> 
> Also, using artifact builds could speed things up for you, if you use
> Mercurial:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Artifact_builds
> 
> Then, you can get to the actual bug. The attached image illustrates very
> nicely what needs to be fixed. You'll be changing these CSS files for
> Netmonitor:
> 
> devtools/client/themes/netmonitor.css
> devtools/client/netmonitor/netmonitor.css
> 
> and maybe others, too.

Cool, I already built the nightly build; I also need to install firebug right?

Comment 5

[Jarda Snajdr [:jsnajdr]]

(In reply to Noah from comment #4)
> Cool, I already built the nightly build; I also need to install firebug
> right?

You don't need to install Firebug. Only enable the Firebug theme in the native devtools.

Comment 6

[Itamar Givon]

Hello, first timer here as well.

I've already built and made the changes I could (I have a question about the brightness filter, but that is for later). I am however new to mercurial, And I was wondering how could I start a CR on this after I've made my commits. Any advice please?

Thank you :D

Comment 7

[Jarda Snajdr [:jsnajdr]]

Hello Itamar,

you need to export your patch into a file and add it as attachment to this bug. Here is a guide on how to submit a patch with Mercurial:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 8

[vemulapatiharsha]

Hello I am a first timer,
can you guide me about how to go about thsi fix ? I have aleady built the nightly.

Comment 9

[Albert Scheiner [:alberts]]

Hey Jarda,
just checking in about the assignment status of this task. It seems from the comments that 2-3 people are already working on this, but it is still set to "ready to take it"!?

Comment 10

[Jarda Snajdr [:jsnajdr]]

(In reply to Albert Scheiner [:alberts] from comment #9)
> just checking in about the assignment status of this task. It seems from the
> comments that 2-3 people are already working on this, but it is still set to
> "ready to take it"!?

Hello Albert!
Yes, multiple people have shown interest in this bug, but there are no tangible results yet - like attaching a work-in-progress patch, asking questions specific to the bug... Many attempts at fixing a good-first-bug never get off the ground, so we usually mark them as assigned only after someone shows some serious progress.

If you are interested in this bug, don't be discouraged and give it a shot!

Comment 11

[Fuqiao Xue [:xfq]]

Attachment: try-to-fix-selection-color-in-netmonitor.png

Hi Jarda,

I'm interested in this bug. I've modified devtools/client/themes/netmonitor.css and rebuilt Firefox. See the results in the attached image.

Will commit the change and push to autoreview later.

(BTW, the value of 'filter: brightness()' was chosen randomly.)

Comment 12

[Fuqiao Xue [:xfq]]

Attachment: Bug 1288577 - Firebug theme - Fix selection color in the net monitor

Review commit: https://reviewboard.mozilla.org/r/83004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83004/

Comment 13

[Fuqiao Xue [:xfq]]

I just found that the headers part in the light/dark theme looks worse than before. I'll attach two screenshots later.

Should I move that part of CSS to firebug-theme.css and add '.theme-firebug' to it? Or perhaps I should move all my changes to firebug-theme.css?

Comment 14

[Fuqiao Xue [:xfq]]

Attachment: light-with-patch.png

Comment 15

[Fuqiao Xue [:xfq]]

Attachment: dark-with-patch.png

Comment 16

[Jarda Snajdr [:jsnajdr]]

(In reply to Xue Fuqiao [:xfq] from comment #13)
> I just found that the headers part in the light/dark theme looks worse than
> before. I'll attach two screenshots later.

Do you plan to further experiment with the filter:brightness value and find a better value? The current one (1.3) already looks pretty good.

> Should I move that part of CSS to firebug-theme.css and add '.theme-firebug'
> to it? Or perhaps I should move all my changes to firebug-theme.css?

You mean the rule for .variables-view-scope:focus? Is it used anywhere else outside the netmonitor? Rules should be in firebug-theme.css only if they are firebug-theme specific and are used across several components.

The patch looks very good, thank you!

Comment 17

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8797402 [details]
Bug 1288577 - Firebug theme - Fix selection color in the net monitor

https://reviewboard.mozilla.org/r/83004/#review82456

Comment 18

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8797402 [details]
Bug 1288577 - Firebug theme - Fix selection color in the net monitor

https://reviewboard.mozilla.org/r/83004/#review82458

::: devtools/client/themes/netmonitor.css:507
(Diff revision 1)
>  .theme-firebug .requests-menu-subitem.requests-menu-status {
>    font-weight: bold;
>  }
>  
>  /* Method Column */
> +

nit: this line shows a whitespace change that is not needed.

Comment 19

[Fuqiao Xue [:xfq]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #16)
> (In reply to Xue Fuqiao [:xfq] from comment #13)
> > I just found that the headers part in the light/dark theme looks worse than
> > before. I'll attach two screenshots later.
> 
> Do you plan to further experiment with the filter:brightness value and find
> a better value? The current one (1.3) already looks pretty good.

I think the current situation is OK. If it's OK for you, it's OK for me.

> > Should I move that part of CSS to firebug-theme.css and add '.theme-firebug'
> > to it? Or perhaps I should move all my changes to firebug-theme.css?
> 
> You mean the rule for .variables-view-scope:focus? Is it used anywhere else
> outside the netmonitor? Rules should be in firebug-theme.css only if they
> are firebug-theme specific and are used across several components.

I didn't find any other use of '.variables-view-scope:focus' outside the netmonitor, so the rule should be in netmonitor.css. But I think a '.theme-firebug' should be added anyway.

What should I do now? IIUC, I should:

1. revert the whitespace change
2. add the '.theme-firebug' selector for '.variables-view-scope:focus'
3. 'hg commit'
4. 'hg histedit'
5. push to autoreview (again)

Am I right?

Thanks for the review!

Comment 20

[Jarda Snajdr [:jsnajdr]]

(In reply to Xue Fuqiao [:xfq] from comment #19)
> I didn't find any other use of '.variables-view-scope:focus' outside the
> netmonitor, so the rule should be in netmonitor.css. But I think a
> '.theme-firebug' should be added anyway.

Yes, the rule should remain in netmonitor.css then, and have a ".theme-firebug" prefix. We don't want to change the text color in the dark and light themes.

> 
> What should I do now? IIUC, I should:
> 
> 1. revert the whitespace change
> 2. add the '.theme-firebug' selector for '.variables-view-scope:focus'
> 3. 'hg commit'
> 4. 'hg histedit'
> 5. push to autoreview (again)
> 
> Am I right?

That's 100% right! Thank you.

Comment 21

[Fuqiao Xue [:xfq]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #18)
> ::: devtools/client/themes/netmonitor.css:507
> (Diff revision 1)
> >  .theme-firebug .requests-menu-subitem.requests-menu-status {
> >    font-weight: bold;
> >  }
> >  
> >  /* Method Column */
> > +
> 
> nit: this line shows a whitespace change that is not needed.

Regarding the whitespace change, on a second thought, IMHO it might make sense in that context.

In netmonitor.css, a comment line for a single rule goes just above the code it is commenting, but if the comment is for a few rules, there will be an empty line seperating the comment line and the rules. For example:

https://dxr.mozilla.org/mozilla-central/rev/da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/devtools/client/themes/netmonitor.css#494-529

Since the Method Column has two rules now (I added one), I think an empty line below that comment makes sense. WDYT?

Comment 22

[Jarda Snajdr [:jsnajdr]]

(In reply to Xue Fuqiao [:xfq] from comment #21)
> > nit: this line shows a whitespace change that is not needed.
> 
> Regarding the whitespace change, on a second thought, IMHO it might make
> sense in that context.

An extra line doesn't hurt anyone, of course. If you want to intentionally add one, It's perfectly OK with me.

Whitespace changes in patches are often unintended accidents, that's why I'm pointing it out.

Comment 23

[Fuqiao Xue [:xfq]]

Comment on attachment 8797402 [details]
Bug 1288577 - Firebug theme - Fix selection color in the net monitor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83004/diff/1-2/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19ad33588d41
Firebug theme - Fix selection color in the net monitor r=jsnajdr

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/19ad33588d41

Comment 26

[Md. Rahimul Islam]

I have successfully reproduce this bug on firefox nightly 50.0a1 (2016-07-21)
with windows 7 (32 bit)
Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0

I found this fix on latest nightly 52.0a1 (2016-10-17) (32-bit)

Mozilla/5.0 (Windows NT 6.1; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID : 20161017030209

Comment 27

[magicp]

verified in latest 52.
How about uplift to 51 ?

Comment 28

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8797402 [details]
Bug 1288577 - Firebug theme - Fix selection color in the net monitor

Approval Request Comment
[Feature/regressing bug #]: 1244054
[User impact if declined]: Broken Netmonitor styling when using Firebug theme
[Describe test coverage new/current, TreeHerder]: Only visual CSS styling, not covered by automated tests
[Risks and why]: low, small patch to Netmonitor CSS styles, easy to test manually
[String/UUID change made/needed]: none

Comment 29

[Gerry Chang [:gchang]]

Comment on attachment 8797402 [details]
Bug 1288577 - Firebug theme - Fix selection color in the net monitor

Fix a UI issue in net monitor. Take it in 51 aurora.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8aea18a0b3df

Comment 31

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 50.0a1 (2016-07-21) on Windows 7, 64 Bit !

This bug's fix is verified with latest Beta !

Build ID   : 20161115182233
User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20161116]