wxWidgets / wxWidgets

Cross-Platform C++ GUI Library
https://www.wxwidgets.org/
5.77k stars 1.7k forks source link

macOS Sonoma scrollbar fix in 3.2 results in no scrollbars in wxGrid #24512

Closed craftyjon closed 1 week ago

craftyjon commented 2 weeks ago

Description

After https://github.com/wxWidgets/wxWidgets/commit/b8744be566195d9adf91be9fd5bb6c2de944c900 was backported, wxGrid no longer shows any scrollbars on Sonoma.

To reproduce, just run the grid sample.

This doesn't seem to happen on master, only on the backports to 3.2, but so far I haven't figured out why.

Current state of 3.2 branch:

image

With b8744be56 reverted:

image

Platform and version information

craftyjon commented 2 weeks ago

@dsa-t pointed out https://github.com/wxWidgets/wxWidgets/issues/24151 in which someone maybe seems to have reported this issue already, but the issue got closed because it doesn't exist on master?

vadz commented 2 weeks ago

I have no idea what's going on here, the only difference between 3.2 and master versions of wxWindow code that I see is a249e21f65 (Fix scrollbar creation for Mac windows not using standard peer, 2023-06-18) but it shouldn't affect this, normally.

@csomor Would you see what's going on here by chance? We really need to fix this before the upcoming release...

craftyjon commented 2 weeks ago

Cherry-picking a249e21f658 seems to fix the issue in a quick test.

craftyjon commented 2 weeks ago

Note, in the "fixed" state, scrollbars are always shown, rather than respecting the macOS system preference of when to show them (auto/when scrolling/always). But I'm not sure if this worked before.

csomor commented 2 weeks ago

@csomor Would you see what's going on here by chance? We really need to fix this before the upcoming release...

Yes, I'm already testing since yesterday evening ;-) I have the suspicion that the parent of the scrollbar is the clip view instead of the nsview, but I have to isolate things first

csomor commented 2 weeks ago

@craftyjon is right, this is the change that was missing, it changes the order of creation for the scrollbars therefore making them embedded in the NSView and not in the NSClipView. We don't need the rest of the commit though, so I'd just apply a minimal change if that's ok @vadz ?

vadz commented 2 weeks ago

@csomor Something like this?

diff --git a/src/osx/window_osx.cpp b/src/osx/window_osx.cpp
index e44e80fb26..3241ded6e2 100644
--- a/src/osx/window_osx.cpp
+++ b/src/osx/window_osx.cpp
@@ -408,19 +408,6 @@ bool wxWindowMac::Create(wxWindowMac *parent,
             m_peer->UseClippingView(m_clipChildren);
     }

-#ifndef __WXUNIVERSAL__
-    // Don't give scrollbars to wxControls unless they ask for them
-    if ( (! IsKindOf(CLASSINFO(wxControl))
-#if wxUSE_STATUSBAR
-        && ! IsKindOf(CLASSINFO(wxStatusBar))
-#endif
-        )
-         || (IsKindOf(CLASSINFO(wxControl)) && ((style & wxHSCROLL) || (style & wxVSCROLL))))
-    {
-        MacCreateScrollBars( style ) ;
-    }
-#endif
-
     wxWindowCreateEvent event((wxWindow*)this);
     GetEventHandler()->AddPendingEvent(event);

@@ -448,6 +435,20 @@ void wxWindowMac::MacPostControlCreate(const wxPoint& pos,
     {
         SetPosition(pos);
     }
+
+#ifndef __WXUNIVERSAL__
+    // Don't give scrollbars to wxControls unless they ask for them
+    if ( (! IsKindOf(CLASSINFO(wxControl))
+#if wxUSE_STATUSBAR
+        && ! IsKindOf(CLASSINFO(wxStatusBar))
+#endif
+        )
+         || (IsKindOf(CLASSINFO(wxControl)) && ((style & wxHSCROLL) || (style & wxVSCROLL))))
+    {
+        MacCreateScrollBars( style ) ;
+    }
+#endif
+
 }

 void wxWindowMac::DoSetWindowVariant( wxWindowVariant variant )

I can push it if it looks ok to you.

csomor commented 2 weeks ago

yes, exactly, thanks, but you'll have to replace the style terms with HasFlags and GetWindowStyle don't you, or store it in a temp named style ?

vadz commented 2 weeks ago

Oops, thanks, it's definitely better if it compiles :-/

I'll check a fix for another problem and push it soon, thanks again and thanks to Jon for testing (and reporting this in the first place, of course)!