633400 - Vertically align geolocation "Learn More..." link with menubutton

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[:Margaret Leibovic]

Attachment: patch

Until we fix bug 615483, we should at least make the "Learn More.." link alignment look a little more polished. Also, on Linux the use of padding instead of margin to align the link is resulting in ugly focus styles.

Right now the themes treat this link differently, and this patch updates gnomestripe and pinstripe to do what winstripe does (although I had to add a -moz-margin-start: 0; to pinstripe to left-align the link correctly).

I will attach screenshots of the changes.

Comment 1

[:Margaret Leibovic]

Attachment: osx changes

Comment 2

[:Margaret Leibovic]

Attachment: linux changes

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 511581 [details] [diff] [review]
patch

Can you add comments that describe where the values come from? (match the toolkit notification button margin for winstripe, match toolkit button margin+padding for pinstripe, override default label margin to match description for pinstripe).

Comment 4

[:Margaret Leibovic]

Attachment: patch for check-in

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 511583 [details]
linux changes

Shouldn't the link text be on the same line as the button label? Why does your change increase the button height?

Comment 6

[:Margaret Leibovic]

Attachment: better patch

So it seems the current .geolocation-text-link padding styles on linux are determining the height of the button because it is vertically filling its containing hbox. Using margin-top and margin-bottom, I was able to get the link aligned with the button label and keep the button height the same.

On closer inspection I also realized that the link height on OSX was 1px off due to the button border, and I also realized that the link alignment is wrong in winstripe, so I updated my patch to include fix these. (I know this change is assuming the custom button styles are applied for all Windows themes, but bug 633400 will probably be fixed before bug 509642, and this is just supposed to be a small polish tweak for Firefox 4).

I will attach screenshots of the changes.

Comment 7

[:Margaret Leibovic]

Attachment: screenshots

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 511794 [details] [diff] [review]
better patch

Can you just add align="center" to the container of the label and the button?

Comment 9

[:Margaret Leibovic]

That would work for Linux, but I think other changes would be needed for Windows and OSX because there's a margin-top on the button. Perhaps that margin should really be on the hbox as well.

Comment 10

[Dão Gottwald [:dao]]

Sounds like the link could just have the same margin-top as the button, which would still simplify things.

Comment 11

[Dão Gottwald [:dao]]

But yes, giving the container a class and setting the margin is probably the best solution.

Comment 12

[:Margaret Leibovic]

Attachment: even better patch

This patch applies the margin to the hbox instead of its contents. I tested on OSX, Windows, and Linux with the geolocation, password, and add-ons notifications.

I applied the margin to Linux even though it didn't exist before because I think the only reason it didn't exist is that we didn't get around to polishing the notifications on Linux. This will make the Linux notifications look at least a little closer to the mockups from bug 577927.

Comment 13

[:Margaret Leibovic]

Attachment: even better patch

Oops, some tabs got in there.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 511854 [details] [diff] [review]
even better patch

I'd prefer popup-notification-button-container. The container doesn't really care about the button type.

Comment 15

[:Margaret Leibovic]

Attachment: patch for check-in

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/32b826fadf94

Comment 17

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110216 Firefox/4.0b12pre

Verified issue and it's no longer present.