Hello button has increased height (so navigation toolbar is 42px in height instead of 40px)

VERIFIED FIXED

Status

defect
P1
normal
Rank:
12
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: arni2033, Assigned: dcritchley)

Tracking

({regression})

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46+ fixed)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
>>>   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
(Reporter)

Comment 1

4 years ago
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
Component: Theme → Client
Priority: -- → P1
Product: Firefox → Hello (Loop)
Version: Trunk → unspecified
Rank: 18
(Assignee)

Updated

4 years ago
Assignee: nobody → dcritchley
(Assignee)

Comment 2

4 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)
Yes, I think removing the 1px is reasonable.
Flags: needinfo?(standard8)

Comment 4

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

Updated

3 years ago
Has STR: --- → yes
(Assignee)

Comment 5

3 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

3 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.
Rank: 18 → 12

Updated

3 years ago
See Also: → 1232254
Blocks: 1232254
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

3 years ago
Will hopefully have this fixed within the next couple days.
Flags: needinfo?(dcritchley)
(Assignee)

Comment 10

3 years ago
Attachment #8709763 - Flags: review?(standard8)
Attachment #8709763 - Flags: review?(edilee)
Attachment #8709763 - Flags: review?(dmose)
(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

3 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 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

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Duplicate of this bug: 1245732

Comment 16

3 years ago
Mark, is this on track to make Firefox 45 ?
Flags: needinfo?(standard8)
(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)
(Reporter)

Updated

3 years ago
Depends on: 1253973
Depends on: 1257672
Moving the icons request over to bug 1257672
Flags: needinfo?(mmaslaney)
Mark was this already fixed in 46?
Flags: needinfo?(standard8)
(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)
[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.
This is rolling out to release now, marking fixed.
(Reporter)

Updated

3 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.