If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make infobar more visible on dark backgrounds

VERIFIED FIXED in Firefox 46

Status

()

Firefox
Developer Tools: Inspector
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: ntim, Assigned: Razvan Balosin, Mentored)

Tracking

unspecified
Firefox 46
Points:
---

Firefox Tracking Flags

(firefox46 verified)

Details

(Whiteboard: [polish-backlog][difficulty=easy][good first bug][lang=css][instructions in comment 5], URL)

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

2 years ago
See testcase. Adding a semi-transparent white border should fix the issue.
Thanks for filing.
Flagging this as an easy one. Would you like to fix it? Maybe you could mark yourself as mentor and help someone fix it?
Whiteboard: [polish-backlog][difficulty=easy]
(Reporter)

Updated

2 years ago
Mentor: ntim.bugs@gmail.com
Whiteboard: [polish-backlog][difficulty=easy] → [polish-backlog][difficulty=easy][good first bug][lang=css]

Comment 2

2 years ago
I am attending a open source course at university, and would like to be assigned to this bug.
(In reply to Matthew Copson from comment #2)
> I am attending a open source course at university, and would like to be
> assigned to this bug.
Tim is the mentor on this bug, which means he can point you in the right direction on how to get started here.
Please use the "need info" box at the bottom of the bug to make sure he sees your questions.
Also, I think it's best if you get started before being assigned to the bug, to make sure it's a good fit for you.

Comment 4

2 years ago
Hi Tim, 
could you send me the file path to this bug please.
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 5

2 years ago
(In reply to Matthew Copson from comment #4)
> Hi Tim, 
> could you send me the file path to this bug please.

Hey Matthew, 
Thank you for your interest in this bug.

In this bug, we want to add a `1px solid rgba(255,255,255,0.2)` border around the inspector info bar. For that, you'll need to add the border here [0] (this will add the border around the rectangular part of the info bar). You'll want to add a border around the arrow pointing towards the element too, for that, you can add `:-moz-native-anonymous .box-model-nodeinfobar-container > .box-model-nodeinfobar::after` to the list of selectors, you should then tweak the position/dimensions/border-color for `::before` in a separate rule, so the `::before` part becomes the arrow border, and the `::after` part the arrow itself.

[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters.css#86
[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters.css#108
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 6

2 years ago
Sidenote: someone else was also interested in this bug (I received an email approximately at the same time as Matthew's comment), I'll give assign it to whoever posts a patch first.
(Reporter)

Comment 7

2 years ago
Created attachment 8694979 [details]
Screenshot of issue

In case the issue isn't clear enough, here's a screenshot.
Good Day Everyone.
Can i please have it as the first bug to fix?

Comment 9

2 years ago
I want to fix this bug can someone guide me through?
(Reporter)

Updated

2 years ago
Whiteboard: [polish-backlog][difficulty=easy][good first bug][lang=css] → [polish-backlog][difficulty=easy][good first bug][lang=css][instructions in comment 5]

Comment 10

2 years ago
I just dont uderstand how do i find the path to go to this piece of code and fix it ?
Am also do not know and how i wish i knew i could help... Am a fresh newbie as well. Lets keep on waiting nd hope something good will come up

Comment 12

2 years ago
I'd like to be assigned to this bug please as I have created a patch for it which I will shortly upload after this message please, thank you.

Comment 13

2 years ago
Created attachment 8695630 [details] [diff] [review]
Rev1 - adding to & removing lines of code for selectors
Attachment #8695630 - Flags: review?(ntim.bugs)
(Reporter)

Updated

2 years ago
Assignee: nobody → olasupov
Status: NEW → ASSIGNED
(Reporter)

Comment 14

2 years ago
Comment on attachment 8695630 [details] [diff] [review]
Rev1 - adding to & removing lines of code for selectors

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

Have you tested your patch ? I'm pretty sure this doesn't give the expected results.
::before should be the infobar arrow border (it's currently the arrow itself). ::after should become the arrow itself, which means it should get all the rules from ::before, but with some position/background/size changes so ::after doesn't fully overlap ::before.

The selector with no ::before or no ::after corresponds to the infobar box. Which means only a border should be added, the selector shouldn't be changed.
Attachment #8695630 - Flags: review?(ntim.bugs) → feedback-

Comment 15

2 years ago
Hi tim, how do I test a patch and what are the prerequisites to carry this out please? Also i'm not too sure I understand your explanation of the problem.
(Reporter)

Updated

2 years ago
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 16

2 years ago
Created attachment 8697082 [details]
Screenshot of infobar on a light background

This is how the infobar looks like on a light background. As you can see, the infobar and its arrow stands out from the page background. In the testcase, the page background is the same as the infobar background, which causes the infobar to be invisible. In this bug, we want to add a border around the infobar and it's arrow so the infobar is visible everywhere.

To test your patch, you must build Firefox from the source (in case you haven't done it yet), here's how you do it: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions .

Then, when you run your copy of Firefox, open the testcase (data:text/html,<html style="background:rgb(52, 60, 69)">). Hit Ctrl+Shift+C (on Win and Linux) or Alt+Cmd+C (on OSX). Hover around the page, and you should see the infobar.

Victor, do you need more information ?
Flags: needinfo?(ntim.bugs) → needinfo?(olasupov)

Comment 17

2 years ago
Hi Tim, thanks for that, yes if possible could you provide me with more information about the border please ?
Flags: needinfo?(olasupov)
(Reporter)

Comment 18

2 years ago
(In reply to Victor Olasupo from comment #17)
> Hi Tim, thanks for that, yes if possible could you provide me with more
> information about the border please ?

Comment 5 has instructions on how to add it. Is there anything specific you don't understand ?
Flags: needinfo?(olasupov)
(In reply to Victor Olasupo from comment #17)
> Hi Tim, thanks for that, yes if possible could you provide me with more
> information about the border please ?
At this stage, I feel like Tim has provided enough information to give it a try, see what you come up with. Don't hesitate to start hacking on it and attach a screenshot or a patch of what you did and ask for feedback.
(Assignee)

Comment 20

2 years ago
Created attachment 8697237 [details] [diff] [review]
highlighters.css 1228829.patch

Hi guys, I fell this bug is perfect as a first bug for me. Since no one solved it by now, I have created a patch and looking for feedback.
Flags: needinfo?(ntim.bugs)
Attachment #8697237 - Flags: review+
Attachment #8697237 - Flags: review+ → review?(ntim.bugs)
(Reporter)

Comment 21

2 years ago
Created attachment 8697292 [details]
Screenshot of Razvan's patch

Razvan, looks great ! 

Victor, are you ok to give this bug to Razvan ? I can suggest other bugs if you like.
Flags: needinfo?(ntim.bugs)

Comment 22

2 years ago
yes sure he can have this bug, no worries, Tim could you please suggest a few bugs thanks
Flags: needinfo?(olasupov)
(Assignee)

Comment 23

2 years ago
Great! I've improved the CSS a bit more (https://codeshare.io/69g21) Should I create another patch and upload it here?

Also, I think increasing the border opacity from .2 to .3 will be better for darker screens. (.2 opacity on a #000 background is hardly noticeable.
(Reporter)

Comment 24

2 years ago
Comment on attachment 8697237 [details] [diff] [review]
highlighters.css 1228829.patch

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

Very nice ! Thanks for your patch ! I just have a few comments about rule duplication.

::: devtools/server/actors/highlighters.css
@@ +132,2 @@
>    border-left-color: transparent;
>    border-right-color: transparent;

Can you create a common ruleset for rules common to `:-moz-native-anonymous .box-model-nodeinfobar-container > .box-model-nodeinfobar:before` and `:-moz-native-anonymous .box-model-nodeinfobar-container > .box-model-nodeinfobar:after` ?

@@ +144,5 @@
>    bottom: 100%;
>    display: block;
>  }
>  
> +:-moz-native-anonymous .box-model-nodeinfobar-container[position="top"]:not([hide-arrow]) > .box-model-nodeinfobar:after {

This ruleset seems duplicated from the previous one, can you just add `:-moz-native-anonymous .box-model-nodeinfobar-container[position="top"]:not([hide-arrow]) > .box-model-nodeinfobar:after` to the previous rule (in the selector list) ?

@@ +150,5 @@
> +  top: 100%;
> +  display: block;
> +}
> +
> +:-moz-native-anonymous .box-model-nodeinfobar-container[position="bottom"]:not([hide-arrow]) > .box-model-nodeinfobar:after {

Same here.
Attachment #8697237 - Flags: review?(ntim.bugs) → feedback+
(Assignee)

Comment 25

2 years ago
Created attachment 8697298 [details] [diff] [review]
1228829v2.patch

Ok, can you take a look now?
Attachment #8697237 - Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
Attachment #8697298 - Flags: feedback+
Assignee: olasupov → contact
(Reporter)

Comment 26

2 years ago
Comment on attachment 8697298 [details] [diff] [review]
1228829v2.patch

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

Seems like this patch applies on top of the first patch. Can you fold them together ?

::: devtools/server/actors/highlighters.css
@@ +117,5 @@
> +  border: 7px solid hsl(214,13%,24%);
> +}
> +
> +:-moz-native-anonymous .box-model-nodeinfobar-container > .box-model-nodeinfobar:before,
> +:-moz-native-anonymous .box-model-nodeinfobar-container > .box-model-nodeinfobar:after {

Can you move this common rule before the 2 specific rules ?

@@ +122,5 @@
>    content: "";
>    display: none;
>  
>    position: absolute;
> +  

nit: trailing whitespace
(Reporter)

Updated

2 years ago
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 27

2 years ago
I'll fold them now, just a minute. Also, moving the common rule before the 2 specific rules will require to add `transparent !important` on the left and right borders so they won't get overwritten by the `border: 7px solid hsl(214,13%,24%);` rule that is following. What do you suggest?
(Assignee)

Comment 28

2 years ago
Created attachment 8697535 [details] [diff] [review]
1228829v3.patch
Attachment #8697298 - Attachment is obsolete: true
Attachment #8697535 - Flags: review?(ntim.bugs)

Comment 29

2 years ago
Created attachment 8697625 [details] [diff] [review]
infobar.patch

first bug, uploaded a patch for a start please check and review
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 30

2 years ago
(In reply to Babatunde Salaam from comment #29)
> Created attachment 8697625 [details] [diff] [review]
> infobar.patch
> 
> first bug, uploaded a patch for a start please check and review

Sorry, someone is already working on this bug.
Flags: needinfo?(ntim.bugs)
(Reporter)

Updated

2 years ago
Attachment #8697625 - Attachment is obsolete: true
(Reporter)

Comment 31

2 years ago
Comment on attachment 8697535 [details] [diff] [review]
1228829v3.patch

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

I don't think it's really important to put the common rules first, especially if it changes specificity.

Also, can you add a commit message to this patch ?
Bug 1228829 - Make infobar more visible on dark backgrounds. r=pbro, ntim
Attachment #8697535 - Flags: review?(pbrosset)
Attachment #8697535 - Flags: review?(ntim.bugs)
Attachment #8697535 - Flags: review+
(Assignee)

Comment 32

2 years ago
Created attachment 8697627 [details] [diff] [review]
1228829v3.patch + commit message

I've just added the commit message. What do I have to do next?
Flags: needinfo?(ntim.bugs)
(Reporter)

Updated

2 years ago
Attachment #8697627 - Flags: review?(pbrosset)
(Reporter)

Updated

2 years ago
Attachment #8697535 - Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
Attachment #8697535 - Flags: review?(pbrosset)
(Reporter)

Updated

2 years ago
Attachment #8695630 - Attachment is obsolete: true
(Reporter)

Comment 33

2 years ago
Comment on attachment 8697627 [details] [diff] [review]
1228829v3.patch + commit message

Let's wait for pbro's review :)
Attachment #8697627 - Flags: review+
Attachment #8697627 - Flags: review?(pbrosset) → review?(zer0)

Comment 34

2 years ago
Created attachment 8697898 [details] [diff] [review]
1228829.patch
Attachment #8697898 - Flags: review?(ntim.bugs)
Attachment #8697898 - Flags: feedback?(ntim.bugs)
(Reporter)

Comment 35

2 years ago
Comment on attachment 8697898 [details] [diff] [review]
1228829.patch

This bug is already assigned, and only awaits peer review. Email me if you'd like bug suggestions.
Attachment #8697898 - Attachment is obsolete: true
Attachment #8697898 - Flags: review?(ntim.bugs)
Attachment #8697898 - Flags: feedback?(ntim.bugs)
I transfered the review to Matteo as he will be a better reviewer than me on this (he's doing a lot of highlighter work at the moment), and I have been kind of swamped with reviews lately.
Matteo, could you take a look at the patch please?
Flags: needinfo?(zer0)
Comment on attachment 8697627 [details] [diff] [review]
1228829v3.patch + commit message

Looks good to me! We could probably avoid adding another pseudo element, but we need to refactor a bit the rest of the code, and it's not worthy at the moment.
Flags: needinfo?(zer0)
Attachment #8697627 - Flags: review?(zer0) → review+
(Reporter)

Updated

2 years ago
Keywords: checkin-needed

Comment 38

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/2e74a9ab0769
Keywords: checkin-needed

Comment 39

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2e74a9ab0769
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
I have reproduced this bug on Nightly 45.0a1 (2015-11-29) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Firefox Developer Edition 46.0a2!

Build ID: 20160216004009
User Agent: Mozilla/5.0 (X11; Linux i686; rv:46.0) Gecko/20100101 Firefox/46.0
QA Whiteboard: [bugday-20160217]
(Reporter)

Updated

2 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Updated

2 years ago
status-firefox46: fixed → verified
You need to log in before you can comment on or make changes to this bug.