[10.10] Make widgets drawn with CoreUI use the Yosemite style

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla34
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Assignee

Description

5 years ago
Our toolbars and toolbar buttons, among other things, look like on 10.9 and haven't adapted the 10.10 look. This is because when calling CUIDraw we pass NULL to the CoreUIRenderer argument.

On 10.10 we can use -[NSAppearance _drawInRect:context:options:] on [NSAppearance appearanceNamed:NSAppearanceNameAqua], which will call CUIDraw with the correct CoreUIRenderer and give us the 10.10 look. It works for everything except scrollbars, as far as I could tell. (Scrollbar thumbs are too thick when drawn through NSAppearance, I don't know why.)

I have patches.
Assignee

Comment 1

5 years ago
Posted patch wip (obsolete) — Splinter Review
Assignee

Comment 2

5 years ago
Posted patch v1 (obsolete) — Splinter Review
Attachment #8474830 - Attachment is obsolete: true
Attachment #8475159 - Flags: review?(smichaud)
Assignee

Comment 3

5 years ago
Posted patch v2 (obsolete) — Splinter Review
Oops, forgot a .size in the max area checks.
Attachment #8475159 - Attachment is obsolete: true
Attachment #8475159 - Flags: review?(smichaud)
Attachment #8475184 - Flags: review?(smichaud)
Assignee

Comment 4

5 years ago
Comment on attachment 8475184 [details] [diff] [review]
v2

Looks like I need to keep using [NSWindow coreUIRenderer] on 10.6 at least. Tryserver is all orange.
Attachment #8475184 - Flags: review?(smichaud)
Assignee

Comment 5

5 years ago
Posted patch v3 (obsolete) — Splinter Review
Attachment #8475184 - Attachment is obsolete: true
Attachment #8475828 - Flags: review?(smichaud)
Markus, does this patch depend on those for bug 1051522?  If not I'll review this patch first.  I hope to get to both bugs by the end of this week.
Assignee

Comment 7

5 years ago
No, this patch does depend on any other patches.
Comment on attachment 8475828 [details] [diff] [review]
v3

This mostly looks fine to me.  But I did notice what seems to be a serious problem:

In a class-dump of the AppKit framework on OS X 10.9 (both 10.9.1 and 10.9.4), I found the following three _drawRect methods of the NSAppearance class:

- (void)_drawInRect:(struct CGRect)arg1 context:(struct CGContext *)arg2
  options:(id)arg3 inWindow:(id)arg4;
- (void)_drawInRect:(struct CGRect)arg1 context:(struct CGContext *)arg2
  options:(id)arg3 inView:(id)arg4;
- (void)_drawInRect:(struct CGRect)arg1 context:(struct CGContext *)arg2
  options:(id)arg3 delegate:(id)arg4;

Notice that all of them have four parameters.  But the definition in your patch has only three.

You do check that your three-parameter method exists before you call it ... but I think that just means that you *never* call it.
Comment on attachment 8475828 [details] [diff] [review]
v3

Oops, forgot to check on Yosemite.  When I did (on Yosemite DP6, the current DP) I found your three-parameter _drawInRect method (and no others).  So the problem isn't as serious as I first thought.  But you're still not using NSAppearance on OS X 10.9.  Is this intentional?
Comment on attachment 8475828 [details] [diff] [review]
v3

> But you're still not using NSAppearance on OS X 10.9.  Is this intentional?

Not using NSAppearance on 10.9 isn't a big deal.  If you change your mind later, we can deal with that then.
Attachment #8475828 - Flags: review?(smichaud) → review+
Assignee

Comment 11

5 years ago
(In reply to Steven Michaud from comment #9)
> But you're still not using
> NSAppearance on OS X 10.9.  Is this intentional?

Yes, this is intentional. I'll add a comment about that.
Assignee

Comment 12

5 years ago
Posted patch v4 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=39aa79c801d8
Attachment #8475828 - Attachment is obsolete: true
Assignee

Comment 13

5 years ago
Posted patch v5Splinter Review
Interestingly, NSAppearance and +[NSAppearance appearanceNamed:] are both available on 10.8, but [NSAppearance appearanceNamed:@"NSAppearanceNameAqua"] throws an NSInternalInconsistencyException on 10.8. I've added an "if (nsCocoaFeatures::OnYosemiteOrLater())" to GetAquaAppearance().
I've also restored the titlebar top line overdrawing in DrawNativeTitlebar that the previous patches were removing. It's still needed.
Attachment #8479871 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0eb0a00238ed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.