1303458 - Developer tools toolbox close button isn't vertically aligned anymore

Status: VERIFIED FIXED

(DevTools :: Framework, defect)

Description

[Matthew N. [:MattN]]

Attachment: Screenshot diff: devtools_1_bottomToolbox.png

This is likely a regression from bug 1276675. See the screenshots at https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=29af101880db7ce7f5f87f58e1ff20988c1c5fc3&newProject=mozilla-central&newRev=edfff7a9e4c43f6d516dfbf69a64e205e5cdb699&filter=devtools

Comment 1

[Brian Grinstead [:bgrins]]

Helen, can you take a look at the asset here?  The close button is using background-size: cover so the SVG itself might be a bit off center.

For a quick reference to what changed, see: https://screenshots.mattn.ca/comparisons/mozilla-central/29af101880db7ce7f5f87f58e1ff20988c1c5fc3/mozilla-central/edfff7a9e4c43f6d516dfbf69a64e205e5cdb699/osx-10-10/devtools_1_bottomToolbox.png.

Comment 2

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> Helen, can you take a look at the asset here?  The close button is using
> background-size: cover so the SVG itself might be a bit off center.
I think Matthew is right and this is a regression from bug 1276675. On my end, the SVG is centered within its viewport: https://cl.ly/2b32101J1P05

Comment 3

[Brian Grinstead [:bgrins]]

Looking at https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/images/close.svg#4, I see `width="17" height="17" viewBox="0 0 20 20"` on the svg element.

Here's a sample of what this looks like when covering a 16x16 element like our close button: http://jsbin.com/dezuzuyixe/edit?html,css,output.  Can you check and see if there's a way to tweak the image to make this become centered?

Note: even if the element size is changed to 20x20 (as in notification box) it doesn't seem centered, which is probably why there's a custom position that landed here: https://hg.mozilla.org/mozilla-central/rev/20db017e3e38#l1.23.

Comment 4

[Deepjyoti Mondal]

What about tweaking the background-position of the image (close.svg) of the close button to vertically center it?

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Deepjyoti Mondal from comment #4)
> What about tweaking the background-position of the image (close.svg) of the
> close button to vertically center it?

We could, but I think updating the viewbox and width/height to be 16 then using `background-size: cover` should work just as well and let us get rid of some of the hardcoded positionings.  I'll upload what I'm thinking

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: close.svg

Update with width/height and viewbox = 16

Comment 7

[Brian Grinstead [:bgrins]]

Deepjyoti, if you have a chance can you let me know if the attachment in Comment 6 fixes the position for the close button in the toolbox (and also in the two places that were updated in Bug 1276675)?

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: with-new-image.png

Here's what I see with the updated image in the toolbo

Comment 9

[Deepjyoti Mondal]

Attachment: notif.png

This is how the notification close button looks on applying the above(16X16) close.svg and setting background-size as cover.

Comment 10

[Deepjyoti Mondal]

Attachment: notification-close-icon.png

This screenshot shows how the close button looked before it was broken in bug 1264671. Here the cross does not occupy the entire button (There is a slight gap between the edge of the circle and the cross).
But in the previous screen shot the cross occupies the entire circle.I am not sure how much visible this difference is but if this can be ignored then I think the 16X16 close.svg is just fine.

Comment 11

[Brian Grinstead [:bgrins]]

Forwarding question to Helen.  Comment 8 and 9 are showing how it looks with the 16x16 background image + cover (i.e. the proposed change here), and Comment 10 is showing what it used to look like in the previous release before anything regressed.

Notice the space between the 'x' and the edge of the circle.  We could also position the background as 'center' and set no repeat to get the look to match the old UI.

Comment 12

[Deepjyoti Mondal]

Attachment: notif2.png

This screen shot shows the result after applying background-position : center and removing background-size : cover

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Deepjyoti Mondal from comment #12)
> Created attachment 8793464 [details]
> notif2.png
> 
> This screen shot shows the result after applying background-position :
> center and removing background-size : cover

That looks good to me, what do you think?  Mind submitting a patch for this?

Comment 14

[Deepjyoti Mondal]

Attachment: bug1303458.patch

That definitely looks good to me too. Here is the patch. Hope this helps.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8793475 [details] [diff] [review]
bug1303458.patch

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

This works for me, thanks.  Can you please update the commit message to be on one line and add r=bgrins at the end?

Comment 16

[Deepjyoti Mondal]

Attachment: bug1303458.patch

done!

Comment 17

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: close-expanded.svg

Sorry for not being more on the ball with this, and thanks for the quick fix Deepjyoti!

This one should be centered.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/190e45ee0115
Changed the dimensions of close.svg and centered the close button icon. r=bgrins

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/190e45ee0115

Comment 20

[Gerry Chang [:gchang]]

Hi Deepjyoti,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51 if this patch is not too risky?

Comment 21

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this bug using Fx 51.0a1, build ID: 20160916030204, on Windows 10 x64.
I can confirm the close button is aligned, I verified using Fx 52.0a1 build ID: 20160926030203, on Windows 10 x64, Ubuntu 14.04 LTS and Mac OS X 10.12.1.

Comment 22

[Deepjyoti Mondal]

Well, the issue has been fixed and the icon looks good now,its vertically aligned. So at present there is no more issue with the devtools close button or the notification-box close button. So I think its safe to uplift this for 51.Still I would like to forward this question to Honza as it is a regression caused by Bug 1276675.

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Deepjyoti Mondal from comment #22)
> Well, the issue has been fixed and the icon looks good now,its vertically
> aligned. So at present there is no more issue with the devtools close button
> or the notification-box close button. So I think its safe to uplift this for
> 51.Still I would like to forward this question to Honza as it is a
> regression caused by Bug 1276675.

Yes, I agree. This should be uplifted to 51.

Thanks!
Honza

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8793662 [details] [diff] [review]
bug1303458.patch

Approval Request Comment
[Feature/regressing bug #]: 1276675 
[User impact if declined]: The 'close' button in the devtools toolbox window will be off center.  
[Describe test coverage new/current, TreeHerder]: None, this is a CSS / image alignment change. See Comment 22
[Risks and why]: Change is limited to CSS within the devtools toolbox
[String/UUID change made/needed]:

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8793662 [details] [diff] [review]
bug1303458.patch

This patch fixes a UI regression. Take it in 51 aurora.

Comment 26

[Gerry Chang [:gchang]]

Hi :bgrins,
May I know why the patch is obsolete and you still ask for uplift?

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Gerry Chang [:gchang] from comment #26)
> Hi :bgrins,
> May I know why the patch is obsolete and you still ask for uplift?

I accidentally marked it as obsolete - undoing that now

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a5aaf227e0a4

Comment 29

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this bug using Fx 51.0a2, build ID: 0160930004005, on Windows 10 x64.
I can confirm the close button is aligned, I verified using Fx 51.0a2 build ID:20161003004005, on Windows 10 x64, Ubuntu 14.04 LTS and Mac OS X 10.11.

Cheers!

Comment 30

[Deepjyoti Mondal]

cool!!:)