Developer tools toolbox close button isn't vertically aligned anymore

VERIFIED FIXED in Firefox 51

Status

DevTools
Framework
VERIFIED FIXED
2 years ago
a month ago

People

(Reporter: MattN, Unassigned)

Tracking

({regression})

Trunk
Firefox 52
regression

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox-esr45 unaffected, firefox50 unaffected, firefox51 verified, firefox52 verified)

Details

Attachments

(7 attachments, 2 obsolete attachments)

Has Regression Range: --- → yes
Keywords: regression
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.
Flags: needinfo?(hholmes)
QA Contact: cristian.comorasu
(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
Flags: needinfo?(hholmes)
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.
Flags: needinfo?(hholmes)

Comment 4

2 years ago
What about tweaking the background-position of the image (close.svg) of the close button to vertically center it?
Flags: needinfo?(bgrinstead)
(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
Flags: needinfo?(bgrinstead)
Created attachment 8792642 [details]
close.svg

Update with width/height and viewbox = 16
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)?
Flags: needinfo?(djmdeveloper060796)
Created attachment 8792643 [details]
with-new-image.png

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

Comment 9

2 years ago
Created attachment 8792985 [details]
notif.png

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

Comment 10

2 years ago
Created attachment 8792989 [details]
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.
Flags: needinfo?(bgrinstead)
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.
Flags: needinfo?(bgrinstead)

Comment 12

2 years ago
Created attachment 8793464 [details]
notif2.png

This screen shot shows the result after applying background-position : center and removing background-size : cover
Flags: needinfo?(bgrinstead)
(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?
Flags: needinfo?(bgrinstead)

Comment 14

2 years ago
Created attachment 8793475 [details] [diff] [review]
bug1303458.patch

That definitely looks good to me too. Here is the patch. Hope this helps.
Attachment #8793475 - Flags: review?(bgrinstead)
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?
Attachment #8793475 - Flags: review?(bgrinstead) → review+

Comment 16

2 years ago
Created attachment 8793662 [details] [diff] [review]
bug1303458.patch

done!
Attachment #8793662 - Flags: review?(bgrinstead)
Created attachment 8793741 [details]
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.
Attachment #8792642 - Attachment is obsolete: true
Flags: needinfo?(hholmes)
Attachment #8793662 - Flags: review?(bgrinstead) → review+

Comment 18

2 years ago
Pushed by bgrinstead@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/190e45ee0115
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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?
Flags: needinfo?(djmdeveloper060796)
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.
status-firefox52: fixed → verified

Comment 22

2 years ago
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.
Flags: needinfo?(djmdeveloper060796) → needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
Attachment #8793475 - Attachment is obsolete: true
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]:
Attachment #8793662 - Attachment is obsolete: true
Attachment #8793662 - Flags: approval-mozilla-aurora?
Comment on attachment 8793662 [details] [diff] [review]
bug1303458.patch

This patch fixes a UI regression. Take it in 51 aurora.
Attachment #8793662 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi :bgrins,
May I know why the patch is obsolete and you still ask for uplift?
Flags: needinfo?(bgrinstead)
(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
Flags: needinfo?(bgrinstead)
Attachment #8793662 - Attachment is obsolete: false

Comment 28

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/a5aaf227e0a4
status-firefox51: affected → fixed
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!
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified

Comment 30

2 years ago
cool!!:)

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.