Ada [:adalucinet] Reporter
	Description • 8 years ago

[Affected versions]:
- latest Nightly 49.0a1

[Affected platforms]:
- Windows 10 64-bit
- Mac OS X 10.10.5
- Ubuntu 16.04 64-bit

[Steps to reproduce]:
1. Launch Firefox.
2. Open Debugger.
3. Pause it (F8).
4. Refresh the page.
5. Select Inspector.
6. Hover over 'X' button via Notification Bar.

[Expected result]:
- Notification Bar is properly displayed.

[Actual result]:
- 'X' button via Notification bar is offset by ~1px.

[Regression range]:
- Not reproducible with latest 48.0a2; will investigate further for the regression range

[Additional notes]:
- Screenshot attached.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 1 • 8 years ago

This is because the close icon is now based on chrome://devtools/skin/images/close.svg (used by devtools) while the original icon used by <xul:notificationbox> is chrome://global/skin/icons/close.png

We might want to replace the current devtools icon by the one coming from Firefox (btw. used also for closing browser tabs). This would probably require transforming png -> svg.

Helen, what do you think is the best to do here?

Honza

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 2 • 8 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #1)
> This is because the close icon is now based on
> chrome://devtools/skin/images/close.svg (used by devtools) while the
> original icon used by <xul:notificationbox> is
> chrome://global/skin/icons/close.png
> 
> We might want to replace the current devtools icon by the one coming from
> Firefox (btw. used also for closing browser tabs). This would probably
> require transforming png -> svg.
> 
> Helen, what do you think is the best to do here?
> 
> Honza

I think it's safe to use the close.svg coming from Firefox, especially if it fixes this issue.

I would say that part of the switch would be ensuring it doesn't negatively affect other parts of devtools.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 3 • 8 years ago

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #2)
> (In reply to Jan Honza Odvarko [:Honza] from comment #1)
> > This is because the close icon is now based on
> > chrome://devtools/skin/images/close.svg (used by devtools) while the
> > original icon used by <xul:notificationbox> is
> > chrome://global/skin/icons/close.png
> > 
> > We might want to replace the current devtools icon by the one coming from
> > Firefox (btw. used also for closing browser tabs). This would probably
> > require transforming png -> svg.
> > 
> > Helen, what do you think is the best to do here?
> > 
> > Honza
> 
> I think it's safe to use the close.svg coming from Firefox, especially if it
> fixes this issue.
Note that the Firefox icon is PNG while we are using SVG, should we convert it? If yes how?

Honza

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 4 • 8 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #2)
> > (In reply to Jan Honza Odvarko [:Honza] from comment #1)
> > > This is because the close icon is now based on
> > > chrome://devtools/skin/images/close.svg (used by devtools) while the
> > > original icon used by <xul:notificationbox> is
> > > chrome://global/skin/icons/close.png
> > > 
> > > We might want to replace the current devtools icon by the one coming from
> > > Firefox (btw. used also for closing browser tabs). This would probably
> > > require transforming png -> svg.
> > > 
> > > Helen, what do you think is the best to do here?
> > > 
> > > Honza
> > 
> > I think it's safe to use the close.svg coming from Firefox, especially if it
> > fixes this issue.
> Note that the Firefox icon is PNG while we are using SVG, should we convert
> it? If yes how?
> 
> Honza

Bryan Bell from the UX team has been working with me on icons—I'm attaching one he designed which fills up the entire SVG viewbox. 

It might be better to update the devtools SVG with this one and make sure that it looks fine across devtools, I'm not sure.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 5 • 8 years ago

Marking this as fix-optional for 49 and 50, since it looks like a minor polish issue and the dev tools team unassigned it and lowered the priority. Honza, would this be a good first bug?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 6 • 8 years ago

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #5)
> Marking this as fix-optional for 49 and 50, since it looks like a minor
> polish issue and the dev tools team unassigned it and lowered the priority.
> Honza, would this be a good first bug?
Yes!

I can mentor anyone who's interesting to fix this.

Honza

	Deepjyoti Mondal Assignee
	Comment 7 • 8 years ago

I would like to pick up this.From where should I start?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 8 • 8 years ago

(In reply to Deepjyoti Mondal from comment #7)
> I would like to pick up this.From where should I start?

This bug happen since we used different icon for the notification box.  The correct icon is chrome://global/skin/icons/close.png.

1. step) Convert chrome://global/skin/icons/close.png to svg
2. step) Replace the existing devtools icon chrome://devtools/skin/images/close.svg
3. step) Check that this bug is fixed (the icon suddenly has proper size)
4. step) Check that places that are already using chrome://devtools/skin/images/close.svg are not broken.


add #2) The issue is the icon size, see also my comment here:
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/shared/components/notification-box.css#L75

This piece of CSS also needs to be fixed.

Honza

	Deepjyoti Mondal Assignee
	Comment 9 • 8 years ago

I replaced chrome://devtools/skin/images/close.svg with chrome://global/skin/icons/close.svg and adjusted the background image position of the close button.The icon is now almost centered.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 10 • 8 years ago

Thanks for working on this!

* When I apply the patch I don't see the icon anymore (see attached screenshot) I am on Win 10
* We don't want to depend on an icon coming from the platform. The icon should be ported into DevTools and replace the existing devtools/client/themes/images/close.svg file.
* All pleases where the current close.svg is used needs to be verified.

Honza

	Deepjyoti Mondal Assignee
	Comment 11 • 8 years ago

Hi Honza, I use ubuntu, I do not have windows machine hence I could not test it on windows.So I need to replace the close.svg under the images folder with the new close-expanded.svg provided by helen 
(Comment #4) and make sure that its looking fine right?

	Evan Tseng [:evanxd]
	Comment 12 • 8 years ago

Deepjyoti is working on this bug. Thanks, Deepjyoti.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 13 • 8 years ago

(In reply to Deepjyoti Mondal from comment #11)
> Hi Honza, I use ubuntu, I do not have windows machine hence I could not test
> it on windows.So I need to replace the close.svg under the images folder
> with the new close-expanded.svg provided by helen 
> (Comment #4) and make sure that its looking fine right?
Yes, correct.

A few more notes:

1) The original image: 
Ubuntu: chrome://global/skin/icons/close.svg
Win: chrome://global/skin/icons/close.png
was 20x20 pixels and the circle around was just fine

2) The image used now chrome://devtools/skin/images/close.svg is using CSS width x height 19x20

Read this comment in notification-box.css 
  /* Use odd value for the width so, the icon can be nicely h-centered.
    This also means that the button is not perfect circle (its using
    10x border radius), but it's less visible visual discrepancy
    than wrong centering of the icon */

3) The svg icon Helen attached is 16x16 just like chrome://global/skin/icons/close.svg, which might be good for Linux, but not for Win where it should be centered in 20x20 rectangle.

So, the thing is: we need SVG icon that is 20x20 so, it can be perfectly centered in 20x20 rectangle on Win/Mac. I guess Linux is fine since it's using 16x16 (and we already have chrome://global/skin/icons/close.svg)

Honza

	Deepjyoti Mondal Assignee
	Comment 14 • 8 years ago

I replaced the close.svg by the icon provided by Helen and this is how it looks in the notification box.

	Deepjyoti Mondal Assignee
	Comment 15 • 8 years ago

The close icon present in this screen shot is a modified version of that provided by helen.I editted the svg's  height and width to make it 20 X 20. Is it alright?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 16 • 8 years ago

Can you attach your SVG file so, I can try it out?

The screenshot doesn't show how it looks like if the mouse hovers over the icon (and this report is about:  Hover over 'X' button via Notification Bar, see the comment #0)

Honza

	Deepjyoti Mondal Assignee
	Comment 17 • 8 years ago

Sorry for missing it earlier, Here it is.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 18 • 8 years ago

NI=me (so, it isn't forgotten)

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 19 • 8 years ago

(In reply to Deepjyoti Mondal from comment #17)
> Created attachment 8771379 [details]
> close.svg
> 
> Sorry for missing it earlier, Here it is.
Still no luck, I am attaching a screenshot of how it looks on my machine.
There are three things on the screenshot:

1) This is how it looks on my machine when:
- replacing the existing devtools close.svg icon with yours (attached in comment #17) 
- modifying `.notificationbox .messageCloseButton` style to use: width: 20px and height: 20px; and background-position: 0 center;
It doesn't look correct.

2) This is how it looks now. Look closer the button isn't a circle, but an ellipse. This is because `.notificationbox .messageCloseButton` style is using width: 19px; currently. This ss the point of this bug and it needs to be fixed. It needs to be 20x20.

3) This is how browser-tab close button works. We want the same for the notification bar close button.

Btw. the background of the notification close button displayed when mouse is hovering over seems to be also wrong. It's bright while it should be dark - just like in case of the browser-tab close icon.

Please test it on your machine carefully. Play with the icon size as well as the `.notificationbox .messageCloseButton` style (width, height, background-position, etc.)


Honza

	Deepjyoti Mondal Assignee
	Comment 20 • 8 years ago

@Honza, I have tweaked the background position and svg viewBox property to center the 'X'. The icon is now 20X20 , I have tested it on my machine and the 'X' is centered and the background is no more oval. Please let me if its alright. Once this problem regarding positioning and size is solved I will start working on the color scheme.Sorry for the delay, my university has reopened after vacations and I was busy with my project work.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 21 • 8 years ago

Comment on attachment 8780830 [details] [diff] [review]
Bug1276675_v2.patch

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

This is a lot better!

Couple of comments:

1) I am attaching a screenshot (Win10) showing how the icon looks with the patch attached and how it looked before the icon was broken (it happened in bug 1264671). Can you make it *exactly* the same (note the size of the cross and the background hover-color)?
2) Btw. there are collisions when applying the patch.

Honza

	Patrick Brosset <:pbro>
	Comment 22 • 8 years ago

[good first bug] whiteboard -> keyword mass change

	Deepjyoti Mondal Assignee
	Comment 23 • 8 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #21)
> Comment on attachment 8780830 [details] [diff] [review]
> Bug1276675_v2.patch
> 
> Review of attachment 8780830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a lot better!
> 
> Couple of comments:
> 
> 1) I am attaching a screenshot (Win10) showing how the icon looks with the
> patch attached and how it looked before the icon was broken (it happened in
> bug 1264671). Can you make it *exactly* the same (note the size of the cross
> and the background hover-color)?


Yup! Let me see the screenshot, I will definitely give it a try.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 24 • 8 years ago

Ah, sorry, here is the screenshot.

(please use NI=me so, your question is not forgotten)

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 25 • 8 years ago

Any progress with this bug?
Any blockers?

It's on our TODO list and it would be great to have it fixed.

Thanks!
Honza

	Deepjyoti Mondal Assignee
	Comment 26 • 8 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #25)
> Any progress with this bug?
> Any blockers?
> 
> It's on our TODO list and it would be great to have it fixed.
> 
> Thanks!
> Honza

Hi Honza, I have been quite busy for the last few days so I have not been able to provide any update. I have seen the image you have attached. I will be uploading my next patch as soon as possible.

	Deepjyoti Mondal Assignee
	Comment 27 • 8 years ago

I have made some changes to the size and color of close.svg. However I am facing a problem. The default background-color rule of the notification bar is set to be InfoBackground. It is supposed to be a light shade as shown in the Screen shot. However the background color of the notification bar is appearing to be dark and the notification message is almost invisible and also the background of the close button appears different.However changing the background color of the notification bar to white solves the problem.I am not sure where the problem is. I think its a different issue so I have not made any changes to the backgorund-color rule of the notification bar in this patch. I have tried to make the close button appear almost same as the one present in the screen shot. Do let me know what further changes I have to make.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 28 • 8 years ago

Comment on attachment 8787576 [details] [diff] [review]
Bug1276675_v3.patch

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

I couldn't apply the patch on the current HEAD (there are conflicts), please rebase.

(In reply to Deepjyoti Mondal from comment #27)
> Created attachment 8787576 [details] [diff] [review]
> Bug1276675_v3.patch
> 
> I have made some changes to the size and color of close.svg. However I am
> facing a problem. The default background-color rule of the notification bar
> is set to be InfoBackground. It is supposed to be a light shade as shown in
> the Screen shot.
I took the screenshot from the browser-tab just to show the icon.
The background of the notification bar should stay as is.


This version looks great!


Last thing: Since the patch is changing `close.svg` file we need to make sure
that it dosn't break existing CSS.
Please search through the devtools code base (the top level `devtools` directory),
find all places where `close.svg` is used and check out the UI to see that it's
still ok.

After this is done, and all is ok, I can R+ the patch.

Honza

::: devtools/client/shared/components/notification-box.css
@@ +77,2 @@
>      This also means that the button is not perfect circle (its font-feature-settings: 
>      10x border radius), but it's less visible visual discrepency

I know this isn't introduced in this patch, but typo: discrepancy

@@ +89,1 @@
>    filter: invert(1);

Interestingly, invert(1) isn't part of the tree, but not even introduced in this patch. Please update to the current m-c HEAD.

	Deepjyoti Mondal Assignee
	Comment 29 • 8 years ago

I have updated my repository.

I have searched devtools code base and have found the following places where close.svg has been used :

1. mozilla-central/devtools/client/themes/toolbox.css : The close icon of the devtools. It looks fine after applying the changes.

2. mozilla-central/devtools/client/themes/responsivedesign.inc.css : the close button of the responsive design mode. The icon doesnt looks centered.It looks displaced towards the upper left corner.

3. mozilla-central/devtools/client/themes/memory.css : close button of memory snapshot under memory,devtools. It looks fine after making the changes

4. mozilla-central/devtools/client/shared/widgets/filter-widget.css : the remove button on filter widget.This too looks alright.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 30 • 8 years ago

(In reply to Deepjyoti Mondal from comment #29)
> Created attachment 8790092 [details] [diff] [review]
> bug1276675_v4.patch
> 
> I have updated my repository.
> 
> I have searched devtools code base and have found the following places where
> close.svg has been used :
> 
> 1. mozilla-central/devtools/client/themes/toolbox.css : The close icon of
> the devtools. It looks fine after applying the changes.
> 
> 2. mozilla-central/devtools/client/themes/responsivedesign.inc.css : the
> close button of the responsive design mode. The icon doesnt looks
> centered.It looks displaced towards the upper left corner.
I made a screenshot (attaching) of the icon and yes I can see the difference.
Can you update your patch and fix the position.
Thanks!

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 31 • 8 years ago

Comment on attachment 8790092 [details] [diff] [review]
bug1276675_v4.patch

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

So, we need to fix the position of the responsive mode close icon.

Honza

	Deepjyoti Mondal Assignee
	Comment 32 • 8 years ago

I have adjusted the position of the close icon of responsive design mode.This is the updated version of the previous patch.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 33 • 8 years ago

Comment on attachment 8790397 [details] [diff] [review]
adjusted the position of the close icon of the responsive design mode

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

Looks great!

I made couple of comments please fix them too.

R+ assuming the comments are fixed.

Thanks for the patch!

Honza

::: devtools/client/shared/components/notification-box.css
@@ +77,3 @@
>      This also means that the button is not perfect circle (its using
>      10x border radius), but it's less visible visual discrepency
>      than wrong centering of the icon */

Please update or remove the comment above. It's no longer valid.

@@ +95,5 @@
>  }
>  
>  .notificationbox .messageCloseButton:active {
>    background-color: rgba(170, 170, 170, .4); /* --toolbar-tab-hover-active */
> +} 

Remove trailing white space

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 34 • 8 years ago

Clearing NI

Honza

	Deepjyoti Mondal Assignee
	Comment 35 • 8 years ago

I have removed the comment and the trailing space.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 36 • 8 years ago

Comment on attachment 8790845 [details] [diff] [review]
removed comment and trailing space

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

Looks good to me.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 37 • 8 years ago

You can mark as 'checkin-needed' in the Keywords field.

Honza

	Pulsebot
	Comment 38 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/20db017e3e38
change the size, position and hover background color of close.svg. Adjusted the position of the close icon of responsive design mode. r=honza

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 39 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/20db017e3e38

	Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com
	Comment 40 • 8 years ago

I can confirm that the positioning of the "X" is in the center of the circle. I tested it using Fx 51.0a1, build ID:20160918030408, on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 LTS.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 41 • 8 years ago

Hi Honza, is this something we'd consider uplifting to Beta50? It's marked as a P1.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 42 • 8 years ago

This bug caused a regression bug 1303458 so, we would have to port that one too.
From my perspective, this is a little change and we can wait for another cycle.
(and have some time to test and see that there is no other regression).


Honza

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 43 • 8 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #42)
> This bug caused a regression bug 1303458 so, we would have to port that one
> too.
> From my perspective, this is a little change and we can wait for another
> cycle.
> (and have some time to test and see that there is no other regression).
> 
> 
> Honza

Thanks. I agree. In that case, let's wontfix this for F50.