Closed Bug 399030 Opened 15 years ago Closed 14 years ago

Draw dropdowns with NSPopupButtonCell

Categories

(Core :: Widget: Cocoa, defect, P3)

All
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: cbarrett, Assigned: mstange)

References

(Depends on 1 open bug)

Details

(Keywords: verified1.9.1)

Attachments

(4 files, 3 obsolete files)

For similar reasons to bug 394892, we want to do this. Additionally, it allows us to draw these widgets without double buffering (they do not render correctly on 10.4 with HITheme), which will result in nicer looking drawing especially for zoomed in pages.
Blocks: 54488
No longer blocks: 394092
Blocks: 394092
No longer blocks: 54488
Flags: blocking1.9+
Target Milestone: --- → mozilla1.9 M10
Attached patch fix 0.7 (obsolete) — Splinter Review
Just storing this here for safe keeping. It should be mostly done except it isn't drawing in its frame correctly, last time I checked.
Priority: -- → P4
Target Milestone: mozilla1.9 M10 → ---
Attachment #285036 - Attachment is obsolete: true
Attached patch prefix 1.0 (obsolete) — Splinter Review
The first change makes us closer to Safari in terms of rendering. Baseline was the distance from the right edge of the "h" in Macintosh to the left edge of the blue area in the "Hardware" dropdown on a buzilla bug.

The second change reduces the height of popups by one, which brings us in line with Safari (and standard system controls).

Ran reftests, they passed.
Attachment #290718 - Flags: review?(joshmoz)
Attached patch wip fix 0.8 (obsolete) — Splinter Review
Just putting this in bugzilla for archival purposes. It's got changes for bug 394892 as well.
Assignee: cbarrett → joshmoz
Priority: P4 → P3
Blocks: 54488
No longer blocks: 394092
Attachment #290718 - Flags: review?(joshmoz)
probably not going to make it for 1.9, tough call but we're fast running out of time
Flags: wanted1.9+
Flags: blocking1.9-
Flags: blocking1.9+
Hardware: PC → All
Depends on: 465069
Attached patch v2Splinter Review
This patch results in several positive changes:

 - <select>s at the default size no longer look blurry / muddy. Instead, the
   natural small size is used, so it looks crisp (like in Safari).
   Josh, let me know if you want me to reduce the snapping tolerance to 1px
   (like we did for buttons in bug 468507) - however, I don't think that's
   necessary; I haven't seen any problems with the snapping. I've tested a lot
   of sites and overall it looks much better to me.

 - The background window state is correct now. (I'll attach a screenshot)

 - A lot of code is removed.

The only downside with this patch is the fact that dropdowns with a large height don't look as good because we have to scale up the regular size manually.
Assignee: joshmoz → mstange
Attachment #290718 - Attachment is obsolete: true
Attachment #291916 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #357336 - Flags: superreview?(roc)
Attachment #357336 - Flags: review?(joshmoz)
This is a case where the new dropdowns don't look as good as the old ones.
(In reply to comment #7)
> Created an attachment (id=357339) [details]
> Screenshot: Stretched dropdown on https://addons.mozilla.org/
> 
> This is a case where the new dropdowns don't look as good as the old ones.

CC'ing Fred, so he is aware of that and probably has an idea.
(In reply to comment #8)
> CC'ing Fred, so he is aware of that and probably has an idea.

Which may fix an AMO case, but won't fix the overall problem. The patch is definitely an improvement for most selects though.
Yeah I can see the problem but I am not sure if it's a good idea to just fix it on the AMO side of things. Fixing it on the client side it he right thing to do here, I would say.
The problem is that Cocoa doesn't give us the option to draw dropdowns of that height. We can't really do much here, other than maybe falling back to the old way of drawing them when they're too high... but then the inactive window state is wrong again, and we get code duplication.
Attachment #357336 - Flags: superreview?(roc) → superreview+
Comment on attachment 357336 [details] [diff] [review]
v2

Please file a bug on the degraded look at larger heights when this lands.
Attachment #357336 - Flags: review?(joshmoz) → review+
I've filed bug 476277.

Pushed: http://hg.mozilla.org/mozilla-central/rev/ee1c68810375
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Depends on: 476277
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Wow, that looks fantastic on trunk. Thanks Markus! Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090201 Minefield/3.2a1pre ID:20090201020604

Markus, does something hinder us to get this into 1.9.1?
Status: RESOLVED → VERIFIED
I think we should get it for 1.9.1, but let's let it bake for a week.
Whiteboard: [needs baking on trunk]
Attachment #357336 - Flags: approval1.9.1?
There is one more side-effect by this patch. Text for up-scaled dropdowns is overlapping the buttons. See bug 480265.
Whiteboard: [needs baking on trunk] → [needs 1.9.1 approval]
Comment on attachment 357336 [details] [diff] [review]
v2

a191=beltzner
Attachment #357336 - Flags: approval1.9.1? → approval1.9.1+
(only trivial merging)
pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/305e88a5f35c
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 approval]
Verified on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090402 Shiretoko/3.5b4pre ID:20090402031241
You need to log in before you can comment on or make changes to this bug.