Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in Firefox 4.0b12

Status

()

Firefox
Theme
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 4.0b12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 511581 [details] [diff] [review]
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.
Attachment #511581 - Flags: review?(gavin.sharp)
Attachment #511581 - Flags: review?(dao)
(Assignee)

Comment 1

7 years ago
Created attachment 511582 [details]
osx changes
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
(Assignee)

Comment 2

7 years ago
Created attachment 511583 [details]
linux changes
(Assignee)

Updated

7 years ago
Attachment #511581 - Attachment is patch: true
Attachment #511581 - Attachment mime type: application/octet-stream → text/plain
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+
(Assignee)

Updated

7 years ago
Attachment #511581 - Flags: review?(dao)
(Assignee)

Comment 4

7 years ago
Created attachment 511588 [details] [diff] [review]
patch for check-in
Attachment #511581 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
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?

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Comment 6

7 years ago
Created attachment 511794 [details] [diff] [review]
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.
Attachment #511588 - Attachment is obsolete: true
Attachment #511794 - Flags: review?(gavin.sharp)
Attachment #511794 - Flags: review?(dao)
(Assignee)

Comment 7

7 years ago
Created attachment 511796 [details]
screenshots
Attachment #511582 - Attachment is obsolete: true
Attachment #511583 - Attachment is obsolete: true
Comment on attachment 511794 [details] [diff] [review]
better patch

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

Comment 9

7 years ago
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.
(Assignee)

Comment 12

7 years ago
Created attachment 511853 [details] [diff] [review]
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.
Attachment #511794 - Attachment is obsolete: true
Attachment #511853 - Flags: review?(gavin.sharp)
Attachment #511853 - Flags: review?(dao)
Attachment #511794 - Flags: review?(gavin.sharp)
Attachment #511794 - Flags: review?(dao)
(Assignee)

Comment 13

7 years ago
Created attachment 511854 [details] [diff] [review]
even better patch

Oops, some tabs got in there.
Attachment #511853 - Attachment is obsolete: true
Attachment #511854 - Flags: review?(gavin.sharp)
Attachment #511854 - Flags: review?(dao)
Attachment #511853 - Flags: review?(gavin.sharp)
Attachment #511853 - Flags: review?(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.
Attachment #511854 - Flags: review?(dao) → review+
Attachment #511854 - Flags: review?(gavin.sharp) → approval2.0+
(Assignee)

Comment 15

7 years ago
Created attachment 511930 [details] [diff] [review]
patch for check-in
Attachment #511854 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/32b826fadf94
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12

Comment 17

7 years ago
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

Updated

5 years ago
No longer blocks: 615483
You need to log in before you can comment on or make changes to this bug.