Devtools notification box background color is too dark for the notification text to be readable.

RESOLVED FIXED in Firefox 56

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: djmdeveloper060796, Assigned: Towkir, Mentored)

Tracking

({good-first-bug})

51 Branch
Firefox 56
x86_64
Linux

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(6 attachments)

Posted image notif_bar.png
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160728203720

Steps to reproduce:

Platform used : Ubuntu 16.04LTS

1. Launch Firefox
2. Open Debugger
3. Press F8 to pause debugger
4. Press F5 to refresh page
5. Select inspector


Actual results:

The notification bar background color is too dark and the notification text cannot be distinguished from the background, hence its not readable.


Expected results:

The notification bar is properly displayed and the notification text is readable and distinguishable from the background.
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Component: Activity Streams: General → Developer Tools
Component: Developer Tools → Developer Tools: Shared Components
Thanks for the report!

Honza
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P2
Hi, 
Once you let me know which color you decide the text to be here, a color code or name would be nice.
I could start with this then.
Thanks
Flags: needinfo?(odvarko)
Thanks Towkir for showing interest in this bug

Presently styling is present only for data-type info and critical in devtools/client/shared/components/notification-box.css, but no styling is added for data-type warning.
So in case of warning, the background color and the text color are set to default ie, InfoBackground and black respectively. As a result of this text cannot be distinguished from background.
I think it would be nice to add a separate styling for data-type warning, keeping the background color as it is you can change the text color to 'white'. I think this will make the warning text visible and distinct from background 

Thanks!
Flags: needinfo?(3ugzilla)
Hi Helen, What do you think the colors should be ? I am not adding some random color here, I think it should be based on some plan/guideline.
Thanks
Flags: needinfo?(odvarko)
Flags: needinfo?(hholmes)
Flags: needinfo?(3ugzilla)
(In reply to [:Towkir] Ahmed from comment #4)
> Hi Helen, What do you think the colors should be ? I am not adding some
> random color here, I think it should be based on some plan/guideline.

Jumping in here to help.

Comment #3 makes good sense to me. We should add new styling for warnings and specify white text color.

@Deepjyoti: what if we specified even the background color for warnings? It would be safer to have both text and bk colors in our CSS.

Honza
Flags: needinfo?(hholmes) → needinfo?(djmdeveloper060796)
> @Deepjyoti: what if we specified even the background color for warnings? It would be safer to have both
> text and bk colors in our CSS.

Yup, it would be better if separate style is added for warning with badckground-color and text color properties just like info and critical.It would also make warning css style consistent with the other two.
Flags: needinfo?(djmdeveloper060796)
Alright, All I was looking for was a fixed color code, if you guys say white, I assume its #fff and for black, it will be #000
But I got some question here:
1. Should we really set the background color as black ?
2. Do we want the notification bar to change according to themes, like white bg and black text for light theme and black bg and white text for dark theme ? If so, how we do it ?

Otherwise if this is going to be a fixed one, I will go for black and white as discussed in the earlier comments. I already have a patch ready
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(odvarko)
Flags: needinfo?(djmdeveloper060796)
(In reply to [:Towkir] Ahmed from comment #8)

> I don't think pure black or white is a good idea :(

What about setting background-color to -moz-Dialog and text color to -moz-DialogText as in global.css. The combination is not bad.What do you say Honza?
Flags: needinfo?(djmdeveloper060796)
Posted image devtools.png
Heres a screenshot
Hi Deepjyoti,
As I said, the color is already being inherited. The only thing we can (if we have to) change is the background color of this:  https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/notification-box.css#36

But after this change, another interesting issue appears which is the border on top and bottom.
Try changing/removing these : https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/notification-box.css#38-39
And check on both light and dark theme, you will find a 1px border visible on dark theme  but nothing on light theme, which means, with the borders enabled, the dark theme shows 2px border on top and 1px at bottom.

What should we actually do with these borders ?

NB: Honza is on a vacation till January 1s I guess. Maybe we should wait for the final decision from him, but meanwhile you can share your opinion on these borders and more.

Thanks
Flags: needinfo?(djmdeveloper060796)
Sorry for the delay.

OK, I can see the problem with borders.

What if we used just: border-bottom: 1px solid lightgray;?

To sum up the patch:
- Specify bk color
- Specify text color
- Specify borders (no top + lightgray bottom)

@Towkir: please attach a patch with you progress so, we can all use the same code for testing.

Thanks for working on this!

Honza
Flags: needinfo?(odvarko) → needinfo?(3ugzilla)
Hi, if this issue is still unassigned I would like to take a look at it. If so, who should I be looking to if I have questions?
I've looked at the reported border problem and I've noticed that the border discrepancy between light/dark theme that was mentioned by @Towkir is actually slightly different from what was reported.

Both light and dark themes have a 2px border on the top, just that with the light theme, the second border is so light that it isn't noticed. This leads me to believe that the issue is likely an overlap between the notification-box border and the devtools border. 

From what was recommended by @Honza, I removed the top border to fix the 2px border-top issue, but I've had a question about the results of the fix.

- With border-bottom: 1px solid lightgray, the border color stays the same between themes, and as a result of this, it doesn't match the border-top of the devtools box. Is this the expected behaviour, or should we be trying to match the bottom and top borders of the notification box?

I've tried to find the most similar color, and am currently using var(--theme-splitter-color) which matches for the light theme, and is similar for the dark theme, but I don't know if there is a better color I should be using to create the match.
Flags: needinfo?(odvarko)
Hi, n.goh
Thanks for your interest on this. I was supposed to attach a patch and did some work too, meanwhile got busy with something else. If you have made some changes that looks good on your local build. Please attach a patch and set the review flag to Honza.
Someone, or I will then assign the bug on you.

Thanks
Flags: needinfo?(djmdeveloper060796)
Hi @Towkir,

In the latest version of Firefox, I noticed that the original issue from this thread (Devtool notification box background) is no longer present, or rather I am unable to reproduce the issue. Is it safe for me to assume that this has since been fixed, and the only bug left to be patched would be the border thickness issue?

Thanks for the response!
It looks the same at my end. Firefox 55.0a1, Ubuntu 16.04
Can you attach a screenshot which you say is been fixed.
Thanks
Flags: needinfo?(3ugzilla)
Posted image fixed_dark_theme.png
@Towkir sorry about the delay, here is a screenshot of what I've considered as "fixed" in the dark theme. The fix in this case being removing the top border and changing bottom border color to --theme-splitter-color. If you want additional screenshots let me know, I have some for the lighter theme, as well as some using the fix suggested by @Honza (use light-gray instead).
Hi n.goh,
From our previous discussion on this bug, you got some things right.
Let me summarize the fix for you. On [1] rule: 

1. remove the "border-top: 1px solid ThreeDShadow;"
2. add "color: var(--theme-body-color-alt);"
3. change background-color to "var(--theme-body-background)"
4. change border-bottom to "1px solid var(--theme-splitter-color)"

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/notification-box.css#35

Let me know if you need anything, Try to submit a patch, also let me know if there is any problem.

[ Note: for now I am not worrying about creating a new rule for .notificationbox .notification[data-type="info"] because the default styling needs some tweaks itself. ]
Flags: needinfo?(odvarko) → needinfo?(n.goh)
(In reply to [:Towkir] Ahmed from comment #19)
> [ Note: for now I am not worrying about creating a new rule for
> .notificationbox .notification[data-type="info"] because the default styling
> needs some tweaks itself. ]
I meant .notificationbox .notification[data-type="warning"]
data-type="info" is already defined there.
Let's get a quick fix out for this so its no longer broken and then figure out the broader / more correct solution in a follow up bug.
I will submit a patch within a few hours assuming n.goh is a bit busy currently.
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Flags: needinfo?(n.goh)
The texts are now readable, also the "info" and "critical" types of messages are fine too. Looks good on my local build.
Hope this helps
Attachment #8887702 - Flags: review?(odvarko)
Comment on attachment 8887702 [details] [diff] [review]
devtools-notification-bar-color.patch

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

Looks reasonable to me.

R+

Thanks for working on this Towkir!

Honza
Attachment #8887702 - Flags: review?(odvarko) → review+
@Towkir: I am attaching a screenshot of the Notification Bar Win10+OSX
Can you please attach screenshot from Ubuntu?

Honza
Here it is, both light and dark theme.
Do you think that the notification bar close icon [messageCloseButton] should have a filter that inverts the cross icon depending on themes ? If yes, you can report a follow up bug.
It seems almost invisible on dark theme.
Also, if this patch is a go, you can add checkin-needed
Flags: needinfo?(3ugzilla)
(In reply to [:Towkir] Ahmed from comment #26)
> Do you think that the notification bar close icon [messageCloseButton]
> should have a filter that inverts the cross icon depending on themes ? If
> yes, you can report a follow up bug.
I created a follow up for this: bug 1382184

Thanks!

Honza
See Also: → 1382187
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a4854dc71a4
Devtools notification box colors are now fixed for the texts to be readable. r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3a4854dc71a4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.