At mouse over, the 'X' button via Notification bar is offset by ~1px

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Developer Tools: Shared Components
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: adalucinet, Assigned: Deepjyoti Mondal)

Tracking

({good-first-bug, regression})

49 Branch
Firefox 51
good-first-bug, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47 unaffected, firefox48 unaffected, firefox49 fix-optional, firefox50 wontfix, firefox51 verified)

Details

(Whiteboard: [reserve-html] [good first bug])

Attachments

(10 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8757953 [details]
Screenshot

[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.
(Reporter)

Updated

2 years ago
QA Whiteboard: [qe-dthtml]
Whiteboard: [devtools-html][triage]

Updated

2 years ago
Blocks: 1263741, 1264671
Flags: qe-verify+
QA Contact: alexandra.lucinet
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
Flags: needinfo?(hholmes)
(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.
Flags: needinfo?(hholmes)
(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
Flags: needinfo?(hholmes)
Created attachment 8758265 [details]
close-expanded.svg

(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.
Flags: needinfo?(hholmes)

Updated

2 years ago
Keywords: regressionwindow-wanted
Component: Developer Tools: Debugger → Developer Tools: Shared Components

Updated

2 years ago
Priority: -- → P2
Whiteboard: [devtools-html][triage] → [devtools-html]
Keywords: regressionwindow-wanted
Keywords: regressionwindow-wanted

Updated

2 years ago
Priority: P2 → P1
Assignee: nobody → odvarko

Updated

2 years ago
Status: NEW → ASSIGNED
Iteration: --- → 50.1

Updated

2 years ago
Assignee: odvarko → nobody
Status: ASSIGNED → NEW
Iteration: 50.1 → ---
Priority: P1 → P2
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?
status-firefox49: affected → fix-optional
status-firefox50: --- → fix-optional
Flags: needinfo?(odvarko)
Whiteboard: [devtools-html] → [devtools-html] [good first bug]
(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
Flags: needinfo?(odvarko)

Updated

2 years ago
Priority: P2 → P3
Whiteboard: [devtools-html] [good first bug] → [reserve-html] [good first bug]
(Assignee)

Comment 7

2 years ago
I would like to pick up this.From where should I start?
Flags: needinfo?(odvarko)
(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
Assignee: nobody → evan
Flags: needinfo?(odvarko)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

2 years ago
Created attachment 8768886 [details] [diff] [review]
Bug1276675.patch

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.
Attachment #8768886 - Flags: review?(odvarko)
Created attachment 8769173 [details]
bug1276675.png

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
Attachment #8768886 - Flags: review?(odvarko)
(Assignee)

Comment 11

2 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?
Flags: needinfo?(odvarko)
Deepjyoti is working on this bug. Thanks, Deepjyoti.
Assignee: evan → nobody
Status: ASSIGNED → NEW

Updated

2 years ago
Assignee: nobody → djmdeveloper060796
Status: NEW → ASSIGNED
(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
Flags: needinfo?(odvarko)
(Assignee)

Comment 14

2 years ago
Created attachment 8771067 [details]
screen3.png

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

Comment 15

2 years ago
Created attachment 8771071 [details]
screen4.png

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?
Flags: needinfo?(odvarko)
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
Flags: needinfo?(odvarko) → needinfo?(djmdeveloper060796)
(Assignee)

Comment 17

2 years ago
Created attachment 8771379 [details]
close.svg

Sorry for missing it earlier, Here it is.
Flags: needinfo?(djmdeveloper060796)

Updated

2 years ago
QA Contact: adalucin → cristian.comorasu
NI=me (so, it isn't forgotten)

Honza
Flags: needinfo?(odvarko)
Created attachment 8779325 [details]
close-icon-tesing-screenshot.png

(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
Flags: needinfo?(odvarko)
(Assignee)

Comment 20

2 years ago
Created attachment 8780830 [details] [diff] [review]
Bug1276675_v2.patch

@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.
Attachment #8768886 - Attachment is obsolete: true
Attachment #8780830 - Flags: review?(odvarko)
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
Attachment #8780830 - Flags: review?(odvarko)
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug
(Assignee)

Comment 23

2 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.
Created attachment 8784274 [details]
notification-close-icon.png

Ah, sorry, here is the screenshot.

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

Honza
Any progress with this bug?
Any blockers?

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

Thanks!
Honza
Flags: needinfo?(djmdeveloper060796)
(Assignee)

Comment 26

2 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.
Flags: needinfo?(djmdeveloper060796)
(Assignee)

Comment 27

2 years ago
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. 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.
Attachment #8787576 - Flags: review?(odvarko)
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.
Attachment #8787576 - Flags: review?(odvarko)
(Assignee)

Comment 29

2 years ago
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.

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.
Flags: needinfo?(odvarko)
Attachment #8790092 - Flags: review?(odvarko)
Attachment #8787576 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
Attachment #8780830 - Attachment is obsolete: true
Created attachment 8790260 [details]
close-button.png

(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
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
Attachment #8790092 - Flags: review?(odvarko)
(Assignee)

Comment 32

2 years ago
Created attachment 8790397 [details] [diff] [review]
adjusted the position of the close icon of the responsive design mode

I have adjusted the position of the close icon of responsive design mode.This is the updated version of the previous patch.
Attachment #8790092 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
Attachment #8790397 - Flags: review?(odvarko)
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
Attachment #8790397 - Flags: review?(odvarko) → review+
Clearing NI

Honza
Flags: needinfo?(odvarko)
(Assignee)

Comment 35

2 years ago
Created attachment 8790845 [details] [diff] [review]
removed comment and trailing space

I have removed the comment and the trailing space.
Attachment #8790397 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
Attachment #8790845 - Flags: review?(odvarko)
Comment on attachment 8790845 [details] [diff] [review]
removed comment and trailing space

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

Looks good to me.
Attachment #8790845 - Flags: review?(odvarko) → review+
You can mark as 'checkin-needed' in the Keywords field.

Honza
Flags: needinfo?(odvarko)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 38

2 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
Keywords: checkin-needed

Comment 39

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/20db017e3e38
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

2 years ago
Iteration: --- → 51.3 - Sep 19
Priority: P3 → P1
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.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+
Hi Honza, is this something we'd consider uplifting to Beta50? It's marked as a P1.
Flags: needinfo?(odvarko)
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
Flags: needinfo?(odvarko)
(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.
status-firefox50: fix-optional → wontfix
You need to log in before you can comment on or make changes to this bug.