Closed Bug 1188001 Opened 4 years ago Closed 4 years ago

Badges on toolbar seems misplaced depends by the OS

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox39 --- unaffected
firefox40 --- unaffected
firefox41 --- unaffected
firefox42 + fixed
firefox43 + fixed

People

(Reporter: zer0, Assigned: zer0, NeedInfo)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(5 files, 3 obsolete files)

Attached image screenshot.png
See the attachment: on Linux (left image), the badge are lower than where they should be; where in OS X they're probably higher. I didn't test on Windows yet, but I would expect the same result of Linux. The right position should be the one seen in the first button of this attachment: https://bug994280.bmoattachments.org/attachment.cgi?id=8472301 (based on the original UX mockup)
Assignee: nobody → zer0
Blocks: 1141538
Keywords: regression
Does this happen on release/beta/aurora or did this only regress on trunk (yet) ? Do we know what regressed this?
Flags: needinfo?(zer0)
The regression was caused by bug 1029937 – so, any Firefox version that has such patch applied. I believe the specific cause was similar reason of bug 1187846: the usage of XUL stack / label changed a bit the look & feel.
Flags: needinfo?(zer0)
[Tracking Requested - why for this release]:
Visible UI regression, we shouldn't ship with this.
Sorry, I didn't think to check the before and after appearance of the badge.

On Windows, I was able to get pretty close to the before appearance using the following tweaks:
* Remove the top and bottom padding
* Increase the top margin offset to -6px
* Increase the end margin offset to -8px
* Add 4px to the minimum width

I guess similar tweaks would apply to the other OSes.
(In reply to neil@parkwaycc.co.uk from comment #4)

> I guess similar tweaks would apply to the other OSes.

On OSX and Linux seems slightly different. I'm working on a patch for all of them (I'm not able to test on Windows 10 atm, but I'm setting a machine for that).
Tracking for 42 because visible U.I. regression.
Attachment #8643041 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8643041 [details] [diff] [review]
Badges on toolbar seems misplaced depends by the OS

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

The height of the badge is now larger than it used to be, at least on Windows. You should probably fix that, too.
Attachment #8643041 - Flags: review?(gijskruitbosch+bugs)
Attachment #8643041 - Attachment is obsolete: true
Attachment #8649314 - Flags: review?(gijskruitbosch+bugs)
Attached image badge.png
I was able to find the original mockup Stephen sent to me – the first one from the left – so I'm attach it to this bug. I also added a screenshot of each OSes with the patch applied as comparison.

Please notice that the font in UX mockup wasn't correct 'cause Stephen told me he couldn't use "-moz-dialog" in the mockup; but the size of the badge it is.
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #10)
> Created attachment 8649315 [details]
> badge.png
> 
> I was able to find the original mockup Stephen sent to me – the first one
> from the left – so I'm attach it to this bug. I also added a screenshot of
> each OSes with the patch applied as comparison.
> 
> Please notice that the font in UX mockup wasn't correct 'cause Stephen told
> me he couldn't use "-moz-dialog" in the mockup; but the size of the badge it
> is.

Did you intentionally only change the font-weight on Linux? Why?
Flags: needinfo?(zer0)
Comment on attachment 8649314 [details] [diff] [review]
Badges on toolbar seems misplaced depends by the OS

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

(going to wait with reviewing this for the answer to that question)
Attachment #8649314 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #11)

> Did you intentionally only change the font-weight on Linux? Why?

Yes, it was intentional. It's because every OS has different way to render the font and using the anti-alias, so in order to match as close as possible the look of the mockup – size of the badge, text, and proportion between the text and the badge – some adjustment is necessary. For the same reason the font size in OS X is 9 instead of 10.
Flags: needinfo?(zer0)
Attachment #8649314 - Flags: review?(gijskruitbosch+bugs)
Attached image linux-bold.png
Here how it looks Linux's badge with the bold text. You can see that is adding pixels to the top, especially, reducing the space, and it seems bigger compare to the others and the mockup.
Please copy:

.toolbarbutton-badge-stack > .toolbarbutton-icon[label]:not([label=""]) {
  -moz-margin-end: 0;
}

from the Windows to the Linux stylesheet while we're here -- right now the button is lopsided (the icon has 2px end-margin).

This also shifts the badge. I think that's fine, it makes Linux match Windows, apart from the font.

Still reviewing.
Comment on attachment 8649314 [details] [diff] [review]
Badges on toolbar seems misplaced depends by the OS

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

This still leads to a 15px-high badge on Windows, instead of 13px. I don't actually care very much, and I don't see a way to fix it (max-height leads to cut-offs, reducing the line-height has no effect, reducing the bottom padding looks wrong and isn't enough to get to 13px anyway, and in any case, while this all works with a badge of "1", I bet using text in the badge with descenders and ascenders would cause issues).

So I'm just going to r+ this, but you probably want ui-review from shorlander.

::: toolkit/themes/linux/global/toolbarbutton.css
@@ +109,5 @@
>  
>  .toolbarbutton-badge {
>    background-color: #d90000;
>    font-size: 10px;
> +  font-weight: normal;

This is the default, so you don't need it.
Attachment #8649314 - Flags: ui-review?(shorlander)
Attachment #8649314 - Flags: review?(gijskruitbosch+bugs)
Attachment #8649314 - Flags: review+
Comment on attachment 8649314 [details] [diff] [review]
Badges on toolbar seems misplaced depends by the OS

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

(In reply to :Gijs Kruitbosch from comment #16)
> This still leads to a 15px-high badge on Windows, instead of 13px. I don't
> actually care very much, and I don't see a way to fix it (max-height leads
> to cut-offs, reducing the line-height has no effect, reducing the bottom
> padding looks wrong and isn't enough to get to 13px anyway, and in any case,
> while this all works with a badge of "1", I bet using text in the badge with
> descenders and ascenders would cause issues).

Looks good to me.

13px vs 15px doesn't matter that much here I think as long as they are consistently the same size. The badge is mostly for numbers or symbols so I don't think ascenders or descenders are that much of a concern.
Attachment #8649314 - Flags: ui-review?(shorlander) → ui-review+
(In reply to :Gijs Kruitbosch from comment #16)

> This still leads to a 15px-high badge on Windows, instead of 13px.

I tested on Windows 8.1, and the badge's height is 13px (14px with shadow), as is shown in the screenshot I attached. If your results are different, as it seems, could you please add a screenshot? Just to understand if there is any visible thing that could indicate such difference between my build and yours (font, the way the anti alias is applied, etc). Probably we're not going to apply any changes, as you said, it's just that this thing is puzzling me and I don't see why we should got two different result.
Flags: needinfo?(gijskruitbosch+bugs)
http://i.imgur.com/ONPpidw.png
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8651076 - Flags: ui-review+
Attachment #8651076 - Flags: review+
Attachment #8649314 - Attachment is obsolete: true
Comment on attachment 8651076 [details] [diff] [review]
Badges on toolbar seems misplaced depends by the OS

Forgot to remove the `font-weight: normal`, so marking this as obsolete.
Attachment #8651076 - Attachment is obsolete: true
Comment on attachment 8651079 [details] [diff] [review]
Badges on toolbar seems misplaced depends by the OS

The new patch as the rule copied from Windows to Linux as for comment 15.

I also think I figured out why we have different height between me and Gijs. It seems that Stephen was right all the way:

Aug 05 16:58:27 <shorlander>	Gijs, zer0: It’s based on the text height
Aug 05 16:58:57 <shorlander>	So if your fonts are scaled differently it will make it bigger/smaller
Aug 05 16:59:43 <Gijs>	it has a hardcoded line-height of 10px.

The XUL label, similar to Bug 1187846, where the min width and max width were ignored, seems not taking in account any line-height too. At least, if the label's content is set by `value` attribute.
There are also other bugs that mentioned this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1192214#c8

I wonder if it's possible to be fixed as was done for bug 1187846. In any case, as discussed here, it's not a blocker to land this patch.
Attachment #8651079 - Flags: ui-review+
Attachment #8651079 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/da1330574019
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
(In reply to :Gijs Kruitbosch from comment #15)
> Please copy:
> 
> .toolbarbutton-badge-stack > .toolbarbutton-icon[label]:not([label=""]) {
>   -moz-margin-end: 0;
> }
> 
> from the Windows to the Linux stylesheet while we're here -- right now the
> button is lopsided (the icon has 2px end-margin).

How does this make any sense? There's supposed to be room between the icon and the label, so the above code looks entirely backwards.

Presumably the problem with the Firefox menu button is that it has a label that we hide and that the icon doesn't get the margin overridden by browser.css like other buttons.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Dão Gottwald [:dao] from comment #26)
> (In reply to :Gijs Kruitbosch from comment #15)
> > Please copy:
> > 
> > .toolbarbutton-badge-stack > .toolbarbutton-icon[label]:not([label=""]) {
> >   -moz-margin-end: 0;
> > }
> > 
> > from the Windows to the Linux stylesheet while we're here -- right now the
> > button is lopsided (the icon has 2px end-margin).
> 
> How does this make any sense? There's supposed to be room between the icon
> and the label, so the above code looks entirely backwards.

The label has padding/margin that ensures there is room if the label is actually visible.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #27)
> (In reply to Dão Gottwald [:dao] from comment #26)
> > (In reply to :Gijs Kruitbosch from comment #15)
> > > Please copy:
> > > 
> > > .toolbarbutton-badge-stack > .toolbarbutton-icon[label]:not([label=""]) {
> > >   -moz-margin-end: 0;
> > > }
> > > 
> > > from the Windows to the Linux stylesheet while we're here -- right now the
> > > button is lopsided (the icon has 2px end-margin).
> > 
> > How does this make any sense? There's supposed to be room between the icon
> > and the label, so the above code looks entirely backwards.
> 
> The label has padding/margin that ensures there is room if the label is
> actually visible.

Does this mean http://hg.mozilla.org/mozilla-central/annotate/04b8c412d9f5/toolkit/themes/windows/global/toolbarbutton.css#l24 should be removed? I suspect it doesn't, and we actually want the extra room between the icon and the label when both are present.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Dão Gottwald [:dao] from comment #28)
> (In reply to :Gijs Kruitbosch from comment #27)
> > (In reply to Dão Gottwald [:dao] from comment #26)
> > > (In reply to :Gijs Kruitbosch from comment #15)
> > > > Please copy:
> > > > 
> > > > .toolbarbutton-badge-stack > .toolbarbutton-icon[label]:not([label=""]) {
> > > >   -moz-margin-end: 0;
> > > > }
> > > > 
> > > > from the Windows to the Linux stylesheet while we're here -- right now the
> > > > button is lopsided (the icon has 2px end-margin).
> > > 
> > > How does this make any sense? There's supposed to be room between the icon
> > > and the label, so the above code looks entirely backwards.
> > 
> > The label has padding/margin that ensures there is room if the label is
> > actually visible.
> 
> Does this mean
> http://hg.mozilla.org/mozilla-central/annotate/04b8c412d9f5/toolkit/themes/
> windows/global/toolbarbutton.css#l24 should be removed? I suspect it
> doesn't, and we actually want the extra room between the icon and the label
> when both are present.

I don't know. I do know that every single button in the main toolbar has a label attribute, and I wouldn't want all their icons to be asymmetrically padded for a label that doesn't show up on any of the main browser's window.

Really, all that happened here was bringing Linux in line with Windows, which already had this rule. If you still want to change something, can we take this discussion to a new (blocking) bug?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #29)
> (In reply to Dão Gottwald [:dao] from comment #28)
> > (In reply to :Gijs Kruitbosch from comment #27)
> > > (In reply to Dão Gottwald [:dao] from comment #26)
> > > > (In reply to :Gijs Kruitbosch from comment #15)
> > > > > Please copy:
> > > > > 
> > > > > .toolbarbutton-badge-stack > .toolbarbutton-icon[label]:not([label=""]) {
> > > > >   -moz-margin-end: 0;
> > > > > }
> > > > > 
> > > > > from the Windows to the Linux stylesheet while we're here -- right now the
> > > > > button is lopsided (the icon has 2px end-margin).
> > > > 
> > > > How does this make any sense? There's supposed to be room between the icon
> > > > and the label, so the above code looks entirely backwards.
> > > 
> > > The label has padding/margin that ensures there is room if the label is
> > > actually visible.
> > 
> > Does this mean
> > http://hg.mozilla.org/mozilla-central/annotate/04b8c412d9f5/toolkit/themes/
> > windows/global/toolbarbutton.css#l24 should be removed? I suspect it
> > doesn't, and we actually want the extra room between the icon and the label
> > when both are present.
> 
> I don't know. I do know that every single button in the main toolbar has a
> label attribute,

Yes, most of those are useful as we display them in other contexts. The menu button can't move elsewhere, though.

> and I wouldn't want all their icons to be asymmetrically
> padded for a label that doesn't show up on any of the main browser's window.

Yes, see the second part of what I wrote in comment 26: "Presumably the problem with the Firefox menu button is that [...] the icon doesn't get the margin overridden by browser.css like other buttons."

> Really, all that happened here was bringing Linux in line with Windows,
> which already had this rule. If you still want to change something, can we
> take this discussion to a new (blocking) bug?

It was already there on Windows, but that doesn't mean the rule isn't broken. I'll file a bug on it.
Depends on: 1198234
Depends on: 1198424
Depends on: 1198222
I've backed this out for causing bug 1198424
Please reopen the bug when you back it out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 43 → ---
(In reply to Dave Townsend [:mossop] from comment #32)
> I've backed this out for causing bug 1198424

This patch needs to be landed again. The current status – without this patch – is a UI regression; you can see that also the position and the size of the badge with the update icon is wrong (see the attachment) compare to how it should be (see badge.png).

So, bug 1198424 is not a regression that can be fixed by backing out this patch; it's another bug that has to be fixed on top of this patch.
Keywords: checkin-needed
Blocks: 1198424
No longer depends on: 1198424
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #35)
> Created attachment 8655338 [details]
> badge-update-screenshot.png
> 
> (In reply to Dave Townsend [:mossop] from comment #32)
> > I've backed this out for causing bug 1198424
> 
> This patch needs to be landed again. The current status – without this patch
> – is a UI regression; you can see that also the position and the size of the
> badge with the update icon is wrong (see the attachment) compare to how it
> should be (see badge.png).

Right, but we shouldn't land this if it causes bug 1198424 since that regression looks worse than the regression this bug is solving.
(In reply to Dave Townsend [:mossop] from comment #36)

> > This patch needs to be landed again. The current status – without this patch
> > – is a UI regression; you can see that also the position and the size of the
> > badge with the update icon is wrong (see the attachment) compare to how it
> > should be (see badge.png).

> Right, but we shouldn't land this if it causes bug 1198424 since that
> regression looks worse than the regression this bug is solving.

But in order to fix bug 1198424 properly, we need this one too. There is nothing wrong in this patch, that's why it should land. Currently we have the badge almost in the middle of the button, and we shouldn't ship like that.
I guess we can also land the patch I wrote in bug 1198424 before this one, and then this. Of course, doing that, the badge's look would be still wrong – not only the position, but also the `height` – until also this patch will land, 'cause the patch order is inverted, but at least the image in the button won't be repeated – due the `no-repeat` prop.
Note: needs to be landed in the same Nightly of bug 1198424
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
This did re-land on mozilla-central: https://hg.mozilla.org/mozilla-central/rev/ccfe369f1099

I notice 42 is also marked as affected. Would uplifting this patch fix the issue in 42?
Flags: needinfo?(zer0)
Depends on: 1202331
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #40)
> This did re-land on mozilla-central:
> https://hg.mozilla.org/mozilla-central/rev/ccfe369f1099
> 
> I notice 42 is also marked as affected. Would uplifting this patch fix the
> issue in 42?

I think so. Notice that also bug 1198424 will needs to be uplifted.
Flags: needinfo?(zer0)
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #41)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #40)
> > This did re-land on mozilla-central:
> > https://hg.mozilla.org/mozilla-central/rev/ccfe369f1099
> > 
> > I notice 42 is also marked as affected. Would uplifting this patch fix the
> > issue in 42?
> 
> I think so. Notice that also bug 1198424 will needs to be uplifted.

Please request approval on all the patches that need uplift to 42.
Flags: needinfo?(zer0)
Comment on attachment 8651079 [details] [diff] [review]
Badges on toolbar seems misplaced depends by the OS

Approval Request Comment
[Feature/regressing bug #]: 1029937
[User impact if declined]: Visible UI regression, the badge is misplaced, the size is wrong, and the position is not consistent across the platforms
[Describe test coverage new/current, TreeHerder]: No automated testing possible due to the visual nature of the bug, thus requires extensive QA on all platforms.
[Risks and why]: users will see a smaller badge located, in some OSes, almost at the center of the button. 
[String/UUID change made/needed]: None
Attachment #8651079 - Flags: approval-mozilla-aurora?
Flags: needinfo?(zer0)
Comment on attachment 8651079 [details] [diff] [review]
Badges on toolbar seems misplaced depends by the OS

Visual regression, taking it.
Attachment #8651079 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Reproduced with Nightly from 2015-07-27 under Ubuntu.
Everything looks good on latest 45.0a1 (from 2015-11-09), across platforms [1] → https://goo.gl/6Cs5Gx; including the one for 'Verify your account' (Sync).

Two questions though:
1. Both Update/Verify account badges vanish as soon as Menu Panel is opened, but 'Verify account' (Sync) badge appears after enter/exit customize mode. With Nightly from 2015-07-27 Update badge remains visible even after clicking on Menu Panel. Is this intended?
2. On beta branch, Update badge is not visible although updates are downloaded and ready to be applied. Is there any pref that I could switch?

Thanks in advance! 

[1] Mac OS X 10.11.1, Windows 7 64-bit and Ubuntu 14.04 32-bit
Flags: needinfo?(zer0)
Hi Ada, my apologies if you didn't hear from me sooner!

1. I think a bug was open for this issue: bug 1327517; still there is no activity on that.

2. Is this still happens? Let me know!
Flags: needinfo?(zer0) → needinfo?(adalucin)
You need to log in before you can comment on or make changes to this bug.