Opened 11 months ago

Last modified 3 months ago

#18146 accepted defect

OSX Mojave Dark Mode support

Reported by: dkulp Owned by: csomor
Priority: normal Milestone:
Component: wxOSX Version: 3.1.1
Keywords: Cc: vaclavslavik, andy@…, rainbow@…
Blocked By: Blocking:
Patch: no

Description

I was testing our apps with Mohave and in "Dark Mode", wxWidgets apps do NOT display properly at all. The text label colors seem correct, but the flat areas and control background remain the light gray. I did some digging and it's due to using the HIThemeBrushCreateCGColor call to query a couple of specific colors. In dark mode, they don't change. I logged a bug with Apple, but got the following response:

"Unfortunately the HITheme API has not been, and will not be, updated to be cognizant of dark mode. Instead, we recommend using NSColor, which obeys the current appearance."

Thus, it would be good to remove the HIThem stuff, or at the very least, provide a way to disable it and query the appropriate colors via NSColor.

Attachments (3)

wxWidgets-dark.patch download (9.8 KB) - added by dkulp 10 months ago.
titlebar-comparison.png download (247.8 KB) - added by vaclavslavik 9 months ago.
testing_background.png download (62.0 KB) - added by vaclavslavik 9 months ago.

Download all attachments as: .zip

Change History (49)

comment:1 Changed 11 months ago by csomor

  • Owner set to csomor
  • Status changed from new to accepted

still trying to get Mojave Installer work on my SSD volumes, but after having reshuffled 10.8 to a different volume, I'm confident I'll get to that eventually

comment:2 Changed 11 months ago by dkulp

  • Cc dan@… added

Note: wxNotebook's don't display the tabs correctly on Mojave. Logged a bug about that with Apple as well. Haven't heard back yet.

comment:3 Changed 11 months ago by vaclavslavik

  • Cc vaclavslavik added

comment:4 Changed 11 months ago by csomor

First step in 71bb680

Changed 10 months ago by dkulp

comment:5 Changed 10 months ago by dkulp

  • Cc dan@… removed

I've attached a patch of some of the stuff I've been working on to try and get my app looking at least partially acceptable. I've only been testing this in dark mode on OSX so it may have issues in normal mode or on other platforms. However, it might help in identifying areas that need to be looked at more in depth.

comment:6 Changed 10 months ago by Stefan Csomor <csomor@…>

In cd02c548/git-wxWidgets:

Implement wxEVT_SYS_COLOUR_CHANGED in wxOSX (#832)

Starting with macOS 10.14 Mojave, system colors can change dynamically
when the user switches between light and dark modes. Detect this by
observing the effectiveAppearance property and emit
wxSysColourChangedEvent accordingly.

See #18146.

comment:7 Changed 10 months ago by csomor

I forgot to link another commit 9cef828 as the NSColor methods were not correctly updating the real appearance when changed during runtime. I found out that this had to be set manually when not running inside certain methods (like drawing etc) where the OS took care of that.

comment:8 Changed 10 months ago by vaclavslavik

I forgot to link another commit ​9cef828 as the NSColor methods were not correctly updating the real

Wouldn't a more robust fix for that be to actually store NSColor in wxColour? It's my understanding it is dynamic in 10.14, à la NSImage, and changes its RGB values if appearance changes, and the problem is that wxColour(WX_NSColor col) ctor extracts RGBA only.

comment:9 Changed 10 months ago by csomor

Interesting point, I thought about having dynamic wxColour instances as well, but it seemed to get complicated, given the fact that we already have an event mechanism for system colour changes which you thankfully wired for macOS. So adopting this seemed to be the easiest way.

If we have really dynamic colors, I wouldn't want to always go through the conversion to CGColorRef for every use of a wxColour , always setting current from effectiveAppearance seems a little bit too much also. So I'd like to keep this as low as possible but still always correct. One idea would be to have dynamic colours store a void* for the current appearance. If the effectiveAppearance and the void* are not identical, an access to a colour would lead to new generation of CGColorRef etc.

What are your thoughts about such an approach ?

comment:10 Changed 10 months ago by vaclavslavik

What I meant was changing wxColour to behave as most GDI classes do: don't have all those m_cgColour, m_red, m_blue, m_green, m_alpha fields, but only store the actual NSColor object and take the RGBA values from that when queried, and [NSColor CGColor] for CoreGraphics interop where needed (that's 10.8+, I'm not sure what the current OSX minimum for wx is).

The point is we wouldn't need to do anything in wx to ensure dynamic behavior: the underlying NSColor would. wx code could use, and even store, wxColour retrieved from wxSystemSettings and it would do the right thing all on its own. Right now, both wx's and user code needs to re-retrieve system colors from wxSystemSettings in response to wxEVT_SYS_COLOUR_CHANGED event. That's - again, as far as I understand it - an avoidable hassle.

comment:11 Changed 10 months ago by csomor

OK, I understand but this way we still would have - at every usage outside draw - set NSAppearance.current to the effectiveAppearance, otherwise the CGColorRef would not be correct

comment:12 Changed 10 months ago by Stefan Csomor <csomor@…>

In 5091d8782/git-wxWidgets:

Support Dark Mode for Status Bar

see #18146

comment:13 follow-up: Changed 10 months ago by Vadim Zeitlin <vadim@…>

In 7da13b273/git-wxWidgets:

Fix wxOSX compilation problem due to missing wx/settings.h

Include the header required by the code using wxSystemSettings added in
5091d878254c80a85a7287383d64168ccbf64bee

See #18146.

comment:14 in reply to: ↑ 13 Changed 10 months ago by csomor

Replying to Vadim Zeitlin <vadim@…>:

In 7da13b273/git-wxWidgets:

Fix wxOSX compilation problem due to missing wx/settings.h

Include the header required by the code using wxSystemSettings added in
5091d878254c80a85a7287383d64168ccbf64bee

See #18146.

Sorry, thanks for fixing.

Stefan

comment:15 Changed 10 months ago by Stefan Csomor <csomor@…>

In 12f8e4b5/git-wxWidgets:

First part of support for Dark Mode for wxRibbon

see #18146

comment:16 Changed 10 months ago by Stefan Csomor <csomor@…>

In d2aafff4a/git-wxWidgets:

Second part of support for Dark Mode for wxRibbon, use flat appearance starting from 10.10

see #18146

comment:17 Changed 10 months ago by Stefan Csomor <csomor@…>

In 805db8baa/git-wxWidgets:

Dark Mode for wxRenderer for HeaderButtons and Sashes

see #18146

comment:18 Changed 10 months ago by Stefan Csomor <csomor@…>

In dcd01218/git-wxWidgets:

Dark Mode for wxPropertyGrid

see #18146, thanks to dkulp

comment:19 Changed 10 months ago by Stefan Csomor <csomor@…>

In c58d7755a/git-wxWidgets:

Dark Mode for non native toolbar

see #18146

comment:20 Changed 10 months ago by Stefan Csomor <csomor@…>

comment:21 Changed 10 months ago by andyr

  • Cc andy@… added

Any news on the wxNotebook tabs issue (tabs are not displayed)? Are we confident this is something Apple are going to fix?

comment:22 Changed 10 months ago by dkulp

  • Cc dan@… added

The have not responded to that report yet. I likely need to build the sample app with static linked wxWidgets or something to attach to the report as a sample.

Also not that I'm having all kinds of refresh issues with wxGLCanvas. I need to check the gl samples to see if they are also affected.

comment:23 follow-up: Changed 10 months ago by dkulp

  • Cc dan@… removed

I filed a new report for the OpenGL issues (penguin sample for example).

I would highly encourage others to file bugs at https://bugreport.apple.com/ as well.

comment:24 Changed 10 months ago by andyr

More about wxNotebook.

wx 3.1.1
Using current Mojave Beta with latest xCode & 10.13 SDK.

The Notebook sample doesn't work on Mojave - the tabs do not display any text.
Also when I copy my app built on 10.10 to Mojave, tabs don't work.

But when Apple do it, it works:
Applications - Utilities - Audio MIDI Setup.app uses NSTabView (that is, it looks like a NSTabView and when I grep the app folder it finds references to NSTabView). It works fine on Mojave and when I copy the version of this app from OS 10.10 to Mojave, that also works fine.

So it does look like we need to change something in wxNotebook, in the way that it uses NSTabView.

I will try to investigate further but I am no Mac expert so if anyone has any ideas, please speak up.

comment:25 Changed 10 months ago by andyr

More about wxNotebook.

The problem is in window.mm, the function:

wxOSXCocoaClassAddWXMethods(Class c)

The line:

wxOSX_CLASS_ADD_METHOD(c, @selector(drawRect:), (IMP) wxOSX_drawRect, "v@:{_NSRect={_NSPoint=ff}{_NSSize=ff}}" )

If I comment out that one line then the text of wxNotebook tabs appears again.

I have no idea what that line (or indeed that function) is for so can someone (Stefan?) please advise. Thanks.

comment:26 Changed 10 months ago by andyr

BTW it's not the thread-related stuff in wxOSX_drawRect that's causing the problem - if I remove that, so wxOSX_drawRect becomes a 4-line function, the problem is still there.

comment:27 Changed 10 months ago by dkulp

  • Cc dan@… added

Without that line, any control that actually draws things (like any of the AUI things, most custom controls, etc...) do not actually draw anything. Thus, it's definitely required for many use cases.

comment:28 Changed 10 months ago by csomor

Thanks Andy for your tests, I'll have to look at things, it is a good starting point. Of course usually we want our own rendering for most controls, and it's strange that calling super is not enough for notebook.

comment:29 Changed 10 months ago by csomor

The mere fact of having overriden drawRect: seems to trigger the problem, even if no wx code is involved at all ... if I don't find another way, I'll modify the method overriding so that we can exclude the drawing override for certain classes

comment:30 Changed 10 months ago by Stefan Csomor <csomor@…>

In 616f0ca7f/git-wxWidgets:

Fixing notebook drawing

Workaround for drawing problems under 10.14 as we do not draw the notebook ourselves, this should be ok on all system versions, for details see #18146

comment:31 in reply to: ↑ 23 Changed 10 months ago by csomor

Replying to dkulp:

I filed a new report for the OpenGL issues (penguin sample for example).

what exactly is not working for you with penguin ?
in my 10.14 Build 18A314h (Dev Beta 2) I have - when built against 10.14 SDK - flickering windows during rotation, when built against 10.12 SDK the binary runs w/o these glitches on the same 10.14 version

comment:32 Changed 10 months ago by csomor

Could be that this is all related to the fact that window backing stores are not used anymore, only CALayers, the entire drawing mechanism has changed

comment:33 Changed 10 months ago by dkulp

  • Cc dan@… removed

It's definitely a compiled with 10.14 SDK issue. I see the same in my app. With 10.14 SDK, there are a lot of refresh issues with OpenGL windows. If you resize the window, it "sometimes" will just display a gray window, other times it will actually work. Lots of flickering where it looks like the swapBuffer is swapping in an empty buffer instead of the draw on one, when rotating the penguin, sometime when you stop, you just get a blank grey window, etc... 10.12SDk works fine, but then you completely lose dark mode.

Anyway, I did log an issue with Apple, but they haven't responded. With OpenGL being "deprecated", I'm unconvinced that they will even care. I've been thinking about spending some time to look more into Metal (or Vulkan via MoltenVK), but I just don't have the time right now.

comment:34 Changed 10 months ago by csomor

you just add a NSRequiresAquaSystemAppearance with NO to info plist, then Dark Mode works even when compiled against SDK 10.12, BUT all the flickering happens as well then ...

Last edited 10 months ago by csomor (previous) (diff)

comment:35 Changed 10 months ago by dkulp

  • Cc dan@… added

I just submitted a pull request that fixes the OpenGL flicker issue. Since OpenGL draws the whole view and doesn't needuse the NSGraphicsContext, this bypasses all of that which fixes the issues.

comment:36 Changed 10 months ago by dkulp

  • Cc dan@… removed

Just a note: the NSTabView workaround is no longer needed with Mojave Beta3.

comment:37 Changed 10 months ago by csomor

Thanks, perfect, I'll remove it :-)

comment:38 Changed 9 months ago by Stefan Csomor <csomor@…>

In 24b8a2523/git-wxWidgets:

Undo change for notebook under 10.14, not needed under Beta 3 anymore

see #18146

comment:39 Changed 9 months ago by vaclavslavik

OK, I understand but this way we still would have - at every usage outside draw - set NSAppearance.current to the effectiveAppearance, otherwise the CGColorRef would not be correct

What about storing both RGB values and NSColor, then?

NSColor really is much more dynamic/magic now. It's not just being different for dark and light modes, but the system colors can change appearance subtly based on view material/appearance, of which there's many more now, and even depending on window position on screen. You can see this in wxListBox if you move it around in dark mode, the background color subtly changes to reflect the background wallpaper behind it (this is not sidebar-like translucency, but a solid, adapted color).

These articles are very useful regarding this:

https://512pixels.net/2018/06/on-macos-mojaves-dark-mode/
https://mackuba.eu/2018/07/04/dark-side-mac-1/
https://mackuba.eu/2018/07/10/dark-side-mac-2/

I am attaching a testing background wallpaper as used in the first article; it's very helpful for better seeing the effects in play.

My motivating reason for looking deeper into this is that the titlebar and toolbar are subtly wrong in wx apps — they have a wrong shade of dark grey and I think this is related to not using NSColor natively. I think we need to support NSColor natively, not only to be able to preserve exact system colors, but to allow user code to use system colors unsupported by wx (e.g. secondaryLabelColor) and safely roundtrip them through wxColour.

Attached is an illustration of the wrong background as compared to Finder. The third row is using either this patch:

diff --git a/src/osx/cocoa/nonownedwnd.mm b/src/osx/cocoa/nonownedwnd.mm
--- a/src/osx/cocoa/nonownedwnd.mm
+++ b/src/osx/cocoa/nonownedwnd.mm
@@ -907,7 +907,7 @@ - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(N
 
 bool wxNonOwnedWindowCocoaImpl::SetBackgroundColour(const wxColour& col )
 {
-    [m_macWindow setBackgroundColor:col.OSXGetNSColor()];
+    [m_macWindow setBackgroundColor:[NSColor windowBackgroundColor]];
     return true;
 }

or this one (same effect, done differently — either not set background at all as above, or make sure it is set to actual real native windowBackgroundColor):

diff --git a/src/osx/nonownedwnd_osx.cpp b/src/osx/nonownedwnd_osx.cpp
--- a/src/osx/nonownedwnd_osx.cpp
+++ b/src/osx/nonownedwnd_osx.cpp
@@ -152,7 +152,7 @@ bool wxNonOwnedWindow::Create(wxWindow *parent,
     wxWindowCreateEvent event(this);
     HandleWindowEvent(event);
 
-    SetBackgroundColour(wxSystemSettings::GetColour( wxSYS_COLOUR_3DFACE ));
+    //SetBackgroundColour(wxSystemSettings::GetColour( wxSYS_COLOUR_3DFACE ));
 
     if ( parent )
         parent->AddChild(this);

The first two rows are with current code (which sets wxFrame bg to wxSYS_COLOUR_3DFACE, which is flat out wrong on macOS) or with _APPWORKSPACE to force use windowBackgroundColor — but as you can see, it behaves differently after going through wxColour and changing from the magical NSColor value it is to a regular RGB color.

Changed 9 months ago by vaclavslavik

Changed 9 months ago by vaclavslavik

comment:40 Changed 9 months ago by csomor

Thanks for the sample, I'll be running these. So you'd suggest going the gdirefdata route, perhaps even having two refclasses ? one specialized for nscolors, being dynamic and going through accessors whenever needed, but perhaps a little slower than the cgcolor based one ?

comment:41 Changed 9 months ago by vadz

FWIW I think using ref data for wxColour would make sense. wxGTK already does it and for about the same reasons I believe.

comment:42 Changed 9 months ago by csomor

Yes, I've seen that, I also see no other option for NSColor/CGColor differences. I've already written a first implementation. I'll add a PR.

comment:44 follow-up: Changed 7 months ago by vaclavslavik

There's a similar problem to the wxColour one with wxBitmap not using NSImage (which, when created via imageNamed: as wxArtProvider does, is also dynamic: not only in the old and supported way of having Retina version and isTemplate attribute, but also by being dynamic for light/dark modes when the application uses asset catalog). To be honest, I wrongly thought NSColor was used as wxBitmap's representation already, but this one seems hard to tackle to me...

comment:45 in reply to: ↑ 44 Changed 7 months ago by csomor

Replying to vaclavslavik:

There's a similar problem to the wxColour one with wxBitmap not using NSImage (which, when created via imageNamed: as wxArtProvider does, is also dynamic: not only in the old and supported way of having Retina version and isTemplate attribute, but also by being dynamic for light/dark modes when the application uses asset catalog). To be honest, I wrongly thought NSColor was used as wxBitmap's representation already, but this one seems hard to tackle to me...

could we take that discussion to the wxBitmap/wxIcon PR ?

https://github.com/wxWidgets/wxWidgets/pull/925

comment:46 Changed 3 months ago by rainbow

  • Cc rainbow@… added

probably an out of band question: I see that the dark mode required many changes and that they are applied only to 3.1.x development branch. Any change to see that in the 3.0.x? In this way I can enable dark mode to homebrew. thanks

Note: See TracTickets for help on using tickets.