Opened 12 years ago

Closed 11 years ago

Last modified 5 years ago

#9563 closed defect (fixed)

Nasty flickering when wxScrolledWindow and wxPanel are combined

Reported by: hajokirchhoff Owned by: robind
Priority: high Milestone: 2.8.11
Component: GUI-generic Version: stable-latest
Keywords: flicker focus Cc:
Blocked By: Blocking:
Patch: no

Description

Widgets hierarchy is as follows:
wxScrolledWindow

wxPanel

wxTextCtrl
wxComboCtrl
<other widgets>

When one of the children of the wxPanel gets the focus and the wxScrolledWindow is too small to display the entire panel, the scrollhelper will first

a) scroll to the bottom of the panel and then
b) will make the child control visible

The effect is that with _every_ focus change event, the entire scrolled window first scrolls to the bottom right, then scrolls back up to the correct position. This does not happen, when the child widgets are immediate children of the wxScrolledWindow. It only happens when the widgets are children of a wxPanel which in turn is a child of the wxScrolledWindow.

The problem is that when the focus changes, the wxPanel propagates its wxFocus event upwards.

containr.cpp:181
propagate the last focus upwards so that our parent can set focus back
to us if it loses it now and regains later; do *not* do this if we are
a toplevel window (e.g. wxDialog) that has another frame as its parent

So what seems to happen is:
A focus event is sent.
wxPanel sees this event and calls

parent->GetEventHandler()->ProcessEvent(eventFocus);

wxScrollHelper intercepts this event and assumes that the wxPanel is about to get the focus. It scrolls the wxScrolledWindow accordingly. Since the wxScrolledWindow is too small to show the entire panel, it scrolls to the bottom/right corner.
Normal event handling continues, the child widget receives the focus and the wxScrolledWindow::HandleEvent sees this event again. The wxScrollHelper will act on the event and scroll the actual child window into view.

To sum it up: The problem seems to be that the event gets duplicated in containr.cpp, where it is propagated from the wxPanel to the parent wxScrolledWindow. The effect is that wxScrolledHelper sees the event twice and one of the events pretends to be a set focus event for the wxPanel.

I don't know how to fix this at the moment and would like to get some input. My first impulse was to add a
if (wxDynamicCast(win, wxPanel)!=0)

return;

to the wxScrollHelper::HandleOnChildFocus, IOW: ignoring all events where a wxPanel is the child. wxPanels as a child would never be scrolled automatically. But that would disable automatic scrolling for widgets derived from wxPanel as well.

A better approach might be
if (win->AcceptsFocus()==false)

return;

Widgets that do not accept a focus should not be handled by HandleOnChildFocus. They should normally not even get a focus event, but in this special wxPanel case they do. So simply return in this case and do nothing.

Attachments (2)

scrl.patch download (3.6 KB) - added by hajokirchhoff 12 years ago.
wxscrolltest.zip download (3.4 KB) - added by hakki_dogusan 12 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 12 years ago by hajokirchhoff

The approach I suggested does not work. A wxPanel with children returns true for AcceptsFocus() so it cannot be used to distinguish between the simulated event and the real one.

Any ideas how I can determine if the Focus Event is the simulated (propagated) event from containr.cpp:181 or the real event?

Regards

Hajo

Changed 12 years ago by hajokirchhoff

comment:2 follow-up: Changed 12 years ago by hajokirchhoff

  • Keywords focus added

Here comes a patch.

Actually the problem is even more complex than I described here. When a deeply nested child gets the focus, HandleOnChildFocus will try to make the topmost wxControlContainer completely visible. This does not work properly.
Consider a wxScrolledWindow of wxSize(100, 100). A nested wxScrolledWindow or wxPanel has a wxSize(500, 500). It will never entirely fit into the scrolled window. When a deeply nested child gets the focus, HandleOnChildFocus will try to make the nested wxPanel visible. It is way to big and HandleOnChildFocus will always scroll to the bottom/right.

But actually it is enough to fit the nested child inside the wxScrolledWindow. Only the window that actually has the focus should be made visible.

The patch fixes this.

a) The artificially generated wxChildFocusEvent will be ignored.
b) The target window will be scrolled only to make the child window visible, but no longer an entire wxPanel.

This makes the automatic scroll feature usable even in nested wxPanel/wxScrolledWindow situations, where the nested wxPanel/wxScrolledWindow is bigger than the top wxScrolledWindow.

To reproduce the problem,

  1. create a wxScrolledWindow of size 100, 100
  2. create a wxPanel of size 500, 500 inside the wxScrolledWindow
  3. create at least two wxTextCtrl inside the wxPanel

Click on any wxTextCtrl or use Tab to focus on them. Without patch: ugly flickering and odd positioning of the wxScrolledWindow. With patch: smooth tabbing between wxTextCtrl inside the wxPanel inside the wxScrolledWindow.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 12 years ago by vadz

Replying to hajokirchhoff:

Here comes a patch.

Thanks!

But actually it is enough to fit the nested child inside the wxScrolledWindow. Only the window that actually has the focus should be made visible.

Yes, I definitely agree with this.

The patch fixes this.

a) The artificially generated wxChildFocusEvent will be ignored.

I didn't understand the part about this artificial event, where exactly is it generated from? And could we avoid generating it in the first place instead of ignoring it later?

b) The target window will be scrolled only to make the child window visible, but no longer an entire wxPanel.

I also don't understand how does the code come to trying to make the entire wxPanel visible. It's supposed to do it only with the focused child but wxPanel itself normally never has focus if it has any children. So how does this happen?

To reproduce the problem,

  1. create a wxScrolledWindow of size 100, 100
  2. create a wxPanel of size 500, 500 inside the wxScrolledWindow
  3. create at least two wxTextCtrl inside the wxPanel

Would you already have some test code doing this? It doesn't seem difficult to recreate this, of course, but if you already have a simple example or a patch to the minimal sample, it would be even simpler.

Thanks!

comment:4 in reply to: ↑ 3 Changed 12 years ago by hajokirchhoff

a) The artificially generated wxChildFocusEvent will be ignored.

I didn't understand the part about this artificial event, where exactly is it generated from? And could we avoid generating it in the first place instead of ignoring it later?

See here:
containr.cpp:181
propagate the last focus upwards so that our parent can set focus back
to us if it loses it now and regains later; do *not* do this if we are
a toplevel window (e.g. wxDialog) that has another frame as its parent

This is where the event is generated. We probably cannot avoid this.

b) The target window will be scrolled only to make the child window visible, but no longer an entire wxPanel.

I also don't understand how does the code come to trying to make the entire wxPanel visible. It's supposed to do it only with the focused child but wxPanel itself normally never has focus if it has any children. So how does this happen?

The code is supposed to do so, but it does not. Instead it takes the focused child and walks up the entire parent hierarchy until it reaches the wxScrolledWindow itself. There is a "while ... parent=parent->GetParent()" loop in the function. It then scrolles the last parent below the wxScrolledWindow into view as opposed to the actual focused child.

Would you already have some test code doing this? It doesn't seem difficult to recreate this, of course, but if you already have a simple example or a patch to the minimal sample, it would be even simpler.

No, sorry, I don't. I hope to be able to create a minimal sample, but am extremely busy right now. Perhaps later this week.

Regards

comment:5 Changed 12 years ago by sunspider

I don't understand the patch... is this it?

"But actually it is enough to fit the nested child inside the wxScrolledWindow. Only the window that actually has the focus should be made visible."

Are we talking about the 'Fit' method? What exactly is the patch?

comment:6 Changed 12 years ago by hajokirchhoff

Nothing to do with 'Fit' at all. Completely different story.

wxScrolledWindow will try to make a child visible, if the child gets the focus, but lies outside the current visible region of the ScrolledWindow.

wxScrolledWindow calculates new scrollbar positions so that the child window becomes visible. It 'scrolls' to the child window.

The bug is: When determining the rectangle that needs to be made visible, the current code walks up the parent hierarchy from the child up to the wxScrolledWindow. It takes the 'topmost' parent of the child, which in most cases will be a wxPanel directly under the wxScrolledWindow itself.

This panel will be much larger than the child itself, as it will almost certainly contain many other controls as well.

The current code tries to make this entire panel visible, rather than only the actual control that has the focus.

This together with another bug in the same code causes wxScrollWindow to scroll all the way down and left and then scroll back again everytime the focus changes. Pressing TAB inside a wxScrolledWindow causes it to scroll/flicker everytime.

This is only visible in wxScrolledWindow with wxPanel or derived classes inside and only if the wxScrolledWindow actually needs to do some scrolling.

The patch fixes this. It no longer walks up the parent hierarchy. It uses the child itself instead or rather uses its immediate parent, but only if the immediate parent can be scrolled fully into view (not clipped). If the parent cannot be shown fully, the child is made visible and only the child.

BTW, I am seeing similar scrolling in wxGrid when the grid is larger than the visible area and uses scrollbars and I edit a control. I'll investigate this further.

Regards

Hajo

comment:7 Changed 12 years ago by robind

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

This sounds the same as a bug reported by a wxPython user, so I'll review the patch.

comment:8 Changed 12 years ago by robind

  • Resolution set to fixed
  • Status changed from accepted to closed

This patch solves the test cases I have for this and a related problem. Applied to both branches, (r54552 and r54553) Thanks!

In future patches please be sure to follow wx Coding Standards. This one contained tabs, had opening braces on the wrong line and lines that were too long.

comment:9 Changed 12 years ago by hajokirchhoff

Thanks for applying. I'll try follow Coding Standards better next time.

Regards

Hajo

comment:10 Changed 12 years ago by botg

  • Resolution fixed deleted
  • Status changed from closed to reopened

Unfortunately this patch causes an annoying regression on OS X if using the generic list control, it keeps jumping to the right/bottom on several actions.

Steps to reproduce:

  • Start samples/listctrl of 2.8 branch on OS X
  • Enable "Mac: Use Generic Control"
  • Enable "Virtual view"
  • Resize columns and main window so that both horizontal and vertical scrollbars are visible
  • Scroll vertically somewhat to the middle
  • Start label editing in the first column, abort it with escape key
  • Repeat previous step several times

Each time the label editing gets started or aborted, the list control scrolls incorrectly.

Reverting this patch restored correct functionality.

comment:11 Changed 12 years ago by RD

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [55042]) Avoid default child window focus behavior in generic wxListCtrl. Fixes #9563

comment:12 Changed 12 years ago by RD

(In [55043]) Avoid default child window focus behavior in generic wxListCtrl. Fixes #9563

Changed 12 years ago by hakki_dogusan

comment:13 Changed 12 years ago by hakki_dogusan

  • Patch unset
  • Resolution fixed deleted
  • Status changed from closed to reopened

Scrolling effect continues with the following hierarcy:

wxTextCtrl or another ctrl which can get focus
wxScrolledWindow

wxPanel

To see:

  • scroll panel somewhere
  • click textctrl
  • click panel --> panel scrolled

Problem exists in wx-2.8.8, wx-2.8.9. wx-2.8.7 is ok.

With the following change problem vanishes (but I'm not sure about the side effects of this change):
src/generic/scrlwing (1381)

wxWindow *actual_focus=wxWindow::FindFocus();
if (/* win != actual_focus && */ <-- commented!

wxDynamicCast(win, wxPanel) != 0 &&
win->GetParent() == m_targetWindow)
if win is a wxPanel and receives the focus, it should not be
scrolled into view
return;

comment:14 Changed 12 years ago by vadz

  • Resolution set to port to stable
  • Status changed from reopened to portneeded

I don't want to modify the code detecting the duplicate child focus event, this is really a horrible hack in its own right and something will need to be done about it on the trunk, e.g. by indicating that this is not a real focus event when we generate it (unfortunately I don't see how to do it in an ABI-preserving way in 2.8). I don't do it now because I (still) have no way to reproduce the original bug, it would be really great if someone could attach an example doing it.

However the fix for the the latest bug (comment 13) seems to be simple enough, see r57402: we just shouldn't try to fit the focused window in our view area if it can't fit anyhow, this is not at all specific to child wxPanels, it would be just as bad for e.g. a child wxTextCtrl. Am I missing something here? If not, i.e. if this doesn't introduce any new problems, it should be backported to 2.8 too.

Your testing would be much appreciated, TIA!

comment:15 Changed 12 years ago by vadz

  • Milestone changed from 2.8.9 to 2.8.11

comment:16 Changed 11 years ago by VZ

(In [60600]) don't scroll to the child which gets focus if it's already fully visible (see #9563) [backport of r57402 from trunk]

comment:17 Changed 11 years ago by vadz

  • Resolution changed from port to stable to fixed
  • Status changed from portneeded to closed

AFAICS this bug is fixed now, please reopen (or create another one) if there are still problems.

comment:18 Changed 5 years ago by kmccarty

See #17154 for an elaboration upon this bug.

Kevin

Note: See TracTickets for help on using tickets.