Closed
Bug 1303458
Opened 8 years ago
Closed 8 years ago
Developer tools toolbox close button isn't vertically aligned anymore
Categories
(DevTools :: Framework, defect)
DevTools
Framework
Tracking
(firefox48 unaffected, firefox49 unaffected, firefox-esr45 unaffected, firefox50 unaffected, firefox51 verified, firefox52 verified)
VERIFIED
FIXED
Firefox 52
Tracking | Status | |
---|---|---|
firefox48 | --- | unaffected |
firefox49 | --- | unaffected |
firefox-esr45 | --- | unaffected |
firefox50 | --- | unaffected |
firefox51 | --- | verified |
firefox52 | --- | verified |
People
(Reporter: MattN, Unassigned)
References
Details
(Keywords: regression)
Attachments
(7 files, 2 obsolete files)
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
Reporter | ||
Updated•8 years ago
|
Has Regression Range: --- → yes
Keywords: regression
Comment 1•8 years ago
|
||
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)
Updated•8 years ago
|
QA Contact: cristian.comorasu
Comment 2•8 years ago
|
||
(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)
Comment 3•8 years ago
|
||
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•8 years ago
|
||
What about tweaking the background-position of the image (close.svg) of the close button to vertically center it?
Flags: needinfo?(bgrinstead)
Comment 5•8 years ago
|
||
(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)
Comment 6•8 years ago
|
||
Update with width/height and viewbox = 16
Comment 7•8 years ago
|
||
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)
Comment 8•8 years ago
|
||
Here's what I see with the updated image in the toolbo
Comment 9•8 years ago
|
||
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•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
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•8 years ago
|
||
This screen shot shows the result after applying background-position : center and removing background-size : cover
Flags: needinfo?(bgrinstead)
Comment 13•8 years ago
|
||
(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•8 years ago
|
||
That definitely looks good to me too. Here is the patch. Hope this helps.
Attachment #8793475 -
Flags: review?(bgrinstead)
Comment 15•8 years ago
|
||
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 17•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8793662 -
Flags: review?(bgrinstead) → review+
Comment 18•8 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment 20•8 years ago
|
||
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)
Comment 21•8 years ago
|
||
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•8 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)
Comment 23•8 years ago
|
||
(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)
Updated•8 years ago
|
Attachment #8793475 -
Attachment is obsolete: true
Comment 24•8 years ago
|
||
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 25•8 years ago
|
||
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+
Comment 26•8 years ago
|
||
Hi :bgrins,
May I know why the patch is obsolete and you still ask for uplift?
Flags: needinfo?(bgrinstead)
Comment 27•8 years ago
|
||
(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)
Updated•8 years ago
|
Attachment #8793662 -
Attachment is obsolete: false
Comment 28•8 years ago
|
||
bugherder uplift |
Comment 29•8 years ago
|
||
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
Comment 30•8 years ago
|
||
cool!!:)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•