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)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: kasper93, Unassigned)
References
Details
(Whiteboard: [devedition-polish][polish-backlog][difficulty=easy])
Attachments
(4 files, 3 obsolete files)
36.05 KB,
image/png
|
Details | |
10.57 KB,
image/png
|
Details | |
301.43 KB,
image/png
|
shorlander
:
ui-review+
|
Details |
1.57 KB,
patch
|
bgrins
:
review+
Gijs
:
review-
|
Details | Diff | Splinter Review |
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
Comment 1•9 years ago
|
||
Brian, thoughts about prioritizing this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bgrinstead)
Comment 2•9 years ago
|
||
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
Comment 3•9 years ago
|
||
(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)
Comment 4•9 years ago
|
||
(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)
Comment 5•9 years ago
|
||
Screenshot of the issue
Updated•9 years ago
|
Whiteboard: [devedition-polish]
Updated•9 years ago
|
Whiteboard: [devedition-polish] → [devedition-polish][devedition-40][difficulty=easy]
Updated•9 years ago
|
Priority: -- → P2
Updated•9 years ago
|
Assignee: nobody → zer0
Flags: needinfo?(bgrinstead)
Comment 6•9 years ago
|
||
removing the ni added – apparently – by mistake. Sorry Brian!
Flags: needinfo?(bgrinstead)
Comment 7•9 years ago
|
||
(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)
Comment 8•9 years ago
|
||
Updated•9 years ago
|
Attachment #8634710 -
Flags: review?(bgrinstead)
Comment 9•9 years ago
|
||
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-
Updated•9 years ago
|
Whiteboard: [devedition-polish][devedition-40][difficulty=easy] → [devedition-polish][polish-backlog][difficulty=easy]
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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)
Comment 13•9 years ago
|
||
The badge is currently almost completely covering the icon in 42.0a2 (20150817004010)
Comment 14•9 years ago
|
||
(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.
Comment 15•9 years ago
|
||
(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.
Comment 16•9 years ago
|
||
(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?
Comment 17•9 years ago
|
||
(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.
Updated•9 years ago
|
Blocks: de-44-polish
Comment 18•9 years ago
|
||
Updated•9 years ago
|
Attachment #8634710 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8643053 -
Attachment is obsolete: true
Comment 19•9 years ago
|
||
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 20•9 years ago
|
||
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;
Comment 21•9 years ago
|
||
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 22•9 years ago
|
||
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 23•9 years ago
|
||
Comment on attachment 8682161 [details]
win7-update-badge.png
Looks good to me. Thank you!
Attachment #8682161 -
Flags: ui-review?(shorlander) → ui-review+
Comment 24•9 years ago
|
||
(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.
Comment 25•9 years ago
|
||
Updated•9 years ago
|
Attachment #8679924 -
Attachment is obsolete: true
Comment 26•9 years ago
|
||
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 27•9 years ago
|
||
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 28•9 years ago
|
||
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 29•9 years ago
|
||
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-
Assignee: zer0 → nobody
Updated•4 years ago
|
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.
Description
•