Closed Bug 1141538 Opened 9 years ago Closed 4 years ago

Badges on toolbar should be positioned lower when using Developer Edition theme.

Categories

(Firefox :: Theme, defect, P2)

All
Windows 8.1
defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: kasper93, Unassigned)

References

Details

(Whiteboard: [devedition-polish][polish-backlog][difficulty=easy])

Attachments

(4 files, 3 obsolete files)

Hi,

Please take a look at those images:
http://i.imgbox.com/UpFJK5SM.png
http://i.imgbox.com/ynM15SGQ.png

In my opinion badges should have at least few pixel padding from title bar. On normal Firefox theme toolbar is higher therefor everything fits nicely.

I'm not sure what would be the best approach to resolve this issue. I think position badges few pixels lower would work.

I noticed also one more thing, "update" badge is on top title bar, while uBlock's badge is truncated. We are talking one pixel here, but still it is surprising that the difference exist in the first place.

Best Regards,
Kacper
Brian, thoughts about prioritizing this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bgrinstead)
Note: figured out the easiest way to reproduce this on a browser that doesn't need updating is by setting badge="⬆" on the #PanelUI-menu-button element
(In reply to :Gijs Kruitbosch from comment #1)
> Brian, thoughts about prioritizing this?

Because of the smaller toolbar in the devedition theme, we will need to change some styling for badged buttons.  To give some space below the title bar, we could do any of these (or a combination of them):

1) Shrink the badge (using `font-size`)
2) Position the badge further down over the icon (using `top`)
3) Limit the top/bottom padding on the badge

This needs some UX help - Stephen, what do you think we should do?

Relevant lines:
https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/toolbarbutton.css#85
https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/toolbarbutton.css#161
https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/toolbarbutton.css#114
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #3)
> Because of the smaller toolbar in the devedition theme, we will need to
> change some styling for badged buttons.  To give some space below the title
> bar, we could do any of these (or a combination of them):
> 
> 1) Shrink the badge (using `font-size`)
> 2) Position the badge further down over the icon (using `top`)
> 3) Limit the top/bottom padding on the badge
> 
> This needs some UX help - Stephen, what do you think we should do?

ni? for Comment 3
Flags: needinfo?(shorlander)
Screenshot of the issue
Whiteboard: [devedition-polish]
Whiteboard: [devedition-polish] → [devedition-polish][devedition-40][difficulty=easy]
Priority: -- → P2
Assignee: nobody → zer0
Flags: needinfo?(bgrinstead)
removing the ni added – apparently – by mistake. Sorry Brian!
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Because of the smaller toolbar in the devedition theme, we will need to
> > change some styling for badged buttons.  To give some space below the title
> > bar, we could do any of these (or a combination of them):
> > 
> > 1) Shrink the badge (using `font-size`)
> > 2) Position the badge further down over the icon (using `top`)
> > 3) Limit the top/bottom padding on the badge
> > 
> > This needs some UX help - Stephen, what do you think we should do?
> 
> ni? for Comment 3

I would move it down a few pixels and see if that works out before altering the size or padding.
Flags: needinfo?(shorlander)
Attachment #8634710 - Flags: review?(bgrinstead)
Comment on attachment 8634710 [details] [diff] [review]
Lowered Badges on toolbar

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

Coincidentally you bumped into Bug 1029937 landing in front of this, which completely changed the way that the badges are done.  I'm not sure if we will still need to do work here, but please pull down the latest code and check to see if we do.  If so I think we will need to adjust styling on the .toolbarbutton-badge element.
Attachment #8634710 - Flags: review?(bgrinstead) → review-
Depends on: 1029937
Whiteboard: [devedition-polish][devedition-40][difficulty=easy] → [devedition-polish][polish-backlog][difficulty=easy]
Comment on attachment 8643053 [details] [diff] [review]
Badges on toolbar should be positioned lower when using Developer Edition theme.

Notice that this patch is supposed to work on top of bug 1029937 and bug 1188001.
Attachment #8643053 - Flags: review?(bgrinstead)
Comment on attachment 8643053 [details] [diff] [review]
Badges on toolbar should be positioned lower when using Developer Edition theme.

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

Note: I haven't reviewed this yet because I've been waiting for Bug 1188001 to land (or for the direction there to be agreed upon).  Seems like the approach here may depend on what happens there.
Attachment #8643053 - Flags: review?(bgrinstead)
The badge is currently almost completely covering the icon in 42.0a2 (20150817004010)
(In reply to Paul Heil from comment #13)
> Created attachment 8648864 [details]
> 41.0b1 badge vs 42.0a2 badge.png
> 
> The badge is currently almost completely covering the icon in 42.0a2
> (20150817004010)

If I had to guess this is fallout from Bug 1029937, and it looks like it makes the fix here much more important.  This bug is waiting on Bug 1188001 to be resolved.
(In reply to Brian Grinstead [:bgrins] from comment #14)

> If I had to guess this is fallout from Bug 1029937, and it looks like it
> makes the fix here much more important.  This bug is waiting on Bug 1188001
> to be resolved.

Considering that Bug 1188001 will makes the badge slightly bigger, in order to be closest to the original UX mockup, I would suggest to make the badge of Dev Edition a bit smaller in comparison
 However I'm afraid that the Dev Edition badge will cover the button's icons more than the regular theme, 'cause we need to lower them, and the button's area also seems smaller.
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #15)
> (In reply to Brian Grinstead [:bgrins] from comment #14)
> 
> > If I had to guess this is fallout from Bug 1029937, and it looks like it
> > makes the fix here much more important.  This bug is waiting on Bug 1188001
> > to be resolved.
> 
> Considering that Bug 1188001 will makes the badge slightly bigger, in order
> to be closest to the original UX mockup, I would suggest to make the badge
> of Dev Edition a bit smaller in comparison
>  However I'm afraid that the Dev Edition badge will cover the button's icons
> more than the regular theme, 'cause we need to lower them, and the button's
> area also seems smaller.

Could we make the badges smaller in the DE theme?
(In reply to Brian Grinstead [:bgrins] from comment #16)

> > Considering that Bug 1188001 will makes the badge slightly bigger, in order
> > to be closest to the original UX mockup, I would suggest to make the badge
> > of Dev Edition a bit smaller in comparison
> >  However I'm afraid that the Dev Edition badge will cover the button's icons
> > more than the regular theme, 'cause we need to lower them, and the button's
> > area also seems smaller.
 
> Could we make the badges smaller in the DE theme?

Yes, that what I've proposed. But as said, they will probably going to cover the button more than regular theme anyway; 'cause the button area in DE theme is smaller as well.
Also, we can't reduce the font's size; so the only thing we can do it's remove as much as padding we can.
Attachment #8634710 - Attachment is obsolete: true
Attachment #8643053 - Attachment is obsolete: true
Comment on attachment 8679924 [details] [diff] [review]
Badges on toolbar should be positioned lower when using Developer Edition theme

I slightly reduced the minimum width of the badges; in addition to lower them. However I'm afraid we can't do more than that, mostly because badges with text, reducing more the font's size would make them unreadable.

We could reduce the size of the badge's with image, like the update's one, that are the one the users will see more; but then will face some inconsistency with text based badges.
Attachment #8679924 - Flags: review?(bgrinstead)
Comment on attachment 8679924 [details] [diff] [review]
Badges on toolbar should be positioned lower when using Developer Edition theme

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

I'm assuming this should work fine across all platforms based on:
https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/toolbarbutton.css#153
https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/toolbarbutton.css#113
https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/toolbarbutton.css#79

I'd like to test this on windows before r+, I will look at that tomorrow.

In the meantime, one thing I notice on osx is that the badge becomes bigger and moves around a bit when the window is unfocused, so there may be some styles that aren't being overridden in that case, maybe on :-moz-window-inactive although I don't see anything that looks too suspicious there.

::: browser/themes/shared/devedition.inc.css
@@ +256,5 @@
>  }
>  
> +/* Lower badges in nav-bar due the lack of extra vertical padding */
> +#nav-bar .toolbarbutton-badge {
> +  min-width:12px;

Nit: spacing

min-width: 12px;
margin-top: -4px !important;
Attached image win7-update-badge.png
Screenshot of badge in default and DE theme with the patch applied - it's not squished against the top of the nav bar in DE anymore.  Stephen, what do you think?
Attachment #8682161 - Flags: ui-review?(shorlander)
Comment on attachment 8679924 [details] [diff] [review]
Badges on toolbar should be positioned lower when using Developer Edition theme

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

Tested this on Windows 7 and it looks good.  Code changes are fine with me, let's wait on ux-review before landing.  Can you either investigate the inactive window thing in Comment 20, or file a follow up bug to track that work?

::: browser/themes/shared/devedition.inc.css
@@ +256,5 @@
>  }
>  
> +/* Lower badges in nav-bar due the lack of extra vertical padding */
> +#nav-bar .toolbarbutton-badge {
> +  min-width:12px;

Nit: spacing

min-width: 12px;
margin-top: -4px !important;
Attachment #8679924 - Flags: review?(bgrinstead) → review+
Comment on attachment 8682161 [details]
win7-update-badge.png

Looks good to me. Thank you!
Attachment #8682161 - Flags: ui-review?(shorlander) → ui-review+
(In reply to Brian Grinstead [:bgrins] from comment #20)

> In the meantime, one thing I notice on osx is that the badge becomes bigger
> and moves around a bit when the window is unfocused, so there may be some
> styles that aren't being overridden in that case, maybe on
> :-moz-window-inactive although I don't see anything that looks too
> suspicious there.

I think this is a regression that is happening since the badge implementation changed. We got a lot of them; apparently in this case the border we add expands the size of the badge, even `box-sizing` seems ignored. An easy fix seems to reduce the padding by 1px in the -moz-window-inactive rule. I'm going to add that on the coming patch.
Attachment #8679924 - Attachment is obsolete: true
Comment on attachment 8688453 [details] [diff] [review]
Badges on toolbar should be positioned lower when using Developer Edition theme

Asking for a new review 'cause the inactive's badge bug.
Attachment #8688453 - Flags: review?(bgrinstead)
Comment on attachment 8688453 [details] [diff] [review]
Badges on toolbar should be positioned lower when using Developer Edition theme

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

This looks it fixes both the default and DE theme and prevents the update badge from appearing to move when the window becomes unfocused in both.  I'm not sure if I should review the toolkit changes, so getting a second opinion from Gijs about if this fix belongs in toolkit or in DE theme.
Attachment #8688453 - Flags: review?(gijskruitbosch+bugs)
Attachment #8688453 - Flags: review?(bgrinstead)
Attachment #8688453 - Flags: review+
Comment on attachment 8688453 [details] [diff] [review]
Badges on toolbar should be positioned lower when using Developer Edition theme

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

Sorry, I haven't had time to look at this today. It'll likely be tomorrow.
Comment on attachment 8688453 [details] [diff] [review]
Badges on toolbar should be positioned lower when using Developer Edition theme

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

::: toolkit/themes/osx/global/toolbarbutton.css
@@ +98,5 @@
>    background-color: rgb(230,230,230);
>    box-shadow: none;
>    border: 1px solid rgb(206,206,206);
>    color: rgb(192,192,192);
> +  padding: 0px 1px;

I'm worried this will get out of sync, or that other (add-on?) code overrides the padding but then doesn't do so for the -moz-window-inactive styling and things go wonky. How about we add:

border: 1px solid transparent;

to the normal styling, reduce the padding there, and then make -moz-window-inactive only set the border-color to rgb(206,206,206); ? That seems to fix the issue just as well for me, and is more likely to not be a "jumpy icon" if people change either the border or the padding of the badge.
Attachment #8688453 - Flags: review?(gijskruitbosch+bugs) → review-
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: