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 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).
Attachment #511581 - Flags: review?(gavin.sharp) → review+
Attachment #511581 - Flags: approval2.0+
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?
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 on attachment 511794 [details] [diff] [review] better patch Can you just add align="center" to the container of the label and the button?
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.
Sounds like the link could just have the same margin-top as the button, which would still simplify things.
But yes, giving the container a class and setting the margin is probably the best solution.
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.
Oops, some tabs got in there.
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.
Attachment #511854 - Flags: review?(dao) → review+
Attachment #511854 - Flags: review?(gavin.sharp) → approval2.0+
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110216 Firefox/4.0b12pre Verified issue and it's no longer present.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.