Closed Bug 856040 Opened 9 years ago Closed 9 years ago

Adjust padding of XUL menulist to match native widget on Windows 7 and up

Categories

(Toolkit :: Themes, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: Paenglab, Assigned: Paenglab)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

From Bug 853431 comment 7:
While this fixes color issues, it doesn't fix the padding issue:
On Windows 7 and up (and probably Vista), our menulist widget (the thing you click to open the menu, not the menu itself) has 3 pixels of extra vertical padding (2px below and 1 above, but that's probably just pixel-snapped centering) that the native menulist does not have.
Thanks for filing this. :)
Summary: Menulist widget needs some padding changes → Adjust padding of XUL menulist to match native widget on Windows 7 and up
Attached patch proposed fix (obsolete) — Splinter Review
This patch makes the menulist widget paddings mostly equal to the native widget. I let a 1px gap on the left of the text to the focus ring. The native widget has no gap but I think this doesn't look good.

This patch needs the patch from Bug 853431 applied first.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #731555 - Flags: ui-review?(fyan)
Attachment #731555 - Flags: review?(fyan)
Comment on attachment 731555 [details] [diff] [review]
proposed fix

Review of attachment 731555 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch on the redundant styles in menulist.css.
Just one small issue when running with a HiDPI display:

::: toolkit/themes/windows/global/menulist-aero.css
@@ +21,5 @@
> +    -moz-margin-start: 0 !important;
> +  }
> +
> +  .menulist-dropmarker {
> +    margin-top: -3px;

At least on my machine (running Windows 8 with a HiDPI display), this looks better when margin-top is -2px, maybe due to pixel snapping. How does it look with -2px to you?
Attachment #731555 - Flags: review?(fyan) → review+
Attached patch proposed fix v2Splinter Review
You're right, I don't know but before -3px was correct and now -2px is better. Maybe it was a rule left during testing.
Attachment #731555 - Attachment is obsolete: true
Attachment #731555 - Flags: ui-review?(fyan)
Attachment #731790 - Flags: ui-review?(fyan)
Attachment #731790 - Flags: review+
Comment on attachment 731790 [details] [diff] [review]
proposed fix v2

Thanks for confirming and for working on this. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/96a67be7802b
Attachment #731790 - Flags: ui-review?(fyan) → ui-review+
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/96a67be7802b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #731790 - Attachment is patch: true
Blocks: 864128
Depends on: 1573046
You need to log in before you can comment on or make changes to this bug.