1487602 - Tracking icons is not visible when selected

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

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

Attachment: tracking-icon.jpg

Network panel is marking tracking resource using an icon (this feature has been introduced in bug 1333994).

This icon is almost invisible when the row is selected. See the attached screenshot.

The contrast between the icon and blue background should be high enough, so the icon is visible (the icon should be brighter).

Honza

Comment 2

[kenokamo]

Can I take this one?

Comment 3

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

Assigned to you.

Thanks for helping!
Honza

Comment 4

[kenokamo]

Attachment: screenshot

I found adding a CSS selector with a light background improves the contrast a lot.

.selected .tracking-resource {
  background-color: #ececec;
  border-radius: 2px;
}

Let me know if you want to do it this way or have any ideas for improvements.

Comment 5

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

@kenokamo: thanks for working on this!

Please need info me next time so, your request isn't lost in bug mail.
Also, having regular patch attached would be better (it's not clear where you made that change).

Could we have the background transparent and the icon white if it's selected (just like the text color)?

Honza

Comment 6

[kenokamo]

Attachment: Screenshot - svg with #ffffff fill

How does this look? 

I created a new file "shield-white.svg" which is the same as "shield.svg" except the fill color is #ffffff. In the CSS I replaced the previous changes I made with

.selected .tracking-resource {
  background-image: url(chrome://devtools/content/netmonitor/src/assets/icons/shield-white.svg);
}

The only problem is that it gets an error "Could not load the image" for the shield-white.svg.  Is there something I need to do to include the new file in the build process?

Also this is my first contribution using bugzilla.  Can you tell me how I make a patch file?  Thanks!

Comment 7

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

The screenshot looks great!

Can we avoid adding a new icon?

E.g. using `filter: brightness(500%);` ?

Honza

Comment 8

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

@kenokamo: any progress with the patch?

Honza

Comment 9

[kenokamo]

Attachment: Using same svg icon with 500% brightness filter

Here is what it looks like using the 500% brightness filter with the same icon.  I like it this way using the same file a lot better.

Comment 10

[kenokamo]

Attachment: patch

I couldn't find how to make the patch file in the contributing docs.  This is a patch I made by running 'hg diff -c tip > patch'.  Let me know if you need me to do it a different way.

Comment 11

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

Comment on attachment 9011584 [details] [diff] [review]
patch

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

Thanks for the patch!

Honza

Comment 12

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

Comment on attachment 9011584 [details] [diff] [review]
patch

Ah, the commit message needs to be set in the patch.
See this page:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch


Also don't forget to set the 'review' flag to `?` and my email address to make proper review request (and I can see it in my review queue)

Thanks,
Honza

Comment 13

[kenokamo]

I looked through the developer guide and couldn't find the hg command to make a patch file with a commit message.  Can you tell me how I do it?  Thanks!

Comment 14

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

I am using HG queues:

# create new patch in the queue from changes in the working directory
hg qnew my-patch --message "Bug 1487602 - Tracking icon is not visible when selected; r=Honza"

# create patch
hg qtip export > my-patch.patch

# now you can pop the patch from the queue
hg qpop

# and remove when not needed anymore (be careful this deletes the changes)
hg qremove my-patch

Does that work for you?

Honza

Comment 15

[kenokamo]

Attachment: 1487602v1.patch

Comment 16

[kenokamo]

Thanks.  I was able to use those steps to create a patch that includes the commit message.  Let me know if it is OK now.

Comment 17

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

Comment on attachment 9012640 [details] [diff] [review]
1487602v1.patch

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

Looks great to me, thanks!
Honza

Comment 18

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

The next step is to set `checkin-needed` keyword to make sure that somebody lands the patch in m-c (Mozilla Central) repository. I am doing it for now, so you can see it in action.

Thanks for the help!
Honza

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71c746601621
Tracking icon is not visible when selected; r=Honza

Comment 20

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/71c746601621

Comment 21

[kenokamo]

Thanks for all your guidance on this Honza!