[Australis] ">>" more icons button in toolbar is off-center

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Theme
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: liuche, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 28
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M9][Australis:P5])

Attachments

(4 attachments, 3 obsolete attachments)

Created attachment 767911 [details]
Screenshot: ">>" more icons not centered

>> button is off-center - see screenshot.

Updated

5 years ago
Blocks: 872617
Whiteboard: [Australis:M?]
(Assignee)

Comment 1

5 years ago
Chenxia, can you still reproduce this on current trunk? It works for me but I'm on retina, don't know if you were seeing this on non-retina...
Flags: needinfo?(liuche)
Seems better for me (also OSX Retina, though not quite perfect.)
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]
Created attachment 772361 [details]
Screenshot: nightly 25.0a1 screenshot

This is on the 25.0a1 (2013/7/08) build. Compared to the "customize" button, the >> arrows look a bit low, but perhaps that's the intent?

Definitely a style enhancement, not a blocker.
Flags: needinfo?(liuche)
Also, this is not on a retina - this a 2011 15" macbook pro running Lion.
Chenxia, I think this has been fixed now. Can you let me know if this bug still exists?
Flags: needinfo?(liuche)
I still see it, the >> being bottom-aligned, but since I don't see the ">>" button on any mocks, maybe that's just implicitly the design. Going to WONTFIX this bug, and someone can reopen it if it is actually a problem.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(liuche)
Resolution: --- → WONTFIX
That's not an implicit design. It is supposed to happen if the buttons on the toolbar don't fit.

Chenxia, what build are you running? Are you sure it is up to date?
Status: RESOLVED → REOPENED
Flags: needinfo?(liuche)
Resolution: WONTFIX → ---
I just updated before commenting, to try to repro. The build is the 2013-08-22 build, 26.0a1. This is running on Lion 10.7.5, and what I'm seeing still matches the screenshots.
Flags: needinfo?(liuche)
Created attachment 794153 [details]
Screenshot: nightly 26.0a1 screenshot
(Assignee)

Comment 10

4 years ago
Shifting the image up 2px fixes it for me on retina, but that should probably happen on the sprite rather than using CSS (it is already in the sprite as an 18x18/36x36px image, like all the other icons). Something for Stephen?
(Assignee)

Comment 11

4 years ago
(In reply to :Gijs Kruitbosch from comment #10)
> Shifting the image up 2px fixes it for me on retina, but that should
> probably happen on the sprite rather than using CSS (it is already in the
> sprite as an 18x18/36x36px image, like all the other icons). Something for
> Stephen?

Stephen, can you add this to your list of stuff to be fixed in the new sprites? Thanks! :-)
Flags: needinfo?(shorlander)
(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to :Gijs Kruitbosch from comment #10)
> > Shifting the image up 2px fixes it for me on retina, but that should
> > probably happen on the sprite rather than using CSS (it is already in the
> > sprite as an 18x18/36x36px image, like all the other icons). Something for
> > Stephen?
> 
> Stephen, can you add this to your list of stuff to be fixed in the new
> sprites? Thanks! :-)

It is already vertically centered in the sprite sheet. Seems like something else is going on there.
Flags: needinfo?(shorlander)
(Assignee)

Comment 13

4 years ago
Created attachment 818363 [details] [diff] [review]
center overflow chevron vertically,

Hrmpf. This is one way to do this.
Attachment #818363 - Flags: review?(jaws)
(Assignee)

Updated

4 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
(Assignee)

Comment 14

4 years ago
Created attachment 818370 [details] [diff] [review]
make places chevron styling more specific,

Or we could do this. I like this better, but if any add-on is counting on this toolbarbutton class, we would be breaking them, so maybe the first is better? Then again, this patch saves us a bunch of ugly !important styling.
Attachment #818370 - Flags: review?(mak77)
Comment on attachment 818363 [details] [diff] [review]
center overflow chevron vertically,

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

Seems weird, is there a margin-top on all buttons here or just the overflow-button? Does this bug not exist for hidpi?
(Assignee)

Comment 16

4 years ago
(In reply to Jared Wein [:jaws] from comment #15)
> Comment on attachment 818363 [details] [diff] [review]
> center overflow chevron vertically,
> 
> Review of attachment 818363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems weird, is there a margin-top on all buttons here or just the
> overflow-button? Does this bug not exist for hidpi?

There's a margin-top on toolbarbutton.chevron set by the bookmarks-related code in browser.css. This also affects hidpi.
Sorry, I meant to say that it looks like your patch is only for hidpi, what about non-hidpi?
Comment on attachment 818363 [details] [diff] [review]
center overflow chevron vertically,

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

I see now where the margin is being set. This patch only fixes HiDPI but the correct patch should fix non-HiDPI as well.
Attachment #818363 - Flags: review?(jaws) → review-
(Assignee)

Comment 19

4 years ago
Created attachment 819017 [details] [diff] [review]
center overflow chevron vertically,

Err, so this is what it should have been. Still think we should just fix the bookmarks styling though (ie take the other patch).
Attachment #819017 - Flags: review?(jaws)
Comment on attachment 819017 [details] [diff] [review]
center overflow chevron vertically,

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

This patch is fine by me, but maybe you'd like to talk to Marco about the other patch and get his opinion on which one to take.
Attachment #819017 - Flags: review?(jaws) → review+
(Assignee)

Updated

4 years ago
Attachment #818363 - Attachment is obsolete: true
Comment on attachment 818370 [details] [diff] [review]
make places chevron styling more specific,

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

::: browser/themes/linux/browser.css
@@ +1550,1 @@
>  }

there is something escaping me...

first: why does nav-bar-overflow-button have "chevron" class? While it sounds "correct" that class was actually just created for the Places toolbar (call that a "too much generic name" bug).
second: In this patch you are removing any toolbarbutton.chevron styling and replacing it with specific Places styling, wouldn't be simpler to just remove "chevron" class from nav-bar-overflow-button then?

I initially thought the idea was to use chevron class to decorate both buttons, but the patch doesn't seem to do that.

That said, I don't see reasons to not change from toolbarbutton.chevron to #PlacesChevron and kill the chevron class completely, if not for backwards compatibility... Though Australis is already breaking most stuff, so may be the right time to do that.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 22

4 years ago
Created attachment 822261 [details] [diff] [review]
don't use chevron class, it only gets us trouble,

Marco makes a good point. I looked at the styles on Windows and OS X, and as far as I can tell, this is much the simpler way of dealing with this.
Attachment #822261 - Flags: review?(jaws)
(Assignee)

Comment 23

4 years ago
Comment on attachment 818370 [details] [diff] [review]
make places chevron styling more specific,

I'm marking this obsolete here... I don't think we should necessarily not do it (IMO it makes the CSS a little cleaner to just use an ID selector rather than nodename+class) but I'm not too fussed about it.
Attachment #818370 - Attachment is obsolete: true
Attachment #818370 - Flags: review?(mak77)
(Assignee)

Updated

4 years ago
Attachment #819017 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #822261 - Flags: review?(jaws) → review+
(Assignee)

Comment 24

4 years ago
https://hg.mozilla.org/projects/ux/rev/577731dedb4d
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M9][Australis:P5][fixed-in-ux]
(Assignee)

Comment 25

4 years ago
https://hg.mozilla.org/mozilla-central/rev/577731dedb4d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P5][fixed-in-ux] → [Australis:M9][Australis:P5]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.