Closed Bug 432529 Opened 12 years ago Closed 11 years ago

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

Categories

(Firefox :: Theme, defect)

x86
Windows Vista
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: whimboo, Assigned: ehsan)

References

Details

(Keywords: polish, verified1.9.1, Whiteboard: [polish-easy] [polish-visual][polish-p3])

Attachments

(5 files, 5 obsolete files)

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.
Assignee: nobody → ehsan.akhgari
Blocks: 425582
Status: NEW → ASSIGNED
Ehsan, could you also have a look at the right bottom corner? The bottom padding seems also to be wrong.
Summary: Left padding of identitiy contextual dialog is a bit too wide → Left/Bottom padding of identitiy contextual dialog is a bit too wide
Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Attachment #319738 - Flags: review?(gavin.sharp)
Keywords: polish
Whiteboard: [has patch][needs review gavin]
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.
Attachment #319738 - Attachment is obsolete: true
Attachment #319738 - Flags: review?(gavin.sharp)
Attached patch Patch (v2) (obsolete) — Splinter Review
Handle both the left and right padding.
Attachment #319747 - Flags: review?(gavin.sharp)
Attachment #319747 - Flags: review?(gavin.sharp) → review?(dao)
Whiteboard: [has patch][needs review gavin] → [has patch][needs review 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?
Attached image Before/After screenshot
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.
Attachment #319747 - Flags: review?(dao) → review-
Attached patch Patch (v2.1) (obsolete) — Splinter Review
Change the padding only if the tooltip appearance is used.
Attachment #319747 - Attachment is obsolete: true
Attachment #320268 - Flags: review?(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.
Attachment #320268 - Flags: review?(dao) → review+
Attached patch [checked-in] Patch (v2.2) (obsolete) — Splinter Review
Addressed comment 9.  Requesting approval for this CSS-only change to correct the display of site identity popup on Vista.
Attachment #320268 - Attachment is obsolete: true
Attachment #320329 - Flags: approval1.9?
Summary: Left/Bottom padding of identitiy contextual dialog is a bit too wide → Left/Bottom padding of identity contextual dialog is a bit too wide
Whiteboard: [has patch][needs review dao] → [has patch][has review][needs approval]
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.
Attachment #320329 - Flags: approval1.9? → approval1.9-
Flags: wanted1.9.0.x?
Whiteboard: [has patch][has review][needs approval] → [has patch][has review][needs approval][RC2?]
Whiteboard: [has patch][has review][needs approval][RC2?] → [has patch][has review][needs approval][RC2-]
This is ready to be pushed on mozilla-central.
Keywords: checkin-needed
Whiteboard: [has patch][has review][needs approval][RC2-] → [has patch][has review][RC2-]
Comment on attachment 320329 [details] [diff] [review]
[checked-in] Patch (v2.2)

This is a nice polish to have for 3.0.1.
Attachment #320329 - Flags: approval1.9.0.1?
Attachment #320329 - Flags: approval1.9.0.1? → approval1.9.0.2?
http://hg.mozilla.org/index.cgi/mozilla-central/rev/98b5f55599f8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1a1
The padding on the bottom is still too large. I don't think that this is expected, right?
You're right, the bottom padding is 2 pixels more than the right padding.  Reopening.  A patch will follow soon.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #320329 - Flags: approval1.9.0.2?
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.
Whiteboard: [has patch][has review][RC2-] → [RC2-]
Attachment #320329 - Attachment description: Patch (v2.2) → [checked-in] Patch (v2.2)
Status: REOPENED → ASSIGNED
Attached patch Final fixes to the paddings (obsolete) — Splinter Review
This ensures a 13px padding on all sides of the identity contextual dialog.
Attachment #320329 - Attachment is obsolete: true
Attachment #332576 - Flags: review?(dao)
Comment on attachment 332576 [details] [diff] [review]
Final fixes to the paddings

Seeing if Gavin can review this faster...
Attachment #332576 - Flags: review?(dao) → review?(gavin.sharp)
Why 13px? Is that what the native styling uses?
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?
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.
Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release).
Flags: wanted1.9.0.x?
(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?
Target Milestone: Firefox 3.1a1 → ---
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?
Attachment #332576 - Flags: review?(gavin.sharp) → review-
Whiteboard: [RC2-] → [RC2-] [polish-easy] [polish-visual]
Ehsan - plan on getting a new patch on this anytime soon?
Whiteboard: [RC2-] [polish-easy] [polish-visual] → [polish-easy] [polish-visual]
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?
Attachment #332576 - Attachment is obsolete: true
>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.
With this patch, the padding looks 10px from all sides.
Attachment #350464 - Flags: review?(gavin.sharp)
Gavin: ping?
Whiteboard: [polish-easy] [polish-visual] → [polish-easy] [polish-visual][has patch][needs review gavin]
Comment on attachment 350464 [details] [diff] [review]
(Hopefully) final patch

sorry for the delay
Attachment #350464 - Flags: review?(gavin.sharp) → review+
http://hg.mozilla.org/mozilla-central/rev/237f8897fa8a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [polish-easy] [polish-visual][has patch][needs review gavin] → [polish-easy] [polish-visual]
Target Milestone: --- → Firefox 3.2a1
Attachment #350464 - Flags: approval1.9.1?
Comment on attachment 350464 [details] [diff] [review]
(Hopefully) final patch

Simple theme-only polish change, requesting approval for 1.9.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
Status: RESOLVED → VERIFIED
Comment on attachment 350464 [details] [diff] [review]
(Hopefully) final patch

a191=beltzner
Attachment #350464 - Flags: approval1.9.1? → approval1.9.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?
(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).
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).
Summary: Left/Bottom padding of identity contextual dialog is a bit too wide → [Aero] Left/Bottom padding of identity contextual dialog is a bit too wide
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.
Whiteboard: [polish-easy] [polish-visual] → [polish-easy] [polish-visual][polish-p3]
You need to log in before you can comment on or make changes to this bug.