432529 - [Aero] Left/Bottom padding of identity contextual dialog is a bit too wide

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Different left and top padding

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050606 Minefield/3.0pre ID:2008050606

The left padding between the border and the Larry icon is a bit too wide when comparing it to the top padding. As Alex mentioned in bug 414700 comment 7 it should be the same width.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ehsan, could you also have a look at the right bottom corner? The bottom padding seems also to be wrong.

Comment 2

[(no longer active)]

Attachment: Patch (v1)

The problem was caused because the tooltip appearance requires 4 pixel on the left side (3 for the curvature and 1 for the border, so this effectively made the left border 14px instead of 10px, as specified in browser.css.  This patch makes the start padding 6px, so that the visual padding appears 10px on all sides of #identity-popup-container.

Comment 3

[(no longer active)]

Comment on attachment 319738 [details] [diff] [review]
Patch (v1)

(In reply to comment #1)
> Ehsan, could you also have a look at the right bottom corner? The bottom
> padding seems also to be wrong.

Oops, didn't catch this comment.  The bottom padding is fine, the right padding should be reduced similarly (see comment 2).  I'll post a new patch right away.

Comment 4

[(no longer active)]

Attachment: Patch (v2)

Handle both the left and right padding.

Comment 5

[Dão Gottwald [:dao]]

I'm not sure what we're trying to fix here. The current dialog certainly doesn't feel unbalanced to me. Ehsan, can you attach a before/after screenshot?

Comment 6

[(no longer active)]

Attachment: Before/After screenshot

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 319747 [details] [diff] [review]
Patch (v2)

As far as I understand comment 2, you should only do this if the tooltip appearance is actually used.

Comment 8

[(no longer active)]

Attachment: Patch (v2.1)

Change the padding only if the tooltip appearance is used.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 320268 [details] [diff] [review]
Patch (v2.1)

A comment would be nice, since it's not obvious why this is needed.

Comment 10

[(no longer active)]

Attachment: [checked-in] Patch (v2.2)

Addressed comment 9.  Requesting approval for this CSS-only change to correct the display of site identity popup on Vista.

Comment 11

[Mike Schroepfer]

Comment on attachment 320329 [details] [diff] [review]
[checked-in] Patch (v2.2)

Sorry we are closed for Firefox 3.0.  This may be a possibility for 3.0.1 or definitely 3.1.

Comment 12

[(no longer active)]

This is ready to be pushed on mozilla-central.

Comment 13

[(no longer active)]

Comment on attachment 320329 [details] [diff] [review]
[checked-in] Patch (v2.2)

This is a nice polish to have for 3.0.1.

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/98b5f55599f8

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bottom padding still too large

The padding on the bottom is still too large. I don't think that this is expected, right?

Comment 16

[(no longer active)]

You're right, the bottom padding is 2 pixels more than the right padding.  Reopening.  A patch will follow soon.

Comment 17

[Samuel Sidler (old account; do not CC)]

Comment on attachment 320329 [details] [diff] [review]
[checked-in] Patch (v2.2)

This needs to land on mozilla-central (and be resolved as FIXED) before requesting approval.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Samuel, this patch was landed on mozilla-central but doesn't completely fix this issue. As Ehsan said, a new patch will be up soon.

Comment 19

[(no longer active)]

Attachment: Final fixes to the paddings

This ensures a 13px padding on all sides of the identity contextual dialog.

Comment 20

[(no longer active)]

Comment on attachment 332576 [details] [diff] [review]
Final fixes to the paddings

Seeing if Gavin can review this faster...

Comment 21

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

Why 13px? Is that what the native styling uses?

Comment 22

[(no longer active)]

No special reason, I just tried to match the top and bottom paddings (14px and 13px respectively) and I chose 13.  Is there any comparative native widget on Vista which we can base this on?

Comment 23

[JC Ahangama]

I think what I saw is also related to what is discussed here, and I report it here because I see a lot of knowledgeable discussion going on.

I think the padding CSS directive is implemented similar to the margin directive by mistake. The following page I made illustrates the problem:
http://www.lovatasinhala.com/ds/hoodiya.htm
The yellow box overshoots by 20 pixels matching the 10px padding. If you use IE, it displays correctly at 640px, the given width of the paragraph. CSS 2.1 explains padding at:
http://www.w3.org/TR/CSS21/box.html#propdef-padding
It tells that padding is WITHIN the paragraph bounding box. Please look at the diagrams and note how the letters touch the edge where there is no padding.

Thank you.

Comment 24

[Samuel Sidler (old account; do not CC)]

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release).

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #22)
> No special reason, I just tried to match the top and bottom paddings (14px and
> 13px respectively) and I chose 13.  Is there any comparative native widget on
> Vista which we can base this on?

Gavin, does it answer your question?

Comment 26

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

Comment on attachment 332576 [details] [diff] [review]
Final fixes to the paddings

>diff --git a/browser/themes/winstripe/browser/browser-aero.css b/browser/themes/winstripe/browser/browser-aero.css

> #identity-popup-container:-moz-system-metric(windows-default-theme) {

>+  -moz-padding-end: 8px;

Looks like this was measured based on the button, which has its own 2px padding. If you measure based on the end of the text in the no-ssl case ("This website does not supply identity information."), this should be 10px, right?

Comment 27

[Shawn Wilsher :sdwilsh]

Ehsan - plan on getting a new patch on this anytime soon?

Comment 28

[(no longer active)]

Attachment: Identity panel on trunk (3x zoom)

So, here is the current situation on an unpatched trunk build.

I think the solution here would be to set the margin-end-value and margin-bottom on the button to 0, and reduce the padding-bottom on #identity-popup-container by 1 pixels.  The 9px right padding of the text in no-ssl case is because of subpixel rendering, which we can ignore, IMO.

Can someone please confirm that this is indeed the desired solution before I convert it into a patch?

Comment 29

[Alex Faaborg [:faaborg] (Firefox UX)]

>Can someone please confirm that this is indeed the desired solution before I
>convert it into a patch?

yep, please generate a patch.  I'll also file a follow up bug to make sure the padding is consistent across all the panels.

Comment 30

[(no longer active)]

Attachment: (Hopefully) final patch

With this patch, the padding looks 10px from all sides.

Comment 31

[(no longer active)]

Gavin: ping?

Comment 32

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

Comment on attachment 350464 [details] [diff] [review]
(Hopefully) final patch

sorry for the delay

Comment 33

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/237f8897fa8a

Comment 34

[(no longer active)]

Comment on attachment 350464 [details] [diff] [review]
(Hopefully) final patch

Simple theme-only polish change, requesting approval for 1.9.1.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks great. Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090222 Minefield/3.2a1pre ID:20090222035031

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 350464 [details] [diff] [review]
(Hopefully) final patch

a191=beltzner

Comment 37

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6fff0eb3350c

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ehsan, we have forgotten to update Luna too. On Windows XP the paddings are still not correct. Filing a new bug or do we wanna reopen this one?

Comment 39

[(no longer active)]

(In reply to comment #38)
> Ehsan, we have forgotten to update Luna too. On Windows XP the paddings are
> still not correct. Filing a new bug or do we wanna reopen this one?

Please file a new bug, and CC me on it.  It would be great if you could attach scaled up screenshots which show the problem (like attachment 349383 [details] for example).

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b4pre) Gecko/20090331 Shiretoko/3.5b4pre

I filed bug 486844 for the XP Luna theme and for classic at all (which also includes Vista).

Comment 41

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P3 - Polish issue that is in a secondary interface, occasionally encountered, or is not easily identifiable.

Secondary UI, and a pretty minor padding change.