Opened 4 years ago

Closed 3 months ago

Last modified 3 months ago

#12271 closed defect (fixed)

wxRadioBox and wxSlider don't inherit parent's background color

Reported by: snowleopard2 Owned by: VZ
Priority: normal Milestone:
Component: wxMSW Version: dev-latest
Keywords: background custom colours Cc:
Blocked By: Blocking: #10895
Patch: yes

Description

With 2.9.1, wxHyperLinkCtrl's background color gets set to the default dialog color (at least under MSW). Other controls have their background color set to it's parent background color, and wxHyperLinkCtrl actually was doing this too throughout 2.9.0, it this seems to have been broken in 2.9.1.

Included patch sets wxHyperLinkCtrl's background color to it's parent's background color during creation.

Attachments (5)

update.diff download (563 bytes) - added by snowleopard2 4 years ago.
Handle-custom-colors-radiobox.patch download (1.2 KB) - added by awi 3 months ago.
Handle custom background colours in wxRadioBox.
Handle-custom-colors-slider.patch download (1.1 KB) - added by awi 3 months ago.
Handle custom background colours in wxSlider.
Repaint-trackbar-in-slider.patch download (1.5 KB) - added by awi 3 months ago.
Repaint trackbar in wxSlider.
Repaint-trackbar-slider_v2.patch download (3.2 KB) - added by awi 3 months ago.
Repaint trackbar in wxSlider v2.

Download all attachments as: .zip

Change History (22)

Changed 4 years ago by snowleopard2

comment:1 Changed 4 years ago by vadz

  • Keywords background added
  • Milestone set to 2.9.2
  • Patch unset
  • Status changed from new to confirmed
  • Summary changed from wxHyperLinkCtrl no longer inherits parent's background color (+ patch to fix) to wxHyperLinkCtrl no longer inherits parent's background color

The control should really be transparent (wxStaticText-like) instead of setting its colour explicitly (if nothing else, it will ensure it still works when the parent colour changes and it would also work with themed, i.e. non-solid, backgrounds) so I don't think this is the correct fix, we need to find the change that really broke this and fix it instead. If anybody could try to do it by bisecting the history it would be great.

FWIW the bug can be seen in the widgets sample, just use Ctrl+Shift+B to change the page background (this also shows that radio buttons are similarly broken...).

comment:2 Changed 4 years ago by catalin

wxHyperLinkCtrl is missing
virtual bool HasTransparentBackground() { return true; }
Adding it to wxHyperlinkCtrlBase will fix the background.

But for wxRadioButton the issue is somewhat stranger. While it has the override present, it looks like the implementation from wxWindow is reached.
Same for wxSlider (can be seen in widgets sample).

comment:3 follow-up: Changed 4 years ago by vadz

  • Component changed from GUI-all to wxMSW
  • Milestone 2.9.2 deleted
  • Summary changed from wxHyperLinkCtrl no longer inherits parent's background color to wxRadioButton and wxSlider don't inherit parent's background color

Thanks, I'll fix wxHyperLinkCtrl. For wxRadioButton and wxSlider I'd need to debug it further as I don't know what's going on here. For the latter one part of the problem is obvious: the label subwindows it uses are non-transparent. But why isn't the slider itself transparent is not clear. Worse, at least under Win7 it actually becomes properly drawn (i.e. with transparent background) after you resize the sample page which i even more mysterious...

comment:4 Changed 4 years ago by VZ

(In [66696]) Override HasTransparentBackground() in wxHyperlinkCtrl to return true.

At least in wxMSW the control must override HasTransparentBackground() to
return true if it really wants its background to be transparent, so do it in
wxHyperlinkCtrlBase to fix the background appearance when using the generic
implementation in wxMSW.

See #12271.

comment:5 in reply to: ↑ 3 Changed 4 months ago by awi

Replying to vadz:

Thanks, I'll fix wxHyperLinkCtrl. For wxRadioButton and wxSlider I'd need to debug it further as I don't know what's going on here. For the latter one part of the problem is obvious: the label subwindows it uses are non-transparent. But why isn't the slider itself transparent is not clear. Worse, at least under Win7 it actually becomes properly drawn (i.e. with transparent background) after you resize the sample page which i even more mysterious...

Problem with non-standard background color for wxRadioBox items is of the same nature as problem with wxSlider labels. wxRadioBox items are raw Win controls (not wxRadioButtons) just as wxSlider labels and they are not the children (in the wx sense) of the wxRadioBox/wxSlider parent and don't participate in the background colour inheritance process.

I can confirm that wxSlider is not initially drawn with proper non default background colour but it is drawn with the proper colour not only at resizing but also if its thumb is moved. It seems that only first drawing is wrong and subsequent repaints are fine. This is really strange.

comment:6 Changed 4 months ago by awi

  • Version changed from 2.9.1 to dev-latest

comment:7 Changed 4 months ago by vadz

  • Blocking 10895 added

I guess we could use the real wxWindows for the slider labels and wxRadioBox buttons. This would be slightly inefficient but better than the alternative... And with wxCompositeControl it shouldn't be too bad.

comment:8 Changed 3 months ago by awi

  • Summary changed from wxRadioButton and wxSlider don't inherit parent's background color to wxRadioBox and wxSlider don't inherit parent's background color
  1. It seems that issue with wrong custom background colour for raw Win controls associated with wxRadioBox and wxSlider controls can be fixed by handling in a special way queries for background colour for the control in DoMSWControlColor() function. The idea of the fix is that response to the query for background colour of associated controls should be the same as response to the call for the colour of main control (title label or trackbar). Currently, for associated controls default colour is always returned.

Patches to fix wxRadioButton and wxSlider are attached.
attachment:Handle-custom-colors-radiobox.patch
attachment:Handle-custom-colors-slider.patch

  1. Win taskbar control being a main element of wxSlider apparently has a bug and doesn't repaint itself when its color has been changed. Manual repainting by sending some message (like WM_ENABLE, WM_SETFOCUS, WM_KILLFOCUS) to the control after the change does the job.

attachment:Repaint-trackbar-in-slider.patch
Unfortunately, this patch introduces additonal member variable to wxSlider class to hold current background color which is necessary to check if trackbar colour was really changed.

comment:9 Changed 3 months ago by awi

  • Keywords cutom colours added
  • Patch set

Changed 3 months ago by awi

Handle custom background colours in wxRadioBox.

Changed 3 months ago by awi

Handle custom background colours in wxSlider.

Changed 3 months ago by awi

Repaint trackbar in wxSlider.

comment:10 Changed 3 months ago by awi

  • Keywords custom added; cutom removed

comment:11 follow-up: Changed 3 months ago by vadz

Thanks a lot for (1), I was sure we were already doing this but it turns out we don't because of a bug in wxControl::DoMSWControlColor() logic: it assumes that sub-windows are always children of the main HWND, but fails to deal with the case when they're actually its siblings. So IMHO a simpler (no code duplication) and more correct (because the code was really supposed to do this already) fix would be

  • src/msw/control.cpp

    diff --git a/src/msw/control.cpp b/src/msw/control.cpp
    index 2543d5f..d76e91e 100644
    a b WXHBRUSH wxControl::DoMSWControlColor(WXHDC pDC, wxColour colBg, WXHWND hWnd) 
    380380            // If this HWND doesn't correspond to a wxWindow, it still might be 
    381381            // one of its children for which we need to set the background 
    382382            // brush, e.g. this is the case for the EDIT control that is part 
    383             // of wxComboBox. Check for this by asking the parent if it has it: 
     383            // of wxComboBox but also e.g. of wxSlider label HWNDs which are 
     384            // logically part of it, but are siblings of the main control at 
     385            // Windows level. 
     386            // 
     387            // So check whether it's a sibling of this window which is part of 
     388            // the same wx object. 
     389            if ( ContainsHWND(hWnd) ) 
     390            { 
     391                win = this; 
     392            } 
     393            else // Or maybe a child sub-window of this one. 
     394            { 
    384395            HWND parent = ::GetParent(hWnd); 
    385396            if ( parent ) 
    386397            { 
    WXHBRUSH wxControl::DoMSWControlColor(WXHDC pDC, wxColour colBg, WXHWND hWnd) 
    389400                    win = winParent; 
    390401             } 
    391402        } 
     403        } 
    392404 
    393405        if ( win ) 
    394406            hbr = win->MSWGetBgBrush(pDC); 

Do you see anything wrong with applying this instead of the first two patches?

comment:12 in reply to: ↑ 11 Changed 3 months ago by awi

Replying to vadz:

Thanks a lot for (1), I was sure we were already doing this but it turns out we don't because of a bug in wxControl::DoMSWControlColor() logic: it assumes that sub-windows are always children of the main HWND, but fails to deal with the case when they're actually its siblings. So IMHO a simpler (no code duplication) and more correct (because the code was really supposed to do this already) fix would be

Do you see anything wrong with applying this instead of the first two patches?

Your patch fixes exactly the root cause of the issue so it's better then my fixes for individual classes. I also thought about this modification but I was afraid if it is not too general and too invasive.

comment:13 Changed 3 months ago by vadz

And concerning (2), we're indeed apparently not the only ones with this problem, see e.g. http://stackoverflow.com/questions/12800581/update-mfcs-csliderctrl (unfortunately the link to the discussion with more details from there doesn't work any more...). So it looks like your patch is indeed the only solution. If we really want to have it in 3.0, we should replace the brush field with a private (i.e. local in src/msw/slider.cpp) hash map with wxSlider* pointers as keys and brushes as values. This is ugly but the only way to do it without breaking the ABI, unfortunately.

comment:14 Changed 3 months ago by awi

Here is another link to the discussion about the problem with repainting the taskbar:
http://www.masmforum.com/board/index.php?PHPSESSID=8d46cd4ecb1688be429ab49694ec53e6&topic=3785.10;wap2

Next version of the patch:
attachment:Repaint-trackbar-slider_v2.patch

Changed 3 months ago by awi

Repaint trackbar in wxSlider v2.

comment:15 Changed 3 months ago by VZ

  • Owner set to VZ
  • Resolution set to fixed
  • Status changed from confirmed to closed

In 76950:

Fix background of wxRadioBox buttons and wxSlider labels in wxMSW.

Handle WM_CTLCOLOR correctly for them, this wasn't done before because the
code assumed that sub-windows (i.e. HWNDs which belong to the same logical wx
control) were always children of the main window, but they could also be its
siblings (like in at least the two above mentioned cases).

Account for this case in wxControl::DoMSWControlColor() too now.

Closes #12271.

comment:16 Changed 3 months ago by VZ

In 76984:

Fix background of wxRadioBox buttons and wxSlider labels in wxMSW.

Handle WM_CTLCOLOR correctly for them, this wasn't done before because the
code assumed that sub-windows (i.e. HWNDs which belong to the same logical wx
control) were always children of the main window, but they could also be its
siblings (like in at least the two above mentioned cases).

Account for this case in wxControl::DoMSWControlColor() too now.

Closes #12271.

comment:17 Changed 3 months ago by VZ

In 76985:

Update wxSlider background when its parent background changes in wxMSW.

The native control doesn't redraw itself, so force it to do it from the
overridden DoMSWControlColor() which is called every time the background
colour might have changed.

See #12271.

Note: See TracTickets for help on using tickets.