Closed
Bug 1233701
Opened 9 years ago
Closed 9 years ago
Hello button has increased height (so navigation toolbar is 42px in height instead of 40px)
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(firefox46+ fixed)
VERIFIED
FIXED
People
(Reporter: arni2033, Assigned: dcritchley)
References
()
Details
(Keywords: regression)
Attachments
(2 files)
>>> My Info: Win7_64, Nightly 46, 32bit, ID 20151217030207
STR:
1. Create new profile, launch it
2. Right-click Hello button on navigation toolbar, click "Move to menu"
Result: Navigation toolbar height reduced by 2 pixels
Expectations: It should always have height == 40px (content-box)
Regressed by bug 1223573
> pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f48352b311eb120a09347efce60ae9045401858c&tochange=19876a153a009b457900c82f27efbc398ad19413
I'm just VERY nervous about that insanely large toolbar. I mean, "isn't it thick enough already?!"
I was close to think that it's made intentionally! Dear G-d... Firefox has been completely regressed. Even on devtools theme navigation toolbox is several pixels thicker than one in GoogleChrome
Updated•9 years ago
|
Component: Theme → Client
Priority: -- → P1
Product: Firefox → Hello (Loop)
Version: Trunk → unspecified
Updated•9 years ago
|
Rank: 18
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dcritchley
Assignee | ||
Comment 2•9 years ago
|
||
I believe I found the culprit here, some css, in the browser.css for windows, that was fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1179295 added this:
#nav-bar .toolbarbutton-1:not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-icon,
#nav-bar .toolbarbutton-1:not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-badge-stack,
#nav-bar .toolbarbutton-1:not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
padding: calc(var(--toolbarbutton-vertical-inner-padding) + 1px) 7px;
}
The "+ 1px" padding is adding to the height of the hello button, whereas, other buttons in the toolbar only have "padding: calc(var(--toolbarbutton-vertical-inner-padding)) 7px;"
Should we fix this here or create an overriding class to remove the "+ 1px"?
Flags: needinfo?(standard8)
Comment 4•9 years ago
|
||
This is your button being treated as a non-default button. Non-default buttons being treated differently is correct. Please don't remove the "+ 1px". If you want to continue to use an 18x18px icon for Hello (not necessary, I believe, as the actual non-transparent content fit in a 16x16px box, last I checked), you should override the rule just for Hello.
When fixing this, please check what's happening on non-Windows, too.
Assignee | ||
Comment 5•9 years ago
|
||
That appears to be what needs to be done. Since Hello was removed from the default toolbar list, the icons need to be resized down to 16x16 from 18x18.
Michael, can we resize these images down?
If not, I can fall back to overriding the CSS for the Loop icon.
Flags: needinfo?(mmaslaney)
Comment 6•9 years ago
|
||
(In reply to David Critchley (:dcritch) from comment #5)
> That appears to be what needs to be done. Since Hello was removed from the
> default toolbar list, the icons need to be resized down to 16x16 from 18x18.
> Michael, can we resize these images down?
> If not, I can fall back to overriding the CSS for the Loop icon.
FWIW, I think in the existing spritesheet, a 16x16 box can capture the entirety of the icon as it is. The outside 1px all around it (making a 2px size difference in both directions) is transparent. So technically, you don't need a new sprite, you would just need to update the rect/region we use from the icon. So instead of:
-moz-image-region: rect(0, 18px, 18px, 0);
from: https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/browser/extensions/loop/skin/shared/loop.css#21
you want:
-moz-image-region: rect(1px, 17px, 17px, 1px);
Of course, it's probably neater to update the raw image files, so you can use 0,16,16,0 instead -- but at least the icons don't need actual resizing.
Updated•9 years ago
|
Rank: 18 → 12
Comment 7•9 years ago
|
||
Hey David, can you use the steps that Gijs has provided in comment #6 to land a fix for this without new icons? We shouldn't be shipping this to users in this state, and we are getting close to this merging in to Beta at which point it will be harder to fix.
Flags: needinfo?(dcritchley)
Assignee | ||
Comment 8•9 years ago
|
||
Will hopefully have this fixed within the next couple days.
Flags: needinfo?(dcritchley)
Comment 9•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8709763 -
Flags: review?(standard8)
Attachment #8709763 -
Flags: review?(edilee)
Attachment #8709763 -
Flags: review?(dmose)
Comment 11•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #4)
> When fixing this, please check what's happening on non-Windows, too.
Thanks for the quick patch David. Were you able to test on non-Windows?
Flags: needinfo?(dcritchley)
Assignee | ||
Comment 12•9 years ago
|
||
Yes, I tested both on a Windows box and my Mac, also Chris Rafuse tested on Linux for me.
Flags: needinfo?(dcritchley)
Comment 13•9 years ago
|
||
Thanks!
Comment 14•9 years ago
|
||
Comment on attachment 8709763 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/93
I've done a few extra checks on Mac/Windows and it looks good. r=Standard8
Attachment #8709763 -
Flags: review?(standard8)
Attachment #8709763 -
Flags: review?(edilee)
Attachment #8709763 -
Flags: review?(dmose)
Attachment #8709763 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 17•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #16)
> Mark, is this on track to make Firefox 45 ?
This is already fixed in development versions of the add-on. We're preparing to land an updated version in trees today, and get it uplifted soon after.
Flags: needinfo?(standard8)
Comment 20•9 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #19)
> Mark was this already fixed in 46?
No, we're going to be shipping it in the 1.3 update that we're planning to release just after 46.
Flags: needinfo?(standard8)
Comment 21•9 years ago
|
||
[Tracking Requested - why for this release]:
[Tracking Requested - why for this release]:
Tracking for 46. we'll mark it fixed for 46 once Hello 1.3 ships.
Comment 22•9 years ago
|
||
This is rolling out to release now, marking fixed.
You need to log in
before you can comment on or make changes to this bug.
Description
•