Firebug theme - Fix selection color in the net monitor

RESOLVED FIXED in Firefox 51

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
Last year

People

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

Tracking

({good-first-bug})

Trunk
Firefox 52

Firefox Tracking Flags

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

Details

Attachments

(5 attachments)

Reporter

Description

3 years ago
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
Reporter

Updated

3 years ago
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

Comment 2

3 years ago
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.

Comment 4

3 years ago
(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.

Comment 6

3 years ago
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

Comment 8

3 years ago
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)
Assignee

Comment 11

3 years ago
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 hidden (mozreview-request)
Assignee

Comment 13

3 years ago
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?
Assignee

Comment 14

3 years ago
Posted image light-with-patch.png
Assignee

Comment 15

3 years ago
Posted 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 17

3 years ago
mozreview-review
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 18

3 years ago
mozreview-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.
Assignee

Comment 19

3 years ago
(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)
Assignee

Comment 21

3 years ago
(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)
Comment hidden (mozreview-request)

Comment 24

3 years ago
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/19ad33588d41
Firebug theme - Fix selection color in the net monitor r=jsnajdr

Comment 25

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/19ad33588d41
Status: NEW → RESOLVED
Closed: 3 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
Reporter

Comment 27

3 years ago
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]

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.