Improve colors of DevTools Notification Bar

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Developer Tools: Shared Components
P3
enhancement
VERIFIED FIXED
9 months ago
5 months ago

People

(Reporter: Honza, Assigned: Abhinav Koppula, Mentored)

Tracking

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

9 months ago
This is a follow up for bug 1304837

Notification Bar colors (text, background, borders, icons) should be revised to make sure that these colors work across supported OSes and DevTools themes. They should also be consistent with other UI DevTools widgets.

Honza
(Reporter)

Comment 1

9 months ago
Created attachment 8887886 [details]
notification-bar-win-osx.png
(Reporter)

Comment 2

9 months ago
Created attachment 8887887 [details]
notification-bar-linux-both-theme.png
(Reporter)

Comment 3

9 months ago
Screenshots of the current state (Win10, OSX, Linux) attached.

Honza
(Reporter)

Comment 4

9 months ago
The notification bar close icon [messageCloseButton] should have a filter that inverts the cross icon depending on themes. It seems almost invisible on dark theme.

See also this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=1304837#c26

Honza

Updated

9 months ago
See Also: → bug 1382187
(Reporter)

Updated

9 months ago
Priority: -- → P3
(Reporter)

Updated

7 months ago
Severity: normal → enhancement
(Reporter)

Updated

7 months ago
Assignee: nobody → 3ugzilla
Mentor: odvarko
Status: NEW → ASSIGNED
(Reporter)

Comment 5

7 months ago
Some pieces of the NotificationBar has already been fixed (e.g. bug 1382187 fixed the icon).

The remaining thing seems to be the text color.

It's currently gray in both light and dark themes and a bit hard to read.
I think it should be black in light theme and white in dark theme.

I believe we can just use: var(--theme-toolbar-color) for the text color.

(you might also see my comment in bug 1382187 comment #8)

Honza
Comment hidden (mozreview-request)
(Assignee)

Comment 7

7 months ago
Hi Honza,
This looked like a small fix to me. I have created a mozreview-request. 
Please let me know if this was what you were looking for.
(Reporter)

Comment 8

7 months ago
mozreview-review
Comment on attachment 8910399 [details]
Bug 1382184 - Change text color of notification-box to 'theme-toolbar-color'.

https://reviewboard.mozilla.org/r/181838/#review187388

Works for me (tested on Win10 and OSX)

R+, thanks!

Honza
Attachment #8910399 - Flags: review?(odvarko) → review+

Comment 9

7 months ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/870ce315712c
Change text color of notification-box to 'theme-toolbar-color'. r=Honza
I'm not sure why this wasn't mentioned by Honza but please note that it is not considered good practice to "steal" bugs from assignees without requesting a needinfo and I'd hate to see that happen more in the future. It creates an environment of competition and urgency that drives people away (not to mention the immediate disappointment of getting your bug taken away).

Thank you.
Assignee: 3ugzilla → abhinav.koppula
(Reporter)

Comment 11

7 months ago
(In reply to Johann Hofmann [:johannh] from comment #10)
> I'm not sure why this wasn't mentioned by Honza but please note that it is
> not considered good practice to "steal" bugs from assignees without
> requesting a needinfo and I'd hate to see that happen more in the future. It
> creates an environment of competition and urgency that drives people away
> (not to mention the immediate disappointment of getting your bug taken away).
> 
> Thank you.

Ah, sorry, that's my mistake! I accidentally assigned the bug to wrong email '3ugzilla@gmail.com' [:Towkir] (previously working on related bug) while I should use this e-mail address: abhinav.koppula@gmail.com (upon our prior offline discussion). So, no steeling happened in this case. Sorry about that.

Honza

Comment 12

7 months ago
(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> Ah, sorry, that's my mistake! I accidentally assigned the bug to wrong email
> '3ugzilla@gmail.com' [:Towkir] (previously working on related bug) while I
> should use this e-mail address: abhinav.koppula@gmail.com (upon our prior
> offline discussion). So, no steeling happened in this case. Sorry about that.
About this, I am pretty sure we talked yesterday over IRC and you assigned this to me, it was unassigned for a long time and we decided to clear it. As a small change, I could do it fast, but things came up with this and I asked questions to you on IRC but you were offline, and I waited.

Guys, it's totally fine for me if someone wants to work on bugs, I would rather help if I can. But from new contributors I would expect them to abide by the Bugzilla Etiquette[0] and ask people on bugs before working on that.
It would be nice if I could hear from :Abhinav regarding this.

Let's now talk about why I waited before submitting my patch:

As we are talking about the improvement of the devtools notification bar,
there are three types of notification bars, "info", "warning" and "critical" [code resides at [1]]
I think this is the bug where we take care of them all, what do you say honza ?
As patch is now landed, we might need a new bug.
[I asked this on IRC, but you were offline]

I guess Abhinav can work on the new bug too :)

Cheers !

[0] https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/notification-box.css#35-58
Flags: needinfo?(odvarko)
Flags: needinfo?(abhinav.koppula)
(In reply to [:Towkir] Ahmed from comment #12)
> (In reply to Jan Honza Odvarko [:Honza] from comment #11)
> > Ah, sorry, that's my mistake! I accidentally assigned the bug to wrong email
> > '3ugzilla@gmail.com' [:Towkir] (previously working on related bug) while I
> > should use this e-mail address: abhinav.koppula@gmail.com (upon our prior
> > offline discussion). So, no steeling happened in this case. Sorry about that.
> About this, I am pretty sure we talked yesterday over IRC and you assigned
> this to me, it was unassigned for a long time and we decided to clear it. As
> a small change, I could do it fast, but things came up with this and I asked
> questions to you on IRC but you were offline, and I waited.
> 
> Guys, it's totally fine for me if someone wants to work on bugs, I would
> rather help if I can. But from new contributors I would expect them to abide
> by the Bugzilla Etiquette[0] and ask people on bugs before working on that.
> It would be nice if I could hear from :Abhinav regarding this.

I think we established that this was a harmless mistake from Honza and there is no need to dwell on it or put Abhinav on the spot for the mistake of someone else. Please be considerate and assume good intentions.

Leaving needinfo on Honza for the technical questions.
Flags: needinfo?(abhinav.koppula)
(Reporter)

Comment 14

7 months ago
(In reply to [:Towkir] Ahmed from comment #12)
> (In reply to Jan Honza Odvarko [:Honza] from comment #11)
> > Ah, sorry, that's my mistake! I accidentally assigned the bug to wrong email
> > '3ugzilla@gmail.com' [:Towkir] (previously working on related bug) while I
> > should use this e-mail address: abhinav.koppula@gmail.com (upon our prior
> > offline discussion). So, no steeling happened in this case. Sorry about that.
> About this, I am pretty sure we talked yesterday over IRC and you assigned
> this to me, it was unassigned for a long time and we decided to clear it. As
> a small change, I could do it fast, but things came up with this and I asked
> questions to you on IRC but you were offline, and I waited.
Yeah, I messed it up I was chatting with both of you yesterday and
mixed who is working on what.

> As we are talking about the improvement of the devtools notification bar,
> there are three types of notification bars, "info", "warning" and "critical"
> [code resides at [1]]
> I think this is the bug where we take care of them all, what do you say
> honza ?
> As patch is now landed, we might need a new bug.
> [I asked this on IRC, but you were offline]
And this is correct I filled bug 1401948

> I guess Abhinav can work on the new bug too :)
Abhinav, do you have time to work on that one?

---

Also, you guys are both doing great work on DevTools and not only me
but others from the DevTools team appreciate that!

Honza
Flags: needinfo?(odvarko)
https://hg.mozilla.org/mozilla-central/rev/870ce315712c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Comment 16

7 months ago
> > I guess Abhinav can work on the new bug too :)
> Abhinav, do you have time to work on that one?
> 
Yes, Honza would be glad to work on it.

Comment 17

5 months ago
I have successfully reproduced this bug with Nightly 56.0a1 (2017-07-19) (32-bit)  on windows 10(32bit)

this bug is verified fix with  latest beta 57.0 (32-bit)
Build ID: 20171106194249
Mozilla/5.0 (Windows NT 10.0; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20171108]
QA Whiteboard: [bugday-20171108]
I have reproduced this bug with Nightly 56.0a1 (2017-07-19) on Ubuntu 16.04 LTS!

This bug is verified as fixed with latest Beta !

Build  ID :  20171102181127
Use Agent :  Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20171108]

Comment 19

5 months ago
As this bug is verified in both linux(comment 17) and windows (comment 18), I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.