Closed Bug 1288577 Opened 4 years ago Closed 4 years ago

Firebug theme - Fix selection color in the net monitor

Categories

(DevTools :: Netmonitor, defect, P2)

defect

Tracking

(firefox47 unaffected, firefox48 affected, firefox49 affected, firefox50 affected, firefox51 fixed, firefox52 verified)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox47 --- unaffected
firefox48 --- affected
firefox49 --- affected
firefox50 --- affected
firefox51 --- fixed
firefox52 --- verified

People

(Reporter: magicp.jp, Assigned: xfq, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(5 files)

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
Has STR: --- → yes
Component: Untriaged → Developer Tools: Netmonitor
OS: Unspecified → All
Hardware: Unspecified → All
Thanks for the report

Honza
Priority: -- → P2
Mentor: jsnajdr
Keywords: good-first-bug
Haven't done this before, how would I get started on this bug?
(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.
(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?
(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.
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
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
Hello I am a first timer,
can you guide me about how to go about thsi fix ? I have aleady built the nightly.
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"!?
Flags: needinfo?(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!
Flags: needinfo?(jsnajdr)
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.)
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?
Attached image light-with-patch.png
Attached image dark-with-patch.png
(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 on attachment 8797402 [details]
Bug 1288577 - Firebug theme - Fix selection color in the net monitor

https://reviewboard.mozilla.org/r/83004/#review82456
Attachment #8797402 - Flags: review?(jsnajdr) → review+
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.
(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!
Flags: needinfo?(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.
Assignee: nobody → xfq.free
Flags: needinfo?(jsnajdr)
(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?
Flags: needinfo?(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.
Flags: needinfo?(jsnajdr)
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/19ad33588d41
Firebug theme - Fix selection color in the net monitor r=jsnajdr
https://hg.mozilla.org/mozilla-central/rev/19ad33588d41
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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
verified in latest 52.
How about uplift to 51 ?
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
Attachment #8797402 - Flags: approval-mozilla-aurora?
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.
Attachment #8797402 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.